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

[eryksun]

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

[eryksun]

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.

[eryksun]

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

[izbyshev]

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:

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

[eryksun]

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.