From 25280769cc658b659489958d8eaa2935b603c93e Mon Sep 17 00:00:00 2001 From: Patrick McLean Date: Mon, 5 Nov 2018 14:33:47 -0800 Subject: [PATCH 1/3] subprocess: Add user, group and extra_groups paremeters to subprocess.Popen This adds a `user` parameter to the Popen constructor that will call setreuid() in the child before calling exec(). This allows processes running as root to safely drop privileges before running the subprocess without having to use a preexec_fn. This also adds a `group` parameter that will call setregid() in the child process before calling exec(). Finally an `extra_groups` parameter was added that will call setgroups() to set the supplimental groups. --- Doc/library/subprocess.rst | 32 +++- Lib/multiprocessing/util.py | 2 +- Lib/subprocess.py | 105 +++++++++++- Lib/test/test_capi.py | 6 +- Lib/test/test_subprocess.py | 152 +++++++++++++++++- .../2019-02-19-17-32-45.bpo-36046.fX9OPj.rst | 2 + Modules/_posixsubprocess.c | 136 +++++++++++++++- 7 files changed, 417 insertions(+), 18 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2019-02-19-17-32-45.bpo-36046.fX9OPj.rst diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index 9ba0d30da5698e..8e52243d60e8dd 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -337,8 +337,9 @@ functions. stderr=None, preexec_fn=None, close_fds=True, shell=False, \ cwd=None, env=None, universal_newlines=None, \ startupinfo=None, creationflags=0, restore_signals=True, \ - start_new_session=False, pass_fds=(), *, \ - encoding=None, errors=None, text=None) + start_new_session=False, pass_fds=(), *, group=None, \ + extra_groups=None, user=None, encoding=None, errors=None, \ + text=None) Execute a child program in a new process. On POSIX, the class uses :meth:`os.execvp`-like behavior to execute the child program. On Windows, @@ -512,6 +513,33 @@ functions. .. versionchanged:: 3.2 *start_new_session* was added. + If *group* is not ``None``, the setregid() system call will be made in the + child process prior to the execution of the subprocess. If the provided + value is a string, it will be looked up via :func:`grp.getgrnam()` and + the value in ``gr_gid`` will be used. If the value is an integer, it + will be passed verbatim. (POSIX only) + + .. availability:: POSIX + .. versionadded:: 3.8 + + If *extra_groups* is not ``None``, the setgroups() system call will be + made in the child process prior to the execution of the subprocess. + Strings provided in *extra_groups* will be looked up via + :func:`grp.getgrnam()` and the values in ``gr_gid`` will be used. + Integer values will be passed verbatim. (POSIX only) + + .. availability:: POSIX + .. versionadded:: 3.8 + + If *user* is not ``None``, the setreuid() system call will be made in the + child process prior to the execution of the subprocess. If the provided + value is a string, it will be looked up via :func:`pwd.getpwnam()` and + the value in ``pw_uid`` will be used. If the value is an integer, it will + be passed verbatim. (POSIX only) + + .. availability:: POSIX + .. versionadded:: 3.8 + If *env* is not ``None``, it must be a mapping that defines the environment variables for the new process; these are used instead of the default behavior of inheriting the current process' environment. diff --git a/Lib/multiprocessing/util.py b/Lib/multiprocessing/util.py index 0c4eb2473273b4..bf7132ba18821c 100644 --- a/Lib/multiprocessing/util.py +++ b/Lib/multiprocessing/util.py @@ -417,7 +417,7 @@ def spawnv_passfds(path, args, passfds): return _posixsubprocess.fork_exec( args, [os.fsencode(path)], True, passfds, None, None, -1, -1, -1, -1, -1, -1, errpipe_read, errpipe_write, - False, False, None) + False, False, None, None, None, None) finally: os.close(errpipe_read) os.close(errpipe_write) diff --git a/Lib/subprocess.py b/Lib/subprocess.py index 0496b447e8ea03..e773a3ec4ba443 100644 --- a/Lib/subprocess.py +++ b/Lib/subprocess.py @@ -53,6 +53,14 @@ import contextlib from time import monotonic as _time +try: + import pwd +except ImportError: + pwd = None +try: + import grp +except ImportError: + grp = None __all__ = ["Popen", "PIPE", "STDOUT", "call", "check_call", "getstatusoutput", "getoutput", "check_output", "run", "CalledProcessError", "DEVNULL", @@ -695,6 +703,12 @@ class Popen(object): start_new_session (POSIX only) + group (POSIX only) + + extra_groups (POSIX only) + + user (POSIX only) + pass_fds (POSIX only) encoding and errors: Text mode encoding and error handling to use for @@ -711,7 +725,8 @@ def __init__(self, args, bufsize=-1, executable=None, shell=False, cwd=None, env=None, universal_newlines=None, startupinfo=None, creationflags=0, restore_signals=True, start_new_session=False, - pass_fds=(), *, encoding=None, errors=None, text=None): + pass_fds=(), *, user=None, group=None, extra_groups=None, + encoding=None, errors=None, text=None): """Create new Popen instance.""" _cleanup() # Held while anything is calling waitpid before returncode has been @@ -809,6 +824,78 @@ def __init__(self, args, bufsize=-1, executable=None, else: line_buffering = False + gid = None + if group is not None: + if not hasattr(os, 'setregid'): + raise ValueError("The 'group' parameter is not supported on the " + "current platform") + + elif isinstance(group, str): + if grp is None: + raise ValueError("The group parameter cannot be a string " + "on systems without the grp module") + + gid = grp.getgrnam(group).gr_gid + elif isinstance(group, int): + gid = group + else: + raise TypeError("Group must be a string or an integer, not {}" + .format(type(group))) + + if gid < 0: + raise ValueError(f"Group ID cannot be negative, got {gid}") + + gids = None + if extra_groups is not None: + if not hasattr(os, 'setgroups'): + raise ValueError("The 'extra_groups' parameter is not " + "supported on the current platform") + + elif isinstance(extra_groups, str): + raise ValueError("Groups must be a list, not a string") + + gids = [] + for extra_group in extra_groups: + if isinstance(extra_group, str): + if grp is None: + raise ValueError("Items in extra_groups cannot be " + "strings on systems without the " + "grp module") + + gids.append(grp.getgrnam(extra_group).gr_gid) + elif isinstance(extra_group, int): + gids.append(extra_group) + else: + raise TypeError("Items in extra_groups must be a string " + "or integer, not {}" + .format(type(extra_group))) + + # make sure that the gids are all positive here so we can do less + # checking in the C code + for gid_check in gids: + if gid_check < 0: + raise ValueError(f"Group ID cannot be negative, got {gid_check}") + + uid = None + if user is not None: + if not hasattr(os, 'setreuid'): + raise ValueError("The 'user' parameter is not supported on " + "the current platform") + + elif isinstance(user, str): + if pwd is None: + raise ValueError("The user parameter cannot be a string " + "on systems without the pwd module") + + uid = pwd.getpwnam(user).pw_uid + elif isinstance(user, int): + uid = user + else: + raise TypeError("User must be a string or an integer") + + if uid < 0: + raise ValueError(f"User ID cannot be negative, got {uid}") + try: if p2cwrite != -1: self.stdin = io.open(p2cwrite, 'wb', bufsize) @@ -833,7 +920,9 @@ def __init__(self, args, bufsize=-1, executable=None, p2cread, p2cwrite, c2pread, c2pwrite, errread, errwrite, - restore_signals, start_new_session) + restore_signals, + gid, gids, uid, + start_new_session) except: # Cleanup if the child failed starting. for f in filter(None, (self.stdin, self.stdout, self.stderr)): @@ -1199,7 +1288,9 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, p2cread, p2cwrite, c2pread, c2pwrite, errread, errwrite, - unused_restore_signals, unused_start_new_session): + unused_restore_signals, + unused_gid, unused_gids, unused_uid, + unused_start_new_session): """Execute program (MS Windows version)""" assert not pass_fds, "pass_fds not supported on Windows." @@ -1506,7 +1597,9 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, p2cread, p2cwrite, c2pread, c2pwrite, errread, errwrite, - restore_signals, start_new_session): + restore_signals, + gid, gids, uid, + start_new_session): """Execute program (POSIX version)""" if isinstance(args, (str, bytes)): @@ -1587,7 +1680,9 @@ def _execute_child(self, args, executable, preexec_fn, close_fds, p2cread, p2cwrite, c2pread, c2pwrite, errread, errwrite, errpipe_read, errpipe_write, - restore_signals, start_new_session, preexec_fn) + restore_signals, start_new_session, + gid, gids, uid, + preexec_fn) self._child_created = True finally: # be sure the FD is closed no matter what diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py index 7c68b2c0fc2c20..c12b7b17d76382 100644 --- a/Lib/test/test_capi.py +++ b/Lib/test/test_capi.py @@ -96,7 +96,7 @@ class Z(object): def __len__(self): return 1 self.assertRaises(TypeError, _posixsubprocess.fork_exec, - 1,Z(),3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17) + 1,Z(),3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20) # Issue #15736: overflow in _PySequence_BytesToCharpArray() class Z(object): def __len__(self): @@ -104,7 +104,7 @@ def __len__(self): def __getitem__(self, i): return b'x' self.assertRaises(MemoryError, _posixsubprocess.fork_exec, - 1,Z(),3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17) + 1,Z(),3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20) @unittest.skipUnless(_posixsubprocess, '_posixsubprocess required for this test.') def test_subprocess_fork_exec(self): @@ -114,7 +114,7 @@ def __len__(self): # Issue #15738: crash in subprocess_fork_exec() self.assertRaises(TypeError, _posixsubprocess.fork_exec, - Z(),[b'1'],3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17) + Z(),[b'1'],3,(1, 2),5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20) @unittest.skipIf(MISSING_C_DOCSTRINGS, "Signature information for builtins requires docstrings") diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index b0b6b06e92759e..2db7f857549773 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -18,6 +18,7 @@ import threading import gc import textwrap +import json from test.support import FakePath try: @@ -32,6 +33,15 @@ except ImportError: _testcapi = None +try: + import pwd +except ImportError: + pwd = None +try: + import grp +except ImportError: + grp = None + if support.PGO: raise unittest.SkipTest("test is not helpful for PGO") @@ -1661,6 +1671,139 @@ def test_start_new_session(self): child_pgid = int(output) self.assertNotEqual(parent_pgid, child_pgid) + @unittest.skipUnless(hasattr(os, 'setreuid'), 'no setreuid on platform') + def test_user(self): + # For code coverage of the user parameter. We don't care if we get an + # EPERM error from it depending on the test execution environment, that + # still indicates that it was called. + + uid = os.geteuid() + test_users = [65534 if uid != 65534 else 65533, uid] + name_uid = "nobody" if sys.platform != 'darwin' else "unknown" + + if pwd is not None: + test_users.append(name_uid) + + for user in test_users: + with self.subTest(user=user): + try: + output = subprocess.check_output( + [sys.executable, "-c", + "import os; print(os.getuid())"], + user=user) + except OSError as e: + if e.errno != errno.EPERM: + raise + else: + if isinstance(user, str): + user_uid = pwd.getpwnam(user).pw_uid + else: + user_uid = user + child_user = int(output) + self.assertEqual(child_user, user_uid) + + with self.assertRaises(ValueError): + subprocess.check_call([sys.executable, "-c", "pass"], user=-1) + + if pwd is None: + with self.assertRaises(ValueError): + subprocess.check_call([sys.executable, "-c", "pass"], user=name_uid) + + @unittest.skipIf(hasattr(os, 'setreuid'), 'setreuid() available on platform') + def test_user_error(self): + with self.assertRaises(ValueError): + subprocess.check_call([sys.executable, "-c", "pass"], user=65535) + + @unittest.skipUnless(hasattr(os, 'setregid'), 'no setregid() on platform') + def test_group(self): + gid = os.getegid() + group_list = [65534 if gid != 65534 else 65533] + name_group = "nogroup" if sys.platform != 'darwin' else "staff" + + if grp is not None: + group_list.append(name_group) + + for group in group_list + [gid]: + with self.subTest(group=group): + try: + output = subprocess.check_output( + [sys.executable, "-c", + "import os; print(os.getgid())"], + group=group) + except OSError as e: + if e.errno != errno.EPERM: + raise + else: + if isinstance(group, str): + group_gid = grp.getgrnam(group).gr_gid + else: + group_gid = group + + child_group = int(output) + self.assertEqual(child_group, group_gid) + + # make sure we bomb on negative values + with self.assertRaises(ValueError): + subprocess.check_call([sys.executable, "-c", "pass"], group=-1) + + if grp is None: + with self.assertRaises(ValueError): + subprocess.check_call([sys.executable, "-c", "pass"], group=name_group) + + @unittest.skipIf(hasattr(os, 'setregid'), 'setregid() available on platform') + def test_group_error(self): + with self.assertRaises(ValueError): + subprocess.check_call([sys.executable, "-c", "pass"], group=65535) + + @unittest.skipUnless(hasattr(os, 'setgroups'), 'no setgroups() on platform') + def test_extra_groups(self): + gid = os.getegid() + group_list = [65534 if gid != 65534 else 65533] + name_group = "nogroup" if sys.platform != 'darwin' else "staff" + perm_error = False + + if grp is not None: + group_list.append(name_group) + + try: + output = subprocess.check_output( + [sys.executable, "-c", + "import os, sys, json; json.dump(os.getgroups(), sys.stdout)"], + extra_groups=group_list) + except OSError as ex: + if ex.errno != errno.EPERM: + raise + perm_error = True + + else: + parent_groups = os.getgroups() + child_groups = json.loads(output) + + if grp is not None: + desired_gids = [grp.getgrnam(g).gr_gid if isinstance(g, str) else g + for g in group_list] + else: + desired_gids = group_list + + if perm_error: + self.assertEqual(set(child_groups), set(parent_groups)) + else: + self.assertEqual(set(desired_gids), set(child_groups)) + + # make sure we bomb on negative values + with self.assertRaises(ValueError): + subprocess.check_call([sys.executable, "-c", "pass"], extra_groups=[-1]) + + if grp is None: + with self.assertRaises(ValueError): + subprocess.check_call([sys.executable, "-c", "pass"], + extra_groups=[name_group]) + + @unittest.skipIf(hasattr(os, 'setgroups'), 'setgroups() available on platform') + def test_extra_groups_error(self): + with self.assertRaises(ValueError): + subprocess.check_call([sys.executable, "-c", "pass"], extra_groups=[]) + def test_run_abort(self): # returncode handles signal termination with support.SuppressCrashReport(): @@ -2704,7 +2847,8 @@ def test_fork_exec(self): True, (), cwd, env_list, -1, -1, -1, -1, 1, 2, 3, 4, - True, True, func) + True, True, + False, 0, False, [], False, 0, func) finally: if not gc_enabled: gc.disable() @@ -2743,7 +2887,9 @@ def __int__(self): True, fds_to_keep, None, [b"env"], -1, -1, -1, -1, 1, 2, 3, 4, - True, True, None) + True, True, + None, None, None, + None) self.assertIn('fds_to_keep', str(c.exception)) finally: if not gc_enabled: @@ -3155,7 +3301,7 @@ def test_getoutput(self): def test__all__(self): """Ensure that __all__ is populated properly.""" - intentionally_excluded = {"list2cmdline", "Handle"} + intentionally_excluded = {"list2cmdline", "Handle", "pwd", "grp"} exported = set(subprocess.__all__) possible_exports = set() import types diff --git a/Misc/NEWS.d/next/Library/2019-02-19-17-32-45.bpo-36046.fX9OPj.rst b/Misc/NEWS.d/next/Library/2019-02-19-17-32-45.bpo-36046.fX9OPj.rst new file mode 100644 index 00000000000000..5e809d6a6032cd --- /dev/null +++ b/Misc/NEWS.d/next/Library/2019-02-19-17-32-45.bpo-36046.fX9OPj.rst @@ -0,0 +1,2 @@ +Added ``user``, ``group`` and ``extra_groups`` parameters to the +subprocess.Popen constructor. Patch by Patrick McLean. diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 81a23c6d330037..5517a5d37e5dfe 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -20,6 +20,11 @@ #ifdef HAVE_DIRENT_H #include #endif +#ifdef HAVE_GRP_H +#include +#endif /* HAVE_GRP_H */ + +#include "posixmodule.h" #ifdef _Py_MEMORY_SANITIZER # include @@ -47,6 +52,12 @@ # define FD_DIR "/proc/self/fd" #endif +#ifdef NGROUPS_MAX +#define MAX_GROUPS NGROUPS_MAX +#else +#define MAX_GROUPS 64 +#endif + #define POSIX_CALL(call) do { if ((call) == -1) goto error; } while (0) @@ -405,6 +416,9 @@ child_exec(char *const exec_array[], int errpipe_read, int errpipe_write, int close_fds, int restore_signals, int call_setsid, + int call_setgid, gid_t gid, + int call_setgroups, size_t groups_size, const gid_t *groups, + int call_setuid, uid_t uid, PyObject *py_fds_to_keep, PyObject *preexec_fn, PyObject *preexec_fn_args_tuple) @@ -482,6 +496,22 @@ child_exec(char *const exec_array[], POSIX_CALL(setsid()); #endif +#ifdef HAVE_SETGROUPS + if (call_setgroups) + POSIX_CALL(setgroups(groups_size, groups)); +#endif /* HAVE_SETGROUPS */ + +#ifdef HAVE_SETREGID + if (call_setgid) + POSIX_CALL(setregid(gid, gid)); +#endif /* HAVE_SETREGID */ + +#ifdef HAVE_SETREUID + if (call_setuid) + POSIX_CALL(setreuid(uid, uid)); +#endif /* HAVE_SETREUID */ + + reached_preexec = 1; if (preexec_fn != Py_None && preexec_fn_args_tuple) { /* This is where the user has asked us to deadlock their program. */ @@ -561,26 +591,33 @@ subprocess_fork_exec(PyObject* self, PyObject *args) PyObject *env_list, *preexec_fn; PyObject *process_args, *converted_args = NULL, *fast_args = NULL; PyObject *preexec_fn_args_tuple = NULL; + PyObject *groups_list; + PyObject *uid_object, *gid_object; int p2cread, p2cwrite, c2pread, c2pwrite, errread, errwrite; int errpipe_read, errpipe_write, close_fds, restore_signals; int call_setsid; + int call_setgid = 0, call_setgroups = 0, call_setuid = 0; + uid_t uid; + gid_t gid, *groups = NULL; PyObject *cwd_obj, *cwd_obj2; const char *cwd; pid_t pid; int need_to_reenable_gc = 0; char *const *exec_array, *const *argv = NULL, *const *envp = NULL; - Py_ssize_t arg_num; + Py_ssize_t arg_num, num_groups = 0; int need_after_fork = 0; int saved_errno = 0; if (!PyArg_ParseTuple( - args, "OOpO!OOiiiiiiiiiiO:fork_exec", + args, "OOpO!OOiiiiiiiiiiOOOO:fork_exec", &process_args, &executable_list, &close_fds, &PyTuple_Type, &py_fds_to_keep, &cwd_obj, &env_list, &p2cread, &p2cwrite, &c2pread, &c2pwrite, &errread, &errwrite, &errpipe_read, &errpipe_write, - &restore_signals, &call_setsid, &preexec_fn)) + &restore_signals, &call_setsid, + &gid_object, &groups_list, &uid_object, + &preexec_fn)) return NULL; if (_PyInterpreterState_Get() != PyInterpreterState_Main()) { @@ -677,6 +714,90 @@ subprocess_fork_exec(PyObject* self, PyObject *args) cwd_obj2 = NULL; } + if (groups_list != Py_None) { +#ifdef HAVE_SETGROUPS + Py_ssize_t i; + unsigned long gid; + + if (!PyList_Check(groups_list)) { + PyErr_SetString(PyExc_TypeError, + "setgroups argument must be a list"); + goto cleanup; + } + num_groups = PySequence_Size(groups_list); + + if (num_groups < 0) + goto cleanup; + + if (num_groups > MAX_GROUPS) { + PyErr_SetString(PyExc_ValueError, "too many groups"); + goto cleanup; + } + + if ((groups = PyMem_RawMalloc(num_groups * sizeof(gid_t))) == NULL) { + PyErr_SetString(PyExc_MemoryError, + "failed to allocate memory for group list"); + goto cleanup; + } + + for (i = 0; i < num_groups; i++) { + PyObject *elem; + elem = PySequence_GetItem(groups_list, i); + if (!elem) + goto cleanup; + if (!PyLong_Check(elem)) { + PyErr_SetString(PyExc_TypeError, + "groups must be integers"); + Py_DECREF(elem); + goto cleanup; + } else { + /* In posixmodule.c UnsignedLong is used as a fallback value + * if the value provided does not fit in a Long. Since we are + * already doing the bounds checking on the Python side, we + * can go directly to an UnsignedLong here. */ + if (!_Py_Gid_Converter(elem, &gid)) { + Py_DECREF(elem); + PyErr_SetString(PyExc_ValueError, "invalid group id"); + goto cleanup; + } + groups[i] = gid; + } + Py_DECREF(elem); + } + call_setgroups = 1; + +#else /* HAVE_SETGROUPS */ + PyErr_BadInternalCall(); + goto cleanup; +#endif /* HAVE_SETGROUPS */ + } + + if (gid_object != Py_None) { +#ifdef HAVE_SETREGID + if (!_Py_Gid_Converter(gid_object, &gid)) + goto cleanup; + + call_setgid = 1; + +#else /* HAVE_SETREGID */ + PyErr_BadInternalCall(); + goto cleanup; +#endif /* HAVE_SETREUID */ + } + + if (uid_object != Py_None) { +#ifdef HAVE_SETREUID + if (!_Py_Uid_Converter(uid_object, &uid)) + goto cleanup; + + call_setuid = 1; + +#else /* HAVE_SETREUID */ + PyErr_BadInternalCall(); + goto cleanup; +#endif /* HAVE_SETREUID */ + } + /* This must be the last thing done before fork() because we do not * want to call PyOS_BeforeFork() if there is any chance of another * error leading to the cleanup: code without calling fork(). */ @@ -709,6 +830,8 @@ subprocess_fork_exec(PyObject* self, PyObject *args) p2cread, p2cwrite, c2pread, c2pwrite, errread, errwrite, errpipe_read, errpipe_write, close_fds, restore_signals, call_setsid, + call_setgid, gid, call_setgroups, num_groups, groups, + call_setuid, uid, py_fds_to_keep, preexec_fn, preexec_fn_args_tuple); _exit(255); return NULL; /* Dead code to avoid a potential compiler warning. */ @@ -753,6 +876,8 @@ subprocess_fork_exec(PyObject* self, PyObject *args) _Py_FreeCharPArray(argv); if (exec_array) _Py_FreeCharPArray(exec_array); + + PyMem_RawFree(groups); Py_XDECREF(converted_args); Py_XDECREF(fast_args); Py_XDECREF(preexec_fn_args_tuple); @@ -766,7 +891,10 @@ PyDoc_STRVAR(subprocess_fork_exec_doc, "fork_exec(args, executable_list, close_fds, cwd, env,\n\ p2cread, p2cwrite, c2pread, c2pwrite,\n\ errread, errwrite, errpipe_read, errpipe_write,\n\ - restore_signals, call_setsid, preexec_fn)\n\ + restore_signals, call_setsid,\n\ + call_setgid, gid, groups_size, gids,\n\ + call_setuid, uid,\n\ + preexec_fn)\n\ \n\ Forks a child process, closes parent file descriptors as appropriate in the\n\ child and dups the few that are needed before calling exec() in the child\n\ From 6359b420240ec7d370cd8f57085dca895499dc76 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 12 Sep 2019 17:33:16 +0100 Subject: [PATCH 2/3] Don't pass via wrong number of args TypeError. --- Lib/test/test_subprocess.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 2db7f857549773..781442a2b0b92a 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -2841,14 +2841,23 @@ def test_fork_exec(self): ([b"arg"], [b"exe"], 123, [b"env"]), ([b"arg"], [b"exe"], None, 123), ): - with self.assertRaises(TypeError): + with self.assertRaises(TypeError) as err: _posixsubprocess.fork_exec( args, exe_list, True, (), cwd, env_list, -1, -1, -1, -1, 1, 2, 3, 4, True, True, - False, 0, False, [], False, 0, func) + False, [], 0, + func) + # Attempt to prevent + # "TypeError: fork_exec() takes exactly N arguments (M given)" + # from passing the test. More refactoring to have us start + # with a valid *args list, confirm a good call with that works + # before mutating it in various ways to ensure that bad calls + # with individual arg type errors raise a typeerror would be + # ideal. Saving that for a future PR... + self.assertNotIn('takes exactly', str(err.exception)) finally: if not gc_enabled: gc.disable() From 812fc45af66593543d404c56ef554649f2502607 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 12 Sep 2019 17:34:08 +0100 Subject: [PATCH 3/3] Use versionadded 3.9, the feature window for 3.8 is closed. --- Doc/library/subprocess.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Doc/library/subprocess.rst b/Doc/library/subprocess.rst index 8e52243d60e8dd..3aa9931d0ee5df 100644 --- a/Doc/library/subprocess.rst +++ b/Doc/library/subprocess.rst @@ -520,7 +520,7 @@ functions. will be passed verbatim. (POSIX only) .. availability:: POSIX - .. versionadded:: 3.8 + .. versionadded:: 3.9 If *extra_groups* is not ``None``, the setgroups() system call will be made in the child process prior to the execution of the subprocess. @@ -529,7 +529,7 @@ functions. Integer values will be passed verbatim. (POSIX only) .. availability:: POSIX - .. versionadded:: 3.8 + .. versionadded:: 3.9 If *user* is not ``None``, the setreuid() system call will be made in the child process prior to the execution of the subprocess. If the provided @@ -538,7 +538,7 @@ functions. be passed verbatim. (POSIX only) .. availability:: POSIX - .. versionadded:: 3.8 + .. versionadded:: 3.9 If *env* is not ``None``, it must be a mapping that defines the environment variables for the new process; these are used instead of the default