From 404c73d924bd999df383a5ed46ca6d65d9b57131 Mon Sep 17 00:00:00 2001 From: Aditya Mandaleeka Date: Mon, 8 Feb 2016 16:14:57 -0800 Subject: [PATCH 1/6] Consolidate environment code in environ.cpp. --- src/pal/src/cruntime/misc.cpp | 364 --------------------- src/pal/src/debug/debug.cpp | 10 +- src/pal/src/include/pal/environ.h | 78 +++++ src/pal/src/include/pal/misc.h | 54 ---- src/pal/src/init/pal.cpp | 9 +- src/pal/src/misc/dbgmsg.cpp | 10 +- src/pal/src/misc/environ.cpp | 506 +++++++++++++++++++++++++----- src/pal/src/thread/process.cpp | 6 +- src/pal/src/thread/thread.cpp | 4 +- 9 files changed, 529 insertions(+), 512 deletions(-) create mode 100644 src/pal/src/include/pal/environ.h diff --git a/src/pal/src/cruntime/misc.cpp b/src/pal/src/cruntime/misc.cpp index 107d5e316d1a..d26734177ace 100644 --- a/src/pal/src/cruntime/misc.cpp +++ b/src/pal/src/cruntime/misc.cpp @@ -20,7 +20,6 @@ Module Name: #include "pal/thread.hpp" #include "pal/threadsusp.hpp" -#include "pal/malloc.hpp" #include "pal/palinternal.h" #include "pal/dbgmsg.h" #include "pal/misc.h" @@ -43,10 +42,6 @@ Module Name: SET_DEFAULT_DEBUG_CHANNEL(CRT); -char **palEnvironment = NULL; - -CRITICAL_SECTION gcsEnvironment; - using namespace CorUnix; /*++ @@ -138,65 +133,6 @@ int * __cdecl PAL_errno( int caller ) return retval; } -/*++ - -Function : _putenv. - -See MSDN for more details. - -Note: The BSD implementation can cause - memory leaks. See man pages for more details. ---*/ -int -__cdecl -_putenv( const char * envstring ) -{ - int ret = -1; - - PERF_ENTRY(_putenv); - ENTRY( "_putenv( %p (%s) )\n", envstring ? envstring : "NULL", envstring ? envstring : "NULL") ; - - if (!envstring) - { - ERROR( "_putenv() called with NULL envstring!\n"); - goto EXIT; - } - - ret = MiscPutenv(envstring, TRUE) ? 0 : -1; - -EXIT: - LOGEXIT( "_putenv returning %d\n", ret); - PERF_EXIT(_putenv); - return ret; -} - -/*++ - -Function : PAL_getenv - -See MSDN for more details. ---*/ -char * __cdecl PAL_getenv(const char *varname) -{ - char *retval; - - PERF_ENTRY(getenv); - ENTRY("getenv (%p (%s))\n", varname ? varname : "NULL", varname ? varname : "NULL"); - - if (strcmp(varname, "") == 0) - { - ERROR("getenv called with a empty variable name\n"); - LOGEXIT("getenv returning NULL\n"); - PERF_EXIT(getenv); - return(NULL); - } - retval = MiscGetenv(varname); - - LOGEXIT("getenv returning %p\n", retval); - PERF_EXIT(getenv); - return(retval); -} - /*++ Function: @@ -361,306 +297,6 @@ void PAL__mm_setcsr(unsigned int i) #endif // _AMD64_ -/*++ -Function: - MiscGetEnvArray - -Get a reference to the process's environ array into palEnvironment - -NOTE: This function MUST be called while holding the gcsEnvironment - critical section (except if the caller is the initialization - routine) ---*/ -void -MiscGetEnvArray(void) -{ -#if HAVE__NSGETENVIRON - palEnvironment = *(_NSGetEnviron()); -#else // HAVE__NSGETENVIRON - extern char **environ; - palEnvironment = environ; -#endif // HAVE__NSGETENVIRON -} - -/*++ -Function: - MiscSetEnvArray - -Make sure the process's environ array is in sync with palEnvironment variable - -NOTE: This function MUST be called while holding the gcsEnvironment - critical section (except if the caller is the initialization - routine) ---*/ -void -MiscSetEnvArray(void) -{ -#if HAVE__NSGETENVIRON - *(_NSGetEnviron()) = palEnvironment; -#else // HAVE__NSGETENVIRON - extern char **environ; - environ = palEnvironment; -#endif // HAVE__NSGETENVIRON -} - -/*++ -Function: - MiscInitialize - -Initialization function called from PAL_Initialize. -Allocates the TLS Index. On systems that use extern variables for -time zone information, this also initializes those variables. - -Note: This is called before debug channels are initialized, so it - cannot use debug tracing calls. ---*/ -BOOL -MiscInitialize(void) -{ - InternalInitializeCriticalSection(&gcsEnvironment); - MiscGetEnvArray(); - - return TRUE; -} - -/*++ -Function: - MiscCleanup - -Termination function called from PAL_Terminate to delete the -TLS Keys created in MiscInitialize ---*/ -void MiscCleanup(void) -{ - TRACE("Cleaning Misc...\n"); - InternalDeleteCriticalSection(&gcsEnvironment); -} - -/*++ -Function: - MiscGetenv - -Gets an environment variable's value from environ. The returned buffer -must not be modified or freed. ---*/ -char *MiscGetenv(const char *name) -{ - int i, length; - char *equals; - char *pRet = NULL; - CPalThread * pthrCurrent = InternalGetCurrentThread(); - - InternalEnterCriticalSection(pthrCurrent, &gcsEnvironment); - - length = strlen(name); - for(i = 0; palEnvironment[i] != NULL; i++) - { - if (memcmp(palEnvironment[i], name, length) == 0) - { - equals = palEnvironment[i] + length; - if (*equals == '\0') - { - pRet = (char *) ""; - goto done; - } - else if (*equals == '=') - { - pRet = equals + 1; - goto done; - } - } - } - -done: - InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment); - return pRet; -} - - -/*++ -Function: - MiscPutenv - -Sets an environment variable's value by directly modifying palEnvironment. -Returns TRUE if the variable was set, or FALSE if PAL_malloc or realloc -failed or if the given string is malformed. ---*/ -BOOL MiscPutenv(const char *string, BOOL deleteIfEmpty) -{ - const char *equals, *existingEquals; - char *copy = NULL; - int length; - int i, j; - bool fOwningCS = false; - BOOL result = FALSE; - CPalThread * pthrCurrent = InternalGetCurrentThread(); - - equals = strchr(string, '='); - if (equals == string || equals == NULL) - { - // "=foo" and "foo" have no meaning - goto done; - } - if (equals[1] == '\0' && deleteIfEmpty) - { - // "foo=" removes foo from the environment in _putenv() on Windows. - // The same string can result from a call to SetEnvironmentVariable() - // with the empty string as the value, but in that case we want to - // set the variable's value to "". deleteIfEmpty will be FALSE in - // that case. - length = strlen(string); - copy = (char *) InternalMalloc(length); - if (copy == NULL) - { - goto done; - } - memcpy(copy, string, length - 1); - copy[length - 1] = '\0'; // Change '=' to '\0' - MiscUnsetenv(copy); - result = TRUE; - } - else - { - // See if we are replacing an item or adding one. - - // Make our copy up front, since we'll use it either way. - copy = strdup(string); - if (copy == NULL) - { - goto done; - } - - length = equals - string; - - InternalEnterCriticalSection(pthrCurrent, &gcsEnvironment); - fOwningCS = true; - - for(i = 0; palEnvironment[i] != NULL; i++) - { - existingEquals = strchr(palEnvironment[i], '='); - if (existingEquals == NULL) - { - // The PAL screens out malformed strings, but - // environ comes from the system, so it might - // have strings without '='. We treat the entire - // string as a name in that case. - existingEquals = palEnvironment[i] + strlen(palEnvironment[i]); - } - if (existingEquals - palEnvironment[i] == length) - { - if (memcmp(string, palEnvironment[i], length) == 0) - { - // Replace this one. Don't free the original, - // though, because there may be outstanding - // references to it that were acquired via - // getenv. This is an unavoidable memory leak. - palEnvironment[i] = copy; - - // Set 'copy' to NULL so it won't be freed - copy = NULL; - - result = TRUE; - break; - } - } - } - if (palEnvironment[i] == NULL) - { - static BOOL sAllocatedEnviron = FALSE; - // Add a new environment variable. - // We'd like to realloc palEnvironment, but we can't do that the - // first time through. - char **newEnviron = NULL; - - if (sAllocatedEnviron) { - if (NULL == (newEnviron = - (char **)InternalRealloc(palEnvironment, (i + 2) * sizeof(char *)))) - { - goto done; - } - } - else - { - // Allocate palEnvironment ourselves so we can realloc it later. - newEnviron = (char **)InternalMalloc((i + 2) * sizeof(char *)); - if (newEnviron == NULL) - { - goto done; - } - for(j = 0; palEnvironment[j] != NULL; j++) - { - newEnviron[j] = palEnvironment[j]; - } - sAllocatedEnviron = TRUE; - } - palEnvironment = newEnviron; - MiscSetEnvArray(); - palEnvironment[i] = copy; - palEnvironment[i + 1] = NULL; - - // Set 'copy' to NULL so it won't be freed - copy = NULL; - - result = TRUE; - } - } -done: - - if (fOwningCS) - { - InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment); - } - if (NULL != copy) - { - InternalFree(copy); - } - return result; -} - -/*++ -Function: - MiscUnsetenv - -Removes a variable from the environment. Does nothing if the variable -does not exist in the environment. ---*/ -void MiscUnsetenv(const char *name) -{ - const char *equals; - int length; - int i, j; - CPalThread * pthrCurrent = InternalGetCurrentThread(); - - length = strlen(name); - - InternalEnterCriticalSection(pthrCurrent, &gcsEnvironment); - for(i = 0; palEnvironment[i] != NULL; i++) - { - equals = strchr(palEnvironment[i], '='); - if (equals == NULL) - { - equals = palEnvironment[i] + strlen(palEnvironment[i]); - } - if (equals - palEnvironment[i] == length) - { - if (memcmp(name, palEnvironment[i], length) == 0) - { - // Remove this one. Don't free it, though, since - // there might be oustanding references to it that - // were acquired via getenv. This is an - // unavoidable memory leak. - for(j = i + 1; palEnvironment[j] != NULL; j++) { } - // i is now the one we want to remove. j is the - // last index in palEnvironment, which is NULL. - - // Shift palEnvironment down by the difference between i and j. - memmove(palEnvironment + i, palEnvironment + i + 1, (j - i) * sizeof(char *)); - } - } - } - InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment); -} - #if defined(_DEBUG) /*++ diff --git a/src/pal/src/debug/debug.cpp b/src/pal/src/debug/debug.cpp index dd4ea277dfad..c71bf591dc67 100644 --- a/src/pal/src/debug/debug.cpp +++ b/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" @@ -174,15 +174,15 @@ 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))) + (NULL != EnvironGetenv(PAL_OUTPUTDEBUGSTRING))) { fprintf(stderr, "%s", lpOutputString); } - + LOGEXIT("OutputDebugStringA returns\n"); PERF_EXIT(OutputDebugStringA); } @@ -370,7 +370,7 @@ 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)) { diff --git a/src/pal/src/include/pal/environ.h b/src/pal/src/include/pal/environ.h new file mode 100644 index 000000000000..00082e7574a8 --- /dev/null +++ b/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 + +--*/ +BOOL EnvironInitialize();////////////// + +/*++ +Function: + EnvironGetenv + +Gets an environment variable's value. +--*/ +char *EnvironGetenv(const char *name); + +/*++ +Function: + EnvironPutenv + +Sets an environment variable's value. +Returns TRUE if the variable was set, or FALSE if malloc or realloc +failed or if the given string is malformed. +--*/ +BOOL EnvironPutenv(const char *string, BOOL deleteIfEmpty); + +/*++ +Function: + EnvironUnsetenv + +Removes a variable from the environment. Does nothing if the variable +does not exist in the environment. +--*/ +void EnvironUnsetenv(const char *name); + +#ifdef __cplusplus +} +#endif // __cplusplus + +#endif /* __ENVIRON_H_ */ + diff --git a/src/pal/src/include/pal/misc.h b/src/pal/src/include/pal/misc.h index 2be2cc06b285..65d59aee60bb 100644 --- a/src/pal/src/include/pal/misc.h +++ b/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 diff --git a/src/pal/src/init/pal.cpp b/src/pal/src/init/pal.cpp index b2efdf2779bb..95c6e59e2656 100644 --- a/src/pal/src/init/pal.cpp +++ b/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,7 +1183,7 @@ 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"); @@ -1327,7 +1328,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")); last_resort: /* last resort : see if the executable is in the current directory. This is diff --git a/src/pal/src/misc/dbgmsg.cpp b/src/pal/src/misc/dbgmsg.cpp index f03db0a2905f..7ff0f26a78fb 100644 --- a/src/pal/src/misc/dbgmsg.cpp +++ b/src/pal/src/misc/dbgmsg.cpp @@ -29,7 +29,7 @@ Module Name: #include "pal/cruntime.h" #include "pal/critsect.h" #include "pal/file.h" -#include "pal/misc.h" +#include "pal/environ.h" /* standard headers */ @@ -168,7 +168,7 @@ BOOL DBG_init_channels(void) /* parse PAL_DBG_CHANNELS environment variable */ - if (!(env_string = MiscGetenv(ENV_CHANNELS))) + if (!(env_string = EnvironGetenv(ENV_CHANNELS))) { env_pcache = env_workstring = NULL; } @@ -316,7 +316,7 @@ BOOL DBG_init_channels(void) PAL_free(env_pcache); /* select output file */ - env_string=MiscGetenv(ENV_FILE); + env_string = EnvironGetenv(ENV_FILE); if(env_string && *env_string!='\0') { if(!strcmp(env_string, "stderr")) @@ -347,7 +347,7 @@ BOOL DBG_init_channels(void) } /* see if we need to disable assertions */ - env_string = MiscGetenv(ENV_ASSERTS); + env_string = EnvironGetenv(ENV_ASSERTS); if(env_string && 0 == strcmp(env_string,"1")) { g_Dbg_asserts_enabled = FALSE; @@ -358,7 +358,7 @@ BOOL DBG_init_channels(void) } /* select ENTRY level limitation */ - env_string = MiscGetenv(ENV_ENTRY_LEVELS); + env_string = EnvironGetenv(ENV_ENTRY_LEVELS); if(env_string) { max_entry_level = atoi(env_string); diff --git a/src/pal/src/misc/environ.cpp b/src/pal/src/misc/environ.cpp index e715db04658a..4cab4641b2a2 100644 --- a/src/pal/src/misc/environ.cpp +++ b/src/pal/src/misc/environ.cpp @@ -8,7 +8,7 @@ Module Name: - environ.c + environ.cpp Abstract: @@ -23,12 +23,18 @@ Revision History: #include "pal/palinternal.h" #include "pal/critsect.h" #include "pal/dbgmsg.h" -#include "pal/misc.h" +#include "pal/environ.h" +#include "pal/malloc.hpp" #include +using namespace CorUnix; + SET_DEFAULT_DEBUG_CHANNEL(MISC); +char **palEnvironment = nullptr; +CRITICAL_SECTION gcsEnvironment; + /*++ Function: GetEnvironmentVariableA @@ -73,34 +79,34 @@ GetEnvironmentVariableA( PERF_ENTRY(GetEnvironmentVariableA); ENTRY("GetEnvironmentVariableA(lpName=%p (%s), lpBuffer=%p, nSize=%u)\n", - lpName?lpName:"NULL", - lpName?lpName:"NULL", lpBuffer, nSize); + lpName ? lpName : "NULL", + lpName ? lpName : "NULL", lpBuffer, nSize); - if (lpName == NULL) + if (lpName == nullptr) { - ERROR("lpName is NULL\n"); + ERROR("lpName is null\n"); SetLastError(ERROR_INVALID_PARAMETER); goto done; } if (lpName[0] == 0) { - TRACE("lpName is empty string\n", lpName); + TRACE("lpName is an empty string\n", lpName); SetLastError(ERROR_ENVVAR_NOT_FOUND); goto done; } - - if (strchr(lpName, '=') != NULL) + + if (strchr(lpName, '=') != nullptr) { // GetEnvironmentVariable doesn't permit '=' in variable names. - value = NULL; + value = nullptr; } else { - value = MiscGetenv(lpName); + value = EnvironGetenv(lpName); ///////// make this not return a copy, or have it fill out the buffer } - - if (value == NULL) + + if (value == nullptr) { TRACE("%s is not found\n", lpName); SetLastError(ERROR_ENVVAR_NOT_FOUND); @@ -111,11 +117,12 @@ GetEnvironmentVariableA( { strcpy_s(lpBuffer, nSize, value); dwRet = strlen(value); - } + } else { dwRet = strlen(value)+1; } + SetLastError(ERROR_SUCCESS); done: @@ -138,36 +145,37 @@ GetEnvironmentVariableW( OUT LPWSTR lpBuffer, IN DWORD nSize) { - CHAR *inBuff = NULL; - CHAR *outBuff = NULL; + CHAR *inBuff = nullptr; + CHAR *outBuff = nullptr; INT inBuffSize; DWORD size = 0; PERF_ENTRY(GetEnvironmentVariableW); ENTRY("GetEnvironmentVariableW(lpName=%p (%S), lpBuffer=%p, nSize=%u)\n", - lpName?lpName:W16_NULLSTRING, - lpName?lpName:W16_NULLSTRING, lpBuffer, nSize); + lpName ? lpName : W16_NULLSTRING, + lpName ? lpName : W16_NULLSTRING, lpBuffer, nSize); - inBuffSize = WideCharToMultiByte( CP_ACP, 0, lpName, -1, - inBuff, 0, NULL, NULL); - if ( 0 == inBuffSize ) + inBuffSize = WideCharToMultiByte(CP_ACP, 0, lpName, -1, + inBuff, 0, nullptr, nullptr); + if (0 == inBuffSize) { - ERROR( "lpName has to be a valid parameter\n" ); - SetLastError( ERROR_INVALID_PARAMETER ); + ERROR("lpName has to be a valid parameter\n"); + SetLastError(ERROR_INVALID_PARAMETER); goto done; } inBuff = (CHAR *)PAL_malloc(inBuffSize); - if (inBuff == NULL) + if (inBuff == nullptr) { ERROR("malloc failed\n"); SetLastError(ERROR_NOT_ENOUGH_MEMORY); goto done; } - - if (nSize) { + + if (nSize) + { outBuff = (CHAR *)PAL_malloc(nSize*2); - if (outBuff == NULL) + if (outBuff == nullptr) { ERROR("malloc failed\n"); SetLastError(ERROR_NOT_ENOUGH_MEMORY); @@ -175,34 +183,35 @@ GetEnvironmentVariableW( } } - if ( 0 == WideCharToMultiByte( CP_ACP, 0, lpName, -1, inBuff, - inBuffSize, NULL, NULL ) ) + if (0 == WideCharToMultiByte(CP_ACP, 0, lpName, -1, inBuff, + inBuffSize, NULL, NULL)) { - ASSERT( "WideCharToMultiByte failed!\n" ); - SetLastError( ERROR_INTERNAL_ERROR ); + ASSERT("WideCharToMultiByte failed!\n"); + SetLastError(ERROR_INTERNAL_ERROR); goto done; } + size = GetEnvironmentVariableA(inBuff, outBuff, nSize); if (size > nSize) { TRACE("Insufficient buffer\n"); } - else if ( size == 0 ) + else if (size == 0) { /* error handle in GetEnvironmentVariableA */ } else { size = MultiByteToWideChar(CP_ACP, 0, outBuff, -1, lpBuffer, nSize); - if ( 0 != size ) + if (0 != size) { /* Not including the NULL. */ size--; } else { - ASSERT( "MultiByteToWideChar failed!\n" ); - SetLastError( ERROR_INTERNAL_ERROR ); + ASSERT("MultiByteToWideChar failed!\n"); + SetLastError(ERROR_INTERNAL_ERROR); size = 0; *lpBuffer = '\0'; } @@ -214,6 +223,7 @@ GetEnvironmentVariableW( LOGEXIT("GetEnvironmentVariableW returns DWORD 0x%x\n", size); PERF_EXIT(GetEnvironmentVariableW); + return size; } @@ -257,8 +267,8 @@ SetEnvironmentVariableW( IN LPCWSTR lpName, IN LPCWSTR lpValue) { - PCHAR name = NULL; - PCHAR value = NULL; + PCHAR name = nullptr; + PCHAR value = nullptr; INT nameSize = 0; INT valueSize = 0; BOOL bRet = FALSE; @@ -269,7 +279,7 @@ SetEnvironmentVariableW( lpName?lpName:W16_NULLSTRING, lpValue?lpValue:W16_NULLSTRING, lpValue?lpValue:W16_NULLSTRING); if ((nameSize = WideCharToMultiByte(CP_ACP, 0, lpName, -1, name, 0, - NULL, NULL)) == 0) + nullptr, nullptr)) == 0) { ERROR("WideCharToMultiByte failed\n"); SetLastError(ERROR_INVALID_PARAMETER); @@ -277,22 +287,22 @@ SetEnvironmentVariableW( } name = (PCHAR)PAL_malloc(sizeof(CHAR)* nameSize); - if (name == NULL) + if (name == nullptr) { ERROR("malloc failed\n"); SetLastError(ERROR_NOT_ENOUGH_MEMORY); goto done; } - if ( 0 == WideCharToMultiByte(CP_ACP, 0, lpName, -1, - name, nameSize, NULL, NULL ) ) + if (0 == WideCharToMultiByte(CP_ACP, 0, lpName, -1, + name, nameSize, nullptr, nullptr)) { - ASSERT( "WideCharToMultiByte returned 0\n" ); - SetLastError( ERROR_INTERNAL_ERROR ); + ASSERT("WideCharToMultiByte returned 0\n"); + SetLastError(ERROR_INTERNAL_ERROR); goto done; } - if ( NULL != lpValue ) + if (lpValue != nullptr) { if ((valueSize = WideCharToMultiByte(CP_ACP, 0, lpValue, -1, value, 0, NULL, NULL)) == 0) @@ -303,29 +313,28 @@ SetEnvironmentVariableW( } value = (PCHAR)PAL_malloc(sizeof(CHAR)*valueSize); - - if ( NULL == value ) + + if (value == nullptr) { ERROR("malloc failed\n"); SetLastError(ERROR_NOT_ENOUGH_MEMORY); goto done; } - - if ( 0 == WideCharToMultiByte( CP_ACP, 0, lpValue, -1, - value, valueSize, NULL, NULL ) ) + + if (0 == WideCharToMultiByte(CP_ACP, 0, lpValue, -1, + value, valueSize, NULL, NULL)) { ASSERT("WideCharToMultiByte failed\n"); SetLastError( ERROR_INTERNAL_ERROR ); goto done; } } - bRet = SetEnvironmentVariableA(name, value); done: PAL_free(value); PAL_free(name); - + LOGEXIT("SetEnvironmentVariableW returning BOOL %d\n", bRet); PERF_EXIT(SetEnvironmentVariableW); return bRet; @@ -363,13 +372,13 @@ PALAPI GetEnvironmentStringsW( VOID) { - WCHAR *wenviron = NULL, *tempEnviron; + WCHAR *wenviron = nullptr, *tempEnviron; int i, len, envNum; PERF_ENTRY(GetEnvironmentStringsW); ENTRY("GetEnvironmentStringsW()\n"); - PALCEnterCriticalSection(&gcsEnvironment); + PALCEnterCriticalSection(&gcsEnvironment); envNum = 0; len = 0; @@ -427,7 +436,7 @@ GetEnvironmentStringsA( PERF_ENTRY(GetEnvironmentStringsA); ENTRY("GetEnvironmentStringsA()\n"); - PALCEnterCriticalSection(&gcsEnvironment); + PALCEnterCriticalSection(&gcsEnvironment); envNum = 0; len = 0; @@ -574,59 +583,60 @@ environment variables of other processes. BOOL PALAPI SetEnvironmentVariableA( - IN LPCSTR lpName, - IN LPCSTR lpValue) + IN LPCSTR lpName, + IN LPCSTR lpValue) { BOOL bRet = FALSE; int nResult =0; PERF_ENTRY(SetEnvironmentVariableA); ENTRY("SetEnvironmentVariableA(lpName=%p (%s), lpValue=%p (%s))\n", - lpName?lpName:"NULL", - lpName?lpName:"NULL", lpValue?lpValue:"NULL", lpValue?lpValue:"NULL"); + lpName ? lpName : "NULL", lpName ? lpName : "NULL", + lpValue ? lpValue : "NULL", lpValue ? lpValue : "NULL"); - /*check if the input variable name is null - * and if so exit*/ - if ((lpName == NULL) || (lpName[0] == 0)) + // exit if the input variable name is null + if ((lpName == nullptr) || (lpName[0] == 0)) { ERROR("lpName is NULL\n"); goto done; } - /*check if the input value is null and if so + + /* check if the input value is null and if so * check if the input name is valid and delete - * the variable name from process environment*/ - if (lpValue == NULL) + * the variable name from process environment */ + if (lpValue == nullptr) { - if ((lpValue = MiscGetenv(lpName)) == NULL) + if ((lpValue = EnvironGetenv(lpName)) == nullptr) { ERROR("Couldn't find environment variable (%s)\n", lpName); SetLastError(ERROR_ENVVAR_NOT_FOUND); goto done; } - MiscUnsetenv(lpName); + + EnvironUnsetenv(lpName); } - /*All the conditions are met. Set the variable*/ else { + // All the conditions are met. Set the variable. int iLen = strlen(lpName) + strlen(lpValue) + 2; LPSTR string = (LPSTR) PAL_malloc(iLen); - if (string == NULL) + if (string == nullptr) { bRet = FALSE; ERROR("Unable to allocate memory\n"); SetLastError(ERROR_NOT_ENOUGH_MEMORY); goto done; } - + sprintf_s(string, iLen, "%s=%s", lpName, lpValue); - nResult = MiscPutenv(string, FALSE) ? 0 : -1; + nResult = EnvironPutenv(string, FALSE) ? 0 : -1; PAL_free(string); string = NULL; - - // If MiscPutenv returns FALSE, it almost certainly failed to - // allocate memory. - if(nResult == -1) + + // If EnvironPutenv returns FALSE, it almost certainly failed to + // allocate memory. ////////////////////////////still true? + if (nResult == -1) { bRet = FALSE; ERROR("Unable to allocate memory\n"); @@ -634,11 +644,357 @@ SetEnvironmentVariableA( goto done; } } + bRet = TRUE; + done: - LOGEXIT("SetEnvironmentVariableA returning BOOL %d\n", bRet); + LOGEXIT("SetEnvironmentVariableA returning BOOL %d\n", bRet); PERF_EXIT(SetEnvironmentVariableA); return bRet; } +/*++ +Function: + MiscGetEnvArray + +Get a reference to the process's environ array into palEnvironment + +NOTE: This function MUST be called while holding the gcsEnvironment + critical section (except if the caller is the initialization + routine) +--*/ +void +MiscGetEnvArray(void) +{ +#if HAVE__NSGETENVIRON + palEnvironment = *(_NSGetEnviron()); +#else // HAVE__NSGETENVIRON + extern char **environ; + palEnvironment = environ; +#endif // HAVE__NSGETENVIRON +} + +/*++ +Function: + MiscSetEnvArray + +Make sure the process's environ array is in sync with palEnvironment variable + +NOTE: This function MUST be called while holding the gcsEnvironment + critical section (except if the caller is the initialization + routine) +--*/ +void +MiscSetEnvArray(void) +{ +#if HAVE__NSGETENVIRON + *(_NSGetEnviron()) = palEnvironment; +#else // HAVE__NSGETENVIRON + extern char **environ; + environ = palEnvironment; +#endif // HAVE__NSGETENVIRON +} + + +/*++ +Function: + EnvironInitialize + +Initialization function called from PAL_Initialize. + +Note: This is called before debug channels are initialized, so it + cannot use debug tracing calls. +--*/ +BOOL +EnvironInitialize(void) +{ + InternalInitializeCriticalSection(&gcsEnvironment); + MiscGetEnvArray(); + + return TRUE; +} + +/*++ + +Function : _putenv. + +See MSDN for more details. + +Note: The BSD implementation can cause + memory leaks. See man pages for more details. +--*/ +int +__cdecl +_putenv( const char * envstring ) +{ + int ret = -1; + + PERF_ENTRY(_putenv); + ENTRY( "_putenv( %p (%s) )\n", envstring ? envstring : "NULL", envstring ? envstring : "NULL") ; + + if (!envstring) + { + ERROR( "_putenv() called with NULL envstring!\n"); + goto EXIT; + } + + ret = EnvironPutenv(envstring, TRUE) ? 0 : -1; + +EXIT: + LOGEXIT( "_putenv returning %d\n", ret); + PERF_EXIT(_putenv); + return ret; +} + +/*++ + +Function : PAL_getenv + +See MSDN for more details. +--*/ +char * __cdecl PAL_getenv(const char *varname) +{ + char *retval; + + PERF_ENTRY(getenv); + ENTRY("getenv (%p (%s))\n", varname ? varname : "NULL", varname ? varname : "NULL"); + + if (strcmp(varname, "") == 0) + { + ERROR("getenv called with a empty variable name\n"); + LOGEXIT("getenv returning NULL\n"); + PERF_EXIT(getenv); + return(NULL); + } + retval = EnvironGetenv(varname); + + LOGEXIT("getenv returning %p\n", retval); + PERF_EXIT(getenv); + return(retval); +} + + +/*++ +Function: + EnvironGetenv + +Gets an environment variable's value from environ. The returned buffer +must not be modified or freed. +--*/ +char *EnvironGetenv(const char *name) +{ + int i, length; + char *equals; + char *pRet = NULL; + CPalThread * pthrCurrent = InternalGetCurrentThread(); + + InternalEnterCriticalSection(pthrCurrent, &gcsEnvironment); + + length = strlen(name); + for(i = 0; palEnvironment[i] != NULL; i++) + { + if (memcmp(palEnvironment[i], name, length) == 0) + { + equals = palEnvironment[i] + length; + if (*equals == '\0') + { + pRet = (char *) ""; + goto done; + } + else if (*equals == '=') + { + pRet = equals + 1; + goto done; + } + } + } + +done: + InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment); + return pRet; +} + +/*++ +Function: + EnvironPutenv + +Sets an environment variable's value by directly modifying palEnvironment. +Returns TRUE if the variable was set, or FALSE if PAL_malloc or realloc +failed or if the given string is malformed. +--*/ +BOOL EnvironPutenv(const char *string, BOOL deleteIfEmpty) +{ + const char *equals, *existingEquals; + char *copy = NULL; + int length; + int i, j; + bool fOwningCS = false; + BOOL result = FALSE; + CPalThread * pthrCurrent = InternalGetCurrentThread(); + + equals = strchr(string, '='); + if (equals == string || equals == NULL) + { + // "=foo" and "foo" have no meaning + goto done; + } + if (equals[1] == '\0' && deleteIfEmpty) + { + // "foo=" removes foo from the environment in _putenv() on Windows. + // The same string can result from a call to SetEnvironmentVariable() + // with the empty string as the value, but in that case we want to + // set the variable's value to "". deleteIfEmpty will be FALSE in + // that case. + length = strlen(string); + copy = (char *) InternalMalloc(length); + if (copy == NULL) + { + goto done; + } + memcpy(copy, string, length - 1); + copy[length - 1] = '\0'; // Change '=' to '\0' + EnvironUnsetenv(copy); + result = TRUE; + } + else + { + // See if we are replacing an item or adding one. + + // Make our copy up front, since we'll use it either way. + copy = strdup(string); + if (copy == NULL) + { + goto done; + } + + length = equals - string; + + InternalEnterCriticalSection(pthrCurrent, &gcsEnvironment); + fOwningCS = true; + + for(i = 0; palEnvironment[i] != NULL; i++) + { + existingEquals = strchr(palEnvironment[i], '='); + if (existingEquals == NULL) + { + // The PAL screens out malformed strings, but + // environ comes from the system, so it might + // have strings without '='. We treat the entire + // string as a name in that case. + existingEquals = palEnvironment[i] + strlen(palEnvironment[i]); + } + if (existingEquals - palEnvironment[i] == length) + { + if (memcmp(string, palEnvironment[i], length) == 0) + { + // Replace this one. Don't free the original, + // though, because there may be outstanding + // references to it that were acquired via + // getenv. This is an unavoidable memory leak. + palEnvironment[i] = copy; + + // Set 'copy' to NULL so it won't be freed + copy = NULL; + + result = TRUE; + break; + } + } + } + if (palEnvironment[i] == NULL) + { + static BOOL sAllocatedEnviron = FALSE; + // Add a new environment variable. + // We'd like to realloc palEnvironment, but we can't do that the + // first time through. + char **newEnviron = NULL; + + if (sAllocatedEnviron) { + if (NULL == (newEnviron = + (char **)InternalRealloc(palEnvironment, (i + 2) * sizeof(char *)))) + { + goto done; + } + } + else + { + // Allocate palEnvironment ourselves so we can realloc it later. + newEnviron = (char **)InternalMalloc((i + 2) * sizeof(char *)); + if (newEnviron == NULL) + { + goto done; + } + for(j = 0; palEnvironment[j] != NULL; j++) + { + newEnviron[j] = palEnvironment[j]; + } + sAllocatedEnviron = TRUE; + } + palEnvironment = newEnviron; + MiscSetEnvArray(); + palEnvironment[i] = copy; + palEnvironment[i + 1] = NULL; + + // Set 'copy' to NULL so it won't be freed + copy = NULL; + + result = TRUE; + } + } +done: + + if (fOwningCS) + { + InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment); + } + if (NULL != copy) + { + InternalFree(copy); + } + return result; +} + +/*++ +Function: + EnvironUnsetenv + +Removes a variable from the environment. Does nothing if the variable +does not exist in the environment. +--*/ +void EnvironUnsetenv(const char *name) +{ + const char *equals; + int length; + int i, j; + CPalThread * pthrCurrent = InternalGetCurrentThread(); + + length = strlen(name); + + InternalEnterCriticalSection(pthrCurrent, &gcsEnvironment); + for(i = 0; palEnvironment[i] != NULL; i++) + { + equals = strchr(palEnvironment[i], '='); + if (equals == NULL) + { + equals = palEnvironment[i] + strlen(palEnvironment[i]); + } + if (equals - palEnvironment[i] == length) + { + if (memcmp(name, palEnvironment[i], length) == 0) + { + // Remove this one. Don't free it, though, since + // there might be oustanding references to it that + // were acquired via getenv. This is an + // unavoidable memory leak. + for(j = i + 1; palEnvironment[j] != NULL; j++) { } + // i is now the one we want to remove. j is the + // last index in palEnvironment, which is NULL. + + // Shift palEnvironment down by the difference between i and j. + memmove(palEnvironment + i, palEnvironment + i + 1, (j - i) * sizeof(char *)); + } + } + } + InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment); +} diff --git a/src/pal/src/thread/process.cpp b/src/pal/src/thread/process.cpp index 4d40781ca6cf..84d1b118b3a1 100644 --- a/src/pal/src/thread/process.cpp +++ b/src/pal/src/thread/process.cpp @@ -30,7 +30,7 @@ Module Name: #include "pal/critsect.h" #include "pal/dbgmsg.h" #include "pal/utils.h" -#include "pal/misc.h" +#include "pal/environ.h" #include "pal/virtual.h" #include "pal/stackstring.hpp" @@ -4358,7 +4358,7 @@ getPath( pThread = InternalGetCurrentThread(); /* Then try to look in the path */ - int iLen2 = strlen(MiscGetenv("PATH"))+1; + int iLen2 = strlen(EnvironGetenv("PATH"))+1; lpPath = (LPSTR) InternalMalloc(iLen2); if (!lpPath) @@ -4367,7 +4367,7 @@ getPath( return FALSE; } - if (strcpy_s(lpPath, iLen2, MiscGetenv("PATH")) != SAFECRT_SUCCESS) + if (strcpy_s(lpPath, iLen2, EnvironGetenv("PATH")) != SAFECRT_SUCCESS) { ERROR("strcpy_s failed!"); return FALSE; diff --git a/src/pal/src/thread/thread.cpp b/src/pal/src/thread/thread.cpp index 07f6c8b680bb..3ba97ada610c 100644 --- a/src/pal/src/thread/thread.cpp +++ b/src/pal/src/thread/thread.cpp @@ -30,7 +30,7 @@ Module Name: #include "pal/process.h" #include "pal/module.h" #include "pal/dbgmsg.h" -#include "pal/misc.h" +#include "pal/environ.h" #include "pal/init.h" #include @@ -1680,7 +1680,7 @@ CorUnix::InitializeGlobalThreadData( // Read in the environment to see whether we need to change the default // thread stack size. // - pszStackSize = MiscGetenv(PAL_THREAD_DEFAULT_STACK_SIZE); + pszStackSize = EnvironGetenv(PAL_THREAD_DEFAULT_STACK_SIZE); if (NULL != pszStackSize) { // Environment variable exists From 57826d55c851c21714ce043ab1606aa07c716c21 Mon Sep 17 00:00:00 2001 From: Aditya Mandaleeka Date: Mon, 8 Feb 2016 18:11:15 -0800 Subject: [PATCH 2/6] Modify environment code to avoid unsafe environ usage. --- src/pal/src/include/pal/environ.h | 3 + src/pal/src/misc/environ.cpp | 469 ++++++++++++++---------------- 2 files changed, 225 insertions(+), 247 deletions(-) diff --git a/src/pal/src/include/pal/environ.h b/src/pal/src/include/pal/environ.h index 00082e7574a8..9a310e55368a 100644 --- a/src/pal/src/include/pal/environ.h +++ b/src/pal/src/include/pal/environ.h @@ -33,6 +33,9 @@ Variables : gcsEnvironment: critical section to synchronize access to palEnvironment --*/ extern char **palEnvironment; +extern int palEnvironmentCapacity; +extern int palEnvironmentCount; + extern CRITICAL_SECTION gcsEnvironment; /*++ diff --git a/src/pal/src/misc/environ.cpp b/src/pal/src/misc/environ.cpp index 4cab4641b2a2..2abf401178bd 100644 --- a/src/pal/src/misc/environ.cpp +++ b/src/pal/src/misc/environ.cpp @@ -33,6 +33,9 @@ using namespace CorUnix; SET_DEFAULT_DEBUG_CHANNEL(MISC); char **palEnvironment = nullptr; +int palEnvironmentCount = 0; +int palEnvironmentCapacity = 0; + CRITICAL_SECTION gcsEnvironment; /*++ @@ -184,7 +187,7 @@ GetEnvironmentVariableW( } if (0 == WideCharToMultiByte(CP_ACP, 0, lpName, -1, inBuff, - inBuffSize, NULL, NULL)) + inBuffSize, nullptr, nullptr)) { ASSERT("WideCharToMultiByte failed!\n"); SetLastError(ERROR_INTERNAL_ERROR); @@ -198,14 +201,14 @@ GetEnvironmentVariableW( } else if (size == 0) { - /* error handle in GetEnvironmentVariableA */ + // handle error in GetEnvironmentVariableA } else { size = MultiByteToWideChar(CP_ACP, 0, outBuff, -1, lpBuffer, nSize); if (0 != size) { - /* Not including the NULL. */ + // -1 for the null. size--; } else @@ -241,11 +244,11 @@ lpName [in] Pointer to a null-terminated string that specifies the environment variable whose value is being set. The operating system creates the environment variable if it does not exist - and lpValue is not NULL. + and lpValue is not null. lpValue [in] Pointer to a null-terminated string containing the new value of the specified environment variable. If this parameter - is NULL, the variable is deleted from the current process's + is null, the variable is deleted from the current process's environment. Return Values @@ -305,7 +308,7 @@ SetEnvironmentVariableW( if (lpValue != nullptr) { if ((valueSize = WideCharToMultiByte(CP_ACP, 0, lpValue, -1, value, - 0, NULL, NULL)) == 0) + 0, nullptr, nullptr)) == 0) { ERROR("WideCharToMultiByte failed\n"); SetLastError(ERROR_INVALID_PARAMETER); @@ -322,7 +325,7 @@ SetEnvironmentVariableW( } if (0 == WideCharToMultiByte(CP_ACP, 0, lpValue, -1, - value, valueSize, NULL, NULL)) + value, valueSize, nullptr, nullptr)) { ASSERT("WideCharToMultiByte failed\n"); SetLastError( ERROR_INTERNAL_ERROR ); @@ -391,7 +394,7 @@ GetEnvironmentStringsW( } wenviron = (WCHAR *)PAL_malloc(sizeof(WCHAR)* (envNum + 1)); - if (wenviron == NULL) + if (wenviron == nullptr) { ERROR("malloc failed\n"); SetLastError(ERROR_NOT_ENOUGH_MEMORY); @@ -407,8 +410,8 @@ GetEnvironmentStringsW( envNum -= len; } - *tempEnviron = 0; /* Put an extra NULL at the end */ - + *tempEnviron = 0; /* Put an extra null at the end */ + EXIT: PALCLeaveCriticalSection(&gcsEnvironment); @@ -634,8 +637,7 @@ SetEnvironmentVariableA( PAL_free(string); string = NULL; - // If EnvironPutenv returns FALSE, it almost certainly failed to - // allocate memory. ////////////////////////////still true? + // If EnvironPutenv returns FALSE, it almost certainly failed to allocate memory. if (nResult == -1) { bRet = FALSE; @@ -653,292 +655,202 @@ SetEnvironmentVariableA( return bRet; } - -/*++ -Function: - MiscGetEnvArray - -Get a reference to the process's environ array into palEnvironment - -NOTE: This function MUST be called while holding the gcsEnvironment - critical section (except if the caller is the initialization - routine) ---*/ -void -MiscGetEnvArray(void) +int ResizeEnvironment(int newSize) { -#if HAVE__NSGETENVIRON - palEnvironment = *(_NSGetEnviron()); -#else // HAVE__NSGETENVIRON - extern char **environ; - palEnvironment = environ; -#endif // HAVE__NSGETENVIRON -} - -/*++ -Function: - MiscSetEnvArray + int ret = 0; -Make sure the process's environ array is in sync with palEnvironment variable + CPalThread * pthrCurrent = InternalGetCurrentThread(); + InternalEnterCriticalSection(pthrCurrent, &gcsEnvironment); -NOTE: This function MUST be called while holding the gcsEnvironment - critical section (except if the caller is the initialization - routine) ---*/ -void -MiscSetEnvArray(void) -{ -#if HAVE__NSGETENVIRON - *(_NSGetEnviron()) = palEnvironment; -#else // HAVE__NSGETENVIRON - extern char **environ; - environ = palEnvironment; -#endif // HAVE__NSGETENVIRON -} + if (newSize < palEnvironmentCount) + { + printf("ERROR! new size < current count\n"); + return -1; + } + palEnvironmentCapacity = newSize; -/*++ -Function: - EnvironInitialize + // If palEnvironment is null, realloc acts like malloc. + palEnvironment = (char**)realloc(palEnvironment, palEnvironmentCapacity * sizeof(char *)); -Initialization function called from PAL_Initialize. + if (!palEnvironment) + { + printf("ERROR! realloc failed\n"); + ret = -1; + } -Note: This is called before debug channels are initialized, so it - cannot use debug tracing calls. ---*/ -BOOL -EnvironInitialize(void) -{ - InternalInitializeCriticalSection(&gcsEnvironment); - MiscGetEnvArray(); + InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment); - return TRUE; + return ret; } -/*++ - -Function : _putenv. - -See MSDN for more details. - -Note: The BSD implementation can cause - memory leaks. See man pages for more details. ---*/ -int -__cdecl -_putenv( const char * envstring ) +int InitializePalEnvironment() { - int ret = -1; + char** sourceEnviron; - PERF_ENTRY(_putenv); - ENTRY( "_putenv( %p (%s) )\n", envstring ? envstring : "NULL", envstring ? envstring : "NULL") ; - - if (!envstring) - { - ERROR( "_putenv() called with NULL envstring!\n"); - goto EXIT; - } + CPalThread * pthrCurrent = InternalGetCurrentThread(); + InternalEnterCriticalSection(pthrCurrent, &gcsEnvironment); - ret = EnvironPutenv(envstring, TRUE) ? 0 : -1; +#if HAVE__NSGETENVIRON + sourceEnviron = *(_NSGetEnviron()); +#else // HAVE__NSGETENVIRON + extern char **environ; + sourceEnviron = environ; +#endif // HAVE__NSGETENVIRON -EXIT: - LOGEXIT( "_putenv returning %d\n", ret); - PERF_EXIT(_putenv); - return ret; -} + int variableCount = 0; + while (sourceEnviron[variableCount] != nullptr) + variableCount++; -/*++ + printf("variableCount is %d\n", variableCount); -Function : PAL_getenv - -See MSDN for more details. ---*/ -char * __cdecl PAL_getenv(const char *varname) -{ - char *retval; + palEnvironmentCount = 0; - PERF_ENTRY(getenv); - ENTRY("getenv (%p (%s))\n", varname ? varname : "NULL", varname ? varname : "NULL"); + ResizeEnvironment(variableCount * 2); //// decide resize factor - if (strcmp(varname, "") == 0) + for (int i = 0; i < variableCount; ++i) { - ERROR("getenv called with a empty variable name\n"); - LOGEXIT("getenv returning NULL\n"); - PERF_EXIT(getenv); - return(NULL); + palEnvironment[i] = strdup(sourceEnviron[i]); + palEnvironmentCount++; } - retval = EnvironGetenv(varname); - LOGEXIT("getenv returning %p\n", retval); - PERF_EXIT(getenv); - return(retval); + InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment); + + return 0; } -/*++ -Function: - EnvironGetenv - -Gets an environment variable's value from environ. The returned buffer -must not be modified or freed. ---*/ -char *EnvironGetenv(const char *name) +void EnvironUnsetenv(const char *name) { - int i, length; - char *equals; - char *pRet = NULL; - CPalThread * pthrCurrent = InternalGetCurrentThread(); + int nameLength = strlen(name); + CPalThread * pthrCurrent = InternalGetCurrentThread(); InternalEnterCriticalSection(pthrCurrent, &gcsEnvironment); - length = strlen(name); - for(i = 0; palEnvironment[i] != NULL; i++) + for (int i = 0; palEnvironment[i] != nullptr; ++i) { - if (memcmp(palEnvironment[i], name, length) == 0) + const char *equalsSignPosition = strchr(palEnvironment[i], '='); + if (equalsSignPosition == nullptr) { - equals = palEnvironment[i] + length; - if (*equals == '\0') - { - pRet = (char *) ""; - goto done; - } - else if (*equals == '=') + equalsSignPosition = palEnvironment[i] + strlen(palEnvironment[i]); + } + + // Check whether the name of this variable has the same length as the one + // we're looking for before proceeding to compare them. + if (equalsSignPosition - palEnvironment[i] == nameLength) + { + if (memcmp(name, palEnvironment[i], nameLength) == 0) { - pRet = equals + 1; - goto done; + InternalFree(palEnvironment[i]); + + // Move the last variable here and set it to null. + palEnvironment[i] = palEnvironment[palEnvironmentCount - 1]; + palEnvironment[palEnvironmentCount - 1] = nullptr; + + palEnvironmentCount--; } } } -done: InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment); - return pRet; } -/*++ -Function: - EnvironPutenv - -Sets an environment variable's value by directly modifying palEnvironment. -Returns TRUE if the variable was set, or FALSE if PAL_malloc or realloc -failed or if the given string is malformed. ---*/ -BOOL EnvironPutenv(const char *string, BOOL deleteIfEmpty) +BOOL EnvironPutenv(const char* entry, BOOL deleteIfEmpty) { - const char *equals, *existingEquals; - char *copy = NULL; - int length; - int i, j; bool fOwningCS = false; BOOL result = FALSE; + CPalThread * pthrCurrent = InternalGetCurrentThread(); - - equals = strchr(string, '='); - if (equals == string || equals == NULL) + + const char *equalsSignPosition = strchr(entry, '='); + if (equalsSignPosition == entry || equalsSignPosition == NULL) { // "=foo" and "foo" have no meaning goto done; } - if (equals[1] == '\0' && deleteIfEmpty) + + if (equalsSignPosition[1] == '\0' && deleteIfEmpty) { // "foo=" removes foo from the environment in _putenv() on Windows. // The same string can result from a call to SetEnvironmentVariable() // with the empty string as the value, but in that case we want to // set the variable's value to "". deleteIfEmpty will be FALSE in // that case. - length = strlen(string); - copy = (char *) InternalMalloc(length); - if (copy == NULL) + int length = strlen(entry); + char* copy = (char *) InternalMalloc(length); + if (copy == nullptr) { goto done; } - memcpy(copy, string, length - 1); - copy[length - 1] = '\0'; // Change '=' to '\0' + + memcpy(copy, entry, length - 1); + + // Change '=' to '\0' + copy[length - 1] = '\0'; + EnvironUnsetenv(copy); result = TRUE; } else { // See if we are replacing an item or adding one. - + // Make our copy up front, since we'll use it either way. - copy = strdup(string); - if (copy == NULL) + char* copy = strdup(entry); + if (copy == nullptr) { goto done; } - - length = equals - string; + + int nameLength = equalsSignPosition - entry; InternalEnterCriticalSection(pthrCurrent, &gcsEnvironment); fOwningCS = true; - - for(i = 0; palEnvironment[i] != NULL; i++) + + int i; + for (i = 0; palEnvironment[i] != nullptr; i++) { - existingEquals = strchr(palEnvironment[i], '='); - if (existingEquals == NULL) + const char *existingEquals = strchr(palEnvironment[i], '='); + if (existingEquals == nullptr) { - // The PAL screens out malformed strings, but - // environ comes from the system, so it might - // have strings without '='. We treat the entire - // string as a name in that case. + // The PAL screens out malformed strings, but the strings which + // came from the system during initialization might not have the + // equals sign. We treat the entire string as a name in that case. existingEquals = palEnvironment[i] + strlen(palEnvironment[i]); } - if (existingEquals - palEnvironment[i] == length) + + if (existingEquals - palEnvironment[i] == nameLength) { - if (memcmp(string, palEnvironment[i], length) == 0) + if (memcmp(entry, palEnvironment[i], nameLength) == 0) { - // Replace this one. Don't free the original, - // though, because there may be outstanding - // references to it that were acquired via - // getenv. This is an unavoidable memory leak. + InternalFree(palEnvironment[i]); palEnvironment[i] = copy; - - // Set 'copy' to NULL so it won't be freed - copy = NULL; - + result = TRUE; break; } } } - if (palEnvironment[i] == NULL) - { - static BOOL sAllocatedEnviron = FALSE; - // Add a new environment variable. - // We'd like to realloc palEnvironment, but we can't do that the - // first time through. - char **newEnviron = NULL; - - if (sAllocatedEnviron) { - if (NULL == (newEnviron = - (char **)InternalRealloc(palEnvironment, (i + 2) * sizeof(char *)))) - { - goto done; - } - } - else + + if (palEnvironment[i] == nullptr) + { + // ASSERT(i <= palEnvironmentCapacity) + if (i == palEnvironmentCapacity) { - // Allocate palEnvironment ourselves so we can realloc it later. - newEnviron = (char **)InternalMalloc((i + 2) * sizeof(char *)); - if (newEnviron == NULL) + // We need more space in our environment + int resizeRet = ResizeEnvironment(palEnvironmentCapacity * 2); + if (resizeRet != 0) { + free(copy); goto done; } - for(j = 0; palEnvironment[j] != NULL; j++) - { - newEnviron[j] = palEnvironment[j]; - } - sAllocatedEnviron = TRUE; } - palEnvironment = newEnviron; - MiscSetEnvArray(); + +// ASSERT(copy != nullptr); palEnvironment[i] = copy; - palEnvironment[i + 1] = NULL; + palEnvironment[i + 1] = nullptr; + palEnvironmentCount++; - // Set 'copy' to NULL so it won't be freed - copy = NULL; - result = TRUE; } } @@ -948,53 +860,116 @@ BOOL EnvironPutenv(const char *string, BOOL deleteIfEmpty) { InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment); } - if (NULL != copy) - { - InternalFree(copy); - } + return result; } -/*++ -Function: - EnvironUnsetenv - -Removes a variable from the environment. Does nothing if the variable -does not exist in the environment. ---*/ -void EnvironUnsetenv(const char *name) +char* EnvironGetenv(const char* name) { - const char *equals; - int length; - int i, j; - CPalThread * pthrCurrent = InternalGetCurrentThread(); - - length = strlen(name); + char *retValue = nullptr; + CPalThread * pthrCurrent = InternalGetCurrentThread(); InternalEnterCriticalSection(pthrCurrent, &gcsEnvironment); - for(i = 0; palEnvironment[i] != NULL; i++) + + int nameLength = strlen(name); + for (int i = 0; palEnvironment[i] != nullptr; ++i) { - equals = strchr(palEnvironment[i], '='); - if (equals == NULL) - { - equals = palEnvironment[i] + strlen(palEnvironment[i]); - } - if (equals - palEnvironment[i] == length) + if (memcmp(palEnvironment[i], name, nameLength) == 0) { - if (memcmp(name, palEnvironment[i], length) == 0) + char *equalsSignPosition = palEnvironment[i] + nameLength; + + // If this is one of the variables which has no equals sign, we + // treat the whole thing as name, so the value is an empty string. + if (*equalsSignPosition == '\0') { - // Remove this one. Don't free it, though, since - // there might be oustanding references to it that - // were acquired via getenv. This is an - // unavoidable memory leak. - for(j = i + 1; palEnvironment[j] != NULL; j++) { } - // i is now the one we want to remove. j is the - // last index in palEnvironment, which is NULL. - - // Shift palEnvironment down by the difference between i and j. - memmove(palEnvironment + i, palEnvironment + i + 1, (j - i) * sizeof(char *)); + retValue = strdup(""); + break; + } + else if (*equalsSignPosition == '=') + { + retValue = equalsSignPosition + 1; + break; } } } + InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment); + return retValue; +} + +/*++ +Function: + EnvironInitialize + +Initialization function called from PAL_Initialize. + +Note: This is called before debug channels are initialized, so it + cannot use debug tracing calls. +--*/ +BOOL +EnvironInitialize(void) +{ + InternalInitializeCriticalSection(&gcsEnvironment); + InitializePalEnvironment(); + + return TRUE; +} + +/*++ + +Function : _putenv. + +See MSDN for more details. + +Note: The BSD implementation can cause + memory leaks. See man pages for more details. +--*/ +int +__cdecl +_putenv( const char * envstring ) +{ + int ret = -1; + + PERF_ENTRY(_putenv); + ENTRY( "_putenv( %p (%s) )\n", envstring ? envstring : "NULL", envstring ? envstring : "NULL") ; + + if (!envstring) + { + ERROR( "_putenv() called with NULL envstring!\n"); + goto EXIT; + } + + ret = EnvironPutenv(envstring, TRUE) ? 0 : -1; + +EXIT: + LOGEXIT( "_putenv returning %d\n", ret); + PERF_EXIT(_putenv); + return ret; +} + +/*++ + +Function : PAL_getenv + +See MSDN for more details. +--*/ +char * __cdecl PAL_getenv(const char *varname) +{ + char *retval; + + PERF_ENTRY(getenv); + ENTRY("getenv (%p (%s))\n", varname ? varname : "NULL", varname ? varname : "NULL"); + + if (strcmp(varname, "") == 0) + { + ERROR("getenv called with a empty variable name\n"); + LOGEXIT("getenv returning NULL\n"); + PERF_EXIT(getenv); + return(NULL); + } + retval = EnvironGetenv(varname); + + LOGEXIT("getenv returning %p\n", retval); + PERF_EXIT(getenv); + return(retval); } From 5381c4eebfe7b8dc3655d1b094cda14a132edeae Mon Sep 17 00:00:00 2001 From: Aditya Mandaleeka Date: Mon, 8 Feb 2016 19:17:09 -0800 Subject: [PATCH 3/6] Remove usage of PALC versions of critsec functions in environ. --- src/pal/src/include/pal/environ.h | 5 +- src/pal/src/misc/environ.cpp | 125 ++++++++++++++---------------- 2 files changed, 60 insertions(+), 70 deletions(-) diff --git a/src/pal/src/include/pal/environ.h b/src/pal/src/include/pal/environ.h index 9a310e55368a..4a303b85dcbe 100644 --- a/src/pal/src/include/pal/environ.h +++ b/src/pal/src/include/pal/environ.h @@ -33,9 +33,6 @@ Variables : gcsEnvironment: critical section to synchronize access to palEnvironment --*/ extern char **palEnvironment; -extern int palEnvironmentCapacity; -extern int palEnvironmentCount; - extern CRITICAL_SECTION gcsEnvironment; /*++ @@ -44,7 +41,7 @@ extern CRITICAL_SECTION gcsEnvironment; EnvironInitialize --*/ -BOOL EnvironInitialize();////////////// +BOOL EnvironInitialize(); /*++ Function: diff --git a/src/pal/src/misc/environ.cpp b/src/pal/src/misc/environ.cpp index 2abf401178bd..c2bdb835c538 100644 --- a/src/pal/src/misc/environ.cpp +++ b/src/pal/src/misc/environ.cpp @@ -134,7 +134,6 @@ GetEnvironmentVariableA( return dwRet; } - /*++ Function: GetEnvironmentVariableW @@ -230,7 +229,6 @@ GetEnvironmentVariableW( return size; } - /*++ Function: SetEnvironmentVariableW @@ -343,7 +341,6 @@ SetEnvironmentVariableW( return bRet; } - /*++ Function: GetEnvironmentStringsW @@ -381,7 +378,8 @@ GetEnvironmentStringsW( PERF_ENTRY(GetEnvironmentStringsW); ENTRY("GetEnvironmentStringsW()\n"); - PALCEnterCriticalSection(&gcsEnvironment); + CPalThread * pthrCurrent = InternalGetCurrentThread(); + InternalEnterCriticalSection(pthrCurrent, &gcsEnvironment); envNum = 0; len = 0; @@ -413,14 +411,13 @@ GetEnvironmentStringsW( *tempEnviron = 0; /* Put an extra null at the end */ EXIT: - PALCLeaveCriticalSection(&gcsEnvironment); + InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment); LOGEXIT("GetEnvironmentStringsW returning %p\n", wenviron); PERF_EXIT(GetEnvironmentStringsW); return wenviron; } - /*++ Function: GetEnvironmentStringsA @@ -433,13 +430,14 @@ PALAPI GetEnvironmentStringsA( VOID) { - char *environ = NULL, *tempEnviron; + char *environ = nullptr, *tempEnviron; int i, len, envNum; PERF_ENTRY(GetEnvironmentStringsA); ENTRY("GetEnvironmentStringsA()\n"); - PALCEnterCriticalSection(&gcsEnvironment); + CPalThread * pthrCurrent = InternalGetCurrentThread(); + InternalEnterCriticalSection(pthrCurrent, &gcsEnvironment); envNum = 0; len = 0; @@ -452,7 +450,7 @@ GetEnvironmentStringsA( } environ = (char *)PAL_malloc(envNum + 1); - if (environ == NULL) + if (environ == nullptr) { ERROR("malloc failed\n"); SetLastError(ERROR_NOT_ENOUGH_MEMORY); @@ -469,17 +467,16 @@ GetEnvironmentStringsA( envNum -= len; } - *tempEnviron = 0; /* Put an extra NULL at the end */ - + *tempEnviron = 0; /* Put an extra null at the end */ + EXIT: - PALCLeaveCriticalSection(&gcsEnvironment); + InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment); LOGEXIT("GetEnvironmentStringsA returning %p\n", environ); PERF_EXIT(GetEnvironmentStringsA); return environ; } - /*++ Function: FreeEnvironmentStringsW @@ -511,19 +508,18 @@ FreeEnvironmentStringsW( IN LPWSTR lpValue) { PERF_ENTRY(FreeEnvironmentStringsW); - ENTRY("FreeEnvironmentStringsW(lpValue=%p (%S))\n", lpValue?lpValue:W16_NULLSTRING, lpValue?lpValue:W16_NULLSTRING); + ENTRY("FreeEnvironmentStringsW(lpValue=%p (%S))\n", lpValue ? lpValue : W16_NULLSTRING, lpValue ? lpValue : W16_NULLSTRING); - if (lpValue != NULL) + if (lpValue != nullptr) { PAL_free(lpValue); } LOGEXIT("FreeEnvironmentStringW returning BOOL TRUE\n"); PERF_EXIT(FreeEnvironmentStringsW); - return TRUE ; + return TRUE; } - /*++ Function: FreeEnvironmentStringsA @@ -537,19 +533,18 @@ FreeEnvironmentStringsA( IN LPSTR lpValue) { PERF_ENTRY(FreeEnvironmentStringsA); - ENTRY("FreeEnvironmentStringsA(lpValue=%p (%s))\n", lpValue?lpValue:"NULL", lpValue?lpValue:"NULL"); + ENTRY("FreeEnvironmentStringsA(lpValue=%p (%s))\n", lpValue ? lpValue : "NULL", lpValue ? lpValue : "NULL"); - if (lpValue != NULL) + if (lpValue != nullptr) { PAL_free(lpValue); } LOGEXIT("FreeEnvironmentStringA returning BOOL TRUE\n"); PERF_EXIT(FreeEnvironmentStringsA); - return TRUE ; + return TRUE; } - /*++ Function: SetEnvironmentVariableA @@ -563,11 +558,11 @@ lpName [in] Pointer to a null-terminated string that specifies the environment variable whose value is being set. The operating system creates the environment variable if it does not exist - and lpValue is not NULL. + and lpValue is not null. lpValue [in] Pointer to a null-terminated string containing the new value of the specified environment variable. If this parameter - is NULL, the variable is deleted from the current process's + is null, the variable is deleted from the current process's environment. Return Values @@ -600,7 +595,7 @@ SetEnvironmentVariableA( // exit if the input variable name is null if ((lpName == nullptr) || (lpName[0] == 0)) { - ERROR("lpName is NULL\n"); + ERROR("lpName is null\n"); goto done; } @@ -635,7 +630,7 @@ SetEnvironmentVariableA( nResult = EnvironPutenv(string, FALSE) ? 0 : -1; PAL_free(string); - string = NULL; + string = nullptr; // If EnvironPutenv returns FALSE, it almost certainly failed to allocate memory. if (nResult == -1) @@ -664,7 +659,7 @@ int ResizeEnvironment(int newSize) if (newSize < palEnvironmentCount) { - printf("ERROR! new size < current count\n"); + ASSERT("ResizeEnvironment: New size < current count!\n"); return -1; } @@ -672,10 +667,8 @@ int ResizeEnvironment(int newSize) // If palEnvironment is null, realloc acts like malloc. palEnvironment = (char**)realloc(palEnvironment, palEnvironmentCapacity * sizeof(char *)); - if (!palEnvironment) { - printf("ERROR! realloc failed\n"); ret = -1; } @@ -684,42 +677,6 @@ int ResizeEnvironment(int newSize) return ret; } -int InitializePalEnvironment() -{ - char** sourceEnviron; - - CPalThread * pthrCurrent = InternalGetCurrentThread(); - InternalEnterCriticalSection(pthrCurrent, &gcsEnvironment); - -#if HAVE__NSGETENVIRON - sourceEnviron = *(_NSGetEnviron()); -#else // HAVE__NSGETENVIRON - extern char **environ; - sourceEnviron = environ; -#endif // HAVE__NSGETENVIRON - - int variableCount = 0; - while (sourceEnviron[variableCount] != nullptr) - variableCount++; - - printf("variableCount is %d\n", variableCount); - - palEnvironmentCount = 0; - - ResizeEnvironment(variableCount * 2); //// decide resize factor - - for (int i = 0; i < variableCount; ++i) - { - palEnvironment[i] = strdup(sourceEnviron[i]); - palEnvironmentCount++; - } - - InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment); - - return 0; -} - - void EnvironUnsetenv(const char *name) { int nameLength = strlen(name); @@ -763,7 +720,7 @@ BOOL EnvironPutenv(const char* entry, BOOL deleteIfEmpty) CPalThread * pthrCurrent = InternalGetCurrentThread(); const char *equalsSignPosition = strchr(entry, '='); - if (equalsSignPosition == entry || equalsSignPosition == NULL) + if (equalsSignPosition == entry || equalsSignPosition == nullptr) { // "=foo" and "foo" have no meaning goto done; @@ -897,6 +854,20 @@ char* EnvironGetenv(const char* name) return retValue; } +char** EnvironGetSystemEnvironment() +{ + char** sysEnviron; + +#if HAVE__NSGETENVIRON + sysEnviron = *(_NSGetEnviron()); +#else // HAVE__NSGETENVIRON + extern char **environ; + sysEnviron = environ; +#endif // HAVE__NSGETENVIRON + + return sysEnviron; +} + /*++ Function: EnvironInitialize @@ -910,7 +881,28 @@ BOOL EnvironInitialize(void) { InternalInitializeCriticalSection(&gcsEnvironment); - InitializePalEnvironment(); + + + CPalThread * pthrCurrent = InternalGetCurrentThread(); + InternalEnterCriticalSection(pthrCurrent, &gcsEnvironment); + + char** sourceEnviron = EnvironGetSystemEnvironment(); + + int variableCount = 0; + while (sourceEnviron[variableCount] != nullptr) + variableCount++; + + palEnvironmentCount = 0; + + ResizeEnvironment(variableCount * 2); //////// decide resize factor + + for (int i = 0; i < variableCount; ++i) + { + palEnvironment[i] = strdup(sourceEnviron[i]); + palEnvironmentCount++; + } + + InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment); return TRUE; } @@ -967,6 +959,7 @@ char * __cdecl PAL_getenv(const char *varname) PERF_EXIT(getenv); return(NULL); } + retval = EnvironGetenv(varname); LOGEXIT("getenv returning %p\n", retval); From a6f241cbc1122ac9fa3efdf9d39646e7848d6219 Mon Sep 17 00:00:00 2001 From: Aditya Mandaleeka Date: Tue, 9 Feb 2016 19:14:28 -0800 Subject: [PATCH 4/6] 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 d26734177ace..708f882c7f49 100644 --- a/src/pal/src/cruntime/misc.cpp +++ b/src/pal/src/cruntime/misc.cpp @@ -30,9 +30,7 @@ Module Name: #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 c71bf591dc67..686d7edf228a 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 ea57173ea026..cd4e26ddee12 100644 --- a/src/pal/src/exception/machexception.cpp +++ b/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 { diff --git a/src/pal/src/exception/machmessage.cpp b/src/pal/src/exception/machmessage.cpp index 2e11da802092..a6f7e57484f2 100644 --- a/src/pal/src/exception/machmessage.cpp +++ b/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" diff --git a/src/pal/src/exception/machmessage.h b/src/pal/src/exception/machmessage.h index 22ec0a75bfd9..80077afa8c9c 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 4a303b85dcbe..f4b8cd9feab1 100644 --- a/src/pal/src/include/pal/environ.h +++ b/src/pal/src/include/pal/environ.h @@ -49,7 +49,7 @@ BOOL EnvironInitialize(); 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 95c6e59e2656..4e6b7d2f172d 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 d05e27222786..83d45f3e230d 100644 --- a/src/pal/src/loader/module.cpp +++ b/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 diff --git a/src/pal/src/map/map.cpp b/src/pal/src/map/map.cpp index f8b30faaeaa5..7d4cf9272e9a 100644 --- a/src/pal/src/map/map.cpp +++ b/src/pal/src/map/map.cpp @@ -24,6 +24,7 @@ Module Name: #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 7ff0f26a78fb..3bd800307fa4 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 c2bdb835c538..3bb4c01f628c 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 @@ BOOL EnvironPutenv(const char* entry, BOOL deleteIfEmpty) 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 84d1b118b3a1..b2f3703c837f 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 3ba97ada610c..0bef8111dc08 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 From bfc8932f80aaada9aa09b4b5ffca33f88b3cf027 Mon Sep 17 00:00:00 2001 From: Aditya Mandaleeka Date: Wed, 10 Feb 2016 18:06:38 -0800 Subject: [PATCH 5/6] Add documentation to environ functions. --- src/pal/src/debug/debug.cpp | 3 +- src/pal/src/include/pal/environ.h | 14 ++-- src/pal/src/init/pal.cpp | 2 +- src/pal/src/misc/environ.cpp | 122 ++++++++++++++++++++++++++---- 4 files changed, 116 insertions(+), 25 deletions(-) diff --git a/src/pal/src/debug/debug.cpp b/src/pal/src/debug/debug.cpp index 686d7edf228a..86ea9f98e42f 100644 --- a/src/pal/src/debug/debug.cpp +++ b/src/pal/src/debug/debug.cpp @@ -179,7 +179,8 @@ OutputDebugStringA( // 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))) + if ((lpOutputString != NULL) && + (NULL != EnvironGetenv(PAL_OUTPUTDEBUGSTRING, /* copyValue */ FALSE))) { fprintf(stderr, "%s", lpOutputString); } diff --git a/src/pal/src/include/pal/environ.h b/src/pal/src/include/pal/environ.h index f4b8cd9feab1..1c0bce21ca1d 100644 --- a/src/pal/src/include/pal/environ.h +++ b/src/pal/src/include/pal/environ.h @@ -40,6 +40,7 @@ extern CRITICAL_SECTION gcsEnvironment; Function: EnvironInitialize +Initialization function for the PAL environment code. --*/ BOOL EnvironInitialize(); @@ -47,17 +48,16 @@ BOOL EnvironInitialize(); Function: EnvironGetenv -Gets an environment variable's value. +Get the value of environment variable with the given name. --*/ -char *EnvironGetenv(const char *name, bool copyValue = true); +char *EnvironGetenv(const char *name, BOOL copyValue = TRUE); /*++ Function: EnvironPutenv -Sets an environment variable's value. -Returns TRUE if the variable was set, or FALSE if malloc or realloc -failed or if the given string is malformed. +Add the environment variable string provided to the PAL version +of the environment. --*/ BOOL EnvironPutenv(const char *string, BOOL deleteIfEmpty); @@ -65,8 +65,8 @@ BOOL EnvironPutenv(const char *string, BOOL deleteIfEmpty); Function: EnvironUnsetenv -Removes a variable from the environment. Does nothing if the variable -does not exist in the environment. +Remove the environment variable with the given name from the PAL +version of the environment if it exists. --*/ void EnvironUnsetenv(const char *name); diff --git a/src/pal/src/init/pal.cpp b/src/pal/src/init/pal.cpp index 4e6b7d2f172d..10871aadaaae 100644 --- a/src/pal/src/init/pal.cpp +++ b/src/pal/src/init/pal.cpp @@ -1325,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")); /////// leak in debug. fix? + 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 diff --git a/src/pal/src/misc/environ.cpp b/src/pal/src/misc/environ.cpp index 3bb4c01f628c..b7ab5584e8d0 100644 --- a/src/pal/src/misc/environ.cpp +++ b/src/pal/src/misc/environ.cpp @@ -120,7 +120,7 @@ GetEnvironmentVariableA( InternalEnterCriticalSection(pthrCurrent, &gcsEnvironment); enteredCritSec = true; - value = EnvironGetenv(lpName, /* copyValue */ false); + value = EnvironGetenv(lpName, /* copyValue */ FALSE); } if (value == nullptr) @@ -627,7 +627,7 @@ SetEnvironmentVariableA( // 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) + if ((lpValue = EnvironGetenv(lpName, /* copyValue */ FALSE)) == nullptr) { ERROR("Couldn't find environment variable (%s)\n", lpName); SetLastError(ERROR_ENVVAR_NOT_FOUND); @@ -673,17 +673,31 @@ SetEnvironmentVariableA( return bRet; } -int ResizeEnvironment(int newSize) -{ - int ret = 0; +/*++ +Function: + ResizeEnvironment + +Resizes the PAL environment buffer. + +Parameters + + newSize + [in] New size of palEnvironment +Return Values + + TRUE on success, FALSE otherwise + +--*/ +BOOL ResizeEnvironment(int newSize) +{ CPalThread * pthrCurrent = InternalGetCurrentThread(); InternalEnterCriticalSection(pthrCurrent, &gcsEnvironment); if (newSize < palEnvironmentCount) { - ASSERT("ResizeEnvironment: New size < current count!\n"); - return -1; + ASSERT("ResizeEnvironment: New size < current count!\n"); + return FALSE; } palEnvironmentCapacity = newSize; @@ -692,14 +706,27 @@ int ResizeEnvironment(int newSize) palEnvironment = (char**)realloc(palEnvironment, palEnvironmentCapacity * sizeof(char *)); if (!palEnvironment) { - ret = -1; + return FALSE; } InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment); - return ret; + return TRUE; } +/*++ +Function: + EnvironUnsetenv + +Remove the environment variable with the given name from the PAL version +of the environment if it exists. + +Parameters + + name + [in] Name of variable to unset. + +--*/ void EnvironUnsetenv(const char *name) { int nameLength = strlen(name); @@ -721,9 +748,10 @@ void EnvironUnsetenv(const char *name) { if (memcmp(name, palEnvironment[i], nameLength) == 0) { + // Free the string we're removing. InternalFree(palEnvironment[i]); - // Move the last variable here and set it to null. + // Move the last environment variable pointer here. palEnvironment[i] = palEnvironment[palEnvironmentCount - 1]; palEnvironment[palEnvironmentCount - 1] = nullptr; @@ -735,6 +763,26 @@ void EnvironUnsetenv(const char *name) InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment); } +/*++ +Function: + EnvironPutenv + +Add the environment variable string provided to the PAL version +of the environment. + +Parameters + + entry + [in] The variable string to add. Should be in the format + "name=value", where value might be empty (see below). + deleteIfEmpty + [in] If this is TRUE, "name=" will unset the 'name' variable. + +Return Values + + TRUE on success, FALSE otherwise + +--*/ BOOL EnvironPutenv(const char* entry, BOOL deleteIfEmpty) { bool fOwningCS = false; @@ -816,19 +864,19 @@ BOOL EnvironPutenv(const char* entry, BOOL deleteIfEmpty) if (palEnvironment[i] == nullptr) { - // ASSERT(i <= palEnvironmentCapacity) + _ASSERTE(i <= palEnvironmentCapacity); if (i == palEnvironmentCapacity) { - // We need more space in our environment + // We need more space in our environment, so let's double the size. int resizeRet = ResizeEnvironment(palEnvironmentCapacity * 2); - if (resizeRet != 0) + if (resizeRet != TRUE) { InternalFree(copy); goto done; } } -// ASSERT(copy != nullptr); + _ASSERTE(copy != nullptr); palEnvironment[i] = copy; palEnvironment[i + 1] = nullptr; palEnvironmentCount++; @@ -846,7 +894,31 @@ BOOL EnvironPutenv(const char* entry, BOOL deleteIfEmpty) return result; } -char* EnvironGetenv(const char* name, bool copyValue) +/*++ +Function: + EnvironGetenv + +Get the value of environment variable with the given name. + +Parameters + + name + [in] The name of the environment variable to get. + copyValue + [in] If this is TRUE, the function will make a copy of the + value and return a pointer to that. Otherwise, it will + return a pointer to the value in the PAL environment + directly. Calling this function with copyValue set to + FALSE is therefore unsafe without taking special pre- + cautions since the pointer may point to garbage later. + +Return Value + + A pointer to the value of the environment variable if it exists, + or nullptr otherwise. + +--*/ +char* EnvironGetenv(const char* name, BOOL copyValue) { char *retValue = nullptr; @@ -884,6 +956,20 @@ char* EnvironGetenv(const char* name, bool copyValue) return retValue; } +/*++ +Function: + EnvironGetSystemEnvironment + +Get a pointer to the array of pointers representing the process's +environment. + +See 'man environ' for details. + +Return Value + + A pointer to the environment. + +--*/ char** EnvironGetSystemEnvironment() { char** sysEnviron; @@ -923,7 +1009,11 @@ EnvironInitialize(void) palEnvironmentCount = 0; - ResizeEnvironment(variableCount * 2); //////// decide resize factor + // We need to decide how much space to allocate. Since we need enough + // space for all of the 'n' current environment variables, but we don't + // know how many more there will be, we will initially make room for + // '2n' variables. If even more are added, we will resize again. + ResizeEnvironment(variableCount * 2); for (int i = 0; i < variableCount; ++i) { From d26de9e1b371ba5598e2e004a4402e5d30d70eff Mon Sep 17 00:00:00 2001 From: Aditya Mandaleeka Date: Thu, 11 Feb 2016 19:08:06 -0800 Subject: [PATCH 6/6] Further cleanup of environment code. --- src/pal/src/exception/machmessage.h | 2 +- src/pal/src/misc/dbgmsg.cpp | 4 +- src/pal/src/misc/environ.cpp | 141 +++++++++++++++------------- 3 files changed, 79 insertions(+), 68 deletions(-) diff --git a/src/pal/src/exception/machmessage.h b/src/pal/src/exception/machmessage.h index 80077afa8c9c..244396cd35af 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 EnvironGetenv("NONPAL_TRACING", /* copyValue */ false); +#define NONPAL_TRACE_ENABLED EnvironGetenv("NONPAL_TRACING", /* copyValue */ false) #define NONPAL_ASSERT(_msg, ...) NONPAL_RETAIL_ASSERT(_msg, __VA_ARGS__) diff --git a/src/pal/src/misc/dbgmsg.cpp b/src/pal/src/misc/dbgmsg.cpp index 3bd800307fa4..c96dbdd50f40 100644 --- a/src/pal/src/misc/dbgmsg.cpp +++ b/src/pal/src/misc/dbgmsg.cpp @@ -819,6 +819,7 @@ bool DBG_ShouldCheckStackAlignment() if (caMode == CheckAlignment_Uninitialized) { char* checkAlignmentSettings; + bool shouldFreeCheckAlignmentSettings = false; if (palEnvironment == nullptr) { // This function might be called before the PAL environment is initialized. @@ -828,12 +829,13 @@ bool DBG_ShouldCheckStackAlignment() else { checkAlignmentSettings = EnvironGetenv(PAL_CHECK_ALIGNMENT_MODE); + shouldFreeCheckAlignmentSettings = true; } caMode = checkAlignmentSettings ? (CheckAlignmentMode)atoi(checkAlignmentSettings) : CheckAlignment_Default; - if (checkAlignmentSettings) + if (checkAlignmentSettings && shouldFreeCheckAlignmentSettings) { InternalFree(checkAlignmentSettings); } diff --git a/src/pal/src/misc/environ.cpp b/src/pal/src/misc/environ.cpp index b7ab5584e8d0..9edfd135d3e0 100644 --- a/src/pal/src/misc/environ.cpp +++ b/src/pal/src/misc/environ.cpp @@ -89,7 +89,6 @@ GetEnvironmentVariableA( lpName ? lpName : "NULL", lpName ? lpName : "NULL", lpBuffer, nSize); - bool enteredCritSec = false; CPalThread * pthrCurrent = InternalGetCurrentThread(); if (lpName == nullptr) @@ -118,37 +117,36 @@ GetEnvironmentVariableA( // 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) + { + DWORD valueLength = strlen(value); + if (valueLength < nSize) + { + strcpy_s(lpBuffer, nSize, value); + dwRet = valueLength; + } + else + { + dwRet = valueLength + 1; + } + + SetLastError(ERROR_SUCCESS); + } + + InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment); } if (value == nullptr) { TRACE("%s is not found\n", lpName); SetLastError(ERROR_ENVVAR_NOT_FOUND); - goto done; - } - - if (strlen(value) < nSize) - { - strcpy_s(lpBuffer, nSize, value); - dwRet = strlen(value); - } - else - { - dwRet = strlen(value)+1; } - SetLastError(ERROR_SUCCESS); - done: - if (enteredCritSec) - { - InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment); - } - LOGEXIT("GetEnvironmentVariableA returns DWORD 0x%x\n", dwRet); PERF_EXIT(GetEnvironmentVariableA); return dwRet; @@ -694,24 +692,26 @@ BOOL ResizeEnvironment(int newSize) CPalThread * pthrCurrent = InternalGetCurrentThread(); InternalEnterCriticalSection(pthrCurrent, &gcsEnvironment); - if (newSize < palEnvironmentCount) + BOOL ret = FALSE; + if (newSize >= palEnvironmentCount) { - ASSERT("ResizeEnvironment: New size < current count!\n"); - return FALSE; + // If palEnvironment is null, realloc acts like malloc. + char **newEnvironment = (char**)realloc(palEnvironment, newSize * sizeof(char *)); + if (newEnvironment != nullptr) + { + // realloc succeeded, so set palEnvironment to what it returned. + palEnvironment = newEnvironment; + palEnvironmentCapacity = newSize; + ret = TRUE; + } } - - palEnvironmentCapacity = newSize; - - // If palEnvironment is null, realloc acts like malloc. - palEnvironment = (char**)realloc(palEnvironment, palEnvironmentCapacity * sizeof(char *)); - if (!palEnvironment) + else { - return FALSE; + ASSERT("ResizeEnvironment: newSize < current palEnvironmentCount!\n"); } InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment); - - return TRUE; + return ret; } /*++ @@ -785,18 +785,27 @@ Return Values --*/ BOOL EnvironPutenv(const char* entry, BOOL deleteIfEmpty) { - bool fOwningCS = false; BOOL result = FALSE; + bool fOwningCS = false; + CPalThread * pthrCurrent = InternalGetCurrentThread(); const char *equalsSignPosition = strchr(entry, '='); if (equalsSignPosition == entry || equalsSignPosition == nullptr) { // "=foo" and "foo" have no meaning - goto done; + return FALSE; } + char* copy = strdup(entry); + if (copy == nullptr) + { + return FALSE; + } + + int nameLength = equalsSignPosition - entry; + if (equalsSignPosition[1] == '\0' && deleteIfEmpty) { // "foo=" removes foo from the environment in _putenv() on Windows. @@ -804,17 +813,9 @@ BOOL EnvironPutenv(const char* entry, BOOL deleteIfEmpty) // with the empty string as the value, but in that case we want to // set the variable's value to "". deleteIfEmpty will be FALSE in // that case. - int length = strlen(entry); - char* copy = (char *) InternalMalloc(length); - if (copy == nullptr) - { - goto done; - } - - memcpy(copy, entry, length - 1); // Change '=' to '\0' - copy[length - 1] = '\0'; + copy[nameLength] = '\0'; EnvironUnsetenv(copy); InternalFree(copy); @@ -825,15 +826,6 @@ BOOL EnvironPutenv(const char* entry, BOOL deleteIfEmpty) { // See if we are replacing an item or adding one. - // Make our copy up front, since we'll use it either way. - char* copy = strdup(entry); - if (copy == nullptr) - { - goto done; - } - - int nameLength = equalsSignPosition - entry; - InternalEnterCriticalSection(pthrCurrent, &gcsEnvironment); fOwningCS = true; @@ -864,10 +856,11 @@ BOOL EnvironPutenv(const char* entry, BOOL deleteIfEmpty) if (palEnvironment[i] == nullptr) { - _ASSERTE(i <= palEnvironmentCapacity); - if (i == palEnvironmentCapacity) + _ASSERTE(i < palEnvironmentCapacity); + if (i == (palEnvironmentCapacity - 1)) { - // We need more space in our environment, so let's double the size. + // We found the first null, but it's the last element in our environment + // block. We need more space in our environment, so let's double its size. int resizeRet = ResizeEnvironment(palEnvironmentCapacity * 2); if (resizeRet != TRUE) { @@ -928,6 +921,11 @@ char* EnvironGetenv(const char* name, BOOL copyValue) int nameLength = strlen(name); for (int i = 0; palEnvironment[i] != nullptr; ++i) { + if (strlen(palEnvironment[i]) < nameLength) + { + continue; + } + if (memcmp(palEnvironment[i], name, nameLength) == 0) { char *equalsSignPosition = palEnvironment[i] + nameLength; @@ -996,6 +994,8 @@ Note: This is called before debug channels are initialized, so it BOOL EnvironInitialize(void) { + BOOL ret = FALSE; + InternalInitializeCriticalSection(&gcsEnvironment); CPalThread * pthrCurrent = InternalGetCurrentThread(); @@ -1013,17 +1013,26 @@ EnvironInitialize(void) // space for all of the 'n' current environment variables, but we don't // know how many more there will be, we will initially make room for // '2n' variables. If even more are added, we will resize again. - ResizeEnvironment(variableCount * 2); + // If there are no variables, we will still make room for 1 entry to + // store a nullptr there. + int initialSize = (variableCount == 0) ? 1 : variableCount * 2; - for (int i = 0; i < variableCount; ++i) + ret = ResizeEnvironment(initialSize); + if (ret == TRUE) { - palEnvironment[i] = strdup(sourceEnviron[i]); - palEnvironmentCount++; + _ASSERTE(palEnvironment != nullptr); + for (int i = 0; i < variableCount; ++i) + { + palEnvironment[i] = strdup(sourceEnviron[i]); + palEnvironmentCount++; + } + + // Set the entry after the last variable to null to indicate the end. + palEnvironment[variableCount] = nullptr; } InternalLeaveCriticalSection(pthrCurrent, &gcsEnvironment); - - return TRUE; + return ret; } /*++ @@ -1044,15 +1053,15 @@ _putenv( const char * envstring ) PERF_ENTRY(_putenv); ENTRY( "_putenv( %p (%s) )\n", envstring ? envstring : "NULL", envstring ? envstring : "NULL") ; - if (!envstring) + if (envstring != nullptr) + { + ret = EnvironPutenv(envstring, TRUE) ? 0 : -1; + } + else { ERROR( "_putenv() called with NULL envstring!\n"); - goto EXIT; } - ret = EnvironPutenv(envstring, TRUE) ? 0 : -1; - -EXIT: LOGEXIT( "_putenv returning %d\n", ret); PERF_EXIT(_putenv); return ret;