Fix all callers of environment functions.

Commit migrated from https://github.com/dotnet/coreclr/commit/a6f241cbc1122ac9fa3efdf9d39646e7848d6219

diff --git a/src/coreclr/src/pal/src/cruntime/misc.cpp b/src/coreclr/src/pal/src/cruntime/misc.cpp
index d267341..708f882 100644 (file)
--- a/src/coreclr/src/pal/src/cruntime/misc.cpp
+++ b/src/coreclr/src/pal/src/cruntime/misc.cpp
@@ -30,9 +30,7 @@ Abstract:
 #include <stdarg.h>
 #include <time.h>
 #include <limits.h>
-#if HAVE_CRT_EXTERNS_H
-#include <crt_externs.h>
-#endif  // HAVE_CRT_EXTERNS_H
+
 #if defined(_AMD64_) || defined(_x86_)
 #include <xmmintrin.h>
 #endif // defined(_AMD64_) || defined(_x86_)

diff --git a/src/coreclr/src/pal/src/debug/debug.cpp b/src/coreclr/src/pal/src/debug/debug.cpp
index c71bf59..686d7ed 100644 (file)
--- a/src/coreclr/src/pal/src/debug/debug.cpp
+++ b/src/coreclr/src/pal/src/debug/debug.cpp
@@ -172,13 +172,14 @@ OutputDebugStringA(
 {
     PERF_ENTRY(OutputDebugStringA);
     ENTRY("OutputDebugStringA (lpOutputString=%p (%s))\n",
-          lpOutputString?lpOutputString:"NULL",
-          lpOutputString?lpOutputString:"NULL");
+          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 != EnvironGetenv(PAL_OUTPUTDEBUGSTRING)))
+    // 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, false)))
     {
         fprintf(stderr, "%s", lpOutputString);
     }
