Skip to content
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.

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

os.dup() fails for standard streams on Windows 7 #81730

Closed
roasterdave mannequin opened this issue Jul 10, 2019 · 17 comments
Closed

os.dup() fails for standard streams on Windows 7 #81730

roasterdave mannequin opened this issue Jul 10, 2019 · 17 comments
Assignees
Labels
3.7 (EOL) end of life 3.8 only security fixes extension-modules C modules in the Modules dir OS-windows release-blocker type-bug An unexpected behavior, bug, or error

Comments

@roasterdave
Copy link
Mannequin

roasterdave mannequin commented Jul 10, 2019

BPO 37549
Nosy @pfmoore, @vstinner, @tjguk, @ned-deily, @ambv, @zware, @eryksun, @zooba, @MojoVampire, @xflr6, @nicoddemus, @ZackerySpytz, @miss-islington, @websurfer5, @roasterdave
PRs
  • bpo-37549: os.dup() fails for standard streams on Windows 7 #15389
  • [3.8] bpo-37549: os.dup() fails for standard streams on Windows 7 (GH-15389) #15437
  • [3.7] bpo-37549: os.dup() fails for standard streams on Windows 7 (GH-15389) #15438
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/zooba'
    closed_at = <Date 2019-08-26.15:39:30.575>
    created_at = <Date 2019-07-10.17:52:05.833>
    labels = ['type-bug', '3.8', 'release-blocker', 'extension-modules', '3.7', 'OS-windows']
    title = 'os.dup() fails for standard streams on Windows 7'
    updated_at = <Date 2019-08-26.15:39:30.574>
    user = 'https://github.com/roasterdave'

    bugs.python.org fields:

    activity = <Date 2019-08-26.15:39:30.574>
    actor = 'steve.dower'
    assignee = 'steve.dower'
    closed = True
    closed_date = <Date 2019-08-26.15:39:30.575>
    closer = 'steve.dower'
    components = ['Extension Modules', 'Windows']
    creation = <Date 2019-07-10.17:52:05.833>
    creator = 'daveb'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 37549
    keywords = ['patch']
    message_count = 17.0
    messages = ['347627', '347632', '347642', '347644', '347668', '347671', '348656', '348671', '348674', '348686', '350193', '350290', '350322', '350323', '350324', '350326', '350327']
    nosy_count = 15.0
    nosy_names = ['paul.moore', 'vstinner', 'tim.golden', 'ned.deily', 'lukasz.langa', 'zach.ware', 'eryksun', 'steve.dower', 'josh.r', 'xflr6', 'Bruno Oliveira', 'ZackerySpytz', 'miss-islington', 'Jeffrey.Kintscher', 'daveb']
    pr_nums = ['15389', '15437', '15438']
    priority = 'release blocker'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue37549'
    versions = ['Python 3.7', 'Python 3.8']

    @roasterdave
    Copy link
    Mannequin Author

    roasterdave mannequin commented Jul 10, 2019

    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
    >>>

    @roasterdave roasterdave mannequin added 3.7 (EOL) end of life 3.8 only security fixes extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error labels Jul 10, 2019
    @MojoVampire
    Copy link
    Mannequin

    MojoVampire mannequin commented Jul 10, 2019

    This seems likely to have been caused by the fixes for bpo-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 bpo-37267 for input.

    @eryksun
    Copy link
    Contributor

    eryksun commented Jul 10, 2019

    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

    @roasterdave
    Copy link
    Mannequin Author

    roasterdave mannequin commented Jul 10, 2019

    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
    >>>

    @vstinner
    Copy link
    Member

    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?

    @eryksun
    Copy link
    Contributor

    eryksun commented Jul 11, 2019

    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.

    @ambv
    Copy link
    Contributor

    ambv commented Jul 29, 2019

    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.

    @vstinner
    Copy link
    Member

    Honestly, I'm not sure of what is the right solution for this issue.

    @zooba
    Copy link
    Member

    zooba commented Jul 29, 2019

    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.

    @eryksun
    Copy link
    Contributor

    eryksun commented Jul 29, 2019

    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.

    @zooba
    Copy link
    Member

    zooba commented Aug 22, 2019

    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 :)

    @ambv
    Copy link
    Contributor

    ambv commented Aug 23, 2019

    This is marked as a release blocker. The last beta is scheduled for Monday. Please decide how to proceed ASAP.

    @eryksun
    Copy link
    Contributor

    eryksun commented Aug 23, 2019

    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.

    @zooba
    Copy link
    Member

    zooba commented Aug 23, 2019

    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 :)

    @zooba zooba self-assigned this Aug 23, 2019
    @zooba
    Copy link
    Member

    zooba commented Aug 23, 2019

    New changeset 5be6660 by Steve Dower (Zackery Spytz) in branch 'master':
    bpo-37549: os.dup() fails for standard streams on Windows 7 (GH-15389)
    5be6660

    @miss-islington
    Copy link
    Contributor

    New changeset f8dc3e8 by Miss Islington (bot) in branch '3.7':
    bpo-37549: os.dup() fails for standard streams on Windows 7 (GH-15389)
    f8dc3e8

    @miss-islington
    Copy link
    Contributor

    New changeset 3921d12 by Miss Islington (bot) in branch '3.8':
    bpo-37549: os.dup() fails for standard streams on Windows 7 (GH-15389)
    3921d12

    @zooba zooba closed this as completed Aug 26, 2019
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes extension-modules C modules in the Modules dir OS-windows release-blocker type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants