bpo-37267: Do not check for FILE_TYPE_CHAR in os.dup() on Windows

[ZackerySpytz]

On Windows, os.dup() no longer creates an inheritable fd when handling a
character file.

https://bugs.python.org/issue37267

[mangrisano]

/cc @vstinner @zooba

[vstinner]

LGTM.

[vstinner]

I tried to understand why _Py_dup() didn't make FILE_TYPE_CHAR non-inheritable. I found my own commit:

commit daf455554bc21b6b5df0a016ab5fa639d36cc595
Author: Victor Stinner <victor.stinner@gmail.com>
Date:   Wed Aug 28 00:53:59 2013 +0200

    Issue #18571: Implementation of the PEP 446: file descriptors and file handles
    are now created non-inheritable; add functions os.get/set_inheritable(),
    os.get/set_handle_inheritable() and socket.socket.get/set_inheritable().

... but I don't recall why I had to use:

+    /* Character files like console cannot be make non-inheritable */
+    if (ftype != FILE_TYPE_CHAR) {
+        if (_Py_set_inheritable(fd, 0, NULL) < 0) {
+            close(fd);
+            return -1;
+        }
+    }

Maybe it was a limitation in older Windows versions?

[vstinner]

Hum, I found someone reporting ERROR_ACCESS_DENIED error when SetHandleInformation(handle, HANDLE_FLAG_INHERIT, 0) is called on a virtual NIC device:
https://stackoverflow.com/questions/46261958/sethandleinformation-failed-with-error-code-error-access-denied-5

Maybe we should try to make the handle non-inheritable but ignore ERROR_ACCESS_DENIED if SetHandleInformation() fails? (and raise for other error codes)

[eryksun]

I tried to understand why _Py_dup() didn't make FILE_TYPE_CHAR non-inheritable.

Maybe it's related to assuming the standard handles have to be inheritable in Windows, and in particular console handles. But the console isn't the only character device in Windows. NUL and COM ports are also character devices.

The standard handles don't have to be inheritable. For one example. if the child is a console application, we can rely on the system to duplicate our standard handles to the child under the following conditions:

• bInheritHandles is false.
• STARTF_USESTDHANDLES is not in the process startup flags, i.e. the parent's standard handles are not overridden.
• DETACHED_PROCESS, CREATE_NEW_CONSOLE, or CREATE_NO_WINDOW are not in the process creation flags, i.e. the parent's console, if any, will be inherited.

That said, if bInheritHandles is true, the standard handles have to be inheritable if the child needs them. (The source of the handle values is either the startup-info record if the flags value includes STARTF_USESTDHANDLES or else the values in the current process.) This doesn't affect subprocess.Popen, generally, except when close_fds is false without overriding any of the standard handles. It always affects os.spawn* and os.system.

This behavior is linked to file descriptors 0-2 as well, since the C runtime couples fds 0-2 with the process standard handles in console applications. For example duping to fd 0 updates the StandardInput handle.

>>> os.dup2(os.dup(0), 0, inheritable=False)
0

In this case, a child Python process exits immediately instead of running the REPL because it's executed with an invalid StandardInput value:

>>> os.system('python.exe')
0
>>> os.spawnl(os.P_WAIT, sys.executable, 'python')
0
>>> subprocess.call('python.exe', close_fds=False)
0

The result is similar in Linux. On the other hand, Windows diverges from Linux in this case if close_fds=True (default). In this case, Windows duplicates the standard handles to the child instead of relying on inheritance:

>>> subprocess.call('python.exe')
Python 3.7.3 (v3.7.3:ef4ec6ed12, Mar 25 2019, 22:22:05) [MSC v.1916 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>>

[eryksun]

Maybe we should try to make the handle non-inheritable but ignore ERROR_ACCESS_DENIED if SetHandleInformation() fails? (and raise for other error codes)

I'd only ignore the error if GetHandleInformation reports that the handle already has the desired value.

[miss-islington]

Thanks @ZackerySpytz for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8.
🐍🍒⛏🤖

[bedevere-bot]

GH-14140 is a backport of this pull request to the 3.8 branch.

[bedevere-bot]

GH-14141 is a backport of this pull request to the 3.7 branch.