8000 gh-82616: Add process_group support to subprocess.Popen (#23930) · python/cpython@f6dd14c · GitHub
[go: up one dir, main page]
Skip to content

Commit f6dd14c

Browse files
gpsheadZackerySpytzserhiy-storchaka
authored
gh-82616: Add process_group support to subprocess.Popen (#23930)
One more thing that can help prevent people from using `preexec_fn`. Also adds conditional skips to two tests exposing ASAN flakiness on the Ubuntu 20.04 Address Sanitizer Github CI system. When that build is run on more modern systems the "problem" does not show up. It seems ASAN implementation related. Co-authored-by: Zackery Spytz <zspytz@gmail.com> Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
1 parent 49fda0c commit f6dd14c

File tree

9 files changed

+73
-28
lines changed
  • Misc/NEWS.d/next/Library
  • Modules

    • 9 files changed

    +73
    -28
    lines changed

    ‎Doc/library/subprocess.rst

    Lines changed: 15 additions & 8 deletions
    Original file line numberDiff line numberDiff line change
    @@ -344,7 +344,8 @@ functions.
    344344
    startupinfo=None, creationflags=0, restore_signals=True, \
    345345
    start_new_session=False, pass_fds=(), *, group=None, \
    346346
    extra_groups=None, user=None, umask=-1, \
    347-
    encoding=None, errors=None, text=None, pipesize=-1)
    347+
    encoding=None, errors=None, text=None, pipesize=-1, \
    348+
    process_group=None)
    348349
    349350
    Execute a child program in a new process. On POSIX, the class uses
    350351
    :meth:`os.execvpe`-like behavior to execute the child program. On Windows,
    @@ -500,18 +501,16 @@ functions.
    500501

    501502
    .. warning::
    502503

    503-
    The *preexec_fn* parameter is not safe to use in the presence of threads
    504+
    The *preexec_fn* parameter is NOT SAFE to use in the presence of threads
    504505
    in your application. The child process could deadlock before exec is
    505506
    called.
    506-
    If you must use it, keep it trivial! Minimize the number of libraries
    507-
    you call into.
    508507

    509508
    .. note::
    510509

    511510
    If you need to modify the environment for the child use the *env*
    512511
    parameter rather than doing it in a *preexec_fn*.
    513-
    The *start_new_session* parameter can take the place of a previously
    514-
    common use of *preexec_fn* to call os.setsid() in the child.
    512+
    The *start_new_session* and *process_group* parameters should take the place of
    513+
    code using *preexec_fn* to call :func:`os.setsid` or :func:`os.setpgid` in the child.
    515514

    516515
    .. versionchanged:: 3.8
    517516

    @@ -568,12 +567,20 @@ functions.
    568567
    .. versionchanged:: 3.2
    569568
    *restore_signals* was added.
    570569

    571-
    If *start_new_session* is true the setsid() system call will be made in the
    572-
    child process prior to the execution of the subprocess. (POSIX only)
    570+
    If *start_new_session* is true the ``setsid()`` system call will be made in the
    571+
    child process prior to the execution of the subprocess.
    573572

    573+
    .. availability:: POSIX
    574574
    .. versionchanged:: 3.2
    575575
    *start_new_session* was added.
    576576

    577+
    If *process_group* is a non-negative integer, the ``setpgid(0, value)`` system call will
    578+
    be made in the child process prior to the execution of the subprocess.
    579+
    580+
    .. availability:: POSIX
    581+
    .. versionchanged:: 3.11
    582+
    *process_group* was added.
    583+
    577584
    If *group* is not ``None``, the setregid() system call will be made in the
    578585
    child process prior to the execution of the subprocess. If the provided
    579586
    value is a string, it will be looked up via :func:`grp.getgrnam()` and

    ‎Lib/multiprocessing/util.py

    Lines changed: 1 addition & 1 deletion
    Original file line numberDiff line numberDiff line change
    @@ -453,7 +453,7 @@ def spawnv_passfds(path, args, passfds):
    453453
    return _posixsubprocess.fork_exec(
    454454
    args, [path], True, passfds, None, None,
    455455
    -1, -1, -1, -1, -1, -1, errpipe_read, errpipe_write,
    456-
    False, False, None, None, None, -1, None,
    456+
    False, False, -1, None, None, None, -1, None,
    457457
    subprocess._USE_VFORK)
    458458
    finally:
    459459
    os.close(errpipe_read)

    ‎Lib/subprocess.py

    Lines changed: 12 additions & 5 deletions
    Original file line numberDiff line numberDiff line change
    @@ -769,6 +769,8 @@ class Popen:
    769769
    770770
    start_new_session (POSIX only)
    771771
    772+
    process_group (POSIX only)
    773+
    772774
    group (POSIX only)
    773775
    774776
    extra_groups (POSIX only)
    @@ -794,7 +796,8 @@ def __init__(self, args, bufsize=-1, executable=None,
    794796
    startupinfo=None, creationflags=0,
    795797
    restore_signals=True, start_new_session=False,
    796798
    pass_fds=(), *, user=None, group=None, extra_groups=None,
    797-
    encoding=None, errors=None, text=None, umask=-1, pipesize=-1):
    799+
    encoding=None, errors=None, text=None, umask=-1, pipesize=-1,
    800+
    process_group=None):
    798801
    """Create new Popen instance."""
    799802
    _cleanup()
    800803
    # Held while anything is calling waitpid before returncode has been
    @@ -900,6 +903,9 @@ def __init__(self, args, bufsize=-1, executable=None,
    900903
    else:
    901904
    line_buffering = False
    902905

    906+
    if process_group is None:
    907+
    process_group = -1 # The internal APIs are int-only
    908+
    903909
    gid = None
    904910
    if group is not None:
    905911
    if not hasattr(os, 'setregid'):
    @@ -1003,7 +1009,7 @@ def __init__(self, args, bufsize=-1, executable=None,
    10031009
    errread, errwrite,
    10041010
    restore_signals,
    10051011
    gid, gids, uid, umask,
    1006-
    start_new_session)
    1012+
    start_new_session, process_group)
    10071013
    except:
    10081014
    # Cleanup if the child failed starting.
    10091015
    for f in filter(None, (self.stdin, self.stdout, self.stderr)):
    @@ -1387,7 +1393,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
    13871393
    unused_restore_signals,
    13881394
    unused_gid, unused_gids, unused_uid,
    13891395
    unused_umask,
    1390-
    unused_start_new_session):
    1396+
    unused_start_new_session, unused_process_group):
    13911397
    """Execute program (MS Windows version)"""
    13921398

    13931399
    assert not pass_fds, "pass_fds not supported on Windows."
    @@ -1719,7 +1725,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
    17191725
    errread, errwrite,
    17201726
    restore_signals,
    17211727
    gid, gids, uid, umask,
    1722-
    start_new_session):
    1728+
    start_new_session, process_group):
    17231729
    """Execute program (POSIX version)"""
    17241730

    17251731
    if isinstance(args, (str, bytes)):
    @@ -1755,6 +1761,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
    17551761
    and (c2pwrite == -1 or c2pwrite > 2)
    17561762
    and (errwrite == -1 or errwrite > 2)
    17571763
    and not start_new_session
    1764+
    and process_group == -1
    17581765
    and gid is None
    17591766
    and gids is None
    17601767
    and uid is None
    @@ -1812,7 +1819,7 @@ def _execute_child(self, args, executable, preexec_fn, close_fds,
    18121819
    errread, errwrite,
    18131820
    errpipe_read, errpipe_write,
    18141821
    restore_signals, start_new_session,
    1815-
    gid, gids, uid, umask,
    1822+
    process_group, gid, gids, uid, umask,
    18161823
    preexec_fn, _USE_VFORK)
    18171824
    self._child_created = True
    18181825
    finally:

    ‎Lib/test/test_asyncio/test_subprocess.py

    Lines changed: 3 additions & 0 deletions
    Original file line numberDiff line numberDiff line change
    @@ -15,6 +15,9 @@
    1515
    if sys.platform != 'win32':
    1616
    from asyncio import unix_events
    1717

    18+
    if support.check_sanitizer(address=True):
    19+
    raise unittest.SkipTest("Exposes ASAN flakiness in GitHub CI")
    20+
    1821
    # Program blocking
    1922
    PROGRAM_BLOCKED = [sys.executable, '-c', 'import time; time.sleep(3600)']
    2023

    ‎Lib/test/test_capi.py

    Lines changed: 3 additions & 3 deletions
    Original file line numberDiff line numberDiff line change
    @@ -140,15 +140,15 @@ class Z(object):
    140140
    def __len__(self):
    141141
    return 1
    142142
    self.assertRaises(TypeError, _posixsubprocess.fork_exec,
    143-
    1,Z(),3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22)
    143+
    1,Z(),3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23)
    144144
    # Issue #15736: overflow in _PySequence_BytesToCharpArray()
    145145
    class Z(object):
    146146
    def __len__(self):
    147147
    return sys.maxsize
    148148
    def __getitem__(self, i):
    149149
    return b'x'
    150150
    self.assertRaises(MemoryError, _posixsubprocess.fork_exec,
    151-
    1,Z(),3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22)
    151+
    1,Z(),3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23)
    152152

    153153
    @unittest.skipUnless(_posixsubprocess, '_posixsubprocess required for this test.')
    154154
    def test_subprocess_fork_exec(self):
    @@ -158,7 +158,7 @@ def __len__(self):
    158158

    159159
    # Issue #15738: crash in subprocess_fork_exec()
    160160
    self.assertRaises(TypeError, _posixsubprocess.fork_exec,
    161-
    Z(),[b'1'],3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22)
    161+
    Z(),[b'1'],3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23)
    162162

    163163
    @unittest.skipIf(MISSING_C_DOCSTRINGS,
    164164
    "Signature information for builtins requires docstrings")

    ‎Lib/test/test_distutils.py

    Lines changed: 2 additions & 0 deletions
    Original file line numberDiff line numberDiff line change
    @@ -23,6 +23,8 @@ def load_tests(*_):
    2323
    def tearDownModule():
    2424
    support.reap_children()
    2525

    26+
    if support.check_sanitizer(address=True):
    27+
    raise unittest.SkipTest("Exposes ASAN flakiness in GitHub CI")
    2628

    2729
    if __name__ == "__main__":
    2830
    unittest.main()

    ‎Lib/test/test_subprocess.py

    Lines changed: 22 additions & 4 deletions
    Original file line numberDiff line numberDiff line change
    @@ -1905,14 +1905,32 @@ def test_start_new_session(self):
    19051905
    output = subprocess.check_output(
    19061906
    [sys.executable, "-c", "import os; print(os.getsid(0))"],
    19071907
    start_new_session=True)
    1908-
    except OSError as e:
    1908+
    except PermissionError as e:
    19091909
    if e.errno != errno.EPERM:
    1910-
    raise
    1910+
    raise # EACCES?
    19111911
    else:
    19121912
    parent_sid = os.getsid(0)
    19131913
    child_sid = int(output)
    19141914
    self.assertNotEqual(parent_sid, child_sid)
    19151915

    1916+
    @unittest.skipUnless(hasattr(os, 'setpgid') and hasattr(os, 'getpgid'),
    1917+
    'no setpgid or getpgid on platform')
    1918+
    def test_process_group_0(self):
    1919+
    # For code coverage of calling setpgid(). We don't care if we get an
    1920+
    # EPERM error from it depending on the test execution environment, that
    1921+
    # still indicates that it was called.
    1922+
    try:
    1923+
    output = subprocess.check_output(
    1924+
    [sys.executable, "-c", "import os; print(os.getpgid(0))"],
    1925+
    process_group=0)
    1926+
    except PermissionError as e:
    1927+
    if e.errno != errno.EPERM:
    1928+
    raise # EACCES?
    1929+
    else:
    1930+
    parent_pgid = os.getpgid(0)
    1931+
    child_pgid = int(output)
    1932+
    self.assertNotEqual(parent_pgid, child_pgid)
    1933+
    19161934
    @unittest.skipUnless(hasattr(os, 'setreuid'), 'no setreuid on platform')
    19171935
    def test_user(self):
    19181936
    # For code coverage of the user parameter. We don't care if we get an
    @@ -3134,7 +3152,7 @@ def test_fork_exec(self):
    31343152
    True, (), cwd, env_list,
    31353153
    -1, -1, -1, -1,
    31363154
    1, 2, 3, 4,
    3137-
    True, True,
    3155+
    True, True, 0,
    31383156
    False, [], 0, -1,
    31393157
    func, False)
    31403158
    # Attempt to prevent
    @@ -3183,7 +3201,7 @@ def __int__(self):
    31833201
    True, fds_to_keep, None, [b"env"],
    31843202
    -1, -1, -1, -1,
    31853203
    1, 2, 3, 4,
    3186-
    True, True,
    3204+
    True, True, 0,
    31873205
    None, None, None, -1,
    31883206
    None, "no vfork")
    31893207
    self.assertIn('fds_to_keep', str(c.exception))

    ‎Misc/NEWS.d/next/Library/2020-12-24-19-11-53.bpo-38435.rEHTAR.rst

    Lines changed: 2 additions & 0 deletions
    Original file line numberDiff line numberDiff line change
    @@ -0,0 +1,2 @@
    1+
    Add a ``process_group`` parameter to :class:`subprocess.Popen` to help move
    2+
    more things off of the unsafe ``preexec_fn`` parameter.

    ‎Modules/_posixsubprocess.c

    Lines changed: 13 additions & 7 deletions
    Original file line numberDiff line numberDiff line change
    @@ -517,7 +517,7 @@ child_exec(char *const exec_array[],
    517517
    int errread, int errwrite,
    518518
    int errpipe_read, int errpipe_write,
    519519
    int close_fds, int restore_signals,
    520-
    int call_setsid,
    520+
    int call_setsid, pid_t pgid_to_set,
    521521
    int call_setgid, gid_t gid,
    522522
    int call_setgroups, size_t groups_size, const gid_t *groups,
    523523
    int call_setuid, uid_t uid, int child_umask,
    @@ -611,6 +611,11 @@ child_exec(char *const exec_array[],
    611611
    POSIX_CALL(setsid());
    612612
    #endif
    613613

    614+
    #ifdef HAVE_SETPGID
    615+
    if (pgid_to_set >= 0)
    616+
    POSIX_CALL(setpgid(0, pgid_to_set));
    617+
    #endif
    618+
    614619
    #ifdef HAVE_SETGROUPS
    615620
    if (call_setgroups)
    616621
    POSIX_CALL(setgroups(groups_size, groups));
    @@ -716,7 +721,7 @@ do_fork_exec(char *const exec_array[],
    716721
    int errread, int errwrite,
    717722
    int errpipe_read, int errpipe_write,
    718723
    int close_fds, int restore_signals,
    719-
    int call_setsid,
    724+
    int call_setsid, pid_t pgid_to_set,
    720725
    int call_setgid, gid_t gid,
    721726
    int call_setgroups, size_t groups_size, const gid_t *groups,
    722727
    int call_setuid, uid_t uid, int child_umask,
    @@ -769,7 +774,7 @@ do_fork_exec(char *const exec_array[],
    769774
    child_exec(exec_array, argv, envp, cwd,
    770775
    p2cread, p2cwrite, c2pread, c2pwrite,
    771776
    errread, errwrite, errpipe_read, errpipe_write,
    772-
    close_fds, restore_signals, call_setsid,
    777+
    close_fds, restore_signals 57AE , call_setsid, pgid_to_set,
    773778
    call_setgid, gid, call_setgroups, groups_size, groups,
    774779
    call_setuid, uid, child_umask, child_sigmask,
    775780
    py_fds_to_keep, preexec_fn, preexec_fn_args_tuple);
    @@ -791,6 +796,7 @@ subprocess_fork_exec(PyObject *module, PyObject *args)
    791796
    int p2cread, p2cwrite, c2pread, c2pwrite, errread, errwrite;
    792797
    int errpipe_read, errpipe_write, close_fds, restore_signals;
    793798
    int call_setsid;
    799+
    pid_t pgid_to_set = -1;
    794800
    int call_setgid = 0, call_setgroups = 0, call_setuid = 0;
    795801
    uid_t uid;
    796802
    gid_t gid, *groups = NULL;
    @@ -806,13 +812,13 @@ subprocess_fork_exec(PyObject *module, PyObject *args)
    806812
    int allow_vfork;
    807813

    808814
    if (!PyArg_ParseTuple(
    809-
    args, "OOpO!OOiiiiiiiiiiOOOiOp:fork_exec",
    815+
    args, "OOpO!OOiiiiiiiiii" _Py_PARSE_PID "OOOiOp:fork_exec",
    810816
    &process_args, &executable_list,
    811817
    &close_fds, &PyTuple_Type, &py_fds_to_keep,
    812818
    &cwd_obj, &env_list,
    813819
    &p2cread, &p2cwrite, &c2pread, &c2pwrite,
    814820
    &errread, &errwrite, &errpipe_read, &errpipe_write,
    815-
    &restore_signals, &call_setsid,
    821+
    &restore_signals, &call_setsid, &pgid_to_set,
    816822
    &gid_object, &groups_list, &uid_object, &child_umask,
    817823
    &preexec_fn, &allow_vfork))
    818824
    return NULL;
    @@ -1016,7 +1022,7 @@ subprocess_fork_exec(PyObject *module, PyObject *args)
    10161022
    pid = do_fork_exec(exec_array, argv, envp, cwd,
    10171023
    p2cread, p2cwrite, c2pread, c2pwrite,
    10181024
    errread, errwrite, errpipe_read, errpipe_write,
    1019-
    close_fds, restore_signals, call_setsid,
    1025+
    close_fds, restore_signals, call_setsid, pgid_to_set,
    10201026
    call_setgid, gid, call_setgroups, num_groups, groups,
    10211027
    call_setuid, uid, child_umask, old_sigmask,
    10221028
    py_fds_to_keep, preexec_fn, preexec_fn_args_tuple);
    @@ -1081,7 +1087,7 @@ PyDoc_STRVAR(subprocess_fork_exec_doc,
    10811087
    "fork_exec(args, executable_list, close_fds, pass_fds, cwd, env,\n\
    10821088
    p2cread, p2cwrite, c2pread, c2pwrite,\n\
    10831089
    errread, errwrite, errpipe_read, errpipe_write,\n\
    1084-
    restore_signals, call_setsid,\n\
    1090+
    restore_signals, call_setsid, pgid_to_set,\n\
    10851091
    gid, groups_list, uid,\n\
    10861092
    preexec_fn)\n\
    10871093
    \n\

    0 commit comments

    Comments
     (0)
    0