[issue37549] os.dup() fails for standard streams on Windows 7

[Steve Dower]

Change by Steve Dower :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[miss-islington]

miss-islington  added the comment:


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


--

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[miss-islington]

miss-islington  added the comment:


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


--
nosy: +miss-islington

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[miss-islington]

Change by miss-islington :


--
pull_requests: +15132
pull_request: https://github.com/python/cpython/pull/15438

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[miss-islington]

Change by miss-islington :


--
pull_requests: +15131
pull_request: https://github.com/python/cpython/pull/15437

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[Steve Dower]

Steve Dower  added the comment:


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


--

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[Steve Dower]

Steve Dower  added the comment:

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

--
assignee:  -> steve.dower

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[Eryk Sun]

Eryk Sun  added the comment:

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

--

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[Bruno Oliveira]

Change by Bruno Oliveira :


--
nosy: +Bruno Oliveira

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[Łukasz Langa]

Łukasz Langa  added the comment:

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

--

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[Steve Dower]

Steve Dower  added the comment:

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

--

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[Zackery Spytz]

Change by Zackery Spytz :


--
keywords: +patch
pull_requests: +15099
stage:  -> patch review
pull_request: https://github.com/python/cpython/pull/15389

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[Eryk Sun]

Eryk Sun  added the comment:

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

--

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[Steve Dower]

Steve Dower  added the comment:

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

--

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[STINNER Victor]

STINNER Victor  added the comment:

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

--

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[Łukasz Langa]

Łukasz Langa  added the comment:

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.

--

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[Sebastian Bank]

Change by Sebastian Bank :


--
nosy: +xflr6

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[STINNER Victor]

Change by STINNER Victor :


--
nosy: +lukasz.langa, ned.deily
priority: normal -> release blocker

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[Jeffrey Kintscher]

Change by Jeffrey Kintscher :


--
nosy: +Jeffrey.Kintscher

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[Eryk Sun]

Eryk Sun  added the comment:

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

--

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[STINNER Victor]

STINNER Victor  added the comment:

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?

--

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[DaveB]

DaveB  added the comment:

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 "", line 1, in 
OSError: [WinError 87] The parameter is incorrect
>>>

--

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[Eryk Sun]

Eryk Sun  added the comment:

> 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

--

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[Josh Rosenberg]

Josh Rosenberg  added the comment:

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.

--
nosy: +ZackerySpytz, josh.r, vstinner

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[Karthikeyan Singaravelan]

Change by Karthikeyan Singaravelan :


--
nosy: +eryksun

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[Karthikeyan Singaravelan]

Change by Karthikeyan Singaravelan :


--
components: +Windows
nosy: +paul.moore, steve.dower, tim.golden, zach.ware

___
Python tracker 

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

[issue37549] os.dup() fails for standard streams on Windows 7

[DaveB]

New submission from DaveB :

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 "", line 1, in 
OSError: [WinError 87] The parameter is incorrect
>>>

--
components: Extension Modules
messages: 347627
nosy: daveb
priority: normal
severity: normal
status: open
title: os.dup() fails for standard streams on Windows 7
type: behavior
versions: 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