Merge pull request #3140 from adityamandaleeka/env_pal

Fix usage of environment in PAL

src/pal/src/debug/debug.cpp

@@ -34,7 +34,7 @@ Revision History:
 #include "pal/process.h"
 #include "pal/context.h"
 #include "pal/debug.h"
-#include "pal/misc.h"
+#include "pal/environ.h"
 #include "pal/malloc.hpp"
 #include "pal/module.h"
 #include "pal/stackstring.hpp"
@@ -172,17 +172,19 @@ OutputDebugStringA(
 {
     PERF_ENTRY(OutputDebugStringA);
     ENTRY("OutputDebugStringA (lpOutputString=%p (%s))\n",
-          lpOutputString?lpOutputString:"NULL",
-          lpOutputString?lpOutputString:"NULL");
-
-    /* as we don't support debug events, we are going to output the debug string
-      to stderr instead of generating OUT_DEBUG_STRING_EVENT */
-    if ( (lpOutputString != NULL) && 
-         (NULL != MiscGetenv(PAL_OUTPUTDEBUGSTRING)))
+          lpOutputString ? lpOutputString : "NULL",
+          lpOutputString ? lpOutputString : "NULL");
+
+    // As we don't support debug events, we are going to output the debug string
+    // to stderr instead of generating OUT_DEBUG_STRING_EVENT. It's safe to tell
+    // EnvironGetenv not to make a copy of the value here since we only want to
+    // check whether it exists, not actually use it.
+    if ((lpOutputString != NULL) &&
+        (NULL != EnvironGetenv(PAL_OUTPUTDEBUGSTRING, /* copyValue */ FALSE)))
     {
         fprintf(stderr, "%s", lpOutputString);
     }
-        
+
     LOGEXIT("OutputDebugStringA returns\n");
     PERF_EXIT(OutputDebugStringA);
 }