@@ -340,11 +341,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 +362,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 +377,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 (!EnvironPutenv (pid_buf, FALSE) || !EnvironPutenv (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

diff --git a/src/coreclr/src/pal/src/exception/machexception.cpp b/src/coreclr/src/pal/src/exception/machexception.cpp
index ea57173..cd4e26d 100644 (file)
--- a/src/coreclr/src/pal/src/exception/machexception.cpp
+++ b/src/coreclr/src/pal/src/exception/machexception.cpp
@@ -29,6 +29,7 @@ Abstract:
 #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
         {

diff --git a/src/coreclr/src/pal/src/exception/machmessage.cpp b/src/coreclr/src/pal/src/exception/machmessage.cpp
index 2e11da8..a6f7e57 100644 (file)
--- a/src/coreclr/src/pal/src/exception/machmessage.cpp
+++ b/src/coreclr/src/pal/src/exception/machmessage.cpp
@@ -16,6 +16,8 @@ Abstract:
 
 #include "config.h"
 #include "pal/dbgmsg.h"
+#include "pal/environ.h"
+#include "pal/malloc.hpp"
 #include "pal/thread.hpp"
 #include "machmessage.h"
 

diff --git a/src/coreclr/src/pal/src/exception/machmessage.h b/src/coreclr/src/pal/src/exception/machmessage.h
index 22ec0a7..80077af 100644 (file)
--- a/src/coreclr/src/pal/src/exception/machmessage.h
+++ b/src/coreclr/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)
 

diff --git a/src/coreclr/src/pal/src/include/pal/environ.h b/src/coreclr/src/pal/src/include/pal/environ.h
index 4a303b8..f4b8cd9 100644 (file)
--- a/src/coreclr/src/pal/src/include/pal/environ.h
+++ b/src/coreclr/src/pal/src/include/pal/environ.h
@@ -49,7 +49,7 @@ Function:
 
 Gets an environment variable's value.
 --*/
-char *EnvironGetenv(const char *name);
+char *EnvironGetenv(const char *name, bool copyValue = true);
 
 /*++
 Function:

diff --git a/src/coreclr/src/pal/src/init/pal.cpp b/src/coreclr/src/pal/src/init/pal.cpp
index 95c6e59..4e6b7d2 100644 (file)
--- a/src/coreclr/src/pal/src/init/pal.cpp
+++ b/src/coreclr/src/pal/src/init/pal.cpp
@@ -1187,15 +1187,12 @@ static LPWSTR INIT_FindEXEPath(LPCSTR exe_name)
     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);
@@ -1328,7 +1325,7 @@ static LPWSTR INIT_FindEXEPath(LPCSTR exe_name)
     }
 
     InternalFree(env_path);
-    TRACE("No %s found in $PATH (%s)\n", exe_name, EnvironGetenv("PATH"));
+    TRACE("No %s found in $PATH (%s)\n", exe_name, EnvironGetenv("PATH")); /////// leak in debug. fix?
 
 last_resort:
     /* last resort : see if the executable is in the current directory. This is

diff --git a/src/coreclr/src/pal/src/loader/module.cpp b/src/coreclr/src/pal/src/loader/module.cpp
index d05e272..83d45f3 100644 (file)
--- a/src/coreclr/src/pal/src/loader/module.cpp
+++ b/src/coreclr/src/pal/src/loader/module.cpp
@@ -30,7 +30,7 @@ Abstract:
 #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

diff --git a/src/coreclr/src/pal/src/map/map.cpp b/src/coreclr/src/pal/src/map/map.cpp
index f8b30fa..7d4cf92 100644 (file)
--- a/src/coreclr/src/pal/src/map/map.cpp
+++ b/src/coreclr/src/pal/src/map/map.cpp
@@ -24,6 +24,7 @@ Abstract:
 #include "pal/init.h"
 #include "pal/critsect.h"
 #include "pal/virtual.h"
+#include "pal/environ.h"
 #include "common.h"
 #include "pal/map.hpp"
 #include "pal/thread.hpp"
@@ -2288,6 +2289,7 @@ void * MAPMapPEFile(HANDLE hFile)
     void * retval;
 #if _DEBUG
     bool forceRelocs = false;
+    char* envVar;
 #endif
 
     ENTRY("MAPMapPEFile (hFile=%p)\n", hFile);
@@ -2393,13 +2395,18 @@ void * MAPMapPEFile(HANDLE hFile)
     }
 
 #if _DEBUG
-    char * envVar;
-    envVar = getenv("PAL_ForceRelocs");
-    if (envVar && strlen(envVar) > 0)
+    envVar = EnvironGetenv("PAL_ForceRelocs");
+    if (envVar)
     {
-        forceRelocs = true;
-        TRACE_(LOADER)("Forcing rebase of image\n");
+        if (strlen(envVar) > 0)
+        {
+            forceRelocs = true;
+            TRACE_(LOADER)("Forcing rebase of image\n");
+        }
+
+        InternalFree(envVar);
     }
+
     void * pForceRelocBase;
     pForceRelocBase = NULL;
     if (forceRelocs)

diff --git a/src/coreclr/src/pal/src/misc/dbgmsg.cpp b/src/coreclr/src/pal/src/misc/dbgmsg.cpp
index 7ff0f26..3bd8003 100644 (file)
--- a/src/coreclr/src/pal/src/misc/dbgmsg.cpp
+++ b/src/coreclr/src/pal/src/misc/dbgmsg.cpp
@@ -150,7 +150,7 @@ Function :
 BOOL DBG_init_channels(void)
 {
     INT i;
-    LPCSTR env_string;
+    LPSTR env_string;
     LPSTR env_workstring;
     LPSTR env_pcache;
     LPSTR entry_ptr;
@@ -168,21 +168,9 @@ BOOL DBG_init_channels(void)
 
     /* parse PAL_DBG_CHANNELS environment variable */
 
-    if (!(env_string = EnvironGetenv(ENV_CHANNELS))) 
-    {
-        env_pcache = env_workstring = NULL;
-    }
-    else
-    {
-        env_pcache = env_workstring = PAL__strdup(env_string);
+    env_string = EnvironGetenv(ENV_CHANNELS);
+    env_pcache = env_workstring = env_string;
 
-        if (env_workstring == NULL)
-        {
-            /* Not enough memory */
-            DeleteCriticalSection(&fprintf_crit_section);
-            return FALSE;
-        }
-    }
     while(env_workstring)
     {
         entry_ptr=env_workstring;
@@ -346,6 +334,11 @@ BOOL DBG_init_channels(void)
         output_file = stderr; /* output to stderr by default */
     }
 
+    if(env_string)
+    {
+        PAL_free(env_string);
+    }
+
     /* see if we need to disable assertions */
     env_string = EnvironGetenv(ENV_ASSERTS);
     if(env_string && 0 == strcmp(env_string,"1"))
@@ -357,11 +350,17 @@ BOOL DBG_init_channels(void)
         g_Dbg_asserts_enabled = TRUE;
     }
 
+    if(env_string)
+    {
+        PAL_free(env_string);
+    }
+
     /* select ENTRY level limitation */
-    env_string = EnvironGetenv(ENV_ENTRY_LEVELS);    
+    env_string = EnvironGetenv(ENV_ENTRY_LEVELS);
     if(env_string)
     {
         max_entry_level = atoi(env_string);
+        PAL_free(env_string);
     }
     else
     {
@@ -819,15 +818,30 @@ bool DBG_ShouldCheckStackAlignment()
     
     if (caMode == CheckAlignment_Uninitialized)
     {
-        const char * checkAlignmentSettings = getenv(PAL_CHECK_ALIGNMENT_MODE);
+        char* checkAlignmentSettings;
+        if (palEnvironment == nullptr)
+        {
+            // This function might be called before the PAL environment is initialized.
+            // In this case, use the system getenv instead.
+            checkAlignmentSettings = ::getenv(PAL_CHECK_ALIGNMENT_MODE);
+        }
+        else
+        {
+            checkAlignmentSettings = EnvironGetenv(PAL_CHECK_ALIGNMENT_MODE);
+        }
+
         caMode = checkAlignmentSettings ?
             (CheckAlignmentMode)atoi(checkAlignmentSettings) : CheckAlignment_Default;
+
+        if (checkAlignmentSettings)
+        {
+            InternalFree(checkAlignmentSettings);
+        }
     }
     
     return caMode == CheckAlignment_On;
 }
 #endif // _DEBUG && __APPLE__
-    
 
 #ifdef __APPLE__
 #include "CoreFoundation/CFUserNotification.h"
@@ -860,10 +874,12 @@ void PAL_DisplayDialog(const char *szTitle, const char *szText)
     
     if (dispDialog == DisplayDialog_Uninitialized)
     {
-        const char * displayDialog = getenv(PAL_DISPLAY_DIALOG);
+        char* displayDialog = EnvironGetenv(PAL_DISPLAY_DIALOG);
         if (displayDialog)
         {
             int i = atoi(displayDialog);
+            InternalFree(displayDialog);
+
             switch (i)
             {
             case 0:

diff --git a/src/coreclr/src/pal/src/misc/environ.cpp b/src/coreclr/src/pal/src/misc/environ.cpp
index c2bdb83..3bb4c01 100644 (file)
--- a/src/coreclr/src/pal/src/misc/environ.cpp
+++ b/src/coreclr/src/pal/src/misc/environ.cpp
@@ -26,6 +26,10 @@ Revision History:
 #include "pal/environ.h"
 #include "pal/malloc.hpp"
 
+#if HAVE_CRT_EXTERNS_H
+#include <crt_externs.h>
+#endif
+
 #include <stdlib.h>
 
 using namespace CorUnix;
@@ -50,11 +54,11 @@ characters.
 Parameters
 
 lpName 
-       [in] Pointer to a null-terminated string that specifies the environment variable. 
+       [in] Pointer to a null-terminated string that specifies the environment variable.
 lpBuffer 
-       [out] Pointer to a buffer to receive the value of the specified environment variable. 
+       [out] Pointer to a buffer to receive the value of the specified environment variable.
 nSize 
-       [in] Specifies the size, in TCHARs, of the buffer pointed to by the lpBuffer parameter. 
+       [in] Specifies the size, in TCHARs, of the buffer pointed to by the lpBuffer parameter.
 
 Return Values
 
@@ -84,7 +88,10 @@ GetEnvironmentVariableA(
     ENTRY("GetEnvironmentVariableA(lpName=%p (%s), lpBuffer=%p, nSize=%u)\n",
         lpName ? lpName : "NULL",
         lpName ? lpName : "NULL", lpBuffer, nSize);
-    
+
+    bool enteredCritSec = false;
+    CPalThread * pthrCurrent = InternalGetCurrentThread();
+
     if (lpName == nullptr)
     {
         ERROR("lpName is null\n");
@@ -106,7 +113,14 @@ GetEnvironmentVariableA(
     }
     else
     {
-        value = EnvironGetenv(lpName); ///////// make this not return a copy, or have it fill out the buffer
+        // Enter the environment critical section so that we can safely get
+        // the environment variable value without EnvironGetenv making an
+        // intermediate copy. We will just copy the string to the output 
+        // buffer anyway, so just stay in the critical section until then.
+        InternalEnterCriticalSection(pthrCurrent, &gcsEnvironment);
+        enteredCritSec = true;
+
+        value = EnvironGetenv(lpName, /* copyValue */ false);
     }
 
     if (value == nullptr)
@@ -129,6 +143,12 @@ GetEnvironmentVariableA(
     SetLastError(ERROR_SUCCESS);
 
 done:
+
+    if (enteredCritSec)
+    {
+        InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment);
+    }
+
     LOGEXIT("GetEnvironmentVariableA returns DWORD 0x%x\n", dwRet);
     PERF_EXIT(GetEnvironmentVariableA);
     return dwRet;
@@ -604,7 +624,10 @@ SetEnvironmentVariableA(
      * the variable name from process environment */
     if (lpValue == nullptr)
     {
-        if ((lpValue = EnvironGetenv(lpName)) == nullptr)
+        // We tell EnvironGetenv not to bother with making a copy of the
+        // value since we're not going to use it for anything interesting
+        // apart from checking whether it's null.
+        if ((lpValue = EnvironGetenv(lpName, /* copyValue */ false)) == nullptr)
         {
             ERROR("Couldn't find environment variable (%s)\n", lpName);
             SetLastError(ERROR_ENVVAR_NOT_FOUND);
@@ -746,6 +769,8 @@ BOOL EnvironPutenv(const char* entry, BOOL deleteIfEmpty)
         copy[length - 1] = '\0';
 
         EnvironUnsetenv(copy);
+        InternalFree(copy);
+
         result = TRUE;
     }
     else
@@ -798,7 +823,7 @@ BOOL EnvironPutenv(const char* entry, BOOL deleteIfEmpty)
                 int resizeRet = ResizeEnvironment(palEnvironmentCapacity * 2);
                 if (resizeRet != 0)
                 {
-                    free(copy);
+                    InternalFree(copy);
                     goto done;
                 }
             }
@@ -821,7 +846,7 @@ done:
     return result;
 }
 
-char* EnvironGetenv(const char* name)
+char* EnvironGetenv(const char* name, bool copyValue)
 {
     char *retValue = nullptr;
 
@@ -839,7 +864,7 @@ char* EnvironGetenv(const char* name)
             // treat the whole thing as name, so the value is an empty string.
             if (*equalsSignPosition == '\0')
             {
-                retValue = strdup("");
+                retValue = (char *)"";
                 break;
             }
             else if (*equalsSignPosition == '=')
@@ -850,6 +875,11 @@ char* EnvironGetenv(const char* name)
         }
     }
 
+    if ((retValue != nullptr) && copyValue)
+    {
+        retValue = strdup(retValue);
+    }
+
     InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment);
     return retValue;
 }
@@ -882,7 +912,6 @@ EnvironInitialize(void)
 {
     InternalInitializeCriticalSection(&gcsEnvironment);
 
-
     CPalThread * pthrCurrent = InternalGetCurrentThread();
     InternalEnterCriticalSection(pthrCurrent, &gcsEnvironment);
 

diff --git a/src/coreclr/src/pal/src/thread/process.cpp b/src/coreclr/src/pal/src/thread/process.cpp
index 84d1b11..b2f3703 100644 (file)
--- a/src/coreclr/src/pal/src/thread/process.cpp
+++ b/src/coreclr/src/pal/src/thread/process.cpp
@@ -4357,19 +4357,13 @@ getPath(
     }
 
     pThread = InternalGetCurrentThread();
+
     /* Then try to look in the path */
-    int iLen2 = strlen(EnvironGetenv("PATH"))+1;
-    lpPath = (LPSTR) InternalMalloc(iLen2);
+    lpPath = EnvironGetenv("PATH");
 
     if (!lpPath)
     {
-        ERROR("couldn't allocate memory for $PATH\n");
-        return FALSE;
-    }
-
-    if (strcpy_s(lpPath, iLen2, EnvironGetenv("PATH")) != SAFECRT_SUCCESS)
-    {
-        ERROR("strcpy_s failed!");
+        ERROR("EnvironGetenv returned NULL for $PATH\n");
         return FALSE;
     }
 

diff --git a/src/coreclr/src/pal/src/thread/thread.cpp b/src/coreclr/src/pal/src/thread/thread.cpp
index 3ba97ad..0bef811 100644 (file)
--- a/src/coreclr/src/pal/src/thread/thread.cpp
+++ b/src/coreclr/src/pal/src/thread/thread.cpp
@@ -1690,6 +1690,8 @@ CorUnix::InitializeGlobalThreadData(
         {
             CPalThread::s_dwDefaultThreadStackSize = dw;
         }
+
+        InternalFree(pszStackSize);
     }
 
 #if !HAVE_MACH_EXCEPTIONS