[issue25386] msvcrt_putch/msvcrt_putwch don't check the return value of _putch/_putwch

[Eryk Sun]

Change by Eryk Sun :


--
versions: +Python 3.10, Python 3.8, Python 3.9 -Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25386] msvcrt_putch/msvcrt_putwch don't check the return value of _putch/_putwch

[STINNER Victor]

Change by STINNER Victor :


--
keywords:  -easy

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25386] msvcrt_putch/msvcrt_putwch don't check the return value of _putch/_putwch

[Josh Snider]

Josh Snider added the comment:

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.

--
keywords: +patch
nosy: +Josh Snider
Added file: http://bugs.python.org/file42501/issue25386.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25386] msvcrt_putch/msvcrt_putwch don't check the return value of _putch/_putwch

[Alexander Riccio]

Alexander Riccio added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25386] msvcrt_putch/msvcrt_putwch don't check the return value of _putch/_putwch

[Eric V. Smith]

Eric V. Smith added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25386] msvcrt_putch/msvcrt_putwch don't check the return value of _putch/_putwch

[Eric V. Smith]

Eric V. Smith added the comment:

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.

--
keywords: +easy
nosy: +eric.smith
stage:  -> needs patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25386] msvcrt_putch/msvcrt_putwch don't check the return value of _putch/_putwch

[eryksun]

eryksun added the comment:

> 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

--
nosy: +eryksun

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25386] msvcrt_putch/msvcrt_putwch don't check the return value of _putch/_putwch

[Alexander Riccio]

Alexander Riccio added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25386] msvcrt_putch/msvcrt_putwch don't check the return value of _putch/_putwch

[Alexander Riccio]

New submission from Alexander Riccio:

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?

--
components: Windows
messages: 252899
nosy: Alexander Riccio, paul.moore, steve.dower, tim.golden, zach.ware
priority: normal
severity: normal
status: open
title: msvcrt_putch/msvcrt_putwch don't check the return value of _putch/_putwch
type: behavior
versions: Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue25386] msvcrt_putch/msvcrt_putwch don't check the return value of _putch/_putwch

[Zachary Ware]

Zachary Ware added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com