This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Patch to fix typos in C code
Type: behavior Stage: resolved
Components: Installation, Interpreter Core, Windows Versions: Python 3.2, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, belopolsky, eric.araujo, gregory.p.smith, loewis, pitrou, python-dev, serhiy.storchaka, typo.pl, vstinner
Priority: high Keywords:

Created on 2008-12-13 07:56 by typo.pl, last changed 2022-04-11 14:56 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
py30dif2.txt typo.pl, 2008-12-17 05:52 unified diff patch for typo bugs in Py3k
Messages (14)
msg77718 - (view) Author: Johnny Lee (typo.pl) Date: 2008-12-13 07:56
I ran my typo.pl perl script that locates possible C/C++ typos.

I found four that looked valid.

Two of the typos were in the Python directory {pythonrun.c, 
dynload_win.c}, two were in PC/bdist_wininst {install.c, extract.c}.

Python/dynload_win.c:
Win32 API FormatMessageW() expects the 6th parameter to be the count of 
characters, NOT the count of bytes.

Python/pythonrun.c:
The source code contains "if (ferr != NULL || ferr != Py_None)". This 
does not work as expected - if ferr == NULL, then the second part of 
the if expression will succeed. Look at the code handling fout about 8 
lines up to see the correct code.

PC/bdist_wininst/extract.c:
Win32 API CreateFileMapping returns NULL on error, not 
INVALID_HANDLE_VALUE.

PC/bdist_wininst/install.c:
Win32 API CreateFileMapping returns NULL on error, not 
INVALID_HANDLE_VALUE.
msg77764 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-12-13 21:09
I don't know Windows APIs but the pythonrun.c bug is genuine.
msg77765 - (view) Author: Johnny Lee (typo.pl) Date: 2008-12-13 21:26
Here are the URLs to the MSDN documentation for CreateFileMapping and 
FormatMessage[A|W]:
<http://msdn.microsoft.com/en-us/library/aa366537.aspx>
<http://msdn.microsoft.com/en-us/library/ms679351.aspx>

For CreateFileMapping(), from the Return Value section:
"If the function fails, the return value is NULL. To get extended error 
information, call GetLastError."

For FormatMessage[A|W]:
nSize [in] 
If the FORMAT_MESSAGE_ALLOCATE_BUFFER flag is not set, this parameter 
specifies the size of the output buffer, in TCHARs.
msg77767 - (view) Author: Johnny Lee (typo.pl) Date: 2008-12-13 23:06
For the dynload_win.c typo, it's technically a possible buffer 
overflow, but you'd need to find an error that had an error message 
that's longer than 259 chars.

In pythonrun.c, the if statements for fout and ferr and almost 
identical. Probably a good idea to convert the if-statement to a 
separate function and call that function with fout and then ferr.
msg77916 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2008-12-16 17:01
It looks to me as though all these are valid, and the patch should be 
applied to py3k and the 3.0 maintenance branch.

Minor nit: the "sizeof(theInfo)" line exceeds 79 characters, in violation 
of PEP 7.

Is there any core developer who's in a position to test this patch on 
Windows?
msg77948 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-12-17 00:24
It's difficult to really test such errors.

Moreover, on the top of the bdist_wininst files, a comment says loudly:  
IF CHANGES TO THIS FILE ARE CHECKED INTO PYTHON CVS, THE RECOMPILED 
BINARIES MUST BE CHECKED IN AS WELL!
msg77956 - (view) Author: Johnny Lee (typo.pl) Date: 2008-12-17 05:52
attached modified diff patch so line length <=79 chars
msg78054 - (view) Author: Johnny Lee (typo.pl) Date: 2008-12-19 06:34
> It's difficult to really test such errors.

When I can't control the called function, I usually step through the 
code in a debugger and change the result variable in question to the 
appropriate value to see if the code handles failed function calls 
correctly. 

In the three Win32 API cases, it's clear that the failure path was not 
tested.

