HPCC-24238 Unsafe code calling setenv after fork

Signed-off-by: Richard Chapman <rchapman@hpccsystems.com>

system/jlib/jmisc.cpp

@@ -35,6 +35,14 @@
 #include <pwd.h>
 #endif
 
+#ifdef _WIN32
+ #include <stdlib.h>
+#elif defined(__linux__) || defined(__FreeBSD__)
+ extern char **environ;
+#elif defined(__APPLE__)
+#include <crt_externs.h>
+#endif
+
 #ifdef LOGCLOCK
 #define MSGFIELD_PRINTLOG MSGFIELD_timeDate | MSGFIELD_msgID | MSGFIELD_process | MSGFIELD_thread | MSGFIELD_code | MSGFIELD_milliTime
 #else
@@ -1002,3 +1010,14 @@ char *mkdtemp(char *_template)
     return nullptr;
 }
 #endif
+
+jlib_decl char **getSystemEnv()
+{
+#ifdef _WIN32
+    return _environ;
+#elif defined (__APPLE__)
+    return *_NSGetEnviron();
+#else
+    return environ;
+#endif
+}

system/jlib/jmisc.hpp

@@ -332,4 +332,5 @@ public:
 extern jlib_decl char *mkdtemp(char *_template);
 #endif
 
+extern jlib_decl char **getSystemEnv();
 #endif

system/jlib/jprop.cpp

@@ -23,15 +23,8 @@
 #include "jexcept.hpp"
 #include "jiter.ipp"
 #include "jregexp.hpp"
+#include "jmisc.hpp"
 #include <stdio.h>
-#ifdef _WIN32
- #include <stdlib.h>
-#elif defined(__linux__) || defined(__FreeBSD__)
- extern char **environ;
-#endif
-#if defined(__APPLE__)
-#include <crt_externs.h>
-#endif
 
 const char *conv2char_ptr(const char *p) { return p; }
 const char *convchar_ptr2(const char *p) { return p; }
