[issue32865] os.pipe creates inheritable FDs with a bad internal state on Windows

[STINNER Victor]

Change by STINNER Victor :


--
nosy:  -vstinner

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32865] os.pipe creates inheritable FDs with a bad internal state on Windows

[Eryk Sun]

Change by Eryk Sun :


--
components: +Extension Modules -Library (Lib)
versions: +Python 3.10, Python 3.9 -Python 3.6, Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32865] os.pipe creates inheritable FDs with a bad internal state on Windows

[STINNER Victor]

Change by STINNER Victor :


--
nosy: +vstinner

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32865] os.pipe creates inheritable FDs with a bad internal state on Windows

[Zackery Spytz]

Change by Zackery Spytz :


--
keywords: +patch
pull_requests: +13622
stage: needs patch -> patch review
pull_request: https://github.com/python/cpython/pull/13739

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32865] os.pipe creates inheritable FDs with a bad internal state on Windows

[Eryk Sun]

Eryk Sun  added the comment:

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

[1]: 
https://docs.microsoft.com/en-us/windows-hardware/drivers/ddi/content/ntifs/nf-ntifs-ntqueryvolumeinformationfile

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

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32865] os.pipe creates inheritable FDs with a bad internal state on Windows

[Nathaniel Smith]

Change by Nathaniel Smith :


--
nosy: +njs

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32865] os.pipe creates inheritable FDs with a bad internal state on Windows

[Alexey Izbyshev]

Alexey Izbyshev  added the comment:

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

--
nosy: +izbyshev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32865] os.pipe creates inheritable FDs with a bad internal state on Windows

[Eryk Sun]

Eryk Sun  added the comment:

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 "", line 1, in 
OSError: [WinError 6] The handle is invalid

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32865] os.pipe creates inheritable FDs with a bad internal state on Windows

[Eryk Sun]

New submission from Eryk Sun :

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 "", line 1, in 
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.

--
components: IO, Library (Lib), Windows
messages: 312283
nosy: eryksun, paul.moore, steve.dower, tim.golden, zach.ware
priority: normal
severity: normal
stage: needs patch
status: open
title: os.pipe  creates inheritable FDs with a bad internal state on Windows
type: behavior
versions: Python 3.6, Python 3.7, Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com