classification
Title: subprocess module does not check WIFSTOPPED on SIGCHLD
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: gregory.p.smith Nosy List: Zach Riggle, gregory.p.smith, ned.deily, python-dev, vstinner
Priority: normal Keywords: patch

Created on 2017-01-20 20:48 by Zach Riggle, last changed 2017-06-26 22:00 by vstinner. This issue is now closed.

Files
File name Uploaded Description Edit
bug.py Zach Riggle, 2017-01-20 20:48 Example script hitting "Should never happen"
issue29335-gps01.diff gregory.p.smith, 2017-01-22 18:27 review
issue29335-gps02.diff gregory.p.smith, 2017-01-22 21:21 review
Pull Requests
URL Status Linked Edit
PR 1757 merged gregory.p.smith, 2017-05-23 06:11
PR 2405 merged vstinner, 2017-06-26 13:00
PR 2410 merged vstinner, 2017-06-26 15:29
PR 2411 merged vstinner, 2017-06-26 15:33
Messages (17)
msg285921 - (view) Author: Zach Riggle (Zach Riggle) Date: 2017-01-20 20:48
The attached script hits some "This should never happen" code in the subprocess module.

These lines here:
https://github.com/python/cpython/blob/2.7/Lib/subprocess.py#L1036-L1038

The root cause is a lack of checking WIFSTOPPED and WSTOPSIG in the handler.

When a process elects into being ptraced via PTRACE_TRACEME, it is stopped on the SIGSEGV instead of terminating, allowing the user to attach a debugger before the kernel destroys the process.

This bug makes it impossible to wait on any process which crashes, which is set up to wait for a debugger.
msg285996 - (view) Author: Zach Riggle (Zach Riggle) Date: 2017-01-22 06:53
To further clarify the report:

When the attached proof-of-concept is executed, a RuntimeException is raised, which has a comment "Should never happen".

The issue isn't due to SIGCHLD, but rather following a waitpid() call.  The code attempts to suss the exit code / reason for waitpid() returning, but does not check for WIFSTOPPED in its handler.
msg286023 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2017-01-22 18:27
The attached patch should fix it.

I want to incorporate a bug.py like regression test into test_subprocess.py.
msg286031 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2017-01-22 21:21
test added.
msg286036 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2017-01-23 01:30
New changeset 269296b2a047 by Gregory P. Smith in branch '3.5':
Issue #29335: Fix subprocess.Popen.wait() when the child process has
https://hg.python.org/cpython/rev/269296b2a047

New changeset ed5255a61648 by Gregory P. Smith in branch '3.6':
Issue #29335: Fix subprocess.Popen.wait() when the child process has
https://hg.python.org/cpython/rev/ed5255a61648

New changeset 4f5e7d018195 by Gregory P. Smith in branch 'default':
Issue #29335: Fix subprocess.Popen.wait() when the child process has
https://hg.python.org/cpython/rev/4f5e7d018195
msg286046 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2017-01-23 04:25
Among other buildbot failures:

http://buildbot.python.org/all/builders/x86%20Tiger%203.6/builds/142/steps/test/logs/stdio

======================================================================
ERROR: test_child_terminated_in_stopped_state (test.test_subprocess.POSIXProcessTestCase)
Test wait() behavior when waitpid returns WIFSTOPPED; issue29335.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/db3l/buildarea/3.6.bolen-tiger/build/Lib/test/test_subprocess.py", line 2514, in test_child_terminated_in_stopped_state
    libc = ctypes.CDLL(libc_name)
  File "/Users/db3l/buildarea/3.6.bolen-tiger/build/Lib/ctypes/__init__.py", line 348, in __init__
    self._handle = _dlopen(self._name, mode)
OSError: dlopen(libc..dylib, 6): image not found

----------------------------------------------------------------------
Ran 260 tests in 102.297s


Also, http://buildbot.python.org/all/builders/x86%20Ubuntu%20Shared%203.x/builds/240/steps/test/logs/stdio
msg286050 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2017-01-23 06:38
New changeset 8e3d412f8e89 by Gregory P. Smith in branch '2.7':
Issue #29335: Fix subprocess.Popen.wait() when the child process has
https://hg.python.org/cpython/rev/8e3d412f8e89
msg286057 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2017-01-23 07:54
thanks Ned, I was awaiting interesting buildbot results. :)

fixed in 2.7 and 3.5 onwards.  thanks for the report Zach.

not closing until I also apply the fix to the subprocess32 backport.
msg286099 - (view) Author: Zach Riggle (Zach Riggle) Date: 2017-01-23 16:52
Of note, there's no need to actually cause a SIGSEGV to generate the signal.

The tests might be more clear to replace:

    libc.printf(ctypes.c_char_p(0xdeadbeef))

with

    os.kill(os.getpid(), signal.SIGSEGV)
msg286100 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-01-23 16:57
If you want crashes, look at the portable faulthandler._sigsegv() :-)
msg286102 - (view) Author: Zach Riggle (Zach Riggle) Date: 2017-01-23 17:21
Neat, though that's not in the standard library.

