[os] pipe() creates inheritable FDs with a bad internal state in Windows

@pfmoore

BPO	32865
Nosy	@pfmoore, @tjguk, @njsmith, @zware, @eryksun, @zooba, @izbyshev
PRs	gh-77046: Pass the _O_NOINHERIT flag to _open_osfhandle() calls #13739

^{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 = None
closed_at = None
created_at = <Date 2018-02-17.15:51:54.875>
labels = ['type-bug', '3.8', '3.9', 'expert-IO', 'extension-modules', '3.10', 'OS-windows']
title = 'os.pipe  creates inheritable FDs with a bad internal state on Windows'
updated_at = <Date 2021-03-20.14:32:47.251>
user = 'https://github.com/eryksun'

bugs.python.org fields:

activity = <Date 2021-03-20.14:32:47.251>
actor = 'vstinner'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Extension Modules', 'Windows', 'IO']
creation = <Date 2018-02-17.15:51:54.875>
creator = 'eryksun'
dependencies = []
files = []
hgrepos = []
issue_num = 32865
keywords = ['patch']
message_count = 4.0
messages = ['312283', '312294', '312297', '312300']
nosy_count = 7.0
nosy_names = ['paul.moore', 'tim.golden', 'njs', 'zach.ware', 'eryksun', 'steve.dower', 'izbyshev']
pr_nums = ['13739']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue32865'
versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

Linked PRs

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

File descriptors in Windows are implemented by the C runtime library's low I/O layer. The CRT maps native File handles to Unix-style file descriptors. Additionally, in order to support inheritance for spawn/exec, the CRT passes inheritable FDs in a reserved field of the CreateProcess STARTUPINFO record. A spawned child process uses this information to initialize its FD table at startup.

Python's implementation of os.pipe creates File handles directly via CreatePipe. These handles are non-inheritable, which we want. However, it wraps them with inheritable file descriptors via _open_osfhandle, which we don't want. The solution is to include the flag _O_NOINHERIT in the _open_osfhandle call, which works even though this flag isn't explicitly documented as supported for this function.

Here's an example of the issue.

    >>> fdr, fdw = os.pipe()
    >>> fdr, fdw
    (3, 4)
    >>> msvcrt.get_osfhandle(3), msvcrt.get_osfhandle(4)
    (440, 444)
    >>> os.get_handle_inheritable(440)
    False
    >>> os.get_handle_inheritable(444)
    False

Note that os.get_inheritable assumes that FD and handle heritability are in sync, so it only queries handle information. The following FDs are actually inheritable, while the underlying File handles are not:

    >>> os.get_inheritable(3)
    False
    >>> os.get_inheritable(4)
    False

This is a flawed assumption baked into _Py_get_inheritable and _Py_set_inheritable, especially since the latter has no effect on FD heritability. The CRT has no public functions to query and modify the heritability of existing FDs. Until it does (and maybe Steve Dower can request this from the MSVC devs), I see no point in pretending something works when it doesn't. This only creates problems.

Let's spawn a child to show that these FDs are inherited in a bad state, which is a potential source of bugs and data corruption:

>>> os.spawnl(os.P_WAIT, sys.executable, 'python')

    Python 3.6.4 (v3.6.4:d48eceb, Dec 19 2017, 06:54:40)
    [MSC v.1900 64 bit (AMD64)] on win32
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import os, msvcrt
    >>> msvcrt.get_osfhandle(3), msvcrt.get_osfhandle(4)
    (440, 444)

As you can see, file descriptors 3 and 4 were inherited with the handle values from the parent process, but the handles themselves were not inherited. We'll be lucky if the handle values happen to be unassigned and remain so. However, they may be assigned or subsequently assigned to random kernel objects (e.g. a File, Job, Process, Thread, etc).

On a related note, Python always opens files as non-inheritable, even for os.open (which IMO makes no sense; we have O_NOINHERIT or O_CLOEXEC for that). It's assumed that the FD can be made inheritable via os.set_inheritable, but this does not work on Windows, and as things stand with the public CRT API, it cannot work. For example:

    >>> os.open('test.txt', os.O_WRONLY)
    3
    >>> msvcrt.get_osfhandle(3)
    460
    >>> os.set_inheritable(3, True)
    >>> os.get_handle_inheritable(460)
    True

>>> os.spawnl(os.P_WAIT, sys.executable, 'python')

    Python 3.6.4 (v3.6.4:d48eceb, Dec 19 2017, 06:54:40)
    [MSC v.1900 64 bit (AMD64)] on win32
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import msvcrt
    >>> msvcrt.get_osfhandle(3)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    OSError: [Errno 9] Bad file descriptor

The Windows handle was of course inherited, but that's not useful in this scenario, since the CRT didn't know to pass the FD in the process STARTUPINFO. It's essentially a leaked handle in the child.

Note that the CRT checks at startup whether an inherited FD is valid by calling GetFileType. If the handle is invalid or not a File, then the FD effectively is not inherited. This doesn't completely avoid the problem, since there's still a chance the handle was already assigned to a File at startup. Also, it has to skip this check if the FD is flagged as a pipe, because a pipe is likely opened in synchronous mode and blocked on a read in the parent, in which case calling GetFileType would deadlock.

For example:

    >>> os.pipe()
    (3, 4)
    >>> os.dup2(4, 5, False) # fd 5 is still inheritable
    >>> os.spawnl(os.P_WAIT, sys.executable, 'python')

    Python 3.6.4 (v3.6.4:d48eceb, Dec 19 2017, 06:54:40)
    [MSC v.1900 64 bit (AMD64)] on win32
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import os, msvcrt
    >>> msvcrt.get_osfhandle(5)
    420

Handle 420 was not inherited, so it may be unassigned, or it could be for some random kernel object. It's assigned in this case:

    >>> os.get_handle_inheritable(420)
    False

I have a function that calls NtQueryObject to get the type name for a kernel handle:

    >>> get_type_name(420)
    'Semaphore'

Now, let's model what could be a confusing bug for someone using this semaphore:

    >>> os.close(5) # BUGBUG
    >>> os.get_handle_inheritable(420)
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
    OSError: [WinError 6] The handle is invalid

Also, it has to skip this check if the FD is flagged as a pipe, because a pipe is likely opened in synchronous mode and blocked on a read in the parent, in which case calling GetFileType would deadlock.

Does an FD get flagged as a pipe by calling GetFileType in _open_osfhandle?

Also, it's totally unclear to me how something like GetFileType can interfere with I/O and cause a deadlock.

My summary of this report. There are two issues.

The first is a bug in os.pipe (creation of an inheritable descriptor with non-inheritable underlying handle). This can be fixed by using _open_osfhandle() correctly.

The second is that os.set_inheritable() / _Py_set_inheritable() is broken on Windows because it may put descriptor and handle heritability out-of-sync (as happens e.g. in os.dup(), os.dup2(), _Py_fopen and friends). This is hard to fix because msvcrt doesn't have something like fcntl(F_SETFD) to change _O_NOINHERIT flag (though one nasty thought is to poke msvcrt internal structures like in _PyVerify_fd removed in bpo-23524).

Both issues cause out-of-sync which is usually hidden and harmless because people prefer subprocess.Popen() which doesn't have the ability to propagate msvcrt-level descriptors at all. The issue surfaces itself if pipes and os.spawn* are involved, leading to two types of inconsistency in the child:

Bogus "valid" descriptors referring to wrong handles
Unexpectedly invalid descriptors and leaked handles

The first is a bug in os.pipe (creation of an inheritable descriptor
with non-inheritable underlying handle). This can be fixed by using
_open_osfhandle() correctly.

This is the only issue to be addressed here, and it's an easy fix. The problem also occurs in Modules/_io/winconsoleio.c.

I digressed on the problems with _Py_get_inheritable and _Py_set_inheritable. Such problems need to be addressed in a separate issue.

Does an FD get flagged as a pipe by calling GetFileType in
_open_osfhandle?

Yes, it calls GetFileType to check for a bad handle (FILE_TYPE_UNKNOWN) and also to flag whether the FD type is FDEV (FILE_TYPE_CHAR) or FPIPE (FILE_TYPE_PIPE). See lowio\osfinfo.cpp in the CRT source code.

Also, it's totally unclear to me how something like GetFileType
can interfere with I/O and cause a deadlock.

You're right to question this. I couldn't reproduce this problem in Windows 10 using a local, anonymous pipe. GetFileType calls NtQueryVolumeInformationFile 1 to get the FileFsDeviceInformation for the referenced File. For local devices, the I/O Manager can simply copy the requested information from the associated Device object (e.g. \Device\NamedPipe); it doesn't even need to call the device driver with an IRP_MJ_QUERY_VOLUME_INFORMATION request. Maybe older implementations of the I/O Manager always waited to acquire the File's internal lock when opened in synchronous mode.

Whether this is (or ever was) a valid reason, it's the reason given for skipping the call in the CRT function initialize_inherited_file_handles_nolock (lowio\ioinit.cpp). There's nothing we can do about it. The CRT will not validate an inherited pipe FD at startup. Also, even this validation can't help if the same handle just happens to be assigned to another File at startup.

Both issues cause out-of-sync which is usually hidden and harmless
because people prefer subprocess.Popen()

Python has to respect file descriptors as a common resource in the process. Other libraries or an embedding application may be using the same CRT. It's not simply a matter of what's commonly used in pure-Python code. That said, os.system() is still commonly used, and the CRT implements _wsystem via common_spawnv<wchar_t>.

one nasty thought is to poke msvcrt internal structures like in _PyVerify_fd

That's an ugly option, but it may be the best (or only possible) approach until MSVC adds a public function to modify the heritability of an FD.

On a related note, in looking at places where FDs are made non-inheritable, I see it's also used in _Py_fopen_obj. This function could be modified to use MSVC's "N" flag to ensure the underlying FD is opened with _O_NOINHERIT. glibc has a similar "e" flag for O_CLOEXEC.

If an application calls os.pipe() and then os.spawnl(), trying to use the pipe file descriptors in the child process can crash the process with EXCEPTION_ACCESS_VIOLATION :-(

Example of script crash.py:

import os
import sys
import textwrap

fd, fd2 = os.pipe()
print(f"parent: fd={fd} fd2={fd2}")

if 1:
    # os.dup() does crash
    code = textwrap.dedent(f"""
        import os
        fd = {fd}
        print(f"child: dup {{fd}}")
        try:
            fd3 = os.dup(fd)
        except OSError as exc:
            print(f"child: dup {{fd}} FAILED: {{exc!r}}>")
        else:
            print(f"child: dup {{fd}} -> {{fd3}}")
            os.close(fd3)
    """)
else:
    # _open_osfhandle() raises OSError(EBADF)
    code = textwrap.dedent(f"""
        import os
        import msvcrt
        fd = {fd}
        print(f"child: _open_osfhandle({{fd}})")
        try:
            handle = msvcrt.open_osfhandle(fd, os.O_RDONLY)
        except OSError as exc:
            handle = f"<error: {{exc!r}}>"
        print(f"child: _open_osfhandle({{fd}}) -> {{handle}}")
    """)

filename = "crash_script.py"
with open(filename, "w") as fp:
    print(code, file=fp, end="")

cmd = [sys.executable, filename]
exitcode = os.spawnl(os.P_WAIT, cmd[0], *cmd)
if exitcode == -1073741819:
    exitcode = f"{exitcode} (EXCEPTION_ACCESS_VIOLATION)"
print(f"os.spawnl() => {exitcode}")

Sometimes, the script goes well:

vstinner@WIN C:\victor\python\main>python crash.py
Running Debug|x64 interpreter...
parent: fd=3 fd2=4
child: dup 3
child: dup 3 -> 5
os.spawnl() => 0

Sometimes, the child process crash with EXCEPTION_ACCESS_VIOLATION:

Running Debug|x64 interpreter...
parent: fd=3 fd2=4
child: dup 3
os.spawnl() => -1073741819 (EXCEPTION_ACCESS_VIOLATION)

Calling os.dup() crash randomly, whereas msvcrt.open_osfhandle() always raises OSError(EBADF) (does not crash).

If an application calls os.pipe() and then os.spawnl(), trying to use the pipe file descriptors in the child process can crash the process with EXCEPTION_ACCESS_VIOLATION

Can you see if this repros when the NOINHERIT flag is added, as proposed in #13739? I expect it'll go away

pythongh-77046: Pass the _O_NOINHERIT flag to _open_osfhandle() calls

@zooba

@zooba:

Yeah, we don't need _osfile anyway,

Well, I wanted to "fix" os.get_inheritable/set_inheritable() to get/set the UCRT flag. It doesn't seem possible. The best we can do is to be more specific in os.get_inheritable/set_inheritable() documentation that only the status of the handle is used, not the state of the file descriptor which can be inconsistent (but should not for FDs created by the Python stdlib).

@zooba:

I just noticed that there's potentially a bug where it attempts to inherit handles that it shouldn't be able to. If we mark them as non-inheritable correctly, that should go away too.

I wrote a new PR #113817 which fix os.pipe() file descriptors and adds a test to check for non regression.

Is it safe to backport such change to stable branches 3.11 and 3.12? It should not impact anyone in practice.

I don't think that Python has many users of the os.spawn*() functions. In Python, we strongly promote the usage of subprocess which is implemented with CreateProcess() and so has a different code path: CreateProcess() doesn't inherit UCRT file descriptors.

spawn*() functions are implemented in UCRT with CreateProcess() using a STARTUPINFOW structure: pass file descriptors in lpReserved2 (and cbReserved2) members:

    __crt_unique_heap_ptr<BYTE> handle_data;
    size_t                      handle_data_size;
    if (!accumulate_inheritable_handles(handle_data.get_address_of(), &handle_data_size, mode != _P_DETACH))
        return -1;

    ...

    STARTUPINFOW startup_info = { };
    startup_info.cb          = sizeof(startup_info);
    startup_info.cbReserved2 = static_cast<WORD>(handle_data_size);
    startup_info.lpReserved2 = handle_data.get();

Well, I wanted to "fix" os.get_inheritable/set_inheritable() to get/set the UCRT flag. It doesn't seem possible. The best we can do is to be more specific in os.get_inheritable/set_inheritable() documentation that only the status of the handle is used, not the state of the file descriptor which can be inconsistent (but should not for FDs created by the Python stdlib).

Ah yeah, I see what you mean.¹ I guess we can recommend msvcrt.open_osfhandle in this case? Probably needs a dup_as_inheritable function, but I also haven't heard of it actually coming up before, so I'm not sure how important it is?

Do people normally do os.pipe and then immediately make one end inheritable?

I hate file descriptors 🙃 ↩

Probably needs a dup_as_inheritable function

What would be the use case to make os.pipe() file descriptors inheritable on Windows? If you use them between threads, Python interpreters or whatever inside the same process, it's not an issue that they cannot be inherited by child processes.

On Unix, to inherit file descriptors in child processes, since file descriptors are not inheritable by default, you should already use pass_fds=[fd] of subprocess. It makes sure that fd is made inheritable when passed to the child process.

On Windows, the subprocess module doesn't support inheriting file descriptors: you should pass handles to startupinfo.lpAttributeList['handle_list'], and then create new file descriptors in the child process. Only os.spawnl() indirectly uses the dark secrets of CreateProcess() (see my previous comment about lpReserved2) to inherit file descriptors.

Note: os.dup2() has an optional inheritable=True parameter.

Note: os.dup2() has an optional inheritable=True parameter.

So it does, but it just calls _Py_set_inheritable if you pass False (True is the default), and leaves you with the same inconsistent state. That can probably be a new bug to fix, I guess, if anyone ever notices (at least if we're uninheritable by default and then dup2 makes it inheritable, I don't know why you'd need to go back?)

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

@eryksun

Fixed by commit 1d75fa4.

@eryksun: If you consider that os.spawnl() must no longer inherit file descriptors on Windows, please open a separated issue. IMO it's a different topic.

I no longer think that the change should be backport to stable 3.11 and 3.12 branches. The change is too subtle, and the issue sounds more hypothetical than a real practical bug.

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

Navigation Menu

Repository navigation

[os] pipe() creates inheritable FDs with a bad internal state in Windows #77046

Linked PRs

23 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

[os] pipe() creates inheritable FDs with a bad internal state in Windows #77046

Description

Linked PRs

Activity

eryksun commented on Feb 17, 2018

eryksun commented on Feb 18, 2018

izbyshev commented on Feb 18, 2018

eryksun commented on Feb 18, 2018

23 remaining items

vstinner commented on Jan 8, 2024

zooba commented on Jan 8, 2024

vstinner commented on Jan 8, 2024

zooba commented on Jan 8, 2024

Footnotes

vstinner commented on Jan 8, 2024

zooba commented on Jan 9, 2024

vstinner commented on Jan 10, 2024

vstinner commented on Jan 10, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions