Fix incorrect env handling in spawnProcess on Posix

Documentation on reactor.spawnProcess says the following about env parameter: env is None: On POSIX: pass os.environ This was not correctly handled in os.posix_spawnp() code path.
charithmadhuranga · Jan 24, 2024 · 90c56ab · 90c56ab
1 parent a4ab53c
commit 90c56ab
Show file tree

Hide file tree

Showing 3 changed files with 129 additions and 1 deletion.
diff --git a/src/twisted/internet/process.py b/src/twisted/internet/process.py
@@ -879,7 +879,7 @@ def _trySpawnInsteadOfFork(
             else:
                 fdState.append((eachFD, isCloseOnExec))
         if environment is None:
-            environment = {}
+            environment = os.environ
 
         setSigDef = [
             everySignal

diff --git a/src/twisted/internet/test/test_process.py b/src/twisted/internet/test/test_process.py
@@ -29,6 +29,7 @@
 from twisted.python.filepath import FilePath, _asFilesystemBytes
 from twisted.python.log import err, msg
 from twisted.python.runtime import platform
+from twisted.test.test_process import Accumulator
 from twisted.trial.unittest import SynchronousTestCase, TestCase
 
 # Get the current Python executable as a bytestring.
@@ -1001,6 +1002,132 @@ def launchProcessAndWait(reactor):
             hamcrest.equal_to(["process already removed as desired"]),
         )
 
+    def checkSpawnProcessEnvironment(self, spawnKwargs, expectedEnv, usePosixSpawnp):
+        """
+        Shared code for testing the environment variables
+        present in the spawned process.
+
+        The spawned process serializes its environ to stderr or stdout (depending on usePTY)
+        which is checked against os.environ of the calling process.
+        """
+        p = Accumulator()
+        d = p.endedDeferred = Deferred()
+
+        reactor = self.buildReactor()
+        reactor._neverUseSpawn = not usePosixSpawnp
+
+        reactor.callWhenRunning(
+            reactor.spawnProcess,
+            p,
+            pyExe,
+            [
+                pyExe,
+                b"-c",
+                networkString(
+                    "import os, sys; "
+                    "env = dict(os.environ); "
+                    # LC_CTYPE is set by python, see https://peps.python.org/pep-0538/
+                    'env.pop("LC_CTYPE", None); '
+                    'env.pop("__CF_USER_TEXT_ENCODING", None); '
+                    "sys.stderr.write(str(sorted(env.items())))"
+                ),
+            ],
+            usePTY=self.usePTY,
+            **spawnKwargs,
+        )
+
+        def shutdown(ign):
+            reactor.stop()
+
+        d.addBoth(shutdown)
+
+        self.runReactor(reactor)
+
+        expectedEnv.pop("LC_CTYPE", None)
+        expectedEnv.pop("__CF_USER_TEXT_ENCODING", None)
+        self.assertEqual(
+            bytes(str(sorted(expectedEnv.items())), "utf-8"),
+            p.outF.getvalue() if self.usePTY else p.errF.getvalue(),
+        )
+
+    def checkSpawnProcessEnvironmentWithPosixSpawnp(self, spawnKwargs, expectedEnv):
+        return self.checkSpawnProcessEnvironment(
+            spawnKwargs, expectedEnv, usePosixSpawnp=True
+        )
+
+    def checkSpawnProcessEnvironmentWithFork(self, spawnKwargs, expectedEnv):
+        return self.checkSpawnProcessEnvironment(
+            spawnKwargs, expectedEnv, usePosixSpawnp=False
+        )
+
+    @onlyOnPOSIX
+    def test_environmentPosixSpawnpEnvNotSet(self):
+        """
+        An empty environment is passed to the spawned process, when the default value of the C{env}
+        is used. That is, when the C{env} argument is not explicitly set.
+
+        In this case posix_spawnp is used as the backend for spawning processes.
+        """
+        return self.checkSpawnProcessEnvironmentWithPosixSpawnp({}, {})
+
+    @onlyOnPOSIX
+    def test_environmentForkEnvNotSet(self):
+        """
+        An empty environment is passed to the spawned process, when the default value of the C{env}
+        is used. That is, when the C{env} argument is not explicitly set.
+
+        In this case fork+execvpe is used as the backend for spawning processes.
+        """
+        return self.checkSpawnProcessEnvironmentWithFork({}, {})
+
+    @onlyOnPOSIX
+    def test_environmentPosixSpawnpEnvNone(self):
+        """
+        The parent process environment is passed to the spawned process, when C{env} is set to
+        C{None}.
+
+        In this case posix_spawnp is used as the backend for spawning processes.
+        """
+        return self.checkSpawnProcessEnvironmentWithPosixSpawnp(
+            {"env": None}, os.environ
+        )
+
+    @onlyOnPOSIX
+    def test_environmentForkEnvNone(self):
+        """
+        The parent process environment is passed to the spawned process, when C{env} is set to
+        C{None}.
+
+        In this case fork+execvpe is used as the backend for spawning processes.
+        """
+        return self.checkSpawnProcessEnvironmentWithFork({"env": None}, os.environ)
+
+    @onlyOnPOSIX
+    def test_environmentPosixSpawnpEnvCustom(self):
+        """
+        The user-specified environment without extra variables from parent process is passed to the
+        spawned process, when C{env} is set to a dictionary.
+
+        In this case posix_spawnp is used as the backend for spawning processes.
+        """
+        return self.checkSpawnProcessEnvironmentWithPosixSpawnp(
+            {"env": {"MYENV1": "myvalue1"}},
+            {"MYENV1": "myvalue1"},
+        )
+
+    @onlyOnPOSIX
+    def test_environmentForkEnvCustom(self):
+        """
+        The user-specified environment without extra variables from parent process is passed to the
+        spawned process, when C{env} is set to a dictionary.
+
+        In this case fork+execvpe is used as the backend for spawning processes.
+        """
+        return self.checkSpawnProcessEnvironmentWithFork(
+            {"env": {"MYENV1": "myvalue1"}},
+            {"MYENV1": "myvalue1"},
+        )
+
 
 globals().update(ProcessTestsBuilder.makeTestCaseClasses())
 

diff --git a/src/twisted/newsfragments/12068.bugfix b/src/twisted/newsfragments/12068.bugfix
@@ -0,0 +1 @@
+twisted.internet.process.Process, used by ``reactor.spawnProcess``, now copies the parent environment when the `env=None` argument is passed on Posix systems and ``os.posix_spawnp`` is used internally.
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		twisted.internet.process.Process, used by ``reactor.spawnProcess``, now copies the parent environment when the `env=None` argument is passed on Posix systems and ``os.posix_spawnp`` is used internally.