And I can't see how the ferr if statement was tested with a failure 
value either.
msg105675 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2010-05-14 01:00
I fixed the typo in pythonrun.c: r81156. I don't use Windows, so I cannot review the other fixes.
msg106636 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-05-28 04:46
I don't run Windows either but I believe sizeof(char) == 1 is guaranteed by the C standard, so the first report (Python/dynload_win.c) is invalid.

The last two appear to be valid [1], and a common mistake. [2]  Unit tests are needed for these.  I also notice that r81156 does not add a unit test either.  ISTM, a test case can be crafted by setting sys.stderr to None.

[1] http://msdn.microsoft.com/en-us/library/aa366537(VS.85).aspx
[2] http://blogs.msdn.com/b/oldnewthing/archive/2004/03/02/82639.aspx
msg106637 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-05-28 05:03
Strike my comment about Python/dynload_win.c - I was looking at the trunk version instead of py3k.  In py3k theInfo is wchar_t and patch seems to be valid.
msg138220 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-06-12 20:36
Martin would be the expert for bdist_wininst.
msg185104 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2013-03-23 23:06
New changeset 4db1b0bb3683 by Gregory P. Smith in branch '3.3':
Fixes issue4653 - Correctly specify the buffer size to FormatMessageW and
http://hg.python.org/cpython/rev/4db1b0bb3683

New changeset ace52be8da89 by Gregory P. Smith in branch 'default':
Fixes issue4653 - Correctly specify the buffer size to FormatMessageW and
http://hg.python.org/cpython/rev/ace52be8da89
msg185105 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2013-03-23 23:08
the pythonrun issue had already been fixed.  the others were still there (valid bugs, but were unlikely to be causing problems given the codepaths).  fixed.

I didn't write a Misc/NEWS entry on the commit because I didn't know how to usefully describe these minor fixes.  someone else is welcome to contribute one if they feel the need.
History
Date User Action Args
2022-04-11 14:56:42adminsetgithub: 48903
2013-03-23 23:08:57gregory.p.smithsetstatus: open -> closed

nosy: + gregory.p.smith
messages: + msg185105

resolution: fixed
stage: test needed -> resolved
2013-03-23 23:06:15python-devsetnosy: + python-dev
messages: + msg185104
2012-04-29 13:39:16mark.dickinsonsetnosy: - mark.dickinson
2012-03-31 19:08:54serhiy.storchakasetnosy: + serhiy.storchaka
2011-06-12 20:36:31eric.araujosetnosy: + eric.araujo

messages: + msg138220
title: Patch to fix typos for Py3K -> Patch to fix typos in C code
2011-06-12 18:38:18terry.reedysetversions: + Python 3.3, - Python 3.1
2010-05-28 05:03:50belopolskysetmessages: + msg106637
2010-05-28 04:46:45belopolskysetnosy: + belopolsky
messages: + msg106636

components: + Windows
stage: commit review -> test needed
2010-05-14 01:00:15vstinnersetmessages: + msg105675
2010-05-13 00:57:45vstinnersetnosy: + vstinner
2010-05-11 20:26:54terry.reedysetversions: + Python 3.2, - Python 3.0
2008-12-19 06:34:22typo.plsetmessages: + msg78054
2008-12-17 05:52:05typo.plsetfiles: + py30dif2.txt
messages: + msg77956
2008-12-17 05:50:09typo.plsetfiles: - py30diff.txt
2008-12-17 00:24:10amaury.forgeotdarcsetnosy: + amaury.forgeotdarc, loewis
messages: + msg77948
2008-12-16 17:01:57mark.dickinsonsetnosy: + mark.dickinson
stage: commit review
messages: + msg77916
versions: + Python 3.1
2008-12-13 23:06:11typo.plsetmessages: + msg77767
2008-12-13 21:26:34typo.plsetmessages: + msg77765
2008-12-13 21:09:07pitrousetpriority: high
type: behavior
messages: + msg77764
nosy: + pitrou
2008-12-13 07:56:55typo.plcreate