classification
Title: os.dup() fails for standard streams on Windows 7
Type: behavior Stage: resolved
Components: Extension Modules, Windows Versions: Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: steve.dower Nosy List: Bruno Oliveira, Jeffrey.Kintscher, ZackerySpytz, daveb, eryksun, josh.r, lukasz.langa, miss-islington, ned.deily, paul.moore, steve.dower, tim.golden, vstinner, xflr6, zach.ware
Priority: release blocker Keywords: patch

Created on 2019-07-10 17:52 by daveb, last changed 2019-08-26 15:39 by steve.dower. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 15389 merged ZackerySpytz, 2019-08-22 15:08
PR 15437 merged miss-islington, 2019-08-23 18:38
PR 15438 merged miss-islington, 2019-08-23 18:39
Messages (17)
msg347627 - (view) Author: DaveB (daveb) Date: 2019-07-10 17:52
os.dup(fd) generates an OSError for standard streams (0: stdin, 1: stdout, 2: stderr) on Windows 7.  Works as expected on Windows 10.

Working backwards we found the issue first appears in Python 3.7.4rc1 and 3.8.0b2 on Windows 7.  Earlier releases work as expected.

>>> import os
>>> os.dup(1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [WinError 87] The parameter is incorrect
>>>
msg347632 - (view) Author: Josh Rosenberg (josh.r) * (Python triager) Date: 2019-07-10 18:38
This seems likely to have been caused by the fixes for #37267, which fixes an issue with os.dup leaving character streams inheritable (when the documentation specifies that the result must be non-inheritable).

The code originally didn't try to make the descriptor non-inheritable because someone believed it wasn't allowed for character files, and the subsequent patch comments say "That was a mistake". Is it possible it wasn't allowed on Windows 7, and is allowed on Windows 10?

I'm nosying the folks from #37267 for input.
msg347642 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2019-07-10 23:46
> OSError: [WinError 87] The parameter is incorrect

This error didn't originate from the C dup() call, since that would be based on errno, like `OSError: [Errno 9] Bad file descriptor`. It must come from _Py_set_inheritable. The only WINAPI call there is SetHandleInformation [1], which is documented to support "console input buffer" and "console screen buffer" handles, though it may be that the documentation is wrong.

Try the following code:

    >>> import os, msvcrt
    >>> msvcrt.get_osfhandle(0)
    84
    >>> os.set_handle_inheritable(msvcrt.get_osfhandle(0), False)

Prior to Windows 8, a console handle is tagged by setting the lower 2 bits (e.g. 3, 7, 11). The system uses this tag to direct calls to special console functions that route requests over the console LPC port. Thus, in the domain of File handles, setting the lower 2 bits is reserved to tag console handles. This should also be true in Windows 8+, even though console handle tagging is no longer used (now they're kernel handles for the ConDrv device). 

The above test will confirm my suspicion, but it looks some time around Windows XP/2003, Microsoft removed the internal [Get|Set]ConsoleHandleInformation functions that used to implement [Get|Set]HandleInformation for console handles, and the people responsible for the change failed to update the docs. Since the internal DuplicateConsoleHandle function wasn't removed, dup() itself still succeeds.

I suggest that _Py_set_inheritable should handle this case. If the call fails with ERROR_INVALID_PARAMETER, and the two tag bits of the handle are set, the handle is possibly (not necessarily) a console handle. In this case, if GetFileType is FILE_TYPE_CHARACTER, then it's a console handle, and the error should be ignored.

[1] https://docs.microsoft.com/en-us/windows/win32/api/handleapi/nf-handleapi-sethandleinformation
msg347644 - (view) Author: DaveB (daveb) Date: 2019-07-10 23:57
Results with Python 3.7.4 on Windows 7

>>> import os, msvcrt
>>> msvcrt.get_osfhandle(0)
15
>>> os.set_handle_inheritable(msvcrt.get_osfhandle(0), False)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [WinError 87] The parameter is incorrect
>>>
msg347668 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-07-11 10:11
Oh, os.dup() doc says: "On Windows, when duplicating a standard stream (0: stdin, 1: stdout, 2: stderr), the new file descriptor is inheritable."
https://docs.python.org/dev/library/os.html

Maybe we should add an inheritable paramater to os.dup() in Python 3.8?
msg347671 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2019-07-11 11:02
> os.dup() doc says: "On Windows, when duplicating a standard stream 
> (0: stdin, 1: stdout, 2: stderr), the new file descriptor is 
> inheritable."

That's not necessarily correct. For example, if stdout is a pipe or disk file:

    C:\>python -V
    Python 3.7.3
    
    C:\>python -c "import os; fd = os.dup(1); print(os.get_inheritable(fd))" | more
    False

    C:\>python -c "import os; fd = os.dup(1); print(os.get_inheritable(fd))" > stdout.txt

    C:\>type stdout.txt
    False

What 3.7.3 does is to skip calling _Py_set_inheritable for all files of type FILE_TYPE_CHAR, which is wrong for character devices in general, such as NUL and serial ports. NUL is a common target for standard I/O redirection.
msg348656 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2019-07-29 13:13
This is marked as release blocker but since there is no movement on the possible solution, I'm releasing 3.80b3 without a fix. Please resolve this by b4, I will block the last beta on this issue.
msg348671 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-07-29 15:45
Honestly, I'm not sure of what is the right solution for this issue.
msg348674 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-07-29 16:10
> What 3.7.3 does is to skip calling _Py_set_inheritable for all files of type FILE_TYPE_CHAR

It sounds like we should probably revert to the middle ground, and skip _raising the error_ if _Py_set_inheritable for files of type FILE_TYPE_CHAR.

As Victor pointed out, the docs already call out the special case in Windows. Since this applies to Windows 7, which will be EOL only a couple months after 3.8's release, I think it's fine to have the slightly less secure behavior and just suppress the error.
msg348686 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2019-07-29 21:57
> It sounds like we should probably revert to the middle ground, and 
> skip _raising the error_ if _Py_set_inheritable for files of type
> FILE_TYPE_CHAR.

The problem is just console pseudohandles in Windows 7 and earlier.  Maybe it should just check for this case before calling SetHandleInformation. For example:

    /* This check can be removed once support for Windows 7 ends. */
    #define CONSOLE_PSEUDOHANDLE(handle) (((ULONG_PTR)(handle) & 0x3) == 0x3 && \
            GetFileType(handle) == FILE_TYPE_CHAR)

    if (!CONSOLE_PSEUDOHANDLE(handle) &&
        !SetHandleInformation(handle, HANDLE_FLAG_INHERIT, flags)) {
        if (raise)
            PyErr_SetFromWindowsErr(0);
        return -1;
    }

We have similar Python code in subprocess Popen._filter_handle_list, which filters out console pseudohandles because they don't work with PROC_THREAD_ATTRIBUTE_HANDLE_LIST.

A similar check could be added to os_set_handle_inheritable_impl in Modules/posixmodule.c.

> As Victor pointed out, the docs already call out the special case in 
> Windows. 

For os.dup it says

    On Windows, when duplicating a standard stream (0: stdin, 1: 
    stdout, 2: stderr), the new file descriptor is inheritable.

The standard handles aren't relevant.

Also, under "inheritance of file descriptors", it says:

    On Windows, non-inheritable handles and file descriptors are closed 
    in child processes, except for standard streams (file descriptors 0,
    1 and 2: stdin, stdout and stderr), which are always inherited.

The standard handles aren't always inherited. If bInheritHandles is TRUE (i.e. close_fds=False), a standard handle has to be iheritable for it to be inherited. 

For example, in Windows 10, make stderr (a ConDrv console handle) non-inheritable and create a child process with close_fds=False:

    >>> os.set_handle_inheritable(msvcrt.get_osfhandle(2), False)
    >>> cmd = 'import sys; print(sys.stderr)'
    >>> subprocess.call(f'python.exe -c "{cmd}"', close_fds=False)
    None
    0

In Windows 7 and earlier, it seems that inheritable console pseudohandles are always inherited, regardless of bInheritHandles -- as long as the child process attaches to the parent's console. SetHandleInformation isn't supported for console pseudohandles, but the inheritable flag is still set or cleared when a console pseudohandle is created via CreateConsoleScreenBuffer, CreateFileW (routed to OpenConsoleW), and DuplicateHandle (routed to DuplicateConsoleHandle).

It's worth mentioning that the system sometimes duplicates (not inherits) standard handles to a child process. A simple example in Windows 10 would be subprocess.call('python.exe'). 

All of the follow requirements must be satisfied for CreateProcessW to duplicate a standard handle to a child:

* it's not a console pseudohandle (e.g. it's a ConDrv console handle in 
  Windows 8+, or handle for the NUL device, a pipe, or disk file)
