8000 bpo-38630: Fix subprocess.Popen.send_signal() race condition (GH-16984) · python/cpython@e85a305 · GitHub
[go: up one dir, main page]
Skip to content 8000

Commit e85a305

Browse files
vstinnervstinner
authored
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.
1 parent ed154c3 commit e85a305

File tree

4 files changed

+57
-3
lines changed

4 files changed

+57
-3
lines changed

‎Doc/library/subprocess.rst

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -774,6 +774,8 @@ Instances of the :class:`Popen` class have the following methods:
774774

775775
Sends the signal *signal* to the child.
776776

777+
Do nothing if the process completed.
778+
777779
.. note::
778780

779781
On Windows, SIGTERM is an alias for :meth:`terminate`. CTRL_C_EVENT and

‎Lib/subprocess.py

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2061,9 +2061,31 @@ def _save_input(self, input):
20612061

20622062
def send_signal(self, sig):
20632063
"""Send a signal to the process."""
2064-
# Skip signalling a process that we know has already died.
2065-
if self.returncode is None:
2066-
os.kill(self.pid, sig)
2064+
# bpo-38630: Polling reduces the risk of sending a signal to the
2065+
# wrong process if the process completed, the Popen.returncode
2066+
# attribute is still None, and the pid has been reassigned
2067+
# (recycled) to a new different process. This race condition can
2068+
# happens in two cases.
2069+
#
2070+
# Case 1. Thread A calls Popen.poll(), thread B calls
2071+
# Popen.send_signal(). In thread A, waitpid() succeed and returns
2072+
# the exit status. Thread B calls kill() because poll() in thread A
2073+
# did not set returncode yet. Calling poll() in thread B prevents
2074+
# the race condition thanks to Popen._waitpid_lock.
2075+
#
2076+
# Case 2. waitpid(pid, 0) has been called directly, without
2077+
# using Popen methods: returncode is still None is this case.
2078+
# Calling Popen.poll() will set returncode to a default value,
2079+
# since waitpid() fails with ProcessLookupError.
2080+
self.poll()
2081+
if self.returncode is not None:
2082+
# Skip signalling a process that we know has already died.
2083+
return
2084+
2085+
# The race condition can still happen if the race condition
2086+
# described above happens between the returncode test
2087+
# and the kill() call.
2088+
os.kill(self.pid, sig)
20672089

20682090
def terminate(self):
20692091
"""Terminate the process with SIGTERM

‎Lib/test/test_subprocess.py

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3120,6 +3120,31 @@ def test_stopped(self):
31203120

31213121
self.assertEqual(returncode, -3)
31223122

3123+
def test_send_signal_race(self):
3124+
# bpo-38630: send_signal() must poll the process exit status to reduce
3125+
# the risk of sending the signal to the wrong process.
3126+
proc = subprocess.Popen(ZERO_RETURN_CMD)
3127+
3128+
# wait until the process completes without using the Popen APIs.
3129+
pid, status = os.waitpid(proc.pid, 0)
3130+
self.assertEqual(pid, proc.pid)
3131+
self.assertTrue(os.WIFEXITED(status), status)
3132+
self.assertEqual(os.WEXITSTATUS(status), 0)
3133+
3134+
# returncode is still None but the process completed.
3135+
self.assertIsNone(proc.returncode)
3136+
3137+
with mock.patch("os.kill") as mock_kill:
3138+
proc.send_signal(signal.SIGTERM)
3139+
3140+
# send_signal() didn't call os.kill() since the process already
3141+
# completed.
3142+
mock_kill.assert_not_called()
3143+
3144+
# Don't check the returncode value: the test reads the exit status,
3145+
# so Popen failed to read it and uses a default returncode instead.
3146+
self.assertIsNotNone(proc.returncode)
3147+
31233148

31243149
@unittest.skipUnless(mswindows, "Windows specific tests")
31253150
class Win32ProcessTestCase(BaseTestCase):

‎Misc/NEWS.d/next/Library/2019-10-29-12-21-10.bpo-38630.Dv6Xrm.rst

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
On Unix, :meth:`subprocess.Popen.send_signal` now polls the process status.
2+
Polling reduces the risk of sending a signal to the wrong process if the
3+
process completed, the :attr:`subprocess.Popen.returncode` attribute is still
4+
``None``, and the pid has been reassigned (recycled) to a new different
5+
process.

0 commit comments

Comments
 (0)
0