Merge topic 'kwsys-environ-cleanup'

e48796b KWSys: Fix SystemTools environment memory handling (#13156)
b10c5cb CTest: Simplify environment save/restore

10 files changed, 336 insertions, 91 deletions

diff --git a/Source/CTest/cmCTestRunTest.cxx b/Source/CTest/cmCTestRunTest.cxx
index 81f18b0..c3de5dc 100644
--- a/Source/CTest/cmCTestRunTest.cxx
+++ b/Source/CTest/cmCTestRunTest.cxx
@@ -671,7 +671,7 @@ bool cmCTestRunTest::ForkProcess(double testTimeOut, bool explicitTimeout,
 
   if (environment && environment->size()>0)
     {
-    cmSystemTools::AppendEnv(environment);
+    cmSystemTools::AppendEnv(*environment);
     }
 
   return this->TestProcess->StartProcess();
diff --git a/Source/CTest/cmCTestScriptHandler.cxx b/Source/CTest/cmCTestScriptHandler.cxx
index 5841b8d..d3ab2ef 100644
--- a/Source/CTest/cmCTestScriptHandler.cxx
+++ b/Source/CTest/cmCTestScriptHandler.cxx
@@ -643,11 +643,7 @@ int cmCTestScriptHandler::RunCurrentScript()
     {
     std::vector<std::string> envArgs;
     cmSystemTools::ExpandListArgument(this->CTestEnv.c_str(),envArgs);
-    // for each variable/argument do a putenv
-    for (unsigned i = 0; i < envArgs.size(); ++i)
-      {
-      cmSystemTools::PutEnv(envArgs[i].c_str());
-      }
+    cmSystemTools::AppendEnv(envArgs);
     }
 
   // now that we have done most of the error checking finally run the
diff --git a/Source/cmCTest.cxx b/Source/cmCTest.cxx
index 3f7fdc7..4aff64b 100644
--- a/Source/cmCTest.cxx
+++ b/Source/cmCTest.cxx
@@ -48,7 +48,7 @@
 #include <float.h>
 #include <ctype.h>
 
-#include <memory> // auto_ptr
+#include <cmsys/auto_ptr.hxx>
 
 #include <cm_zlib.h>
 #include <cmsys/Base64.h>
@@ -509,7 +509,7 @@ int cmCTest::Initialize(const char* binary_dir, cmCTestStartCommand* command)
   cmake cm;
   cmGlobalGenerator gg;
   gg.SetCMakeInstance(&cm);
-  std::auto_ptr<cmLocalGenerator> lg(gg.CreateLocalGenerator());
+  cmsys::auto_ptr<cmLocalGenerator> lg(gg.CreateLocalGenerator());
   cmMakefile *mf = lg->GetMakefile();
   if ( !this->ReadCustomConfigurationFileTree(this->BinaryDir.c_str(), mf) )
     {
@@ -1277,7 +1277,6 @@ int cmCTest::RunTest(std::vector<const char*> argv,
                      std::ostream* log, double testTimeOut,
                      std::vector<std::string>* environment)
 {
-  std::vector<std::string> origEnv;
   bool modifyEnv = (environment && environment->size()>0);
 
   // determine how much time we have
@@ -1334,9 +1333,11 @@ int cmCTest::RunTest(std::vector<const char*> argv,
       }
     std::string oldpath = cmSystemTools::GetCurrentWorkingDirectory();
 
+    cmsys::auto_ptr<cmSystemTools::SaveRestoreEnvironment> saveEnv;
     if (modifyEnv)
       {
-      origEnv = cmSystemTools::AppendEnv(environment);
+      saveEnv.reset(new cmSystemTools::SaveRestoreEnvironment);
+      cmSystemTools::AppendEnv(*environment);
       }
 
     *retVal = inst.Run(args, output);
@@ -1351,11 +1352,6 @@ int cmCTest::RunTest(std::vector<const char*> argv,
       "Internal cmCTest object used to run test." << std::endl
       <<  *output << std::endl);
 
-    if (modifyEnv)
-      {
-      cmSystemTools::RestoreEnv(origEnv);
-      }
-
     return cmsysProcess_State_Exited;
     }
   std::vector<char> tempOutput;
@@ -1364,9 +1360,11 @@ int cmCTest::RunTest(std::vector<const char*> argv,
     *output = "";
     }
 
+  cmsys::auto_ptr<cmSystemTools::SaveRestoreEnvironment> saveEnv;
   if (modifyEnv)
     {
-    origEnv = cmSystemTools::AppendEnv(environment);
+    saveEnv.reset(new cmSystemTools::SaveRestoreEnvironment);
+    cmSystemTools::AppendEnv(*environment);
     }
 
   cmsysProcess* cp = cmsysProcess_New();
@@ -1436,11 +1434,6 @@ int cmCTest::RunTest(std::vector<const char*> argv,
     }
   cmsysProcess_Delete(cp);
 
-  if (modifyEnv)
-    {
-    cmSystemTools::RestoreEnv(origEnv);
-    }
-
   return result;
 }
 
diff --git a/Source/cmSystemTools.cxx b/Source/cmSystemTools.cxx
index 25975e0..89f03f6 100644
--- a/Source/cmSystemTools.cxx
+++ b/Source/cmSystemTools.cxx
@@ -1633,33 +1633,28 @@ std::vector<std::string> cmSystemTools::GetEnvironmentVariables()
 }
 
 //----------------------------------------------------------------------
-std::vector<std::string> cmSystemTools::AppendEnv(
-  std::vector<std::string>* env)
+void cmSystemTools::AppendEnv(std::vector<std::string> const& env)
 {
-  std::vector<std::string> origEnv = GetEnvironmentVariables();
-
-  if (env && env->size()>0)
+  for(std::vector<std::string>::const_iterator eit = env.begin();
+      eit != env.end(); ++eit)
     {
-    std::vector<std::string>::const_iterator eit;
-
-    for (eit = env->begin(); eit!= env->end(); ++eit)
-      {
-      PutEnv(eit->c_str());
-      }
+    cmSystemTools::PutEnv(eit->c_str());
     }
-
-  return origEnv;
 }
 
 //----------------------------------------------------------------------
-void cmSystemTools::RestoreEnv(const std::vector<std::string>& env)
+cmSystemTools::SaveRestoreEnvironment::SaveRestoreEnvironment()
 {
-  std::vector<std::string>::const_iterator eit;
+  this->Env = cmSystemTools::GetEnvironmentVariables();
+}
 
+//----------------------------------------------------------------------
+cmSystemTools::SaveRestoreEnvironment::~SaveRestoreEnvironment()
+{
   // First clear everything in the current environment:
-  //
   std::vector<std::string> currentEnv = GetEnvironmentVariables();
-  for (eit = currentEnv.begin(); eit!= currentEnv.end(); ++eit)
+  for(std::vector<std::string>::const_iterator
+        eit = currentEnv.begin(); eit != currentEnv.end(); ++eit)
     {
     std::string var(*eit);
 
@@ -1669,27 +1664,11 @@ void cmSystemTools::RestoreEnv(const std::vector<std::string>& env)
       var = var.substr(0, pos);
       }
 
-    UnsetEnv(var.c_str());
+    cmSystemTools::UnsetEnv(var.c_str());
     }
 
   // Then put back each entry from the original environment:
-  //
-  for (eit = env.begin(); eit!= env.end(); ++eit)
-    {
-    PutEnv(eit->c_str());
-    }
-}
-
-//----------------------------------------------------------------------
-cmSystemTools::SaveRestoreEnvironment::SaveRestoreEnvironment()
-{
-  this->Env = cmSystemTools::GetEnvironmentVariables();
-}
-
-//----------------------------------------------------------------------
-cmSystemTools::SaveRestoreEnvironment::~SaveRestoreEnvironment()
-{
-  cmSystemTools::RestoreEnv(this->Env);
+  cmSystemTools::AppendEnv(this->Env);
 }
 #endif
 