* the target executable is a console application (e.g. python.exe, not 
  pythonw.exe)
* handle inheritance is disabled (i.e. bInheritHandles is FALSE)
* the startup-info standard handles aren't used (i.e. 
  STARTF_USESTDHANDLES isn't set)
* the call isn't flagged to execute without a console or to allocate a
  new console (i.e. no DETACHED_PROCESS, CREATE_NEW_CONSOLE, or 
  CREATE_NO_WINDOW)

In this case, CreateProcessW also has to update the handle value in the child since generally the duplicate has a new value. With inheritance, in contrast, the handle value is the same in the parent and child.
msg350193 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-08-22 15:13
Thanks Zackery for the patch!

> The problem is just console pseudohandles in Windows 7 and earlier.

Should we add a version check as well [1]? I'm not totally comfortable with bitmasking a handle here, but if we only do this additional check on Win7 then I'm okay with it.

And if we add the check, I'd rather this be a function than a macro. It's getting too complicated to read :)

[1]: https://docs.microsoft.com/en-us/windows/win32/api/versionhelpers/nf-versionhelpers-iswindows8orgreater
msg350290 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2019-08-23 14:05
This is marked as a release blocker. The last beta is scheduled for Monday. Please decide how to proceed ASAP.
msg350322 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2019-08-23 18:02
> Should we add a version check as well [1]? I'm not totally comfortable 
> with bitmasking a handle here, but if we only do this additional 
> check on Win7 then I'm okay with it.

