From afdd51343cafbc02443fa6f7a2166af951a67c64 Mon Sep 17 00:00:00 2001 From: Martin Panter Date: Mon, 30 Nov 2015 02:21:41 +0000 Subject: [PATCH] Issue #25764: Preserve subprocess fork exception when preexec_fn used Also fix handling of failure to release the import lock. --- Lib/test/test_subprocess.py | 16 ++++++++++++++++ Misc/NEWS | 3 +++ Modules/_posixsubprocess.c | 35 ++++++++++++++++++----------------- 3 files changed, 37 insertions(+), 17 deletions(-) diff --git a/Lib/test/test_subprocess.py b/Lib/test/test_subprocess.py index 2ce59517a79..188f3375c14 100644 --- a/Lib/test/test_subprocess.py +++ b/Lib/test/test_subprocess.py @@ -1416,6 +1416,22 @@ class POSIXProcessTestCase(BaseTestCase): if not enabled: gc.disable() + def test_preexec_fork_failure(self): + # The internal code did not preserve the previous exception when + # re-enabling garbage collection + try: + from resource import getrlimit, setrlimit, RLIMIT_NPROC + except ImportError as err: + self.skipTest(err) # RLIMIT_NPROC is specific to Linux and BSD + limits = getrlimit(RLIMIT_NPROC) + [_, hard] = limits + setrlimit(RLIMIT_NPROC, (0, hard)) + self.addCleanup(setrlimit, RLIMIT_NPROC, limits) + # Forking should raise EAGAIN, translated to BlockingIOError + with self.assertRaises(BlockingIOError): + subprocess.call([sys.executable, '-c', ''], + preexec_fn=lambda: None) + def test_args_string(self): # args is a string fd, fname = tempfile.mkstemp() diff --git a/Misc/NEWS b/Misc/NEWS index 76296000b45..0d4977fc2fa 100644 --- a/Misc/NEWS +++ b/Misc/NEWS @@ -115,6 +115,9 @@ Core and Builtins Library ------- +- Issue #25764: In the subprocess module, preserve any exception caused by + fork() failure when preexec_fn is used. + - Issue #6478: _strptime's regexp cache now is reset after changing timezone with time.tzset(). diff --git a/Modules/_posixsubprocess.c b/Modules/_posixsubprocess.c index 452d592f150..1490223d3f3 100644 --- a/Modules/_posixsubprocess.c +++ b/Modules/_posixsubprocess.c @@ -47,17 +47,25 @@ #define POSIX_CALL(call) do { if ((call) == -1) goto error; } while (0) -/* Given the gc module call gc.enable() and return 0 on success. */ +/* If gc was disabled, call gc.enable(). Return 0 on success. */ static int -_enable_gc(PyObject *gc_module) +_enable_gc(int need_to_reenable_gc, PyObject *gc_module) { PyObject *result; _Py_IDENTIFIER(enable); + PyObject *exctype, *val, *tb; - result = _PyObject_CallMethodId(gc_module, &PyId_enable, NULL); - if (result == NULL) - return 1; - Py_DECREF(result); + if (need_to_reenable_gc) { + PyErr_Fetch(&exctype, &val, &tb); + result = _PyObject_CallMethodId(gc_module, &PyId_enable, NULL); + if (exctype != NULL) { + PyErr_Restore(exctype, val, tb); + } + if (result == NULL) { + return 1; + } + Py_DECREF(result); + } return 0; } @@ -691,6 +699,7 @@ subprocess_fork_exec(PyObject* self, PyObject *args) _PyImport_ReleaseLock() < 0 && !PyErr_Occurred()) { PyErr_SetString(PyExc_RuntimeError, "not holding the import lock"); + pid = -1; } import_lock_held = 0; @@ -702,9 +711,8 @@ subprocess_fork_exec(PyObject* self, PyObject *args) _Py_FreeCharPArray(exec_array); /* Reenable gc in the parent process (or if fork failed). */ - if (need_to_reenable_gc && _enable_gc(gc_module)) { - Py_XDECREF(gc_module); - return NULL; + if (_enable_gc(need_to_reenable_gc, gc_module)) { + pid = -1; } Py_XDECREF(preexec_fn_args_tuple); Py_XDECREF(gc_module); @@ -726,14 +734,7 @@ cleanup: 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) { - PyObject *exctype, *val, *tb; - PyErr_Fetch(&exctype, &val, &tb); - _enable_gc(gc_module); - PyErr_Restore(exctype, val, tb); - } + _enable_gc(need_to_reenable_gc, gc_module); Py_XDECREF(gc_module); return NULL; }