diff --git a/Source/cmSystemTools.h b/Source/cmSystemTools.h
index 5f21de2..69673c9 100644
--- a/Source/cmSystemTools.h
+++ b/Source/cmSystemTools.h
@@ -371,16 +371,8 @@ public:
   /** Get the list of all environment variables */
   static std::vector<std::string> GetEnvironmentVariables();
 
-  /** Append multiple variables to the current environment.
-      Return the original environment, as it was before the
-      append. */
-  static std::vector<std::string> AppendEnv(
-    std::vector<std::string>* env);
-
-  /** Restore the full environment to "env" - use after
-      AppendEnv to put the environment back to the way it
-      was. */
-  static void RestoreEnv(const std::vector<std::string>& env);
+  /** Append multiple variables to the current environment. */
+  static void AppendEnv(std::vector<std::string> const& env);
 
   /** Helper class to save and restore the environment.
       Instantiate this class as an automatic variable on
diff --git a/Source/kwsys/CMakeLists.txt b/Source/kwsys/CMakeLists.txt
index ca496ab..520cafb 100644
--- a/Source/kwsys/CMakeLists.txt
+++ b/Source/kwsys/CMakeLists.txt
@@ -552,11 +552,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 4d83293..8ab580f 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<char*>
+#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;
+}
 
-kwsysDeletingCharVector::~kwsysDeletingCharVector()
+#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)
 {
-#ifndef KWSYS_DO_NOT_CLEAN_PUTENV
-  for(kwsys_stl::vector<char*>::iterator i = this->begin();
-      i != this->end(); ++i)
+  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)
     {
-    delete []*i;
+    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::PutEnv(const char* value)
+
+bool SystemTools::UnPutEnv(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 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;
+      }
+    }
+};
+
+class kwsysEnv: public kwsys_stl::set<const char*, kwsysEnvCompare>
+{
+  class Free
+  {
+    const char* Env;
+  public:
+    Free(const char* env): Env(env) {}
+    ~Free() { free(const_cast<char*>(this->Env)); }
+  };
+public:
+  typedef kwsys_stl::set<const char*, kwsysEnvCompare> derived;
+  ~kwsysEnv()
+    {
+    for(derived::iterator i = this->begin(); i != this->end(); ++i)
+      {
+      kwsysUnPutEnv(*i);
+      free(const_cast<char*>(*i));
+      }
+    }
+  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<void>(oldEnv);
+    char* newEnv = strdup(env);
+    this->insert(newEnv);
+    return putenv(newEnv) == 0;
+    }
+  bool UnPut(const char* env)
+    {
+    Free oldEnv(this->Release(env));
+    static_cast<void>(oldEnv);
+    return kwsysUnPutEnv(env) == 0;
+    }
+};
+
+static kwsysEnv kwsysEnvInstance;
+
+bool SystemTools::PutEnv(const char* env)
+{
+  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 04f1978..5171125 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 903be9b..16124d3 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 <stdlib.h>
+int main()
+{
+  return setenv("A", "B", 1);
+}
+#endif
+
+#ifdef TEST_KWSYS_CXX_HAS_UNSETENV
+#include <stdlib.h>
+int main()
+{
+  unsetenv("A");
+  return 0;
+}
+#endif
+
+#ifdef TEST_KWSYS_CXX_HAS_ENVIRON_IN_STDLIB_H
+#include <stdlib.h>
+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 c0e74af..3ac0cb3 100644
--- a/Source/kwsys/testSystemTools.cxx
+++ b/Source/kwsys/testSystemTools.cxx
@@ -328,6 +328,58 @@ bool CheckStringOperations()
 }
 
 //----------------------------------------------------------------------------
+
+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*[])
 {
   bool res = true;
@@ -356,5 +408,7 @@ int testSystemTools(int, char*[])
 
   res &= CheckStringOperations();
 
+  res &= CheckEnvironmentOperations();
+
   return res ? 0 : 1;
 }