Skip to content

gh-77046: Pass the _O_NOINHERIT flag to _open_osfhandle() calls #113817

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
merged 4 commits into from
Jan 10, 2024
+68 −6
Merged

gh-77046: Pass the _O_NOINHERIT flag to _open_osfhandle() calls #113817

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7bb228d
gh-77046: Pass the _O_NOINHERIT flag to _open_osfhandle() calls
vstinner Jan 8, 2024
bd97d5d
Use SuppressCrashReport
vstinner Jan 8, 2024
0547f21
Pass _O_NOINHERIT explicitly
vstinner Jan 8, 2024
dcdc5ff
add comment to clarify the code
vstinner Jan 10, 2024
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
8 changes: 6 additions & 2 deletions Doc/library/msvcrt.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down
55 changes: 55 additions & 0 deletions Lib/test/test_os.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4485,6 +4485,61 @@ 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.
#
# 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)

code = textwrap.dedent(f"""
import errno
import os
import test.support
try:
import msvcrt
except ImportError:
msvcrt = None

fd = {fd}

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
# get_osfhandle(fd) failed with EBADF as expected
else:
raise Exception("get_osfhandle() must fail")

Copy link
Member

Choose a reason for hiding this comment

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

With else it would be clearer that the following code is not a fallback, but an alternative version.

Oh, and because almost all code is different, you can use different scripts, depending on the OS.

Copy link
Member Author

Choose a reason for hiding this comment

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

With else it would be clearer that the following code is not a fallback

Sorry, else on which line? Would you mind to elaborate?

The code calls msvcrt.get_osfhandle() and os.dup() on Windows on purpose. I explained it in a comment a few lines before.

Oh, and because almost all code is different, you can use different scripts, depending on the OS.

I prefer to have a single code base on all platforms, since most of the code is in common. Only the code using msvcrt is optional. Even on Windows, msvcrt may not be available on Python implementations other than CPython.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see now that there is a possibility for the code above to not raise an exception. So the code below this line can be executed on Windows.

Copy link
Member Author

Choose a reason for hiding this comment

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

The code makes sure that msvcrt.get_osfhandle() and os.dup() both raise an exception on the "not inherited" file descriptor.

try:
fd3 = os.dup(fd)
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")
""")

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
Expand Down
3 changes: 3 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2024-01-08-14-34-02.gh-issue-77046.sDUh2d.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
4 changes: 2 additions & 2 deletions Modules/_io/winconsoleio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions Modules/posixmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down