@@ -340,11 +342,14 @@ DebugBreakCommand()
 {
 #ifdef ENABLE_RUN_ON_DEBUG_BREAK
     extern MODSTRUCT exe_module;
-    const char *command_string = getenv (PAL_RUN_ON_DEBUG_BREAK);
-    if (command_string) {
+
+    char *command_string = EnvironGetenv(PAL_RUN_ON_DEBUG_BREAK);
+    if (command_string)
+    {
         char pid_buf[sizeof (PID_TEXT) + 32];
         PathCharString exe_bufString;
         int libNameLength = 10;
+
         if (exe_module.lib_name != NULL)
         {
             libNameLength = PAL_wcslen(exe_module.lib_name);
@@ -358,10 +363,13 @@ DebugBreakCommand()
             goto FAILED;
         }
 
-        if (snprintf (pid_buf, sizeof (pid_buf), PID_TEXT "%d", getpid()) <= 0) {
+        if (snprintf (pid_buf, sizeof (pid_buf), PID_TEXT "%d", getpid()) <= 0)
+        {
             goto FAILED;
         }
-        if (snprintf (exe_buf, sizeof (CHAR) * (dwexe_buf + 1), EXE_TEXT "%ls", (wchar_t *)exe_module.lib_name) <= 0) {
+
+        if (snprintf (exe_buf, sizeof (CHAR) * (dwexe_buf + 1), EXE_TEXT "%ls", (wchar_t *)exe_module.lib_name) <= 0)
+        {
             goto FAILED;
         }
 
@@ -370,16 +378,28 @@ DebugBreakCommand()
            variables in the child process, but if we do that we can't check
            for errors. putenv/setenv can fail when out of memory */
 
-        if (!MiscPutenv (pid_buf, FALSE) || !MiscPutenv (exe_buf, FALSE)) {
+        if (!EnvironPutenv (pid_buf, FALSE) || !EnvironPutenv (exe_buf, FALSE))
+        {
             goto FAILED;
         }
-        if (run_debug_command (command_string)) {
+
+        if (run_debug_command (command_string))
+        {
             goto FAILED;
         }
+
+        InternalFree(command_string);
         return 1;
     }
+
     return 0;
+
 FAILED:
+    if (command_string)
+    {
+        InternalFree(command_string);
+    }
+
     fprintf (stderr, "Failed to execute command: '%s'\n", command_string);
     return -1;
 #else // ENABLE_RUN_ON_DEBUG_BREAK

src/pal/src/exception/machexception.cpp

@@ -29,6 +29,7 @@ Module Name:
 #include "pal/process.h"
 #include "pal/virtual.h"
 #include "pal/map.hpp"
+#include "pal/environ.h"
 
 #include "machmessage.h"
 
@@ -170,10 +171,11 @@ GetExceptionMask()
     {
         exMode = MachException_Default;
 
-        const char * exceptionSettings = getenv(PAL_MACH_EXCEPTION_MODE);
+        char* exceptionSettings = EnvironGetenv(PAL_MACH_EXCEPTION_MODE);
         if (exceptionSettings)
         {
             exMode = (MachExceptionMode)atoi(exceptionSettings);
+            InternalFree(exceptionSettings);
         }
         else
         {

src/pal/src/exception/machmessage.cpp

@@ -16,6 +16,8 @@ Module Name:
 
 #include "config.h"
 #include "pal/dbgmsg.h"
+#include "pal/environ.h"
+#include "pal/malloc.hpp"
 #include "pal/thread.hpp"
 #include "machmessage.h"
 

src/pal/src/exception/machmessage.h

@@ -56,7 +56,7 @@ using namespace CorUnix;
 
 #ifdef _DEBUG
 
-#define NONPAL_TRACE_ENABLED getenv("NONPAL_TRACING")
+#define NONPAL_TRACE_ENABLED EnvironGetenv("NONPAL_TRACING", /* copyValue */ false)
 
 #define NONPAL_ASSERT(_msg, ...) NONPAL_RETAIL_ASSERT(_msg, __VA_ARGS__)
 
@@ -67,7 +67,7 @@ using namespace CorUnix;
     } while (false)
 
 // Debug-only output with printf-style formatting.
-#define NONPAL_TRACE(_format, ...) do {                                                  \
+#define NONPAL_TRACE(_format, ...) do {                                              \
         if (NONPAL_TRACE_ENABLED) printf("NONPAL_TRACE: " _format, ## __VA_ARGS__);  \
     } while (false)
 

src/pal/src/include/pal/environ.h

@@ -0,0 +1,78 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+/*++
+
+
+
+Module Name:
+
+    include/pal/environ.h
+
+Abstract:
+    Header file for functions manipulating environment variables
+
+
+--*/
+
+#ifndef __ENVIRON_H_
+#define __ENVIRON_H_
+
+#ifdef __cplusplus
+extern "C"
+{
+#endif // __cplusplus
+
+/*++
+Variables :
+
+    palEnvironment: a global variable equivalent to environ on systems on 
+                    which that exists, and a pointer to an array of environment 
+                    strings on systems without environ.
+    gcsEnvironment: critical section to synchronize access to palEnvironment
+--*/
+extern char **palEnvironment;
+extern CRITICAL_SECTION gcsEnvironment;
+
+/*++
+
+Function:
+  EnvironInitialize
+
+Initialization function for the PAL environment code.
+--*/
+BOOL EnvironInitialize();
+
+/*++
+Function:
+  EnvironGetenv
+
+Get the value of environment variable with the given name.
+--*/
+char *EnvironGetenv(const char *name, BOOL copyValue = TRUE);
+
+/*++
+Function:
+  EnvironPutenv
+
+Add the environment variable string provided to the PAL version
+of the environment.
+--*/
+BOOL EnvironPutenv(const char *string, BOOL deleteIfEmpty);
+
+/*++
+Function:
+  EnvironUnsetenv
+
+Remove the environment variable with the given name from the PAL
+version of the environment if it exists.
+--*/
+void EnvironUnsetenv(const char *name);
+
+#ifdef __cplusplus
+}
+#endif // __cplusplus
+
+#endif /* __ENVIRON_H_ */
+

src/pal/src/include/pal/misc.h

@@ -26,17 +26,6 @@ extern "C"
 {
 #endif // __cplusplus
 
-/*++
-Variables :
-
-    palEnvironment: a global variable equivalent to environ on systems on 
-                    which that exists, and a pointer to an array of environment 
-                    strings on systems without environ.
-    gcsEnvironment: critical section to synchronize access to palEnvironment
---*/
-extern char **palEnvironment;
-extern CRITICAL_SECTION gcsEnvironment;
-
 /*++
 Function :
 
@@ -87,49 +76,6 @@ Function :
 --*/
 void MsgBoxCleanup( void );
 
-/*++
-
-Function:
-  MiscInitialize
-
---*/
-BOOL MiscInitialize();
-
-/*++
-Function:
-  MiscCleanup
-
---*/
-VOID MiscCleanup();
-
-/*++
-Function:
-  MiscGetenv
-
-Gets an environment variable's value from environ. The returned buffer
-must not be modified or freed.
---*/
-char *MiscGetenv(const char *name);
-
-/*++
-Function:
-  MiscPutenv
-
-Sets an environment variable's value by directly modifying environ.
-Returns TRUE if the variable was set, or FALSE if malloc or realloc
-failed or if the given string is malformed.
---*/
-BOOL MiscPutenv(const char *string, BOOL deleteIfEmpty);
-
-/*++
-Function:
-  MiscUnsetenv
-
-Removes a variable from the environment. Does nothing if the variable
-does not exist in the environment.
---*/
-void MiscUnsetenv(const char *name);
-
 #ifdef __cplusplus
 }
 #endif // __cplusplus

src/pal/src/init/pal.cpp

@@ -34,6 +34,7 @@ Module Name:
 #include "pal/module.h"
 #include "pal/virtual.h"
 #include "pal/misc.h"
+#include "pal/environ.h"
 #include "pal/utils.h"
 #include "pal/debug.h"
 #include "pal/locale.h"
@@ -240,14 +241,14 @@ Initialize(
         }
 
         // Initialize the environment.
-        if (FALSE == MiscInitialize())
+        if (FALSE == EnvironInitialize())
         {
             goto CLEANUP0;
         }
 
         // Initialize debug channel settings before anything else.
         // This depends on the environment, so it must come after
-        // MiscInitialize.
+        // EnvironInitialize.
         if (FALSE == DBG_init_channels())
         {
             goto CLEANUP0;
@@ -1182,19 +1183,16 @@ static LPWSTR INIT_FindEXEPath(LPCSTR exe_name)
 
     /* no path was specified : search $PATH */
 
-    env_path = MiscGetenv("PATH");
+    env_path = EnvironGetenv("PATH");
     if (!env_path || *env_path=='\0')
     {
         WARN("$PATH isn't set.\n");
-        goto last_resort;
-    }
+        if (env_path != NULL)
+        {
+            InternalFree(env_path);
+        }
 
-    /* get our own copy of env_path so we can modify it */
-    env_path = strdup(env_path);
-    if (!env_path)
-    {
-        ERROR("Not enough memory to copy $PATH!\n");
-        return NULL;
+        goto last_resort;
     }
 
     exe_name_length=strlen(exe_name);
@@ -1327,7 +1325,7 @@ static LPWSTR INIT_FindEXEPath(LPCSTR exe_name)
     }
 
     InternalFree(env_path);
-    TRACE("No %s found in $PATH (%s)\n", exe_name, MiscGetenv("PATH"));
+    TRACE("No %s found in $PATH (%s)\n", exe_name, EnvironGetenv("PATH", FALSE));
 
 last_resort:
     /* last resort : see if the executable is in the current directory. This is

src/pal/src/loader/module.cpp

@@ -30,7 +30,7 @@ Module Name:
 #include "pal/utils.h"
 #include "pal/init.h"
 #include "pal/modulename.h"
-#include "pal/misc.h"
+#include "pal/environ.h"
 #include "pal/virtual.h"
 #include "pal/map.hpp"
 #include "pal/stackstring.hpp"
@@ -745,12 +745,17 @@ PAL_LOADLoadPEFile(HANDLE hFile)
 #ifdef _DEBUG
     if (loadedBase != nullptr)
     {
-        char* envVar = getenv("PAL_ForcePEMapFailure");
-        if (envVar && strlen(envVar) > 0)
+        char* envVar = EnvironGetenv("PAL_ForcePEMapFailure");
+        if (envVar)
         {
-            TRACE("Forcing failure of PE file map, and retry\n");
-            PAL_LOADUnloadPEFile(loadedBase); // unload it
-            loadedBase = MAPMapPEFile(hFile); // load it again
+            if (strlen(envVar) > 0)
+            {
+                TRACE("Forcing failure of PE file map, and retry\n");
+                PAL_LOADUnloadPEFile(loadedBase); // unload it
+                loadedBase = MAPMapPEFile(hFile); // load it again
+            }
+
+            InternalFree(envVar);
         }
     }
 #endif // _DEBUG