Skip to content

bpo-40138: Fix Windows os.waitpid() for large exit code #19637

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
vstinner merged 3 commits into from
Apr 22, 2020
Merged

bpo-40138: Fix Windows os.waitpid() for large exit code #19637

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
530a70d
bpo-40138: Fix Windows os.waitpid() for large exit code
vstinner Apr 21, 2020
8b807bb
Fix test on Windows
vstinner Apr 22, 2020
279c4b2
Oops, fix tests on Linux
vstinner Apr 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 50 additions & 16 deletions Lib/test/test_os.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

@eryksun eryksun Apr 22, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spawnv concatenates the arguments into a command line that gets passed to CreateProcessW in the lpCommandLine parameter. No shell is involved here. Unlike Python's subprocess.list2cmdline, the CRT's spawn family does not create a command line according to MSVC rules. It simply concatenates with spaces, which leaves it up to the caller to ensure that each argument is quoted and escaped according to the rules of the target application.

You can avoid the need to escape quotes in an argument by using only single quotes, which have no special meaning in MSVC argv parsing. For example: args = ['python', '-c', f'"{code}"'], where code contains no double quote characters.

There's no need for args[0] to be a fully-qualified path with spawnv. Actually, what you're doing by setting args[0] = sys.executable is wrong. It only works because the test build's path to "python.exe" contains no spaces. If the path has spaces and isn't quoted, then C argv[0] will only include up to the first space in the path, and the rest will be in argv[1], and so on. If you want to use the full path, it has to be quoted. For example, where sys.executable is "C:\Program Files\Python38\python.exe":

>>> os.spawnv(os.P_WAIT, sys.executable, [f'"{sys.executable}"', '-V'])
Python 3.8.1
0

Failing example without quoting:

>>> os.spawnv(os.P_WAIT, sys.executable, [sys.executable, '-V'])
C:\Program: can't open file 'Files\Python38\python.exe': [Errno 2] No such file or directory
2

Preferably, just use unqualified "python":

>>> os.spawnv(os.P_WAIT, sys.executable, ['python', '-V'])
Python 3.8.1
0

@vstinner vstinner Apr 22, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I didn't think about quoting sys.executable. I fixed the test.

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):
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 2 additions & 0 deletions Misc/NEWS.d/next/Library/2020-04-22-00-05-10.bpo-40138.i_oGqa.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
18 changes: 5 additions & 13 deletions Modules/clinic/posixmodule.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

35 changes: 29 additions & 6 deletions Modules/posixmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

@eryksun eryksun Apr 22, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any possibility that os.waitpid could simply stop shifting the status value by a byte in Windows? With the introduction of os.waitstatus_to_exitcode, scripts no longer need to make assumptions about the status value format, and the Windows implementation of waitstatus_to_exitcode could simply return the status value unchanged.

@vstinner vstinner Apr 22, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any possibility that os.waitpid could simply stop shifting the status value by a byte in Windows?

That would be a backward incompatible changes. If you look at changes that I made in https://bugs.python.org/issue40094 to use os.waitstatus_to_exitcode(), you can see that even in the Python stdlib, plently of code used directly "status >> 8" without even checking os.WIFEXITED().

I don't think that it's worth it to change how waitpid() encodes the exit code in the exit status. You should now use os.waitstatus_to_exitcode() to write portable code.

}
#endif

Expand Down Expand Up @@ -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.

Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down