Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

subprocess module does not check WIFSTOPPED on SIGCHLD #73521

Closed
ZachRiggle mannequin opened this issue Jan 20, 2017 · 17 comments
Closed

subprocess module does not check WIFSTOPPED on SIGCHLD #73521

ZachRiggle mannequin opened this issue Jan 20, 2017 · 17 comments
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@ZachRiggle
Copy link
Mannequin

ZachRiggle mannequin commented Jan 20, 2017

BPO 29335
Nosy @gpshead, @vstinner, @ned-deily
PRs
  • bpo-29335 - apply suggested test_subprocess unittest simplifications. #1757
  • bpo-30764: test_subprocess uses SuppressCrashReport #2405
  • [3.6] bpo-30764: test_subprocess uses SuppressCrashReport (#2405) #2410
  • [3.5] bpo-30764: test_subprocess uses SuppressCrashReport (#2405) #2411
  • Files
  • bug.py: Example script hitting "Should never happen"
  • issue29335-gps01.diff
  • issue29335-gps02.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/gpshead'
    closed_at = <Date 2017-05-24.01:36:09.178>
    created_at = <Date 2017-01-20.20:48:08.437>
    labels = ['type-bug', 'library']
    title = 'subprocess module does not check WIFSTOPPED on SIGCHLD'
    updated_at = <Date 2017-06-26.22:00:53.909>
    user = 'https://bugs.python.org/ZachRiggle'

    bugs.python.org fields:

    activity = <Date 2017-06-26.22:00:53.909>
    actor = 'vstinner'
    assignee = 'gregory.p.smith'
    closed = True
    closed_date = <Date 2017-05-24.01:36:09.178>
    closer = 'gregory.p.smith'
    components = ['Library (Lib)']
    creation = <Date 2017-01-20.20:48:08.437>
    creator = 'Zach Riggle'
    dependencies = []
    files = ['46363', '46385', '46386']
    hgrepos = []
    issue_num = 29335
    keywords = ['patch']
    message_count = 17.0
    messages = ['285921', '285996', '286023', '286031', '286036', '286046', '286050', '286057', '286099', '286100', '286102', '294251', '294308', '296907', '296910', '296957', '296959']
    nosy_count = 5.0
    nosy_names = ['gregory.p.smith', 'vstinner', 'ned.deily', 'python-dev', 'Zach Riggle']
    pr_nums = ['1757', '2405', '2410', '2411']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue29335'
    versions = ['Python 2.7', 'Python 3.5', 'Python 3.6']

    @ZachRiggle
    Copy link
    Mannequin Author

    ZachRiggle mannequin commented Jan 20, 2017

    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.

    @ZachRiggle ZachRiggle mannequin added the stdlib Python modules in the Lib dir label Jan 20, 2017
    @ZachRiggle
    Copy link
    Mannequin Author

    ZachRiggle mannequin commented Jan 22, 2017

    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.

    @gpshead gpshead self-assigned this Jan 22, 2017
    @gpshead gpshead changed the title Python 2.7 subprocess module does not check WIFSTOPPED on SIGCHLD subprocess module does not check WIFSTOPPED on SIGCHLD Jan 22, 2017
    @gpshead
    Copy link
    Member

    gpshead commented Jan 22, 2017

    The attached patch should fix it.

    I want to incorporate a bug.py like regression test into test_subprocess.py.

    @gpshead
    Copy link
    Member

    gpshead commented Jan 22, 2017

    test added.

    @gpshead gpshead added the type-bug An unexpected behavior, bug, or error label Jan 22, 2017
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 23, 2017

    New changeset 269296b2a047 by Gregory P. Smith in branch '3.5':
    Issue bpo-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 bpo-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 bpo-29335: Fix subprocess.Popen.wait() when the child process has
    https://hg.python.org/cpython/rev/4f5e7d018195

    @ned-deily
    Copy link
    Member

    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; bpo-29335.
    ----------------------------------------------------------------------

    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

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 23, 2017

    New changeset 8e3d412f8e89 by Gregory P. Smith in branch '2.7':
    Issue bpo-29335: Fix subprocess.Popen.wait() when the child process has
    https://hg.python.org/cpython/rev/8e3d412f8e89

    @gpshead
    Copy link
    Member

    gpshead commented Jan 23, 2017

    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.

    @ZachRiggle
    Copy link
    Mannequin Author

    ZachRiggle mannequin commented Jan 23, 2017

    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)
    

    @vstinner
    Copy link
    Member

    If you want crashes, look at the portable faulthandler._sigsegv() :-)

    @ZachRiggle
    Copy link
    Mannequin Author

    ZachRiggle mannequin commented Jan 23, 2017

    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'

    @gpshead
    Copy link
    Member

    gpshead commented May 23, 2017

    New changeset 56bc3b7 by Gregory P. Smith in branch 'master':
    bpo-29335 - apply suggested test_subprocess simplifications from haypo and Zach: (bpo-1757)
    56bc3b7

    @gpshead
    Copy link
    Member

    gpshead commented May 24, 2017

    Fixed applied to subprocess32 in google/python-subprocess32@0f1958e

    @gpshead gpshead closed this as completed May 24, 2017
    @vstinner
    Copy link
    Member

    New changeset cdee3f1 by Victor Stinner in branch 'master':
    bpo-30764: test_subprocess uses SuppressCrashReport (bpo-2405)
    cdee3f1

    @vstinner
    Copy link
    Member

    New changeset 849b062 by Victor Stinner in branch '3.5':
    bpo-30764: test_subprocess uses SuppressCrashReport (bpo-2405) (bpo-2411)
    849b062

    @vstinner
    Copy link
    Member

    New changeset 9ad50d9 by Victor Stinner in branch '3.6':
    bpo-30764: test_subprocess uses SuppressCrashReport (bpo-2405) (bpo-2410)
    9ad50d9

    @vstinner
    Copy link
    Member

    New changeset 2097b9e by Victor Stinner in branch '2.7':
    [2.7] bpo-30764: test_subprocess uses SuppressCrashReport (bpo-2405) (bpo-2412)
    2097b9e

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants