From 8f437aac068e13fe8418bb0374baf4395e7b4994 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Sun, 5 Oct 2014 17:25:19 +0200 Subject: [PATCH] Issue #22290: Fix error handling in the _posixsubprocess module. * Don't call the garbage collector with an exception set: it causes an assertion to fail in debug mode. * Enhance also error handling if allocating an array for the executable list failed. * Add an unit test for 4 different errors in the _posixsubprocess module. --- Lib/test/test_subprocess.py | 33 +++++++++++++++++++++++++++++++++ Modules/_posixsubprocess.c | 20 ++++++++++++++------ 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 9616da03f33..538111564af 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -2216,6 +2216,39 @@ class POSIXProcessTestCase(BaseTestCase): self.assertNotIn(fd, remaining_fds) + @support.cpython_only + def test_fork_exec(self): + # Issue #22290: fork_exec() must not crash on memory allocation failure + # or other errors + import _posixsubprocess + gc_enabled = gc.isenabled() + try: + # Use a preexec function and enable the garbage collector + # to force fork_exec() to re-enable the garbage collector + # on error. + func = lambda: None + gc.enable() + + executable_list = "exec" # error: must be a sequence + + for args, exe_list, cwd, env_list in ( + (123, [b"exe"], None, [b"env"]), + ([b"arg"], 123, None, [b"env"]), + ([b"arg"], [b"exe"], 123, [b"env"]), + ([b"arg"], [b"exe"], None, 123), + ): + with self.assertRaises(TypeError): + _posixsubprocess.fork_exec( + args, exe_list, + True, [], cwd, env_list, + -1, -1, -1, -1, + 1, 2, 3, 4, + True, True, func) + finally: + if not gc_enabled: + gc.disable() + + @unittest.skipUnless(mswindows, "Windows specific tests") class Win32ProcessTestCase(BaseTestCase): diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index f84cd26e8d9..a8c2be223d7 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -538,6 +538,7 @@ subprocess_fork_exec(PyObject* self, PyObject *args) int need_to_reenable_gc = 0; char *const *exec_array, *const *argv = NULL, *const *envp = NULL; Py_ssize_t arg_num; + int import_lock_held = 0; if (!PyArg_ParseTuple( args, "OOpOOOiiiiiiiiiiO:fork_exec", @@ -590,10 +591,8 @@ subprocess_fork_exec(PyObject* self, PyObject *args) } exec_array = _PySequence_BytesToCharpArray(executable_list); - if (!exec_array) { - Py_XDECREF(gc_module); - return NULL; - } + if (!exec_array) + goto cleanup; /* Convert args and env into appropriate arguments for exec() */ /* These conversions are done in the parent process to avoid allocating @@ -635,6 +634,7 @@ subprocess_fork_exec(PyObject* self, PyObject *args) if (!preexec_fn_args_tuple) goto cleanup; _PyImport_AcquireLock(); + import_lock_held = 1; } if (cwd_obj != Py_None) { @@ -682,6 +682,7 @@ subprocess_fork_exec(PyObject* self, PyObject *args) PyErr_SetString(PyExc_RuntimeError, "not holding the import lock"); } + import_lock_held = 0; /* Parent process */ if (envp) @@ -704,18 +705,25 @@ subprocess_fork_exec(PyObject* self, PyObject *args) return PyLong_FromPid(pid); cleanup: + if (import_lock_held) + _PyImport_ReleaseLock(); if (envp) _Py_FreeCharPArray(envp); if (argv) _Py_FreeCharPArray(argv); - _Py_FreeCharPArray(exec_array); + if (exec_array) + _Py_FreeCharPArray(exec_array); Py_XDECREF(converted_args); Py_XDECREF(fast_args); Py_XDECREF(preexec_fn_args_tuple); /* Reenable gc if it was disabled. */ - if (need_to_reenable_gc) + if (need_to_reenable_gc) { + PyObject *exctype, *val, *tb; + PyErr_Fetch(&exctype, &val, &tb); _enable_gc(gc_module); + PyErr_Restore(exctype, val, tb); + } Py_XDECREF(gc_module); return NULL; }