fixup environment inheritance

obsoletes D825704?  populate our env array from environ,
then overlay local changes that were made to m_envs.

I didn't make this conditional on CLI but could if that is desirable.

I didn't tackle the allocate-after-fork issue that @jdelong raised in D825704

hphp/runtime/base/execution_context.h

@@ -318,6 +318,8 @@ public:
    */
   String getenv(CStrRef name) const;
   void setenv(CStrRef name, CStrRef value);
+  Array getEnvs() const { return m_envs; }
+
   String getTimeZone() const { return m_timezone;}
   void setTimeZone(CStrRef timezone) { m_timezone = timezone;}
   String getDefaultTimeZone() const { return m_timezoneDefault;}

hphp/runtime/ext/ext_process.cpp

@@ -34,6 +34,7 @@
 # define _NSIG NSIG
 #endif
 
+extern char **environ;
 
 namespace HPHP {
 
@@ -761,6 +762,38 @@ Variant f_proc_open(CStrRef cmd, CArrRef descriptorspec, VRefParam pipes,
     scwd = g_context->getCwd().c_str();
   }
 
+  Array enva;
+
+  if (env.isNull()) {
+    // Build out an environment that conceptually matches what we'd
+    // see if we were to iterate the environment and call getenv()
+    // for each name.
+
+    // Env vars defined in the hdf file go in first
+    for (std::map<string, string>::const_iterator iter =
+        RuntimeOption::EnvVariables.begin();
+        iter != RuntimeOption::EnvVariables.end(); ++iter) {
+      enva.set(String(iter->first), String(iter->second));
+    }
+
+    // global environment overrides the hdf
+    for (char **env = environ; env && *env; env++) {
+      char *p = strchr(*env, '=');
+      if (p) {
+        String name(*env, p - *env, CopyString);
+        String val(p + 1, CopyString);
+        enva.set(name, val);
+      }
+    }
+
+    // and then any putenv() changes take precedence
+    for (ArrayIter iter(g_context->getEnvs()); iter; ++iter) {
+      enva.set(iter.first(), iter.second());
+    }
+  } else {
+    enva = env.toArray();
+  }
+
   pid_t child;
 
   if (LightProcess::Available()) {
@@ -776,7 +809,7 @@ Variant f_proc_open(CStrRef cmd, CArrRef descriptorspec, VRefParam pipes,
     }
 
     std::vector<std::string> envs;
-    for (ArrayIter iter(env.toArray()); iter; ++iter) {
+    for (ArrayIter iter(enva); iter; ++iter) {
       StringBuffer nvpair;
       nvpair += iter.first().toString();
       nvpair += '=';
@@ -787,7 +820,7 @@ Variant f_proc_open(CStrRef cmd, CArrRef descriptorspec, VRefParam pipes,
     child = LightProcess::proc_open(cmd.c_str(), created, intended,
                                     scwd.c_str(), envs);
     assert(child);
-    return post_proc_open(cmd, pipes, env, items, child);
+    return post_proc_open(cmd, pipes, enva, items, child);
   } else {
     /* the unix way */
     Lock lock(DescriptorItem::s_mutex);
@@ -795,7 +828,7 @@ Variant f_proc_open(CStrRef cmd, CArrRef descriptorspec, VRefParam pipes,
     child = fork();
     if (child) {
       // the parent process
-      return post_proc_open(cmd, pipes, env, items, child);
+      return post_proc_open(cmd, pipes, enva, items, child);
     }
   }
 
@@ -811,14 +844,10 @@ Variant f_proc_open(CStrRef cmd, CArrRef descriptorspec, VRefParam pipes,
   if (scwd.length() > 0 && chdir(scwd.c_str())) {
     // chdir failed, the working directory remains unchanged
   }
-  if (!env.isNull()) {
-    vector<String> senvs; // holding those char *
-    char **envp = build_envp(env.toArray(), senvs);
-    execle("/bin/sh", "sh", "-c", cmd.data(), NULL, envp);
-    free(envp);
-  } else {
-    execl("/bin/sh", "sh", "-c", cmd.data(), NULL);
-  }
+  vector<String> senvs; // holding those char *
+  char **envp = build_envp(enva, senvs);
+  execle("/bin/sh", "sh", "-c", cmd.data(), NULL, envp);
+  free(envp);
   _exit(127);
 }
 

hphp/test/test_ext_process.cpp

@@ -49,6 +49,7 @@ bool TestExtProcess::RunTests(const std::string &which) {
   RUN_TEST(test_proc_open);
   RUN_TEST(test_proc_terminate);
   RUN_TEST(test_proc_close);
+  RUN_TEST(test_proc_open_env_inh);
   RUN_TEST(test_proc_get_status);
   RUN_TEST(test_proc_nice);
   RUN_TEST(test_escapeshellarg);
@@ -79,6 +80,7 @@ bool TestExtProcess::RunTests(const std::string &which) {
   RUN_TEST(test_proc_open);
   RUN_TEST(test_proc_terminate);
   RUN_TEST(test_proc_close);
+  RUN_TEST(test_proc_open_env_inh);
   RUN_TEST(test_proc_get_status);
   RUN_TEST(test_proc_nice);
   LightProcess::Close();
@@ -284,6 +286,66 @@ bool TestExtProcess::test_system() {
   return Count(true);
 }
 
+bool TestExtProcess::test_proc_open_env_inh() {
+  Array descriptorspec =
+    CREATE_MAP3(0, CREATE_VECTOR2("pipe", "r"),
+                1, CREATE_VECTOR2("pipe", "w"),
+                2, CREATE_VECTOR3("file", "/tmp/error-output.txt", "a"));
+
+  Variant pipes;
+
+  g_context->setenv("inherit_me", "please");
+  Variant process = f_proc_open("echo $inherit_me", descriptorspec, ref(pipes));
+  VERIFY(!same(process, false));
+
+  {
+    File *f = pipes[1].toObject().getTyped<File>();
+    VERIFY(f->valid());
+    StringBuffer sbuf;
+    sbuf.read(f);
+    f->close();
+    VS(sbuf.detach(), "please\n");
+  }
+
+  VS(f_proc_close(process.toObject()), 0);
+
+  // Ensure that PATH makes it through too
+  process = f_proc_open("echo $PATH", descriptorspec, ref(pipes));
+  VERIFY(!same(process, false));
+
+  {
+    File *f = pipes[1].toObject().getTyped<File>();
+    VERIFY(f->valid());
+    StringBuffer sbuf;
+    sbuf.read(f);
+    f->close();
+
+    VERIFY(sbuf.length() != 0);
+  }
+
+  VS(f_proc_close(process.toObject()), 0);
+
+  // And check that the libc putenv() takes effect, even though we don't
+  // want to use that in a threaded environment
+
+  putenv("ZOO=animals");
+  process = f_proc_open("echo $ZOO", descriptorspec, ref(pipes));
+  VERIFY(!same(process, false));
+
+  {
+    File *f = pipes[1].toObject().getTyped<File>();
+    VERIFY(f->valid());
+    StringBuffer sbuf;
+    sbuf.read(f);
+    f->close();
+    VS(sbuf.detach(), "animals\n");
+  }
+
+  VS(f_proc_close(process.toObject()), 0);
+
+  return Count(true);
+}
+
 bool TestExtProcess::test_proc_open() {
   Array descriptorspec =
     CREATE_MAP3(0, CREATE_VECTOR2("pipe", "r"),

hphp/test/test_ext_process.h

@@ -49,6 +49,7 @@ class TestExtProcess : public TestCppExt {
   bool test_proc_open();
   bool test_proc_terminate();
   bool test_proc_close();
+  bool test_proc_open_env_inh();
   bool test_proc_get_status();
   bool test_proc_nice();
   bool test_escapeshellarg();