classification
Title: _PyIO_get_console_type fails for various paths
Type: behavior Stage: resolved
Components: IO, Library (Lib), Windows Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: steve.dower Nosy List: eryksun, paul.moore, python-dev, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: patch

Created on 2016-09-15 03:51 by eryksun, last changed 2017-03-31 16:36 by dstufft. This issue is now closed.

Files
File name Uploaded Description Edit
issue_28164_01.patch eryksun, 2017-01-31 03:20 review
issue_28164_02.patch eryksun, 2017-01-31 12:29 review
Pull Requests
URL Status Linked Edit
PR 552 closed dstufft, 2017-03-31 16:36
Messages (17)
msg276512 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2016-09-15 03:51
_PyIO_get_console_type currently hard codes the names "CON", "CONIN$", and "CONOUT$" and doesn't use a case-insensitive comparison. For example, opening "conin$" doesn't get directed to WindowsConsoleIO:

    >>> open('conin$', 'rb', buffering=0)
    <_io.FileIO name='conin$' mode='rb' closefd=True>

unlike CONIN$:

    >>> open('CONIN$', 'rb', buffering=0)
   <_io._WindowsConsoleIO mode='rb' closefd=True>

This also ignores the special handling of DOS devices in existing directories. The normal DOS-device check (i.e. if the parent directory exists, call GetFullPathName to normalize the path) isn't reliable, unfortunately. Prior to Windows 8, CreateFile special-cases parsing console paths by calling BaseIsThisAConsoleName, which has an annoying speed hack that makes it buggy. Ultimately the only way to know if a path opens the console is to open it and check the handle.
msg276839 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2016-09-17 20:29
I'll make it case insensitive, but unfortunately I can't find a good way to check that a handle is actually a real console handle or what type it is. Plus the way iomodule is designed we'd need to open it twice, which I'd rather not do.
msg276844 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-09-17 20:51
New changeset d0bab9fda568 by Steve Dower in branch '3.6':
Issue #28161: Opening CON for write access fails
https://hg.python.org/cpython/rev/d0bab9fda568

New changeset 187a114b9ef4 by Steve Dower in branch 'default':
Issue #28161: Opening CON for write access fails
https://hg.python.org/cpython/rev/187a114b9ef4
msg276864 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2016-09-18 01:08
> check that a handle is actually a real console handle or 
> what type it is

Did you mean a path here? Certainly you can check a handle, but that means opening the path twice.

You can use GetFullPathName to classify the path, and then use GetFullPathName again when opening it. Keep the original path as the `name`. When classifying the path, ignore the DOS-device prefix, "\\.\" or "\\?\", if present, and do a case-insensitive match for CON, CONIN$, or CONOUT$.

GetFullPathName transforms paths containing the "CON" device, e.g. "C:\Temp\con   :.spam  " =>  "\\.\CON". This ignores trailing spaces and anything after the first "." or ":" in the last component. It also translates forward slashes, e.g. "//./con" => "\\.\con". On Windows 8+ it also works with CONIN$ and CONOUT$, transforming them to "\\.\CONIN$" and "\\.\CONOUT$". 

This is reliable on Windows 8+, without having to also call GetFullPathName again when opening the path. The problem with Windows Vista and 7 is a speed hack it has. In these older versions, opening the console has to be handled specially by the function OpenConsoleW, so like you have to do here, in Windows 7 there's an internal BaseIsThisAConsoleName function to classify a path. 

This function always checks for "CON", "CONIN$", or "CONOUT$" (case insensitive). But it also matches "\\.\CON" and in some cases also "CON" in the last component. Since BaseIsThisAConsoleName always has to be called, the path search could hurt performance. So there's a dubious hack to only look for "CON" if the path starts with "\", "c", or "C" or ends with "n", "N", "or ":". Thus "C:\temp\con  " opens the console because it starts with "C", but not "Z:\temp\con  " because it starts with "Z" and ends with a space.
msg286507 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2017-01-31 03:20
I had reopened this issue with a suggestion for expanding the supported paths in msg276864, but it's languished for a while now. I've attached a patch implementing the change to _PyIO_get_console_type that I had suggested. Here are some example paths that this change enables (tested in Windows 10; need to check Windows 7):

    \\.\conin$
    \\.\conout$
    \\.\con
    //?/con
    C:/Temp/con
    C:/Temp/conout$
    C:/Temp/conin$

Paths such as "C:/Temp/con" are recognized by first converting to a device path such as r"\\.\con". It's fine if the directory doesn't exist because CreateFile will fail anyway in that case.

This patch also fixes an error in handling the return value of _get_osfhandle when it fails. 

It also adds a couple checks to __init__ to provide a better error message when io._WindowsConsoleIO is called directly for some reason instead of via open(). If the console type can't be determined it should error out immediately instead of printing an unrelated error about not being able to open the input/output buffer.
msg286516 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2017-01-31 04:57
I'm okay with this patch. We move closer to being right without degrading the normal case, and I don't think the edge cases are important (and the behavior in those cases will be acceptable).

With a couple of tests to make sure the path comparisons don't get broken in the future, this will be ready to merge.
msg286522 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2017-01-31 12:23
I added some tests to ensure open() returns an instance of _WindowsConsoleIO for a few console paths. It also checks that opening a non-console file raises a ValueError, both for a path and an fd, as well for a negative file descriptor.
msg287005 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2017-02-04 23:14
New changeset ed0c05c739c9 by Steve Dower in branch '3.6':
Issue #28164: Correctly handle special console filenames (patch by Eryk Sun)
https://hg.python.org/cpython/rev/ed0c05c739c9

