Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

_PyIO_get_console_type fails for various paths #72351

Closed
eryksun opened this issue Sep 15, 2016 · 17 comments
Closed

_PyIO_get_console_type fails for various paths #72351

eryksun opened this issue Sep 15, 2016 · 17 comments
Assignees
Labels
3.7 (EOL) end of life OS-windows stdlib Python modules in the Lib dir topic-IO type-bug An unexpected behavior, bug, or error

Comments

@eryksun
Copy link
Contributor

eryksun commented Sep 15, 2016

BPO 28164
Nosy @pfmoore, @tjguk, @zware, @eryksun, @zooba
PRs
  • [Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
  • Files
  • issue_28164_01.patch
  • issue_28164_02.patch
  • 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 = 'https://github.com/zooba'
    closed_at = <Date 2017-02-06.22:54:02.916>
    created_at = <Date 2016-09-15.03:51:03.787>
    labels = ['3.7', 'type-bug', 'library', 'expert-IO', 'OS-windows']
    title = '_PyIO_get_console_type fails for various paths'
    updated_at = <Date 2017-03-31.16:36:11.181>
    user = 'https://github.com/eryksun'

    bugs.python.org fields:

    activity = <Date 2017-03-31.16:36:11.181>
    actor = 'dstufft'
    assignee = 'steve.dower'
    closed = True
    closed_date = <Date 2017-02-06.22:54:02.916>
    closer = 'steve.dower'
    components = ['Library (Lib)', 'Windows', 'IO']
    creation = <Date 2016-09-15.03:51:03.787>
    creator = 'eryksun'
    dependencies = []
    files = ['46461', '46463']
    hgrepos = []
    issue_num = 28164
    keywords = ['patch']
    message_count = 17.0
    messages = ['276512', '276839', '276844', '276864', '286507', '286516', '286522', '287005', '287014', '287019', '287027', '287030', '287047', '287177', '287178', '287179', '287180']
    nosy_count = 6.0
    nosy_names = ['paul.moore', 'tim.golden', 'python-dev', 'zach.ware', 'eryksun', 'steve.dower']
    pr_nums = ['552']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue28164'
    versions = ['Python 3.6', 'Python 3.7']

    @eryksun
    Copy link
    Contributor Author

    eryksun commented Sep 15, 2016

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

    @eryksun eryksun added 3.7 (EOL) end of life stdlib Python modules in the Lib dir OS-windows topic-IO type-bug An unexpected behavior, bug, or error labels Sep 15, 2016
    @zooba
    Copy link
    Member

    zooba commented Sep 17, 2016

    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.

    @zooba zooba self-assigned this Sep 17, 2016
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Sep 17, 2016

    New changeset d0bab9fda568 by Steve Dower in branch '3.6':
    Issue bpo-28161: Opening CON for write access fails
    https://hg.python.org/cpython/rev/d0bab9fda568

    New changeset 187a114b9ef4 by Steve Dower in branch 'default':
    Issue bpo-28161: Opening CON for write access fails
    https://hg.python.org/cpython/rev/187a114b9ef4

    @zooba zooba closed this as completed Sep 17, 2016
    @eryksun
    Copy link
    Contributor Author

    eryksun commented Sep 18, 2016

    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.

    @eryksun eryksun reopened this Sep 18, 2016
    @eryksun
    Copy link
    Contributor Author

    eryksun commented Jan 31, 2017

    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.

    @zooba
    Copy link
    Member

    zooba commented Jan 31, 2017

    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.

    @eryksun
    Copy link
    Contributor Author

    eryksun commented Jan 31, 2017

    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.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 4, 2017

    New changeset ed0c05c739c9 by Steve Dower in branch '3.6':
    Issue bpo-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 bpo-28164 and issue bpo-29409
    https://hg.python.org/cpython/rev/a5538734cc87

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 5, 2017

    New changeset 164fc49825081694c9c44c4a17c75b30c5ba8f51 by Steve Dower in branch 'master':
    Issue bpo-28164: Correctly handle special console filenames (patch by Eryk Sun)
    164fc49

    New changeset 965f8d68a8dcc2ebb2480fe7e9121ac7efdee54e by Steve Dower in branch 'master':
    Merge issue bpo-28164 and issue bpo-29409
    965f8d6

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 5, 2017

    New changeset 164fc49825081694c9c44c4a17c75b30c5ba8f51 by Steve Dower in branch '3.6':
    Issue bpo-28164: Correctly handle special console filenames (patch by Eryk Sun)
    164fc49

    @zooba
    Copy link
    Member

    zooba commented Feb 5, 2017

    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.

    @zooba
    Copy link
    Member

    zooba commented Feb 5, 2017

    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.

    @eryksun
    Copy link
    Contributor Author

    eryksun commented Feb 5, 2017

    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)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 6, 2017

    New changeset 4463e311f5bd by Steve Dower in branch '3.6':
    Issue bpo-28164: Improves test on Windows 7
    https://hg.python.org/cpython/rev/4463e311f5bd

    New changeset 8132bcc1522d by Steve Dower in branch 'default':
    Issue bpo-28164: Improves test on Windows 7
    https://hg.python.org/cpython/rev/8132bcc1522d

    @zooba
    Copy link
    Member

    zooba commented Feb 6, 2017

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

    @zooba zooba closed this as completed Feb 6, 2017
    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 6, 2017

    New changeset c6c32889c9d80ffd599a57fd0d4c4a88deece29b by Steve Dower in branch 'master':
    Issue bpo-28164: Improves test on Windows 7
    c6c3288

    New changeset dc5c4bc90770d6a5875666434cf797c77eb3dcad by Steve Dower in branch 'master':
    Issue bpo-28164: Improves test on Windows 7
    dc5c4bc

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Feb 6, 2017

    New changeset c6c32889c9d80ffd599a57fd0d4c4a88deece29b by Steve Dower in branch '3.6':
    Issue bpo-28164: Improves test on Windows 7
    c6c3288

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life OS-windows stdlib Python modules in the Lib dir topic-IO type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants