gh-77046: os.pipe() sets _O_NOINHERIT flag on fds (#113817)

vstinner · ZackerySpytz · web-flow · commit 1d75fa43a25e · 2024-01-10T23:02:17.000+01:00
On Windows, set _O_NOINHERIT flag on file descriptors
created by os.pipe() and io.WindowsConsoleIO.

Add test_pipe_spawnl() to test_os.

Co-authored-by: Zackery Spytz &lt;zspytz@gmail.com&gt;
diff --git 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/Lib/test/test_os.py b/Lib/test/test_os.py
@@ -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")
+
+                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
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
@@ -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/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
@@ -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);

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+On Windows, file descriptors wrapping Windows handles are now created non`
	`2`	+inheritable by default (:pep:`446`). Patch by Zackery Spytz and Victor
	`3`	`+Stinner.`