A version check should not be necessary. For file objects, setting both handle tag bits has always been a reserved signature for console pseudohandles. Here's the definition in the published console code:

https://github.com/microsoft/terminal/blob/b726a3d05d35928becc8313f6ac5536c9f503645/dep/Console/winconp.h#L460

It's mostly vestigial in Windows 8+ due to the addition of the ConDrv console device. Code that routed calls to LPC-based implementations when passed a console pseudohandle has all been removed. The only remaining use I know of is in the console attach code. If a process is started with STARTF_USESTDHANDLES, then AllocConsole() will only replace handles that are NULL or console pseudohandles. The docs for GetStdHandle explain this:

    Attach/detach behavior

    When attaching to a new console, standard handles are always replaced
    with console handles unless STARTF_USESTDHANDLES was specified during
    process creation.

    If the existing value of the standard handle is NULL, or the existing
    value of the standard handle looks like a console pseudohandle, the
    handle is replaced with a console handle.

    When a parent uses both CREATE_NEW_CONSOLE and STARTF_USESTDHANDLES to
    create a console process, standard handles will not be replaced unless
    the existing value of the standard handle is NULL or a console
    pseudohandle.

I confirmed that this is still relevant in Windows 10 by testing an inherited pipe handle in stdout and stderr and manually setting the tag bits in the hStdError handle value in STARTUPINFO. The console attach code preserved the regular handle in stdout but replaced the console pseudohandle in stderr. So the API is still reserving these tag bits in file handles, at least in one place, which means they're still reserved in general, even though setting them will no longer break a ReadFile call, for one example, like it would in Windows 7 (in which the API would route the call to the internal ReadConsole implementation). 

