From 89143b4ed7e6b48484caac5754cca2fdb8a07460 Mon Sep 17 00:00:00 2001 From: Alexey Izbyshev Date: Sun, 25 Oct 2020 14:55:05 +0300 Subject: [PATCH 1/4] 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. --- Modules/_posixsubprocess.c | 52 +++++++++++++------------------------- 1 file changed, 17 insertions(+), 35 deletions(-) diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 5e5fbb2e79a7f82..dca6ab891521aa9 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -87,8 +87,8 @@ get_posixsubprocess_state(PyObject *module) #define _posixsubprocessstate_global get_posixsubprocess_state(PyState_FindModule(&_posixsubprocessmodule)) -/* 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; @@ -98,15 +98,16 @@ _enable_gc(int need_to_reenable_gc, PyObject *gc_module) PyErr_Fetch(&exctype, &val, &tb); result = PyObject_CallMethodNoArgs( gc_module, _posixsubprocessstate_global->enable); + if (result == NULL) { + /* We might have created a child process at this point, + * so we have no good way to handle a failure to reenable GC. */ + PyErr_Clear(); + } + Py_XDECREF(result); if (exctype != NULL) { PyErr_Restore(exctype, val, tb); } - if (result == NULL) { - return 1; - } - Py_DECREF(result); } - return 0; } @@ -774,7 +775,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; @@ -1010,8 +1011,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; @@ -1050,50 +1049,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; - } - PyMem_RawFree(groups); - 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); } From 551f6115f9fad42994998105c4c08daed52473d0 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sat, 31 Oct 2020 14:06:22 -0700 Subject: [PATCH 2/4] Code defensively: also check for pid == -1 Code defensively and also check `pid == -1` when setting the exception as that is the condition we return NULL on to let Python know there was an exception. In theory they are the same. But that depends on `fork()` or `vfork()` behaving as documented and setting errno properly (likely true... but if not we still want an error). This also defends against future bugs if the above code were refactored wrong. --- Modules/_posixsubprocess.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index dca6ab891521aa9..2c2591798b8d41c 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -1053,7 +1053,8 @@ subprocess_fork_exec(PyObject* self, PyObject *args) PyOS_AfterFork_Parent(); cleanup: - if (saved_errno != 0) { + if (saved_errno != 0 || pid == -1) { + assert(pid == -1); errno = saved_errno; /* We can't call this above as PyOS_AfterFork_Parent() calls back * into Python code which would see the unreturned error. */ From 72d86c474ed254fdbeaa3e73ea586485b08720c8 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sat, 31 Oct 2020 14:38:53 -0700 Subject: [PATCH 3/4] PyErr_Print instead of PyErr_Clear on gc.en fail This at least lets the user know that something serious has gone wrong. --- Modules/_posixsubprocess.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 2c2591798b8d41c..57dd115619f5e05 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -99,9 +99,10 @@ _enable_gc(int need_to_reenable_gc, PyObject *gc_module) result = PyObject_CallMethodNoArgs( gc_module, _posixsubprocessstate_global->enable); if (result == NULL) { - /* We might have created a child process at this point, - * so we have no good way to handle a failure to reenable GC. */ - PyErr_Clear(); + /* 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) { From 3f333c10980732501b58b7cbe264e54cf6c2fe23 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Sat, 31 Oct 2020 17:33:07 -0700 Subject: [PATCH 4/4] undo my added pid == -1 check, it was wrong --- Modules/_posixsubprocess.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 57dd115619f5e05..a00e13739aca4f5 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -1054,8 +1054,7 @@ subprocess_fork_exec(PyObject* self, PyObject *args) PyOS_AfterFork_Parent(); cleanup: - if (saved_errno != 0 || pid == -1) { - assert(pid == -1); + 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. */