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

Explicit cast to HANDLE for Read/WriteConsoleW calls #1342

Merged
merged 1 commit into from Aug 1, 2019

Conversation

@OEP
Copy link
Contributor

commented Jun 20, 2019

We have a custom build of Python 2.7 which is built against MSVC 14. We found that Click's windows console functionality was degraded after making the switch the MSVC 14, hitting that explicit raise OSError in _WindowsConsoleWriter.write() with errno 6 (invalid file handle).

I run into the error using click.echo(), just for some cases, but it seems like you hit one of the bad cases and it ruins the rest of the cases. For example, here are the results I get for individual calls:

click.echo('')  # Fails
click.echo('f') # Succeeds

but run the two in sequence and they both fail:

import click
for s in ['', 'f']:
    try:
        click.echo(s)
        print "Pass", repr(s)
    except OSError:
        print "Fail", repr(s)

My output:

Fail ''
f
Fail 'f'

I may be a very remote corner case, since the MSVC 9 build of Python 2.7.16 and the MSVC 14 build of Python 3.6.8 both work fine.

Stack for reference:

Traceback (most recent call last):
  File "script.py", line 10, in <module>
    click.echo(s)
  File "utils.py", line 260, in echo
    file.write(message)
  File "_compat.py", line 628, in _safe_write
    return _write(s)
  File "ansitowin32.py", line 40, in write
    self.__convertor.write(text)
  File "ansitowin32.py", line 141, in write
    self.write_and_convert(text)
  File "ansitowin32.py", line 169, in write_and_convert
    self.write_plain_text(text, cursor, len(text))
  File "ansitowin32.py", line 175, in write_plain_text
    self.wrapped.flush()
  File "_winconsole.py", line 164, in write
    raise OSError(self._get_error_message(GetLastError()))
exceptions.OSError, Windows error 6
@davidism

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

What is HANDLE and why does it fix this?

@OEP

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

It's a Windows API type which is just a void pointer. The docs for WriteConsoleW/ReadConsoleW say the first argument should be a HANDLE.

If you step through the code self.handle is an int, which ctypes converts to a signed C int if I read the code right, and converting int to void* didn't seem like a safe cast to me. Most of the other args which actually get used are already correctly wrapped in a ctypes type, and error number 6 is just "invalid handle," so it seemed like a good thing to try. And it happened to fix my problem.

As for why or if it causes the specific behavior I ran into, I'm less sure about. I could wave my hands and say "undefined behavior" but I was hoping someone in the community could say with more conviction.

@davidism

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Does this still work for the older builds that didn't demonstrate the issue before? Sorry for the questions, I am not a Windows dev 😩

@OEP

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

These are the builds that I tried, and they all seem to work with this patch:

  • Unofficial MSVC 14 Python 2.7 build (the one that exhibits the problem)
  • Unofficial MSVC 10 Python 2.7 build
  • Unofficial MSVC 14 Python 3.6 build
  • Official Python 2.7 build
  • Official Python 3.7 build
See [1]. The C call expects the first argument to be of type HANDLE.
Most Python variants can directly and safely pass the int, but not all.
This can cause click to raise OSError with errno 6 (invalid file
handle).

[1] https://docs.microsoft.com/en-us/windows/console/writeconsole
@davidism davidism force-pushed the OEP:winconsole branch from c80ca06 to e1714b8 Aug 1, 2019
@davidism davidism changed the base branch from master to 7.x Aug 1, 2019
@davidism davidism added this to the 7.1 milestone Aug 1, 2019
@davidism davidism merged commit 017ea84 into pallets:7.x Aug 1, 2019
10 checks passed
10 checks passed
Tests Build #20190801.9 succeeded
Details
Tests (Job Docs) Job Docs succeeded
Details
Tests (Job PyPy 3 Linux) Job PyPy 3 Linux succeeded
Details
Tests (Job Python 2.7 Linux) Job Python 2.7 Linux succeeded
Details
Tests (Job Python 2.7 Windows) Job Python 2.7 Windows succeeded
Details
Tests (Job Python 3.5 Linux) Job Python 3.5 Linux succeeded
Details
Tests (Job Python 3.6 Linux) Job Python 3.6 Linux succeeded
Details
Tests (Job Python 3.7 Linux) Job Python 3.7 Linux succeeded
Details
Tests (Job Python 3.7 Mac) Job Python 3.7 Mac succeeded
Details
Tests (Job Python 3.7 Windows) Job Python 3.7 Windows succeeded
Details
@OEP OEP deleted the OEP:winconsole branch Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.