From e8aadd0680a208a4963aa801a5a9d505b4af7add Mon Sep 17 00:00:00 2001 From: Alexey Izbyshev Date: Sun, 1 Nov 2020 08:33:08 +0300 Subject: [PATCH] bpo-42146: Unify cleanup in subprocess_fork_exec() (GH-22970) * bpo-42146: Unify cleanup in subprocess_fork_exec() Also ignore errors from _enable_gc(): * They are always suppressed by the current code due to a bug. * _enable_gc() is only used if `preexec_fn != None`, which is unsafe. * We don't have a good way to handle errors in case we successfully created a child process. Conflict:NA Reference:https://github.com/python/cpython/commit/d3b4e068077dd26927ae7485bd0303e09d962c02 Co-authored-by: Gregory P. Smith Signed-off-by: hanxinke --- Modules/_posixsubprocess.c | 52 +++++++++++++------------------------- 1 file changed, 18 insertions(+), 34 deletions(-) diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 2981253..fe801e9 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -69,8 +69,8 @@ #define POSIX_CALL(call) do { if ((call) == -1) goto error; } while (0) -/* If gc was disabled, call gc.enable(). Return 0 on success. */ -static int +/* If gc was disabled, call gc.enable(). Ignore errors. */ +static void _enable_gc(int need_to_reenable_gc, PyObject *gc_module) { PyObject *result; @@ -80,15 +80,17 @@ _enable_gc(int need_to_reenable_gc, PyObject *gc_module) if (need_to_reenable_gc) { PyErr_Fetch(&exctype, &val, &tb); result = _PyObject_CallMethodId(gc_module, &PyId_enable, NULL); + if (result == NULL) { + /* We might have created a child process at this point, we + * we have no good way to handle a failure to reenable GC + * and return information about the child process. */ + PyErr_Print(); + } + Py_XDECREF(result); if (exctype != NULL) { PyErr_Restore(exctype, val, tb); } - if (result == NULL) { - return 1; - } - Py_DECREF(result); } - return 0; } @@ -761,7 +763,7 @@ subprocess_fork_exec(PyObject* self, PyObject *args) int child_umask; PyObject *cwd_obj, *cwd_obj2 = NULL; const char *cwd; - pid_t pid; + pid_t pid = -1; int need_to_reenable_gc = 0; char *const *exec_array, *const *argv = NULL, *const *envp = NULL; Py_ssize_t arg_num, num_groups = 0; @@ -982,8 +984,6 @@ subprocess_fork_exec(PyObject* self, PyObject *args) sigset_t all_sigs; sigfillset(&all_sigs); if ((saved_errno = pthread_sigmask(SIG_BLOCK, &all_sigs, &old_sigs))) { - errno = saved_errno; - PyErr_SetFromErrno(PyExc_OSError); goto cleanup; } old_sigmask = &old_sigs; @@ -1022,49 +1022,33 @@ subprocess_fork_exec(PyObject* self, PyObject *args) } #endif - Py_XDECREF(cwd_obj2); - if (need_after_fork) PyOS_AfterFork_Parent(); - if (envp) - _Py_FreeCharPArray(envp); - if (argv) - _Py_FreeCharPArray(argv); - _Py_FreeCharPArray(exec_array); - /* Reenable gc in the parent process (or if fork failed). */ - if (_enable_gc(need_to_reenable_gc, gc_module)) { - pid = -1; - } - Py_XDECREF(preexec_fn_args_tuple); - Py_XDECREF(gc_module); - - if (pid == -1) { +cleanup: + if (saved_errno != 0) { errno = saved_errno; /* We can't call this above as PyOS_AfterFork_Parent() calls back * into Python code which would see the unreturned error. */ PyErr_SetFromErrno(PyExc_OSError); - return NULL; /* fork() failed. */ } - return PyLong_FromPid(pid); - -cleanup: + Py_XDECREF(preexec_fn_args_tuple); + PyMem_RawFree(groups); Py_XDECREF(cwd_obj2); if (envp) _Py_FreeCharPArray(envp); + Py_XDECREF(converted_args); + Py_XDECREF(fast_args); if (argv) _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); _enable_gc(need_to_reenable_gc, gc_module); Py_XDECREF(gc_module); - return NULL; + + return pid == -1 ? NULL : PyLong_FromPid(pid); } -- 2.23.0