New changeset a5538734cc87 by Steve Dower in branch 'default':
Merge issue #28164 and issue #29409
https://hg.python.org/cpython/rev/a5538734cc87
msg287014 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2017-02-05 00:00
New changeset 164fc49825081694c9c44c4a17c75b30c5ba8f51 by Steve Dower in branch 'master':
Issue #28164: Correctly handle special console filenames (patch by Eryk Sun)
https://github.com/python/cpython/commit/164fc49825081694c9c44c4a17c75b30c5ba8f51

New changeset 965f8d68a8dcc2ebb2480fe7e9121ac7efdee54e by Steve Dower in branch 'master':
Merge issue #28164 and issue #29409
https://github.com/python/cpython/commit/965f8d68a8dcc2ebb2480fe7e9121ac7efdee54e
msg287019 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2017-02-05 00:00
New changeset 164fc49825081694c9c44c4a17c75b30c5ba8f51 by Steve Dower in branch '3.6':
Issue #28164: Correctly handle special console filenames (patch by Eryk Sun)
https://github.com/python/cpython/commit/164fc49825081694c9c44c4a17c75b30c5ba8f51
msg287027 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2017-02-05 00:30
Looks like the "need to check Win7" part was actually important... there are buildbot failures from this. I *think* they only require test handling updates.
msg287030 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2017-02-05 01:30
Okay, seems like Windows 7 GetFullPathName on conin$ and conout$ returns the path appended to the current directory, and you need to specify the full name as "conin$" or "conout$" to access the console - otherwise you have a file by that name.

We should include a comparison for these names exactly before using GetFullPathName.
msg287047 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2017-02-05 12:56
It's an ugly inconsistency that GetFullPathName fails for bare CONIN$ and CONOUT$ prior to Windows 8, in that it gives a different result from simply passing those names to CreateFile. Anyway, thanks for modifying it to work correctly in this case.

We should test that _WindowsConsoleIO isn't used on Windows 7 and earlier when accessing CONIN$ or CONOUT$ in a directory. How about the following?

        temp_path = tempfile.mkdtemp()
        self.addCleanup(support.rmtree, temp_path)

        conout_path = os.path.join(temp_path, 'CONOUT$')
   
        with open(conout_path, 'wb', buffering=0) as f:
            if sys.getwindowsversion()[:2] > (6, 1):
                self.assertIsInstance(f, ConIO)
            else:
                self.assertNotIsInstance(f, ConIO)
msg287177 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2017-02-06 22:53
New changeset 4463e311f5bd by Steve Dower in branch '3.6':
Issue #28164: Improves test on Windows 7
https://hg.python.org/cpython/rev/4463e311f5bd

New changeset 8132bcc1522d by Steve Dower in branch 'default':
Issue #28164: Improves test on Windows 7
https://hg.python.org/cpython/rev/8132bcc1522d
msg287178 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2017-02-06 22:54
That test looks good for me, and I verified it on both Win7 and Win10. (Hopefully we don't have any Win8.1 edge cases in here...)
msg287179 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2017-02-06 23:00
New changeset c6c32889c9d80ffd599a57fd0d4c4a88deece29b by Steve Dower in branch 'master':
Issue #28164: Improves test on Windows 7
https://github.com/python/cpython/commit/c6c32889c9d80ffd599a57fd0d4c4a88deece29b

New changeset dc5c4bc90770d6a5875666434cf797c77eb3dcad by Steve Dower in branch 'master':
Issue #28164: Improves test on Windows 7
https://github.com/python/cpython/commit/dc5c4bc90770d6a5875666434cf797c77eb3dcad
msg287180 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2017-02-06 23:00
New changeset c6c32889c9d80ffd599a57fd0d4c4a88deece29b by Steve Dower in branch '3.6':
Issue #28164: Improves test on Windows 7
https://github.com/python/cpython/commit/c6c32889c9d80ffd599a57fd0d4c4a88deece29b
History
Date User Action Args
2017-03-31 16:36:11dstufftsetpull_requests: + pull_request865
2017-02-06 23:00:30python-devsetmessages: + msg287180
2017-02-06 23:00:26python-devsetmessages: + msg287179
2017-02-06 22:54:02steve.dowersetstatus: open -> closed

messages: + msg287178
2017-02-06 22:53:33python-devsetmessages: + msg287177
2017-02-05 12:56:11eryksunsetmessages: + msg287047
2017-02-05 01:30:16steve.dowersetmessages: + msg287030
2017-02-05 00:30:43steve.dowersetmessages: + msg287027
2017-02-05 00:00:34python-devsetmessages: + msg287019
2017-02-05 00:00:31python-devsetmessages: + msg287014
2017-02-04 23:14:35python-devsetmessages: + msg287005
2017-01-31 12:29:50eryksunsetfiles: + issue_28164_02.patch
2017-01-31 12:28:52eryksunsetfiles: - issue_28164_02.patch
2017-01-31 12:23:23eryksunsetfiles: + issue_28164_02.patch

messages: + msg286522
2017-01-31 04:57:54steve.dowersetmessages: + msg286516
2017-01-31 03:20:06eryksunsetfiles: + issue_28164_01.patch
keywords: + patch
messages: + msg286507
2016-09-18 01:08:13eryksunsetstatus: closed -> open

messages: + msg276864
2016-09-17 20:53:28steve.dowersetstatus: open -> closed
resolution: fixed
stage: test needed -> resolved
2016-09-17 20:51:44python-devsetnosy: + python-dev
messages: + msg276844
2016-09-17 20:29:03steve.dowersetassignee: steve.dower
messages: + msg276839
2016-09-15 03:51:03eryksuncreate