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

msvcrt_putch/msvcrt_putwch don't check the return value of _putch/_putwch #69573

Open
ariccio mannequin opened this issue Oct 12, 2015 · 8 comments
Open

msvcrt_putch/msvcrt_putwch don't check the return value of _putch/_putwch #69573

ariccio mannequin opened this issue Oct 12, 2015 · 8 comments
Labels
3.11 only security fixes 3.12 bugs and security fixes 3.13 bugs and security fixes build The build process and cross-build easy OS-windows type-bug An unexpected behavior, bug, or error

Comments

@ariccio
Copy link
Mannequin

ariccio mannequin commented Oct 12, 2015

BPO 25386
Nosy @pfmoore, @ericvsmith, @tjguk, @zware, @eryksun, @zooba, @ariccio
Files
  • issue25386.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 = None
    closed_at = None
    created_at = <Date 2015-10-12.23:50:45.826>
    labels = ['3.10', 'type-bug', '3.8', '3.9', 'OS-windows']
    title = "msvcrt_putch/msvcrt_putwch don't check the return value of _putch/_putwch"
    updated_at = <Date 2021-03-17.20:54:41.118>
    user = 'https://github.com/ariccio'

    bugs.python.org fields:

    activity = <Date 2021-03-17.20:54:41.118>
    actor = 'eryksun'
    assignee = 'none'
    closed = False
    closed_date = None
    closer = None
    components = ['Windows']
    creation = <Date 2015-10-12.23:50:45.826>
    creator = 'Alexander Riccio'
    dependencies = []
    files = ['42501']
    hgrepos = []
    issue_num = 25386
    keywords = ['patch']
    message_count = 8.0
    messages = ['252899', '252900', '252903', '252921', '252963', '253044', '253316', '263634']
    nosy_count = 8.0
    nosy_names = ['paul.moore', 'eric.smith', 'tim.golden', 'zach.ware', 'eryksun', 'steve.dower', 'Josh Snider', 'Alexander Riccio']
    pr_nums = []
    priority = 'normal'
    resolution = None
    stage = 'needs patch'
    status = 'open'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue25386'
    versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

    @ariccio
    Copy link
    Mannequin Author

    ariccio mannequin commented Oct 12, 2015

    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?

    @ariccio ariccio mannequin added OS-windows type-bug An unexpected behavior, bug, or error labels Oct 12, 2015
    @ariccio
    Copy link
    Mannequin Author

    ariccio mannequin commented Oct 12, 2015

    For your convenience, the MSDN docs for the _putch/_putwch functions: https://msdn.microsoft.com/en-us/library/azb6c04e.aspx

    @zware
    Copy link
    Member

    zware commented Oct 13, 2015

    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.

    @ericvsmith
    Copy link
    Member

    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.

    @eryksun
    Copy link
    Contributor

    eryksun commented Oct 13, 2015

    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.

    @ericvsmith
    Copy link
    Member

    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?

    @ariccio
    Copy link
    Mannequin Author

    ariccio mannequin commented Oct 22, 2015

    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.

    @JoshSnider
    Copy link
    Mannequin

    JoshSnider mannequin commented Apr 17, 2016

    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.

    @vstinner vstinner removed the easy label Jul 29, 2019
    @eryksun eryksun added 3.8 only security fixes 3.9 only security fixes 3.10 only security fixes labels Mar 17, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @iritkatriel iritkatriel added build The build process and cross-build easy 3.11 only security fixes 3.12 bugs and security fixes 3.13 bugs and security fixes and removed 3.10 only security fixes 3.9 only security fixes 3.8 only security fixes labels Nov 24, 2023
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 only security fixes 3.12 bugs and security fixes 3.13 bugs and security fixes build The build process and cross-build easy OS-windows type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants