[Patches] [ python-Patches-1365916 ] [PATCH] mmap fails on AMD64

Thu Dec 15 06:46:38 CET 2005

Patches item #1365916, was opened at 2005-11-24 14:22
Message generated for change (Comment added) made by nnorwitz
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1365916&group_id=5470

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: Modules
Group: None
Status: Open
Resolution: None
Priority: 9
Submitted By: Joe Wreschnig (piman)
Assigned to: Nobody/Anonymous (nobody)
Summary: [PATCH] mmap fails on AMD64

Initial Comment:
mmap_move_method is not 64-bit safe, and fails on the
following code:

from mmap import mmap
f = open('test.out', 'ab+')
f.write('ABCDEabcde')
f.flush()
m = mmap(f.fileno(), 10)
m.move(5, 0, 5)
m.read(10)

To fix this, mmapmodule.c:538 needs to use "LLL:move"
instead of "iii:move" (this also fixes #1921169, which
changed it from "iii" to "III").

This is in both Python 2.3 and 2.4.

----------------------------------------------------------------------

>Comment By: Neal Norwitz (nnorwitz)
Date: 2005-12-14 21:46

Message:
Logged In: YES 
user_id=33168

Michael, I'm not sure I understand the issues in detail,
however I will try to respond.  signed/unsigned issues are
probably used and checked inconsitently--be wary of them. 
There are potential issues when the high bit is set and
whether that means a long or unsigned value.  The meaning
has changed in some versions of python.

I would definitely like to see shadowed variables fixed.

If you don't believe a comment is correct, you should fix
the problem whatever it is.  So fix the comment and/or code
as you see fit.

If the patch gets too large, feel free to break it up into
several related components if that makes sense.

I'm just starting to read over your analysis.  Thanks for
looking into these issues.

----------------------------------------------------------------------

Comment By: Michael Urman (michaelurman)
Date: 2005-12-14 21:38

Message:
Logged In: YES 
user_id=1404983

Issues I've run into, and would like pre-patch feedback:

Regarding signed/unsigned in general - is it python-code
safe to change a signed format to an unsigned? Reading
getargs.c it looks like bounds are only checked for the
signed formats. This could be either good or bad.

fcntlmodule.c: the reported char* type of arg on line 174 is
due to a shadow variable (already scoped-out) earlier in the
function. Is this worth changing?

xxmodule.c: the use of # with a long is preceded by a
comment about testing bad format characters. I'm unconvinced
whether this is what it meant.

----------------------------------------------------------------------

Comment By: Armin Rigo (arigo)
Date: 2005-12-14 12:06

Message:
Logged In: YES 
user_id=4771

No, I'd also be interested in hearing about a decent reusable parser for C.  The only one I know about is gccxml, and it seems to be an incredible mess to compile and to be broken on every other gcc version.

----------------------------------------------------------------------

Comment By: Michael Urman (michaelurman)
Date: 2005-12-14 11:11

Message:
Logged In: YES 
user_id=1404983

Alright, I'll work on creating a patch against HEAD. I'll
see what I can do about writing tests for this, but in
general I expect the only place to test it will be at the
higher level with samples like the one in this report; it's
certainly not worth writing ways to invasively check if a
ParseTuple the right values in most scenarios.

The BuildValue support from my script is very rough, but
I'll see what I can do. If you know of any easy to use C
parsers in Python, I wouldn't mind modifying the checker to
get better results with it.

----------------------------------------------------------------------

Comment By: Armin Rigo (arigo)
Date: 2005-12-14 09:40

Message:
Logged In: YES 
user_id=4771

Nice results, great work.  My humble opinion on this is that all known instances should be fixed.

The type size_t could be an int or a long depending on compiler/platform (or maybe even something else), so it should just not be used.  Any signed/unsigned conflict should also be investigated; most but not all seem harmless.

Of course, tests would be nice, but I don't completely see the point of spending too much efforts testing dark corners that we are pretty confident are getting fixed, when the code is under-tested to start with.

The Py_BuildValue() logs are more tedious to look through.  There are definitely some int-vs-long conflicts.  There are signed/unsigned conflicts, but some are intended, e.g. in binascii -- in these cases, though, an explicit cast would not hurt.

In any case if you embark on this clean-up, I volunteer to review and apply it (be sure to work on the subversion head).

----------------------------------------------------------------------

Comment By: Michael Urman (michaelurman)
Date: 2005-12-14 09:11

Message:
Logged In: YES 
user_id=1404983

Hi Armin,
Joe finally convinced me to get the SF account myself. I've
done some further analysis on this already at
http://www.tortall.net/mu/blog/2005/12/01 and would be
willing to create relevant patches for whatever change
coverage level you would like to see.  I.e. just for
mmapmodule, just for glaring errors, or for creating
temporary local variables to ensure all format string
passing is okay in all ParseTuple instances known to be bad.
Please let me know what patch or patches you would like me
to create.

----------------------------------------------------------------------

Comment By: Armin Rigo (arigo)
Date: 2005-12-13 03:09

Message:
Logged In: YES 
user_id=4771

Could you do a complete review of the i/I/l/L issue in
mmapmodule.c?  There seems to be a fair amount of
inconsistencies between them and the declared types of
the variables -- for example, I don't think that assigning
to size_t variables is a good idea; PyArg_ParseTuple()
should only assign to int/unsigned int/long/unsigned long
variables.

----------------------------------------------------------------------

Comment By: Joe Wreschnig (piman)
Date: 2005-11-28 13:36

Message:
Logged In: YES 
user_id=796

This is causing data corruption. There is also a trivial
patch. There is no good reason not to fix it immediately.

----------------------------------------------------------------------

Comment By: Jeff Epler (jepler)
Date: 2005-11-27 11:25

Message:
Logged In: YES 
user_id=2772

I haven't tested the patch, but the above testcase does fail
on my linux x86_64 machine with "ValueError: source or
destination out of range" on the call to m.move(), but
succeed on my linux x86 machine.

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1365916&group_id=5470