classification
Title: msvcrt_putch/msvcrt_putwch don't check the return value of _putch/_putwch
Type: behavior Stage: needs patch
Components: Windows Versions: Python 3.6
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Alexander Riccio, Josh Snider, eric.smith, eryksun, paul.moore, steve.dower, tim.golden, zach.ware
Priority: normal Keywords: patch

Created on 2015-10-12 23:50 by Alexander Riccio, last changed 2019-07-29 11:51 by vstinner.

Files
File name Uploaded Description Edit
issue25386.patch Josh Snider, 2016-04-17 22:11 review
Messages (8)
msg252899 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2015-10-12 23:50
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)
    return PyErr_SetFromErrno(PyExc_IOError);


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?
msg252900 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2015-10-12 23:53
For your convenience, the MSDN docs for the _putch/_putwch functions: https://msdn.microsoft.com/en-us/library/azb6c04e.aspx
msg252903 - (view) Author: Zachary Ware (zach.ware) * (Python committer) Date: 2015-10-13 02:29
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.
msg252921 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2015-10-13 09:02
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.
msg252963 - (view) Author: Eryk Sun (eryksun) * (Python triager) Date: 2015-10-13 21:31
> 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. 

[1]: https://msdn.microsoft.com/en-us/library/078sfkak
[2]: https://msdn.microsoft.com/en-us/library/kswce429
msg253044 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2015-10-15 13:04
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?
msg253316 - (view) Author: Alexander Riccio (Alexander Riccio) * Date: 2015-10-22 06:27
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's a great test, thanks.

That IS a great test :)

> I'm curious: did you [Alexander] have some code where you're actually expecting an exception, or did you just stumble across this case?

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.
msg263634 - (view) Author: Josh Snider (Josh Snider) * Date: 2016-04-17 22:11
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.
History
Date User Action Args
2019-07-29 11:51:31vstinnersetkeywords: - easy
2016-04-17 22:11:34Josh Snidersetfiles: + issue25386.patch

nosy: + Josh Snider
messages: + msg263634

keywords: + patch
2015-10-22 06:27:52Alexander Ricciosetmessages: + msg253316
2015-10-15 13:04:47eric.smithsetmessages: + msg253044
2015-10-13 21:31:50eryksunsetnosy: + eryksun
messages: + msg252963
2015-10-13 09:02:29eric.smithsetnosy: + eric.smith
messages: + msg252921

keywords: + easy
stage: needs patch
2015-10-13 02:29:32zach.waresetmessages: + msg252903
2015-10-12 23:53:44Alexander Ricciosetmessages: + msg252900
2015-10-12 23:50:45Alexander Ricciocreate