From a6f241cbc1122ac9fa3efdf9d39646e7848d6219 Mon Sep 17 00:00:00 2001 From: Aditya Mandaleeka Date: Tue, 9 Feb 2016 19:14:28 -0800 Subject: [PATCH] Fix all callers of environment functions. --- src/pal/src/cruntime/misc.cpp | 4 +-- src/pal/src/debug/debug.cpp | 43 ++++++++++++++++++-------- src/pal/src/exception/machexception.cpp | 4 ++- src/pal/src/exception/machmessage.cpp | 2 ++ src/pal/src/exception/machmessage.h | 4 +-- src/pal/src/include/pal/environ.h | 2 +- src/pal/src/init/pal.cpp | 15 ++++----- src/pal/src/loader/module.cpp | 17 +++++++---- src/pal/src/map/map.cpp | 17 ++++++++--- src/pal/src/misc/dbgmsg.cpp | 54 +++++++++++++++++++++------------ src/pal/src/misc/environ.cpp | 49 ++++++++++++++++++++++++------ src/pal/src/thread/process.cpp | 12 ++------ src/pal/src/thread/thread.cpp | 2 ++ 13 files changed, 148 insertions(+), 77 deletions(-) diff --git a/src/pal/src/cruntime/misc.cpp b/src/pal/src/cruntime/misc.cpp index d267341..708f882 100644 --- a/src/pal/src/cruntime/misc.cpp +++ b/src/pal/src/cruntime/misc.cpp @@ -30,9 +30,7 @@ Abstract: #include #include #include -#if HAVE_CRT_EXTERNS_H -#include -#endif // HAVE_CRT_EXTERNS_H + #if defined(_AMD64_) || defined(_x86_) #include #endif // defined(_AMD64_) || defined(_x86_) diff --git a/src/pal/src/debug/debug.cpp b/src/pal/src/debug/debug.cpp index c71bf59..686d7ed 100644 --- a/src/pal/src/debug/debug.cpp +++ b/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/pal/src/exception/machexception.cpp b/src/pal/src/exception/machexception.cpp index ea57173..cd4e26d 100644 --- a/src/pal/src/exception/machexception.cpp +++ b/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/pal/src/exception/machmessage.cpp b/src/pal/src/exception/machmessage.cpp index 2e11da8..a6f7e57 100644 --- a/src/pal/src/exception/machmessage.cpp +++ b/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/pal/src/exception/machmessage.h b/src/pal/src/exception/machmessage.h index 22ec0a7..80077af 100644 --- a/src/pal/src/exception/machmessage.h +++ b/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/pal/src/include/pal/environ.h b/src/pal/src/include/pal/environ.h index 4a303b8..f4b8cd9 100644 --- a/src/pal/src/include/pal/environ.h +++ b/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/pal/src/init/pal.cpp b/src/pal/src/init/pal.cpp index 95c6e59..4e6b7d2 100644 --- a/src/pal/src/init/pal.cpp +++ b/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/pal/src/loader/module.cpp b/src/pal/src/loader/module.cpp index d05e272..83d45f3 100644 --- a/src/pal/src/loader/module.cpp +++ b/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/pal/src/map/map.cpp b/src/pal/src/map/map.cpp index f8b30fa..7d4cf92 100644 --- a/src/pal/src/map/map.cpp +++ b/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/pal/src/misc/dbgmsg.cpp b/src/pal/src/misc/dbgmsg.cpp index 7ff0f26..3bd8003 100644 --- a/src/pal/src/misc/dbgmsg.cpp +++ b/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/pal/src/misc/environ.cpp b/src/pal/src/misc/environ.cpp index c2bdb83..3bb4c01 100644 --- a/src/pal/src/misc/environ.cpp +++ b/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 +#endif + #include 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/pal/src/thread/process.cpp b/src/pal/src/thread/process.cpp index 84d1b11..b2f3703 100644 --- a/src/pal/src/thread/process.cpp +++ b/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/pal/src/thread/thread.cpp b/src/pal/src/thread/thread.cpp index 3ba97ad..0bef811 100644 --- a/src/pal/src/thread/thread.cpp +++ b/src/pal/src/thread/thread.cpp @@ -1690,6 +1690,8 @@ CorUnix::InitializeGlobalThreadData( { CPalThread::s_dwDefaultThreadStackSize = dw; } + + InternalFree(pszStackSize); } #if !HAVE_MACH_EXCEPTIONS -- 2.7.4