@@ -143,15 +136,7 @@ public:
     }
     void loadSystem()
     {
-#ifdef _WIN32
-        char **e = _environ;
-#else
-#if defined (__APPLE__)
-        char **e = *_NSGetEnviron();
-#else
-        char **e = environ;
-#endif
-#endif
+        char **e = getSystemEnv();
         while (*e)
         {
             loadProp (*e);

system/jlib/jptree.cpp

@@ -25,13 +25,9 @@
 #include "jregexp.hpp"
 #include "jstring.hpp"
 #include "jutil.hpp"
+#include "jmisc.hpp"
 #include "yaml.h"
 
-#ifdef __APPLE__
-#include <crt_externs.h>
-#define environ (*_NSGetEnviron())
-#endif
-
 #include <initializer_list>
 
 #define MAKE_LSTRING(name,src,length) \
@@ -8057,7 +8053,7 @@ jlib_decl IPropertyTree * loadConfiguration(const char * defaultYaml, const char
     if (delta)
         mergeConfiguration(*config, *delta, altNameAttribute);
 
-    const char * * environment = const_cast<const char * *>(environ);
+    const char * * environment = const_cast<const char * *>(getSystemEnv());
     for (const char * * cur = environment; *cur; cur++)
     {
         applyEnvironmentConfig(*config, envPrefix, *cur);

system/jlib/jthread.cpp

@@ -1427,8 +1427,7 @@ class CWindowsPipeProcess: implements IPipeProcess, public CInterface
     CriticalSection sect;
     bool aborted;
     StringAttr allowedprogs;
-    StringArray envVars;
-    StringArray envValues;
+    StringArray env;
 
 public:
     IMPLEMENT_IINTERFACE;
@@ -1507,7 +1506,7 @@ public:
         
         PROCESS_INFORMATION ProcessInformation;
 
-        // MORE - should create a new environment block that is copy of parent's, then set all the values in envVars/envValues, and pass it
+        // MORE - should create a new environment block that is copy of parent's, then set all the values in env array, and pass it
 
         if (!CreateProcess(NULL, (char *)prog, NULL,NULL,TRUE,0,NULL, dir&&*dir?dir:NULL, &StartupInfo,&ProcessInformation)) {
             if (_title) {
@@ -1533,8 +1532,29 @@ public:
         assertex(var);
         if (!value)
             value = "";
-        envVars.append(var);
-        envValues.append(value);
+        VStringBuffer s("%s=%s", var, value);
+        aindex_t found = lookupEnv(s);
+        if (found != NotFound)
+            env.replace(s, found);
+        else
+            env.append(s);
+    }
+
+    aindex_t lookupEnv(const char *_s)
+    {
+        ForEachItemIn(idx, env)
+        {
+            const char *v = env.item(idx);
+            const char *s = _s;
+            while (*s && *v && *s==*v)
+            {
+                if (*s=='=')
+                    return idx;
+                s++;
+                v++;
+            }
+        }
+        return NotFound;
     }
 
     size32_t read(size32_t sz, void *buf)
@@ -1942,8 +1962,7 @@ protected: friend class PipeWriterThread;
     MemoryBuffer stderrbuf;
     size32_t stderrbufsize;
     StringAttr allowedprogs;
-    StringArray envVars;
-    StringArray envValues;
+    StringArray env;
 
     void clearUtilityThreads(bool clearStderr)
     {
@@ -2031,6 +2050,24 @@ public:
             started.signal();
             throw makeOsException(errno);
         }
+        ConstPointerArrayOf<char> envp;
+        if (env.length())
+        {
+            ForEachItemIn(idx, env)
+            {
+                envp.append(env.item(idx));
+            }
+            // Now add any existing environment variables that were not overridden
+            char **e = getSystemEnv();
+            while (*e)
+            {
+                const char *entry = *e++;
+                if (lookupEnv(entry) != NotFound)
+                    continue;
+                envp.append(entry);
+            }
+            envp.append(nullptr);
+        }
 
         /* NB: Important to call splitargs (which calls malloc) before the fork()
          * and not in the child process. Because performing malloc in the child
@@ -2062,6 +2099,9 @@ public:
                 return;
             }
         }
+        // NOTE - from here to the execvp/_exit call, we must only call "signal-safe" functions, that do not do any memory allocation
+        // fork() only clones the one thread from parent process, meaning any mutexes/semaphores protecting multi-threaded access
+        // to (for example) malloc cannot be relied upon not to be locked, and will never be unlocked if they are.
         if (pipeProcess==0) { // child
             if (newProcessGroup)//Force the child process into its own process group, so we can terminate it and its children.
                 setpgid(0,0);
@@ -2085,11 +2125,10 @@ public:
                 if (chdir(dir) == -1)
                     throw MakeStringException(-1, "CLinuxPipeProcess::run: could not change dir to %s", dir.get());
             }
-            ForEachItemIn(idx, envVars)
-            {
-                ::setenv(envVars.item(idx), envValues.item(idx), 1);
-            }
-            execvp(argv[0],argv);
+            if (envp.length())
+                execve(argv[0], argv, (char *const *) envp.detach());
+            else
+                execvp(argv[0], argv);
             if (haserror)
             {
                 Owned<IException> e = createPipeErrnoExceptionV(errno, "exec failed: %s", prog.get());
@@ -2175,10 +2214,31 @@ public:
         assertex(var);
         if (!value)
             value = "";
-        envVars.append(var);
-        envValues.append(value);
+        VStringBuffer s("%s=%s", var, value);
+        aindex_t found = lookupEnv(s);
+        if (found != NotFound)
+            env.replace(s, found);
+        else
+            env.append(s);
     }
-    
+
+    aindex_t lookupEnv(const char *_s)
+    {
+        ForEachItemIn(idx, env)
+        {
+            const char *v = env.item(idx);
+            const char *s = _s;
+            while (*s && *v && *s==*v)
+            {
+                if (*s=='=')
+                    return idx;
+                s++;
+                v++;
+            }
+        }
+        return NotFound;
+    }
+
     size32_t read(size32_t sz, void *buf)
     {
         CriticalBlock block(sect); 

testing/unittests/unittests.cpp

@@ -831,4 +831,37 @@ class ThreadedPersistStressTest : public CppUnit::TestFixture
 
 CPPUNIT_TEST_SUITE_REGISTRATION( ThreadedPersistStressTest );
 CPPUNIT_TEST_SUITE_NAMED_REGISTRATION( ThreadedPersistStressTest, "ThreadedPersistStressTest" );
+
+
+#ifndef _WIN32
+class PipeRunTest : public CppUnit::TestFixture
+{
+    CPPUNIT_TEST_SUITE( PipeRunTest  );
+        CPPUNIT_TEST(testRun);
+    CPPUNIT_TEST_SUITE_END();
+
+    void testRun()
+    {
+        Owned<IPipeProcess> pipe = createPipeProcess();
+        setenv("OLDVAR", "old", 1);
+        setenv("TESTVAR", "oldtest", 1);
+        pipe->setenv("TESTVAR", "well");
+        pipe->setenv("TESTVAR", "hello");
+        pipe->setenv("AX", "ax");
+        pipe->setenv("BCD", "bcd");
+        pipe->setenv("ABCD", "abcd");
+        ASSERT(pipe->run("/bin/bash", "/bin/bash -c 'echo $TESTVAR $OLDVAR $AX $BCD $ABCD'", ".", false, true, false));
+        byte buf[4096];
+        size32_t read = pipe->read(sizeof(buf),buf);
+        ASSERT(read==22);
+        ASSERT(memcmp(buf, "hello old ax bcd abcd\n", 22)==0);
+        ASSERT(pipe->wait()==0);
+
+    }
+};
+
+CPPUNIT_TEST_SUITE_REGISTRATION( PipeRunTest );
+CPPUNIT_TEST_SUITE_NAMED_REGISTRATION( PipeRunTest, "PipeRunTest" );
+#endif
+
 #endif // _USE_CPPUNIT