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

_io._WindowsConsoleIO breaks in the face of fd redirection #74740

Closed
segevfiner mannequin opened this issue Jun 2, 2017 · 8 comments
Closed

_io._WindowsConsoleIO breaks in the face of fd redirection #74740

segevfiner mannequin opened this issue Jun 2, 2017 · 8 comments
Labels
3.8 only security fixes 3.9 only security fixes 3.10 only security fixes OS-windows topic-IO type-bug An unexpected behavior, bug, or error

Comments

@segevfiner
Copy link
Mannequin

segevfiner mannequin commented Jun 2, 2017

BPO 30555
Nosy @pfmoore, @tjguk, @zware, @eryksun, @zooba, @segevfiner
PRs
  • bpo-30555: Fix WindowsConsoleIO errors in the presence of fd redirection #1927
  • 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 2017-06-02.16:56:53.131>
    labels = ['type-bug', '3.8', '3.9', 'expert-IO', '3.10', 'OS-windows']
    title = '_io._WindowsConsoleIO breaks in the face of fd redirection'
    updated_at = <Date 2021-04-23.22:04:03.017>
    user = 'https://github.com/segevfiner'

    bugs.python.org fields:

    activity = <Date 2021-04-23.22:04:03.017>
    actor = 'steve.dower'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Windows', 'IO']
    creation = <Date 2017-06-02.16:56:53.131>
    creator = 'Segev Finer'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 30555
    keywords = []
    message_count = 5.0
    messages = ['295041', '295047', '388796', '391745', '391746']
    nosy_count = 6.0
    nosy_names = ['paul.moore', 'tim.golden', 'zach.ware', 'eryksun', 'steve.dower', 'Segev Finer']
    pr_nums = ['1927']
    priority = 'normal'
    resolution = None
    stage = 'backport needed'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue30555'
    versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

    @segevfiner
    Copy link
    Mannequin Author

    segevfiner mannequin commented Jun 2, 2017

    _WindowsConsoleIO works by taking the handle out of the file descriptors of the stdio handles (get_osfhandle) and reading/writing to it directly using ReadConsoleW/WriteConsoleW.

    The problem is that some Python code wants to do file descriptor level redirection by overwriting the standard file descriptors using dup2. The problem is that this is also going to close the handle that Python itself is going to use to read/write to the console. Leading to the following:

        >>> fd = os.open('stdout.txt', os.O_CREAT | os.O_WRONLY)
        >>> os.dup2(fd, 1)
        >>> print('spam')
        Traceback (most recent call last):
          File "<stdin>", line 1, in <module>
        OSError: [WinError 87] The parameter is incorrect  # "OSError: [WinError 6] The handle is invalid" after https://bugs.python.org/issue30544

    This manifests itself for example in Pytest which does fd redirection to capture test output. See pytest-dev/py#103 for the issue. And see pytest-dev/pytest#2462 for an WIP attempt to workaround this.

    This issue also impacts other code that uses console handles itself like colorama: pytest-dev/pytest#2465. Though that code is likely going to have to deal with this by itself.

    Those file descriptors are an emulation implemented by the Universal CRT and you can see their implementation in your copy of the Windows SDK. It's quite possible that this doesn't happen in older CRT versions either since colorama doesn't seem to break on Python 2.7 for example.

    One way I can think working around this is that Python will DuplicateHandle the console handles so that even if dup2 closes the original ones it will keep on working. We can also switch to calling _get_osfhandle always instead of caching the handle, it will break when the fd is redirected to a non-console. But so does _WindowsConsoleIO in general since it will try to continue writing to the console despite the redirection, meaning that Python code doing redirection has to handle sys.std* anyhow. Though I'm not sure about the performance of constantly calling _get_osfhandle. And their yet might be other ways to solve this.

    Also see comment by eryksun https://bugs.python.org/msg294988

    Solving this in CPython will remove the need for hacks like the PR I referenced.

    @segevfiner segevfiner mannequin added 3.7 (EOL) end of life OS-windows topic-IO type-bug An unexpected behavior, bug, or error labels Jun 2, 2017
    @zooba
    Copy link
    Member

    zooba commented Jun 2, 2017

    We can also switch to calling _get_osfhandle always instead of caching the handle, it will break when the fd is redirected to a non-console. But so does _WindowsConsoleIO in general since it will try to continue writing to the console despite the redirection, meaning that Python code doing redirection has to handle sys.std* anyhow.

    This might be the best approach, ultimately. I bet there's an optimization here, though it's not obvious.

    Probably the main issue here is readline not properly handling sys.std*. If that was fixed, I think many of these problems (at least at the interactive prompt) would simply go away.

    @eryksun
    Copy link
    Contributor

    eryksun commented Mar 16, 2021

    PR 1927 is a definite improvement. By using _get_osfhandle() instead of caching the handle value, it guarantees that access to the original console file can be saved and restored via fd_save = dup(fd) and dup2(fd_save, fd). It also allows duping a new open of CON, CONIN$, or CONOUT$, or even a new screen buffer from CreateConsoleScreenBuffer(), to the existing fileno(), and the _WindowsConsoleIO() instance will march along none the wiser.

    @eryksun eryksun added 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes and removed 3.7 (EOL) end of life labels Mar 16, 2021
    @zooba
    Copy link
    Member

    zooba commented Apr 23, 2021

    New changeset 5e437fb by Segev Finer in branch 'master':
    bpo-30555: Fix WindowsConsoleIO fails in the presence of fd redirection (GH-1927)
    5e437fb

    @zooba
    Copy link
    Member

    zooba commented Apr 23, 2021

    Thanks for persisting with this change.

    Looks like backports will have to be manual, and I'm not going to get to them today myself, but if someone else wants to do it then feel free.

    I'm pretty sure this won't have any (negative) visible impact, but just in case, let's let a 3.10 release go out with it before merging the backports. I'd rather have no change in earlier versions than a regression.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @erlend-aasland
    Copy link
    Contributor

    ISTM this was fixed. I'm marking this as pending close. cc. @zooba

    @erlend-aasland erlend-aasland added the pending The issue will be closed if no feedback is provided label Jan 5, 2024
    @erlend-aasland
    Copy link
    Contributor

    AFAICS, gh-77046 is a duplicate of this issue and can be closed.

    @zooba
    Copy link
    Member

    zooba commented Jan 5, 2024

    It was fixed but not backported, which is why I left it open. The backports are irrelevant now, so yeah, can close.

    @zooba zooba closed this as completed Jan 5, 2024
    @erlend-aasland erlend-aasland removed the pending The issue will be closed if no feedback is provided label Jan 5, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes 3.9 only security fixes 3.10 only security fixes OS-windows topic-IO type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants