From 7bb228dc5370dd7ed66c7931950c01c391cb97fe Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Mon, 8 Jan 2024 14:32:14 +0100 Subject: [PATCH 1/4] gh-77046: Pass the _O_NOINHERIT flag to _open_osfhandle() calls _Py_open_osfhandle_noraise() now always set the _O_NOINHERIT flag. Add test_pipe_spawnl() to test_os. Co-Authored-By: Zackery Spytz --- Lib/test/test_os.py | 49 +++++++++++++++++++ ...4-01-08-14-34-02.gh-issue-77046.sDUh2d.rst | 3 ++ Python/fileutils.c | 4 ++ 3 files changed, 56 insertions(+) create mode 100644 Misc/NEWS.d/next/Core and Builtins/2024-01-08-14-34-02.gh-issue-77046.sDUh2d.rst diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index c66c5797471413..fd3d01fd3eb3ab 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -4485,6 +4485,55 @@ def test_openpty(self): self.assertEqual(os.get_inheritable(master_fd), False) self.assertEqual(os.get_inheritable(slave_fd), False) + @unittest.skipUnless(hasattr(os, 'spawnl'), "need os.openpty()") + def test_pipe_spawnl(self): + # gh-77046: On Windows, os.pipe() file descriptors must be created with + # _O_NOINHERIT to make them non-inheritable. UCRT has no public API to + # get (_osfile(fd) & _O_NOINHERIT), so use a functional test. + fd, fd2 = os.pipe() + self.addCleanup(os.close, fd) + self.addCleanup(os.close, fd2) + + # Make sure that fd is not inherited by a child process created by + # os.spawnl(): get_osfhandle() and dup() must fail with EBADF. + code = textwrap.dedent(f""" + import errno + import os + try: + import msvcrt + except ImportError: + msvcrt = None + + fd = {fd} + + if msvcrt is not None: + try: + handle = msvcrt.get_osfhandle(fd) + except OSError as exc: + if exc.errno != errno.EBADF: + raise + else: + raise Exception("get_osfhandle() must fail") + + try: + fd3 = os.dup(fd) + except OSError as exc: + if exc.errno != errno.EBADF: + raise + else: + os.close(fd3) + raise Exception("dup must fail") + """) + + filename = os_helper.TESTFN + self.addCleanup(os_helper.unlink, os_helper.TESTFN) + with open(filename, "w") as fp: + print(code, file=fp, end="") + + cmd = [sys.executable, filename] + exitcode = os.spawnl(os.P_WAIT, cmd[0], *cmd) + self.assertEqual(exitcode, 0) + class PathTConverterTests(unittest.TestCase): # tuples of (function name, allows fd arguments, additional arguments to diff --git a/Misc/NEWS.d/next/Core and Builtins/2024-01-08-14-34-02.gh-issue-77046.sDUh2d.rst b/Misc/NEWS.d/next/Core and Builtins/2024-01-08-14-34-02.gh-issue-77046.sDUh2d.rst new file mode 100644 index 00000000000000..9f0f144451df6c --- /dev/null +++ b/Misc/NEWS.d/next/Core and Builtins/2024-01-08-14-34-02.gh-issue-77046.sDUh2d.rst @@ -0,0 +1,3 @@ +On Windows, file descriptors wrapping Windows handles are now created non +inheritable by default (:pep:`446`). Patch by Zackery Spytz and Victor +Stinner. diff --git a/Python/fileutils.c b/Python/fileutils.c index 882d3299575cf3..43522bc1ee7795 100644 --- a/Python/fileutils.c +++ b/Python/fileutils.c @@ -2771,6 +2771,10 @@ _Py_get_osfhandle(int fd) int _Py_open_osfhandle_noraise(void *handle, int flags) { + // PEP 446: Create non inheritable file descriptor by default, + // always set the _O_NOINHERIT flag. + flags |= _O_NOINHERIT; + int fd; _Py_BEGIN_SUPPRESS_IPH fd = _open_osfhandle((intptr_t)handle, flags); From bd97d5da9adcc7326084a64b8b950caad8f2908b Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Mon, 8 Jan 2024 18:52:24 +0100 Subject: [PATCH 2/4] Use SuppressCrashReport --- Lib/test/test_os.py | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index fd3d01fd3eb3ab..ae23bcc201589f 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -4499,6 +4499,7 @@ def test_pipe_spawnl(self): code = textwrap.dedent(f""" import errno import os + import test.support try: import msvcrt except ImportError: @@ -4506,23 +4507,24 @@ def test_pipe_spawnl(self): fd = {fd} - if msvcrt is not None: + with test.support.SuppressCrashReport(): + if msvcrt is not None: + try: + handle = msvcrt.get_osfhandle(fd) + except OSError as exc: + if exc.errno != errno.EBADF: + raise + else: + raise Exception("get_osfhandle() must fail") + try: - handle = msvcrt.get_osfhandle(fd) + fd3 = os.dup(fd) except OSError as exc: if exc.errno != errno.EBADF: raise else: - raise Exception("get_osfhandle() must fail") - - try: - fd3 = os.dup(fd) - except OSError as exc: - if exc.errno != errno.EBADF: - raise - else: - os.close(fd3) - raise Exception("dup must fail") + os.close(fd3) + raise Exception("dup must fail") """) filename = os_helper.TESTFN From 0547f215a9b9b3146f4b22f4685d1f05c1fce9c3 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Tue, 9 Jan 2024 00:41:36 +0100 Subject: [PATCH 3/4] Pass _O_NOINHERIT explicitly --- Doc/library/msvcrt.rst | 8 ++++++-- Modules/_io/winconsoleio.c | 4 ++-- Modules/posixmodule.c | 4 ++-- Python/fileutils.c | 4 ---- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/Doc/library/msvcrt.rst b/Doc/library/msvcrt.rst index 0b059e746c61af..2a6d980ab78a60 100644 --- a/Doc/library/msvcrt.rst +++ b/Doc/library/msvcrt.rst @@ -75,10 +75,14 @@ File Operations .. function:: open_osfhandle(handle, flags) Create a C runtime file descriptor from the file handle *handle*. The *flags* - parameter should be a bitwise OR of :const:`os.O_APPEND`, :const:`os.O_RDONLY`, - and :const:`os.O_TEXT`. The returned file descriptor may be used as a parameter + parameter should be a bitwise OR of :const:`os.O_APPEND`, + :const:`os.O_RDONLY`, :const:`os.O_TEXT` and :const:`os.O_NOINHERIT`. + The returned file descriptor may be used as a parameter to :func:`os.fdopen` to create a file object. + The file descriptor is inheritable by default. Pass :const:`os.O_NOINHERIT` + flag to make it non inheritable. + .. audit-event:: msvcrt.open_osfhandle handle,flags msvcrt.open_osfhandle diff --git a/Modules/_io/winconsoleio.c b/Modules/_io/winconsoleio.c index 6680488b740cfc..602a2619c4694d 100644 --- a/Modules/_io/winconsoleio.c +++ b/Modules/_io/winconsoleio.c @@ -391,9 +391,9 @@ _io__WindowsConsoleIO___init___impl(winconsoleio *self, PyObject *nameobj, } if (self->writable) - self->fd = _Py_open_osfhandle_noraise(handle, _O_WRONLY | _O_BINARY); + self->fd = _Py_open_osfhandle_noraise(handle, _O_WRONLY | _O_BINARY | _O_NOINHERIT); else - self->fd = _Py_open_osfhandle_noraise(handle, _O_RDONLY | _O_BINARY); + self->fd = _Py_open_osfhandle_noraise(handle, _O_RDONLY | _O_BINARY | _O_NOINHERIT); if (self->fd < 0) { PyErr_SetFromErrnoWithFilenameObject(PyExc_OSError, nameobj); CloseHandle(handle); diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 39b1f3cb7b2b9b..179497a21b5a95 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -11578,8 +11578,8 @@ os_pipe_impl(PyObject *module) Py_BEGIN_ALLOW_THREADS ok = CreatePipe(&read, &write, &attr, 0); if (ok) { - fds[0] = _Py_open_osfhandle_noraise(read, _O_RDONLY); - fds[1] = _Py_open_osfhandle_noraise(write, _O_WRONLY); + fds[0] = _Py_open_osfhandle_noraise(read, _O_RDONLY | _O_NOINHERIT); + fds[1] = _Py_open_osfhandle_noraise(write, _O_WRONLY | _O_NOINHERIT); if (fds[0] == -1 || fds[1] == -1) { CloseHandle(read); CloseHandle(write); diff --git a/Python/fileutils.c b/Python/fileutils.c index 43522bc1ee7795..882d3299575cf3 100644 --- a/Python/fileutils.c +++ b/Python/fileutils.c @@ -2771,10 +2771,6 @@ _Py_get_osfhandle(int fd) int _Py_open_osfhandle_noraise(void *handle, int flags) { - // PEP 446: Create non inheritable file descriptor by default, - // always set the _O_NOINHERIT flag. - flags |= _O_NOINHERIT; - int fd; _Py_BEGIN_SUPPRESS_IPH fd = _open_osfhandle((intptr_t)handle, flags); From dcdc5ff3871a63206dc43c2ae3cae6fdf1202ec3 Mon Sep 17 00:00:00 2001 From: Victor Stinner Date: Wed, 10 Jan 2024 13:14:27 +0100 Subject: [PATCH 4/4] add comment to clarify the code --- Lib/test/test_os.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index ae23bcc201589f..bff6e604cccdd6 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -4490,12 +4490,14 @@ def test_pipe_spawnl(self): # gh-77046: On Windows, os.pipe() file descriptors must be created with # _O_NOINHERIT to make them non-inheritable. UCRT has no public API to # get (_osfile(fd) & _O_NOINHERIT), so use a functional test. + # + # Make sure that fd is not inherited by a child process created by + # os.spawnl(): get_osfhandle() and dup() must fail with EBADF. + fd, fd2 = os.pipe() self.addCleanup(os.close, fd) self.addCleanup(os.close, fd2) - # Make sure that fd is not inherited by a child process created by - # os.spawnl(): get_osfhandle() and dup() must fail with EBADF. code = textwrap.dedent(f""" import errno import os @@ -4514,6 +4516,7 @@ def test_pipe_spawnl(self): except OSError as exc: if exc.errno != errno.EBADF: raise + # get_osfhandle(fd) failed with EBADF as expected else: raise Exception("get_osfhandle() must fail") @@ -4522,6 +4525,7 @@ def test_pipe_spawnl(self): except OSError as exc: if exc.errno != errno.EBADF: raise + # os.dup(fd) failed with EBADF as expected else: os.close(fd3) raise Exception("dup must fail")