We've been relying on checking the console pseudohandle tag bits without a version check in subprocess.Popen._filter_handle_list for a while now. Console pseudohandles have to be removed from the handle list, else CreateProcessW will fail.
msg350323 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-08-23 18:37
Thanks, Eryk. In that case, I'll merge the PR.

Since Python 3.9 will not support Windows 7, we can remove it from master as soon as the backports are done :)
msg350324 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-08-23 18:38
New changeset 5be666010e4df65dc4d831435cc92340ea369f94 by Steve Dower (Zackery Spytz) in branch 'master':
bpo-37549: os.dup() fails for standard streams on Windows 7 (GH-15389)
https://github.com/python/cpython/commit/5be666010e4df65dc4d831435cc92340ea369f94
msg350326 - (view) Author: miss-islington (miss-islington) Date: 2019-08-23 19:01
New changeset f8dc3e85ab01e1b0345738670dcb3993da7ec436 by Miss Islington (bot) in branch '3.7':
bpo-37549: os.dup() fails for standard streams on Windows 7 (GH-15389)
https://github.com/python/cpython/commit/f8dc3e85ab01e1b0345738670dcb3993da7ec436
msg350327 - (view) Author: miss-islington (miss-islington) Date: 2019-08-23 19:04
New changeset 3921d12174c1998d9df7a08d036a7fef2d587a64 by Miss Islington (bot) in branch '3.8':
bpo-37549: os.dup() fails for standard streams on Windows 7 (GH-15389)
https://github.com/python/cpython/commit/3921d12174c1998d9df7a08d036a7fef2d587a64
History
Date User Action Args
2019-08-26 15:39:30steve.dowersetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2019-08-23 19:04:30miss-islingtonsetmessages: + msg350327
2019-08-23 19:01:43miss-islingtonsetnosy: + miss-islington
messages: + msg350326
2019-08-23 18:39:03miss-islingtonsetpull_requests: + pull_request15132
2019-08-23 18:38:56miss-islingtonsetpull_requests: + pull_request15131
2019-08-23 18:38:44steve.dowersetmessages: + msg350324
2019-08-23 18:37:59steve.dowersetassignee: steve.dower
messages: + msg350323
2019-08-23 18:02:23eryksunsetmessages: + msg350322
2019-08-23 14:08:52Bruno Oliveirasetnosy: + Bruno Oliveira
2019-08-23 14:05:25lukasz.langasetmessages: + msg350290
2019-08-22 15:13:14steve.dowersetmessages: + msg350193
2019-08-22 15:08:13ZackerySpytzsetkeywords: + patch
stage: patch review
pull_requests: + pull_request15099
2019-07-29 21:57:39eryksunsetmessages: + msg348686
2019-07-29 16:10:09steve.dowersetmessages: + msg348674
2019-07-29 15:45:35vstinnersetmessages: + msg348671
2019-07-29 13:13:01lukasz.langasetmessages: + msg348656
2019-07-21 10:32:08xflr6setnosy: + xflr6
2019-07-14 09:08:25vstinnersetpriority: normal -> release blocker
nosy: + ned.deily, lukasz.langa
2019-07-12 20:05:01Jeffrey.Kintschersetnosy: + Jeffrey.Kintscher
2019-07-11 11:02:44eryksunsetmessages: + msg347671
2019-07-11 10:11:42vstinnersetmessages: + msg347668
2019-07-10 23:57:40davebsetmessages: + msg347644
2019-07-10 23:46:02eryksunsetmessages: + msg347642
2019-07-10 18:38:20josh.rsetnosy: + ZackerySpytz, vstinner, josh.r
messages: + msg347632
2019-07-10 18:19:17xtreaksetnosy: + eryksun
2019-07-10 18:02:57xtreaksetnosy: + paul.moore, tim.golden, zach.ware, steve.dower
components: + Windows
2019-07-10 17:52:05davebcreate