-
-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
msvcrt_putch/msvcrt_putwch don't check the return value of _putch/_putwch #69573
Comments
A minor issue (probably qualifies for the "easy" keyword): All functions in msvcrtmodule.c (I'm looking at http://svn.python.org/projects/python/trunk/PC/msvcrtmodule.c) except msvcrt_putch and msvcrt_putwch properly check return values against error codes, and call one of the PyErr_Set* functions to properly bubble the error up the call stack. _putch returns EOF on failure, and _putwch returns WEOF on failure. Like the rest of the functions in that file, I imagine that the fix would involve something like: if (_putch(ch) == EOF) Note: the Python msvcrt documentation (https://docs.python.org/3/library/msvcrt.html) says: "Changed in version 3.3: Operations in this module now raise OSError where IOError was raised." ...so if you were to backport this minor fix, you need to consider that (not sure what difference that makes). Should I take a stab at patching this myself (I've never done this for the Python project) or shall I leave that to the devs? |
For your convenience, the MSDN docs for the _putch/_putwch functions: https://msdn.microsoft.com/en-us/library/azb6c04e.aspx |
Feel free to create a patch yourself, we'll be happy to review it. Note though that the python project on svn.python.org is old and abandoned; we moved to Mercurial several years ago (see https://hg.python.org/cpython/). I haven't checked to see whether the issue you report has already been fixed in the years since moving away from svn. |
Although the function names have changed (I think due to Argument Clinic), the reported issue still applies to the current code: https://hg.python.org/cpython/file/tip/PC/msvcrtmodule.c#l309 I'll let other decide if the change is actually desirable. It would be interesting to know under what circumstances these functions can fail. |
The CRT _put[w]ch and _get[w]ch[e] functions will fail when called in a process that doesn't have a console. (Except get[w]ch may succeed if it follows unget[w]ch.) This is the case when running under pythonw.exe if the process hasn't first attached to a console via AllocConsole() or AttachConsole(). It also applies to python.exe when run as a detached process (i.e. the creation flag DETACHED_PROCESS) or after having detached the console via FreeConsole(). Note that even though the docs for get[w]ch 1 and get[w]che 2 state that "[t]here is no error return", these functions actually do return an error value of [W]EOF. This has been the case since at least back to Windows NT. Maybe Steve Dower can shed light on why this is undocumented. Here's an example, tested on 64-bit Windows 10 in Python 3.5. This example calls the console I/O functions using both msvcrt and ctypes. Since there's no attached console, an error is expected, except when calling ungetwch followed by getwch. import os
import sys
import subprocess
cmds_msvcrt = [
"import msvcrt; msvcrt.ungetwch('a'); msvcrt.getwch()",
"import msvcrt; msvcrt.ungetwch('a'); msvcrt.ungetwch('a')",
"import msvcrt; msvcrt.getwch()",
"import msvcrt; msvcrt.putwch('a')",
]
csetup = "import sys,ctypes; ucrt = ctypes.cdll.ucrtbase; "
cmds_ctypes = [
csetup + "ucrt._ungetwch(ord('a')); sys.exit(ucrt._getwch())",
csetup + "ucrt._ungetwch(ord('a')); sys.exit(ucrt._ungetwch(ord('a')))",
csetup + "sys.exit(ucrt._getwch())",
csetup + "sys.exit(ucrt._putwch(ord('a')))",
]
def test(cmds):
pythonw = os.path.join(sys.prefix, 'pythonw.exe')
return [subprocess.call([pythonw, '-c', cmd]) for cmd in cmds]
>>> test(cmds_msvcrt)
[0, 1, 0, 0] >>> test(cmds_ctypes)
[97, 65535, 65535, 65535] 65535 is WEOF. |
That's a great test, thanks. If we were designing this from scratch, I agree that raising an exception is the right thing to do. But the questions is: can we change the behavior now? I think it's unlikely that anyone is relying on these functions returning [W]EOF, and that raising an exception is unlikely to cause problems. I'm curious: did you [Alexander] have some code where you're actually expecting an exception, or did you just stumble across this case? |
Sorry for the delay: Gmail actually directed the update emails to my spam folder! Gmail said (something like): "It is in violation of Google's recommended email sender guidelines." ...and it's apparently not the first time this has happened with the python bugtracker: https://mail.python.org/pipermail/tracker-discuss/2015-January/003989.html Anyways:
That IS a great test :)
Nope, I was just poking around the sources. I was curious to see what CPython's C implementation looked like, and decided to poke around some of the dustier, and the less used corners. That, and I'm really OCD about checking return values. |
Here's a patch that raises an exception when _put[w]ch and _get[w]ch[e] are run without a console. Like Eryk Sun says, the get's work fine if you do unget before hand. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: