[Python-checkins] bpo-38630: Fix subprocess.Popen.send_signal() race condition (GH-16984)

[Victor Stinner]

https://github.com/python/cpython/commit/e85a305503bf83c5a8ffb3a988dfe7b67461cbee
commit: e85a305503bf83c5a8ffb3a988dfe7b67461cbee
branch: master
author: Victor Stinner <vstinner at python.org>
committer: GitHub <noreply at github.com>
date: 2020-01-15T17:38:55+01:00
summary:

bpo-38630: Fix subprocess.Popen.send_signal() race condition (GH-16984)

On Unix, subprocess.Popen.send_signal() now polls the process status.
Polling reduces the risk of sending a signal to the wrong process if
the process completed, the Popen.returncode attribute is still None,
and the pid has been reassigned (recycled) to a new different
process.

files:
A Misc/NEWS.d/next/Library/2019-10-29-12-21-10.bpo-38630.Dv6Xrm.rst
M Doc/library/subprocess.rst
M Lib/subprocess.py
M Lib/test/test_subprocess.py

diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst
index f2e5463d755bb..74857480360dc 100644
--- a/Doc/library/subprocess.rst
+++ b/Doc/library/subprocess.rst
@@ -774,6 +774,8 @@ Instances of the :class:`Popen` class have the following methods:
 
    Sends the signal *signal* to the child.
 
+   Do nothing if the process completed.
+
    .. note::
 
       On Windows, SIGTERM is an alias for :meth:`terminate`. CTRL_C_EVENT and
diff --git a/Lib/subprocess.py b/Lib/subprocess.py
index 30f0d1be154c4..79dffd349a30e 100644
--- a/Lib/subprocess.py
+++ b/Lib/subprocess.py
@@ -2061,9 +2061,31 @@ def _save_input(self, input):
 
         def send_signal(self, sig):
             """Send a signal to the process."""
-            # Skip signalling a process that we know has already died.
-            if self.returncode is None:
-                os.kill(self.pid, sig)
+            # bpo-38630: Polling reduces the risk of sending a signal to the
+            # wrong process if the process completed, the Popen.returncode
+            # attribute is still None, and the pid has been reassigned
+            # (recycled) to a new different process. This race condition can
+            # happens in two cases.
+            #
+            # Case 1. Thread A calls Popen.poll(), thread B calls
+            # Popen.send_signal(). In thread A, waitpid() succeed and returns
+            # the exit status. Thread B calls kill() because poll() in thread A
+            # did not set returncode yet. Calling poll() in thread B prevents
+            # the race condition thanks to Popen._waitpid_lock.
+            #
+            # Case 2. waitpid(pid, 0) has been called directly, without
+            # using Popen methods: returncode is still None is this case.
+            # Calling Popen.poll() will set returncode to a default value,
+            # since waitpid() fails with ProcessLookupError.
+            self.poll()
+            if self.returncode is not None:
+                # Skip signalling a process that we know has already died.
+                return
+
+            # The race condition can still happen if the race condition
+            # described above happens between the returncode test
+            # and the kill() call.
+            os.kill(self.pid, sig)
 
         def terminate(self):
             """Terminate the process with SIGTERM
diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py
index 2073fd146177a..f1fb93455dd7d 100644
--- a/Lib/test/test_subprocess.py
+++ b/Lib/test/test_subprocess.py
@@ -3120,6 +3120,31 @@ def test_stopped(self):
 
         self.assertEqual(returncode, -3)
 
+    def test_send_signal_race(self):
+        # bpo-38630: send_signal() must poll the process exit status to reduce
+        # the risk of sending the signal to the wrong process.
+        proc = subprocess.Popen(ZERO_RETURN_CMD)
+
+        # wait until the process completes without using the Popen APIs.
+        pid, status = os.waitpid(proc.pid, 0)
+        self.assertEqual(pid, proc.pid)
+        self.assertTrue(os.WIFEXITED(status), status)
+        self.assertEqual(os.WEXITSTATUS(status), 0)
+
+        # returncode is still None but the process completed.
+        self.assertIsNone(proc.returncode)
+
+        with mock.patch("os.kill") as mock_kill:
+            proc.send_signal(signal.SIGTERM)
+
+        # send_signal() didn't call os.kill() since the process already
+        # completed.
+        mock_kill.assert_not_called()
+
+        # Don't check the returncode value: the test reads the exit status,
+        # so Popen failed to read it and uses a default returncode instead.
+        self.assertIsNotNone(proc.returncode)
+
 
 @unittest.skipUnless(mswindows, "Windows specific tests")
 class Win32ProcessTestCase(BaseTestCase):
diff --git a/Misc/NEWS.d/next/Library/2019-10-29-12-21-10.bpo-38630.Dv6Xrm.rst b/Misc/NEWS.d/next/Library/2019-10-29-12-21-10.bpo-38630.Dv6Xrm.rst
new file mode 100644
index 0000000000000..1a4d59205ab18
--- /dev/null
+++ b/Misc/NEWS.d/next/Library/2019-10-29-12-21-10.bpo-38630.Dv6Xrm.rst
@@ -0,0 +1,5 @@
+On Unix, :meth:`subprocess.Popen.send_signal` now polls the process status.
+Polling reduces the risk of sending a signal to the wrong process if the
+process completed, the :attr:`subprocess.Popen.returncode` attribute is still
+``None``, and the pid has been reassigned (recycled) to a new different
+process.