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

[SourceForge.net]

Patches item #1365916, was opened at 2005-11-24 16:22
Message generated for change (Comment added) made by michaelurman
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: Michael Urman (michaelurman)
Date: 2005-12-14 13: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 11: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 11: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 05: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 15: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 13: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