The current logic for getting a handle to libc could also be simplified via ctypes.util.find_library (https://docs.python.org/3/library/ctypes.html#finding-shared-libraries).

Darwin:

    >>> import ctypes.util
    >>> ctypes.util.find_library('c')
    '/usr/lib/libc.dylib'
    
Linux:

    >>> import ctypes.util
    >>> ctypes.util.find_library('c')
    'libc.so.6'
msg294251 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2017-05-23 14:49
New changeset 56bc3b768c3cc3817031b56d5e7a279aa1296bc9 by Gregory P. Smith in branch 'master':
bpo-29335 - apply suggested test_subprocess simplifications from haypo and Zach: (#1757)
https://github.com/python/cpython/commit/56bc3b768c3cc3817031b56d5e7a279aa1296bc9
msg294308 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2017-05-24 01:36
Fixed applied to subprocess32 in https://github.com/google/python-subprocess32/commit/0f1958e982bf44db569470def7281bcafa2a8b0e
msg296907 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-26 15:23
New changeset cdee3f14f7f4c995e7eedb0bf6a67e260c739f7d by Victor Stinner in branch 'master':
bpo-30764: test_subprocess uses SuppressCrashReport (#2405)
https://github.com/python/cpython/commit/cdee3f14f7f4c995e7eedb0bf6a67e260c739f7d
msg296910 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-26 16:05
New changeset 849b062a82ca2f09e33259d34067faba196c9e23 by Victor Stinner in branch '3.5':
bpo-30764: test_subprocess uses SuppressCrashReport (#2405) (#2411)
https://github.com/python/cpython/commit/849b062a82ca2f09e33259d34067faba196c9e23
msg296957 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-26 22:00
New changeset 9ad50d94599aed0c37beaf78948ec271c8aa3881 by Victor Stinner in branch '3.6':
bpo-30764: test_subprocess uses SuppressCrashReport (#2405) (#2410)
https://github.com/python/cpython/commit/9ad50d94599aed0c37beaf78948ec271c8aa3881
msg296959 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-06-26 22:00
New changeset 2097b9e0ef32ab7a0d745edc0f707c615780c006 by Victor Stinner in branch '2.7':
[2.7] bpo-30764: test_subprocess uses SuppressCrashReport (#2405) (#2412)
https://github.com/python/cpython/commit/2097b9e0ef32ab7a0d745edc0f707c615780c006
History
Date User Action Args
2017-06-26 22:00:53vstinnersetmessages: + msg296959
2017-06-26 22:00:35vstinnersetmessages: + msg296957
2017-06-26 16:05:23vstinnersetmessages: + msg296910
2017-06-26 15:33:59vstinnersetpull_requests: + pull_request2461
2017-06-26 15:29:23vstinnersetpull_requests: + pull_request2459
2017-06-26 15:23:05vstinnersetmessages: + msg296907
2017-06-26 13:00:30vstinnersetpull_requests: + pull_request2453
2017-05-24 01:36:09gregory.p.smithsetstatus: open -> closed

messages: + msg294308
stage: commit review -> resolved
2017-05-23 14:49:16gregory.p.smithsetmessages: + msg294251
2017-05-23 06:11:39gregory.p.smithsetpull_requests: + pull_request1843
2017-04-01 05:47:34serhiy.storchakasetpull_requests: - pull_request916
2017-03-31 16:36:17dstufftsetpull_requests: + pull_request916
2017-01-23 17:21:54Zach Rigglesetmessages: + msg286102
2017-01-23 16:57:09vstinnersetmessages: + msg286100
2017-01-23 16:56:47vstinnersetnosy: + vstinner
2017-01-23 16:52:20Zach Rigglesetmessages: + msg286099
2017-01-23 07:54:50gregory.p.smithsetresolution: fixed
messages: + msg286057
stage: patch review -> commit review
2017-01-23 06:38:41python-devsetmessages: + msg286050
2017-01-23 04:25:02ned.deilysetnosy: + ned.deily
messages: + msg286046
2017-01-23 01:30:45python-devsetnosy: + python-dev
messages: + msg286036
2017-01-22 21:21:49gregory.p.smithsetfiles: + issue29335-gps02.diff
type: behavior
messages: + msg286031

stage: test needed -> patch review
2017-01-22 18:27:53gregory.p.smithsetfiles: + issue29335-gps01.diff
keywords: + patch
messages: + msg286023

stage: test needed
2017-01-22 17:57:58gregory.p.smithsettitle: Python 2.7 subprocess module does not check WIFSTOPPED on SIGCHLD -> subprocess module does not check WIFSTOPPED on SIGCHLD
2017-01-22 17:57:39gregory.p.smithsetassignee: gregory.p.smith
2017-01-22 17:57:21gregory.p.smithsetversions: + Python 3.5, Python 3.6
2017-01-22 06:53:39Zach Rigglesetmessages: + msg285996
2017-01-20 21:02:48ned.deilysetnosy: + gregory.p.smith
2017-01-20 20:48:08Zach Rigglecreate