From e48796b26b5b2b75401b48d21927b1d2912d4dca Mon Sep 17 00:00:00 2001 From: Brad King Date: Thu, 26 Apr 2012 10:36:53 -0400 Subject: [PATCH] KWSys: Fix SystemTools environment memory handling (#13156) The SystemTools::PutEnv function tries to provide the "putenv" API without leaking memory. However, the kwsysDeletingCharVector singleton frees memory that may still be referenced by the environment table, having been placed there by putenv. If any static destruction or processing by an external tool happens after the singleton is destroyed and accesses the environment it will read invalid memory. Replace use of putenv with setenv/unsetenv when available. The latter manage internal copies of the values passed instead of referencing the original memory. When setenv/unsetenv are not available use putenv with a singleton that removes its values from the environment before freeing their memory. This requires an "unputenv" implementation. On at least some platforms it must be written in terms of "putenv" because other APIs are not available and direct modification of the "environ" global is not safe (e.g. on Windows there is interaction with "wenviron"). Fortunately either putenv("A=") or putenv("A") will remove "A" from the environment on these platforms. On other platforms fall back to direct manipulation of "environ". Also add UnPutEnv to the API and add a test for the behavior of both. --- Source/kwsys/CMakeLists.txt | 13 +- Source/kwsys/SystemTools.cxx | 238 ++++++++++++++++++++++--- Source/kwsys/SystemTools.hxx.in | 6 +- Source/kwsys/kwsysPlatformTestsCXX.cxx | 26 +++ Source/kwsys/testSystemTools.cxx | 54 ++++++ bootstrap | 37 +++- 6 files changed, 345 insertions(+), 29 deletions(-) diff --git a/Source/kwsys/CMakeLists.txt b/Source/kwsys/CMakeLists.txt index 0ef4e28c8..f51712cc0 100644 --- a/Source/kwsys/CMakeLists.txt +++ b/Source/kwsys/CMakeLists.txt @@ -553,11 +553,16 @@ SET_SOURCE_FILES_PROPERTIES(ProcessUNIX.c System.c PROPERTIES COMPILE_FLAGS "-DKWSYS_C_HAS_PTRDIFF_T=${KWSYS_C_HAS_PTRDIFF_T} -DKWSYS_C_HAS_SSIZE_T=${KWSYS_C_HAS_SSIZE_T}" ) -IF(KWSYS_DO_NOT_CLEAN_PUTENV) - # Disable cleanup of putenv memory for issues with GCOV. +IF(KWSYS_USE_SystemTools) + KWSYS_PLATFORM_CXX_TEST(KWSYS_CXX_HAS_SETENV + "Checking whether CXX compiler has setenv" DIRECT) + KWSYS_PLATFORM_CXX_TEST(KWSYS_CXX_HAS_UNSETENV + "Checking whether CXX compiler has unsetenv" DIRECT) + KWSYS_PLATFORM_CXX_TEST(KWSYS_CXX_HAS_ENVIRON_IN_STDLIB_H + "Checking whether CXX compiler has environ in stdlib.h" DIRECT) SET_SOURCE_FILES_PROPERTIES(SystemTools.cxx PROPERTIES - COMPILE_FLAGS -DKWSYS_DO_NOT_CLEAN_PUTENV=1) -ENDIF(KWSYS_DO_NOT_CLEAN_PUTENV) + COMPILE_FLAGS "-DKWSYS_CXX_HAS_SETENV=${KWSYS_CXX_HAS_SETENV} -DKWSYS_CXX_HAS_UNSETENV=${KWSYS_CXX_HAS_UNSETENV} -DKWSYS_CXX_HAS_ENVIRON_IN_STDLIB_H=${KWSYS_CXX_HAS_ENVIRON_IN_STDLIB_H}") +ENDIF() #----------------------------------------------------------------------------- # Choose a directory for the generated headers. diff --git a/Source/kwsys/SystemTools.cxx b/Source/kwsys/SystemTools.cxx index 4d83293d9..8ab580f0e 100644 --- a/Source/kwsys/SystemTools.cxx +++ b/Source/kwsys/SystemTools.cxx @@ -25,6 +25,8 @@ #include KWSYS_HEADER(ios/fstream) #include KWSYS_HEADER(ios/sstream) +#include KWSYS_HEADER(stl/set) + // Work-around CMake dependency scanning limitation. This must // duplicate the above list of headers. #if 0 @@ -78,6 +80,14 @@ # undef _WIN32 #endif +#if !KWSYS_CXX_HAS_ENVIRON_IN_STDLIB_H +# if defined(_WIN32) +extern __declspec(dllimport) char **environ; +# else +extern char **environ; +# endif +#endif + #ifdef __CYGWIN__ extern "C" void cygwin_conv_to_win32_path(const char *path, char *win32_path); #endif @@ -371,38 +381,224 @@ bool SystemTools::GetEnv(const char* key, kwsys_stl::string& result) } } -#ifdef __INTEL_COMPILER -#pragma warning disable 444 +//---------------------------------------------------------------------------- + +#if defined(__CYGWIN__) || defined(__GLIBC__) +# define KWSYS_PUTENV_NAME /* putenv("A") removes A. */ +#elif defined(_WIN32) +# define KWSYS_PUTENV_EMPTY /* putenv("A=") removes A. */ #endif -class kwsysDeletingCharVector : public kwsys_stl::vector +#if KWSYS_CXX_HAS_UNSETENV +/* unsetenv("A") removes A from the environment. + On older platforms it returns void instead of int. */ +static int kwsysUnPutEnv(const char* env) { -public: - ~kwsysDeletingCharVector(); + if(const char* eq = strchr(env, '=')) + { + std::string name(env, eq-env); + unsetenv(name.c_str()); + } + else + { + unsetenv(env); + } + return 0; +} + +#elif defined(KWSYS_PUTENV_EMPTY) || defined(KWSYS_PUTENV_NAME) +/* putenv("A=") or putenv("A") removes A from the environment. */ +static int kwsysUnPutEnv(const char* env) +{ + int err = 0; + const char* eq = strchr(env, '='); + size_t const len = eq? (size_t)(eq-env) : strlen(env); +# ifdef KWSYS_PUTENV_EMPTY + size_t const sz = len + 2; +# else + size_t const sz = len + 1; +# endif + char local_buf[256]; + char* buf = sz > sizeof(local_buf) ? (char*)malloc(sz) : local_buf; + if(!buf) + { + return -1; + } + strncpy(buf, env, len); +# ifdef KWSYS_PUTENV_EMPTY + buf[len] = '='; + buf[len+1] = 0; + if(putenv(buf) < 0) + { + err = errno; + } +# else + buf[len] = 0; + if(putenv(buf) < 0 && errno != EINVAL) + { + err = errno; + } +# endif + if(buf != local_buf) + { + free(buf); + } + if(err) + { + errno = err; + return -1; + } + return 0; +} + +#else +/* Manipulate the "environ" global directly. */ +static int kwsysUnPutEnv(const char* env) +{ + const char* eq = strchr(env, '='); + size_t const len = eq? (size_t)(eq-env) : strlen(env); + int in = 0; + int out = 0; + while(environ[in]) + { + if(strlen(environ[in]) > len && + environ[in][len] == '=' && + strncmp(env, environ[in], len) == 0) + { + ++in; + } + else + { + environ[out++] = environ[in++]; + } + } + while(out < in) + { + environ[out++] = 0; + } + return 0; +} +#endif + +//---------------------------------------------------------------------------- + +#if KWSYS_CXX_HAS_SETENV + +/* setenv("A", "B", 1) will set A=B in the environment and makes its + own copies of the strings. */ +bool SystemTools::PutEnv(const char* env) +{ + if(const char* eq = strchr(env, '=')) + { + std::string name(env, eq-env); + return setenv(name.c_str(), eq+1, 1) == 0; + } + else + { + return kwsysUnPutEnv(env) == 0; + } +} + +bool SystemTools::UnPutEnv(const char* env) +{ + return kwsysUnPutEnv(env) == 0; +} + +#else + +/* putenv("A=B") will set A=B in the environment. Most putenv implementations + put their argument directly in the environment. They never free the memory + on program exit. Keep an active set of pointers to memory we allocate and + pass to putenv, one per environment key. At program exit remove any + environment values that may still reference memory we allocated. Then free + the memory. This will not affect any environment values we never set. */ + +# ifdef __INTEL_COMPILER +# pragma warning disable 444 /* base has non-virtual destructor */ +# endif + +/* Order by environment key only (VAR from VAR=VALUE). */ +struct kwsysEnvCompare +{ + bool operator() (const char* l, const char* r) const + { + const char* leq = strchr(l, '='); + const char* req = strchr(r, '='); + size_t llen = leq? (leq-l) : strlen(l); + size_t rlen = req? (req-r) : strlen(r); + if(llen == rlen) + { + return strncmp(l,r,llen) < 0; + } + else + { + return strcmp(l,r) < 0; + } + } }; -kwsysDeletingCharVector::~kwsysDeletingCharVector() +class kwsysEnv: public kwsys_stl::set { -#ifndef KWSYS_DO_NOT_CLEAN_PUTENV - for(kwsys_stl::vector::iterator i = this->begin(); - i != this->end(); ++i) + class Free + { + const char* Env; + public: + Free(const char* env): Env(env) {} + ~Free() { free(const_cast(this->Env)); } + }; +public: + typedef kwsys_stl::set derived; + ~kwsysEnv() { - delete []*i; + for(derived::iterator i = this->begin(); i != this->end(); ++i) + { + kwsysUnPutEnv(*i); + free(const_cast(*i)); + } } -#endif -} -bool SystemTools::PutEnv(const char* value) + const char* Release(const char* env) + { + const char* old = 0; + derived::iterator i = this->find(env); + if(i != this->end()) + { + old = *i; + this->erase(i); + } + return old; + } + bool Put(const char* env) + { + Free oldEnv(this->Release(env)); + static_cast(oldEnv); + char* newEnv = strdup(env); + this->insert(newEnv); + return putenv(newEnv) == 0; + } + bool UnPut(const char* env) + { + Free oldEnv(this->Release(env)); + static_cast(oldEnv); + return kwsysUnPutEnv(env) == 0; + } +}; + +static kwsysEnv kwsysEnvInstance; + +bool SystemTools::PutEnv(const char* env) { - static kwsysDeletingCharVector localEnvironment; - char* envVar = new char[strlen(value)+1]; - strcpy(envVar, value); - int ret = putenv(envVar); - // save the pointer in the static vector so that it can - // be deleted on exit - localEnvironment.push_back(envVar); - return ret == 0; + return kwsysEnvInstance.Put(env); } +bool SystemTools::UnPutEnv(const char* env) +{ + return kwsysEnvInstance.UnPut(env); +} + +#endif + +//---------------------------------------------------------------------------- + const char* SystemTools::GetExecutableExtension() { #if defined(_WIN32) || defined(__CYGWIN__) || defined(__VMS) diff --git a/Source/kwsys/SystemTools.hxx.in b/Source/kwsys/SystemTools.hxx.in index 04f197842..5171125ce 100644 --- a/Source/kwsys/SystemTools.hxx.in +++ b/Source/kwsys/SystemTools.hxx.in @@ -749,7 +749,11 @@ public: /** Put a string into the environment of the form var=value */ - static bool PutEnv(const char* value); + static bool PutEnv(const char* env); + + /** Remove a string from the environment. + Input is of the form "var" or "var=value" (value is ignored). */ + static bool UnPutEnv(const char* env); /** * Get current working directory CWD diff --git a/Source/kwsys/kwsysPlatformTestsCXX.cxx b/Source/kwsys/kwsysPlatformTestsCXX.cxx index 903be9bdb..16124d3a8 100644 --- a/Source/kwsys/kwsysPlatformTestsCXX.cxx +++ b/Source/kwsys/kwsysPlatformTestsCXX.cxx @@ -393,6 +393,32 @@ int main(int, char **argv) } #endif +#ifdef TEST_KWSYS_CXX_HAS_SETENV +#include +int main() +{ + return setenv("A", "B", 1); +} +#endif + +#ifdef TEST_KWSYS_CXX_HAS_UNSETENV +#include +int main() +{ + unsetenv("A"); + return 0; +} +#endif + +#ifdef TEST_KWSYS_CXX_HAS_ENVIRON_IN_STDLIB_H +#include +int main() +{ + char* e = environ[0]; + return e? 0:1; +} +#endif + #ifdef TEST_KWSYS_CXX_TYPE_INFO /* Collect fundamental type information and save it to a CMake script. */ diff --git a/Source/kwsys/testSystemTools.cxx b/Source/kwsys/testSystemTools.cxx index c0e74af3e..3ac0cb399 100644 --- a/Source/kwsys/testSystemTools.cxx +++ b/Source/kwsys/testSystemTools.cxx @@ -327,6 +327,58 @@ bool CheckStringOperations() return res; } +//---------------------------------------------------------------------------- + +bool CheckPutEnv(const char* env, const char* name, const char* value) +{ + if(!kwsys::SystemTools::PutEnv(env)) + { + kwsys_ios::cerr << "PutEnv(\"" << env + << "\") failed!" << kwsys_ios::endl; + return false; + } + const char* v = kwsys::SystemTools::GetEnv(name); + v = v? v : "(null)"; + if(strcmp(v, value) != 0) + { + kwsys_ios::cerr << "GetEnv(\"" << name << "\") returned \"" + << v << "\", not \"" << value << "\"!" << kwsys_ios::endl; + return false; + } + return true; +} + +bool CheckUnPutEnv(const char* env, const char* name) +{ + if(!kwsys::SystemTools::UnPutEnv(env)) + { + kwsys_ios::cerr << "UnPutEnv(\"" << env << "\") failed!" + << kwsys_ios::endl; + return false; + } + if(const char* v = kwsys::SystemTools::GetEnv(name)) + { + kwsys_ios::cerr << "GetEnv(\"" << name << "\") returned \"" + << v << "\", not (null)!" << kwsys_ios::endl; + return false; + } + return true; +} + +bool CheckEnvironmentOperations() +{ + bool res = true; + res &= CheckPutEnv("A=B", "A", "B"); + res &= CheckPutEnv("B=C", "B", "C"); + res &= CheckPutEnv("C=D", "C", "D"); + res &= CheckPutEnv("D=E", "D", "E"); + res &= CheckUnPutEnv("A", "A"); + res &= CheckUnPutEnv("B=", "B"); + res &= CheckUnPutEnv("C=D", "C"); + /* Leave "D=E" in environment so a memory checker can test for leaks. */ + return res; +} + //---------------------------------------------------------------------------- int testSystemTools(int, char*[]) { @@ -356,5 +408,7 @@ int testSystemTools(int, char*[]) res &= CheckStringOperations(); + res &= CheckEnvironmentOperations(); + return res ? 0 : 1; } diff --git a/bootstrap b/bootstrap index 00697549a..5b431fd78 100755 --- a/bootstrap +++ b/bootstrap @@ -395,7 +395,6 @@ cmake_kwsys_config_replace_string () echo "${APPEND}" > "${OUTFILE}${_tmp}" cat "${INFILE}" | sed "/./ {s/\@KWSYS_NAMESPACE\@/cmsys/g; - s/@KWSYS_DO_NOT_CLEAN_PUTENV@/0/g; s/@KWSYS_BUILD_SHARED@/${KWSYS_BUILD_SHARED}/g; s/@KWSYS_LFS_AVAILABLE@/${KWSYS_LFS_AVAILABLE}/g; s/@KWSYS_LFS_REQUESTED@/${KWSYS_LFS_REQUESTED}/g; @@ -1039,6 +1038,9 @@ KWSYS_STL_HAS_ALLOCATOR_NONTEMPLATE=0 KWSYS_STL_HAS_ALLOCATOR_REBIND=0 KWSYS_STL_HAS_ALLOCATOR_MAX_SIZE_ARGUMENT=0 KWSYS_STL_HAS_ALLOCATOR_OBJECTS=0 +KWSYS_CXX_HAS_SETENV=0 +KWSYS_CXX_HAS_UNSETENV=0 +KWSYS_CXX_HAS_ENVIRON_IN_STDLIB_H=0 KWSYS_CXX_HAS_CSTDDEF=0 KWSYS_CXX_HAS_NULL_TEMPLATE_ARGS=0 KWSYS_CXX_HAS_MEMBER_TEMPLATES=0 @@ -1049,6 +1051,33 @@ KWSYS_CXX_HAS_ARGUMENT_DEPENDENT_LOOKUP=0 KWSYS_STL_STRING_HAVE_ISTREAM=1 KWSYS_STL_STRING_HAVE_OSTREAM=1 +if cmake_try_run "${cmake_cxx_compiler}" \ + "${cmake_cxx_flags} -DTEST_KWSYS_CXX_HAS_SETENV" \ + "${cmake_source_dir}/Source/kwsys/kwsysPlatformTestsCXX.cxx" >> cmake_bootstrap.log 2>&1; then + KWSYS_CXX_HAS_SETENV=1 + echo "${cmake_cxx_compiler} has setenv" +else + echo "${cmake_cxx_compiler} does not have setenv" +fi + +if cmake_try_run "${cmake_cxx_compiler}" \ + "${cmake_cxx_flags} -DTEST_KWSYS_CXX_HAS_UNSETENV" \ + "${cmake_source_dir}/Source/kwsys/kwsysPlatformTestsCXX.cxx" >> cmake_bootstrap.log 2>&1; then + KWSYS_CXX_HAS_UNSETENV=1 + echo "${cmake_cxx_compiler} has unsetenv" +else + echo "${cmake_cxx_compiler} does not have unsetenv" +fi + +if cmake_try_run "${cmake_cxx_compiler}" \ + "${cmake_cxx_flags} -DTEST_KWSYS_CXX_HAS_ENVIRON_IN_STDLIB_H" \ + "${cmake_source_dir}/Source/kwsys/kwsysPlatformTestsCXX.cxx" >> cmake_bootstrap.log 2>&1; then + KWSYS_CXX_HAS_ENVIRON_IN_STDLIB_H=1 + echo "${cmake_cxx_compiler} has environ in stdlib.h" +else + echo "${cmake_cxx_compiler} does not have environ in stdlib.h" +fi + if cmake_try_run "${cmake_cxx_compiler}" \ "${cmake_cxx_flags} -DTEST_KWSYS_STL_HAVE_STD" \ "${cmake_source_dir}/Source/kwsys/kwsysPlatformTestsCXX.cxx" >> cmake_bootstrap.log 2>&1; then @@ -1365,7 +1394,7 @@ cmake_replace_string "${cmake_source_dir}/Source/kwsys/kwsys_stl.hxx.in" \ cmake_replace_string "${cmake_bootstrap_dir}/cmsys/stl/stl.hxx_a" \ "${cmake_bootstrap_dir}/cmsys/stl/stl.hxx_b" KWSYS_NAMESPACE cmsys -for a in string vector map algorithm; do +for a in string vector set map algorithm; do cmake_replace_string "${cmake_bootstrap_dir}/cmsys/stl/stl.hxx_b" \ "${cmake_bootstrap_dir}/cmsys/stl/${a}" KWSYS_STL_HEADER ${a} done @@ -1396,6 +1425,7 @@ if [ "x${cmake_cxx_flags}" != "x" ]; then fi cmake_c_flags_String="-DKWSYS_STRING_C" +cmake_cxx_flags_SystemTools="-DKWSYS_CXX_HAS_SETENV=${KWSYS_CXX_HAS_SETENV} -DKWSYS_CXX_HAS_UNSETENV=${KWSYS_CXX_HAS_UNSETENV} -DKWSYS_CXX_HAS_ENVIRON_IN_STDLIB_H=${KWSYS_CXX_HAS_ENVIRON_IN_STDLIB_H}" cmake_c_flags="${cmake_c_flags}-I`cmake_escape \"${cmake_bootstrap_dir}\"` -I`cmake_escape \"${cmake_source_dir}/Source\"` \ -I`cmake_escape \"${cmake_bootstrap_dir}\"`" cmake_cxx_flags="${cmake_cxx_flags} -I`cmake_escape \"${cmake_bootstrap_dir}\"` -I`cmake_escape \"${cmake_source_dir}/Source\"` \ @@ -1421,8 +1451,9 @@ for a in ${KWSYS_C_SOURCES} ${KWSYS_C_MINGW_SOURCES}; do done for a in ${KWSYS_CXX_SOURCES}; do src=`cmake_escape "${cmake_source_dir}/Source/kwsys/${a}.cxx"` + src_flags=`eval echo \\${cmake_cxx_flags_\${a}}` echo "${a}.o : ${src} ${dep}" >> "${cmake_bootstrap_dir}/Makefile" - echo " ${cmake_cxx_compiler} ${cmake_cxx_flags} -DKWSYS_NAMESPACE=cmsys -c ${src} -o ${a}.o" >> "${cmake_bootstrap_dir}/Makefile" + echo " ${cmake_cxx_compiler} ${cmake_cxx_flags} -DKWSYS_NAMESPACE=cmsys ${src_flags} -c ${src} -o ${a}.o" >> "${cmake_bootstrap_dir}/Makefile" done if ${cmake_system_mingw}; then src=`cmake_escape "${cmake_bootstrap_dir}/cmsysProcessFwd9xEnc.c"`