From 530a70db5dcab286a053cdd6ede08e8150363bc2 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 21 Apr 2020 23:48:53 +0200 Subject: [PATCH 1/3] bpo-40138: Fix Windows os.waitpid() for large exit code Fix the Windows implementation of os.waitpid() for exit code larger than "INT_MAX >> 8". The exit status is now interpreted as an unsigned number. os.waitstatus_to_exitcode() now accepts wait status larger than INT_MAX. --- Lib/test/test_os.py | 66 ++++++++++++++----- .../2020-04-22-00-05-10.bpo-40138.i_oGqa.rst | 2 + Modules/clinic/posixmodule.c.h | 18 ++--- Modules/posixmodule.c | 35 ++++++++-- 4 files changed, 86 insertions(+), 35 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2020-04-22-00-05-10.bpo-40138.i_oGqa.rst diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index 73dc064d5ff752..bed1105473782c 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -2789,40 +2789,70 @@ def test_getppid(self): # We are the parent of our subprocess self.assertEqual(int(stdout), os.getpid()) - def test_waitpid(self): - args = [sys.executable, '-c', 'pass'] - # Add an implicit test for PyUnicode_FSConverter(). - pid = os.spawnv(os.P_NOWAIT, FakePath(args[0]), args) - support.wait_process(pid, exitcode=0) - - def test_waitstatus_to_exitcode(self): - exitcode = 23 + def check_waitpid(self, code, exitcode, callback=None): + # On Windows, os.spawnv() spawns a shell: run Python with a filename, + # rather than with -c CODE, to avoid the need to quote the code. filename = support.TESTFN self.addCleanup(support.unlink, filename) with open(filename, "w") as fp: - print(f'import sys; sys.exit({exitcode})', file=fp) + print(code, file=fp) fp.flush() + args = [sys.executable, filename] pid = os.spawnv(os.P_NOWAIT, args[0], args) + if callback is not None: + callback(pid) + + # don't use support.wait_process() to test directly os.waitpid() + # and os.waitstatus_to_exitcode() pid2, status = os.waitpid(pid, 0) self.assertEqual(os.waitstatus_to_exitcode(status), exitcode) self.assertEqual(pid2, pid) + def test_waitpid(self): + self.check_waitpid(code='pass', exitcode=0) + + def test_waitstatus_to_exitcode(self): + exitcode = 23 + code = f'import sys; sys.exit({exitcode})' + self.check_waitpid(code, exitcode=exitcode) + + with self.assertRaises(TypeError): + os.waitstatus_to_exitcode(0.0) + + @unittest.skipUnless(sys.platform == 'win32', 'win32-specific test') + def test_waitpid_windows(self): + # bpo-40138: test os.waitpid() and os.waitstatus_to_exitcode() + # with exit code larger than INT_MAX. + STATUS_CONTROL_C_EXIT = 0xC000013A + code = f'import _winapi; _winapi.ExitProcess({STATUS_CONTROL_C_EXIT})' + self.check_waitpid(code, exitcode=STATUS_CONTROL_C_EXIT) + + @unittest.skipUnless(sys.platform == 'win32', 'win32-specific test') + def test_waitstatus_to_exitcode_windows(self): + max_exitcode = 2 ** 32 - 1 + for exitcode in (0, 1, 5, max_exitcode): + self.assertEqual(os.waitstatus_to_exitcode(exitcode << 8), + exitcode) + + # invalid values + with self.assertRaises(ValueError): + os.waitstatus_to_exitcode((max_exitcode + 1) << 8) + with self.assertRaises(OverflowError): + os.waitstatus_to_exitcode(-1) + # Skip the test on Windows @unittest.skipUnless(hasattr(signal, 'SIGKILL'), 'need signal.SIGKILL') def test_waitstatus_to_exitcode_kill(self): + code = f'import time; time.sleep({support.LONG_TIMEOUT})' signum = signal.SIGKILL - args = [sys.executable, '-c', - f'import time; time.sleep({support.LONG_TIMEOUT})'] - pid = os.spawnv(os.P_NOWAIT, args[0], args) - os.kill(pid, signum) + def kill_process(pid): + os.kill(pid, signum) - pid2, status = os.waitpid(pid, 0) - self.assertEqual(os.waitstatus_to_exitcode(status), -signum) - self.assertEqual(pid2, pid) + self.check_waitpid(code, exitcode=-signum, callback=kill_process) class SpawnTests(unittest.TestCase): @@ -2884,6 +2914,10 @@ def test_spawnv(self): exitcode = os.spawnv(os.P_WAIT, args[0], args) self.assertEqual(exitcode, self.exitcode) + # Test for PyUnicode_FSConverter() + exitcode = os.spawnv(os.P_WAIT, FakePath(args[0]), args) + self.assertEqual(exitcode, self.exitcode) + @requires_os_func('spawnve') def test_spawnve(self): args = self.create_args(with_env=True) diff --git a/Misc/NEWS.d/next/Library/2020-04-22-00-05-10.bpo-40138.i_oGqa.rst b/Misc/NEWS.d/next/Library/2020-04-22-00-05-10.bpo-40138.i_oGqa.rst new file mode 100644 index 00000000000000..ad5faf3865751a --- /dev/null +++ b/Misc/NEWS.d/next/Library/2020-04-22-00-05-10.bpo-40138.i_oGqa.rst @@ -0,0 +1,2 @@ +Fix the Windows implementation of :func:`os.waitpid` for exit code larger than +``INT_MAX >> 8``. The exit status is now interpreted as an unsigned number. diff --git a/Modules/clinic/posixmodule.c.h b/Modules/clinic/posixmodule.c.h index 9a605e4841a008..a2b4566443b517 100644 --- a/Modules/clinic/posixmodule.c.h +++ b/Modules/clinic/posixmodule.c.h @@ -8839,7 +8839,7 @@ PyDoc_STRVAR(os_waitstatus_to_exitcode__doc__, {"waitstatus_to_exitcode", (PyCFunction)(void(*)(void))os_waitstatus_to_exitcode, METH_FASTCALL|METH_KEYWORDS, os_waitstatus_to_exitcode__doc__}, static PyObject * -os_waitstatus_to_exitcode_impl(PyObject *module, int status); +os_waitstatus_to_exitcode_impl(PyObject *module, PyObject *status_obj); static PyObject * os_waitstatus_to_exitcode(PyObject *module, PyObject *const *args, Py_ssize_t nargs, PyObject *kwnames) @@ -8848,22 +8848,14 @@ os_waitstatus_to_exitcode(PyObject *module, PyObject *const *args, Py_ssize_t na static const char * const _keywords[] = {"status", NULL}; static _PyArg_Parser _parser = {NULL, _keywords, "waitstatus_to_exitcode", 0}; PyObject *argsbuf[1]; - int status; + PyObject *status_obj; args = _PyArg_UnpackKeywords(args, nargs, NULL, kwnames, &_parser, 1, 1, 0, argsbuf); if (!args) { goto exit; } - if (PyFloat_Check(args[0])) { - PyErr_SetString(PyExc_TypeError, - "integer argument expected, got float" ); - goto exit; - } - status = _PyLong_AsInt(args[0]); - if (status == -1 && PyErr_Occurred()) { - goto exit; - } - return_value = os_waitstatus_to_exitcode_impl(module, status); + status_obj = args[0]; + return_value = os_waitstatus_to_exitcode_impl(module, status_obj); exit: return return_value; @@ -9426,4 +9418,4 @@ os_waitstatus_to_exitcode(PyObject *module, PyObject *const *args, Py_ssize_t na #ifndef OS_WAITSTATUS_TO_EXITCODE_METHODDEF #define OS_WAITSTATUS_TO_EXITCODE_METHODDEF #endif /* !defined(OS_WAITSTATUS_TO_EXITCODE_METHODDEF) */ -/*[clinic end generated code: output=545c08f76d7a6286 input=a9049054013a1b77]*/ +/*[clinic end generated code: output=ba73b68f1c435ff6 input=a9049054013a1b77]*/ diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 2157cbbe5d9b58..3386be0fbc85a0 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -7972,8 +7972,10 @@ os_waitpid_impl(PyObject *module, intptr_t pid, int options) if (res < 0) return (!async_err) ? posix_error() : NULL; + unsigned long long ustatus = (unsigned int)status; + /* shift the status left a byte so this is more like the POSIX waitpid */ - return Py_BuildValue(_Py_PARSE_INTPTR "i", res, status << 8); + return Py_BuildValue(_Py_PARSE_INTPTR "K", res, ustatus << 8); } #endif @@ -13829,7 +13831,7 @@ os__remove_dll_directory_impl(PyObject *module, PyObject *cookie) /*[clinic input] os.waitstatus_to_exitcode - status: int + status as status_obj: object Convert a wait status to an exit code. @@ -13847,10 +13849,20 @@ This function must not be called if WIFSTOPPED(status) is true. [clinic start generated code]*/ static PyObject * -os_waitstatus_to_exitcode_impl(PyObject *module, int status) -/*[clinic end generated code: output=c7c2265731f79b7a input=edfa5ca5006276fb]*/ +os_waitstatus_to_exitcode_impl(PyObject *module, PyObject *status_obj) +/*[clinic end generated code: output=db50b1b0ba3c7153 input=7fe2d7fdaea3db42]*/ { + if (PyFloat_Check(status_obj)) { + PyErr_SetString(PyExc_TypeError, + "integer argument expected, got float" ); + return NULL; + } #ifndef MS_WINDOWS + int status = _PyLong_AsInt(status_obj); + if (status == -1 && PyErr_Occurred()) { + return NULL; + } + WAIT_TYPE wait_status; WAIT_STATUS_INT(wait_status) = status; int exitcode; @@ -13889,8 +13901,19 @@ os_waitstatus_to_exitcode_impl(PyObject *module, int status) #else /* Windows implementation: see os.waitpid() implementation which uses _cwait(). */ - int exitcode = (status >> 8); - return PyLong_FromLong(exitcode); + unsigned long long status = PyLong_AsUnsignedLongLong(status_obj); + if (status == (unsigned long long)-1 && PyErr_Occurred()) { + return NULL; + } + + unsigned long long exitcode = (status >> 8); + /* ExitProcess() accepts an UINT type: + reject exit code which doesn't fit in an UINT */ + if (exitcode > UINT_MAX) { + PyErr_Format(PyExc_ValueError, "invalid exit code: %llu", exitcode); + return NULL; + } + return PyLong_FromUnsignedLong((unsigned long)exitcode); #endif } #endif From 8b807bb16f40624b1664b40a815aa84878e4e7b5 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 22 Apr 2020 14:57:43 +0200 Subject: [PATCH 2/3] Fix test on Windows --- Lib/test/test_os.py | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index bed1105473782c..980c1fd8b6a68d 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -2790,17 +2790,13 @@ def test_getppid(self): self.assertEqual(int(stdout), os.getpid()) def check_waitpid(self, code, exitcode, callback=None): - # On Windows, os.spawnv() spawns a shell: run Python with a filename, - # rather than with -c CODE, to avoid the need to quote the code. - filename = support.TESTFN - self.addCleanup(support.unlink, filename) - - with open(filename, "w") as fp: - print(code, file=fp) - fp.flush() - - args = [sys.executable, filename] - pid = os.spawnv(os.P_NOWAIT, args[0], args) + if sys.platform == 'win32': + # On Windows, os.spawnv() simply joins arguments with spaces: + # arguments need to be quoted + args = [f'"{sys.executable}"', '-c', f'"{code}"'] + else: + args = [sys.executable, filename] + pid = os.spawnv(os.P_NOWAIT, sys.executable, args) if callback is not None: callback(pid) From 279c4b237089fadd65012f96e42bd869e9b7c47e Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 22 Apr 2020 15:36:32 +0200 Subject: [PATCH 3/3] Oops, fix tests on Linux --- Lib/test/test_os.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index 980c1fd8b6a68d..74aef472434284 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -2795,7 +2795,7 @@ def check_waitpid(self, code, exitcode, callback=None): # arguments need to be quoted args = [f'"{sys.executable}"', '-c', f'"{code}"'] else: - args = [sys.executable, filename] + args = [sys.executable, '-c', code] pid = os.spawnv(os.P_NOWAIT, sys.executable, args) if callback is not None: