[Patches] [ python-Patches-1038854 ] Fix struct.pack on 64-bit archs (broken on 2.*)

Thu Nov 11 23:20:46 CET 2004

Patches item #1038854, was opened at 2004-10-02 07:07
Message generated for change (Comment added) made by sgatwasetde
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=305470&aid=1038854&group_id=5470

Category: Modules
Group: Python 2.4
Status: Open
Resolution: None
Priority: 5
Submitted By: Stefan Grundmann (sgatwasetde)
Assigned to: Nobody/Anonymous (nobody)
Summary: Fix struct.pack on 64-bit archs (broken on 2.*)

Initial Comment:

Description:

The code in the struct module assumes that sizeof(long)
== sizeof(int)
which is wrong on (most) 64-bit architectures (linux on
amd64 with a 32-bit userland is an exception). 

How To Repeat:

on a 32-bit platform struct.pack behaves as expected:

$  uname -m  -r  -s
FreeBSD 4.10-STABLE i386
$ python -c "import struct; print repr(struct.pack('I',
4294967296))"
Traceback (most recent call last):
  File "<string>", line 1, in ?
OverflowError: long int too large to convert

on a 64-bit platform it treats integers as longs 
(it does not check for over/underflows and returns the
lower 4 byte): 

$ uname -m  -r  -s
FreeBSD 5.2-CURRENT sparc64
$ python -c "import struct; print repr(struct.pack('I',
4294967296))" 
'\x00\x00\x00\x00'

Fix:

in python/python/dist/src/Modules/structmodule.c:
np_uint() and np_int() have to check for 
over/underflows using MAX_UINT, MAX_INT, MIN_INT  as
np_ushort() and np_short() already do for MAX_USHORT, ...

the attached patch does this
(diff was generated using diff -rNu and  Revision 2.62
of python/python/dist/src/Modules/structmodule.c)

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

>Comment By: Stefan Grundmann (sgatwasetde)
Date: 2004-11-11 23:20

Message:
Logged In: YES 
user_id=1131807

Issuing a warning instead of an error might be a good idea 
(to give the user-base some time adapt).  
But then we still have to deal with the fact that the some 
python modules are broken on 64 bit (at least big-endian). 
The gzip module for example does not work correctly even 
with the old code (because of stuctmodule).  
And there is user code out there that relies on correct  
overflow detection (which was the reason i submitted the 
patch). 

Another way would be to omit the overflow detection 
completely and heave the user take care of it. This will break 
a lot of applications but is imho more consistent then the old   
behavior (check some cases on some architectures). 

Personally i would like to have a structmodule in python 2.4 
that works a one would expect (overflow detection) but that  
is not my decision to make. The fact that code which is 
under control of the python project uses struct.pack (...) in 
erroneous ways should be fixed (even if it does work on 32 
bit archs atm). 

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

Comment By: Armin Rigo (arigo)
Date: 2004-11-10 22:31

Message:
Logged In: YES 
user_id=4771

This indicate that similar breakage will probably occur in user code if we add range checking.  Do we want to take the risk?  It looks overkill, but what about issuing a warning and turning it into an error in the next version?

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

Comment By: Stefan Grundmann (sgatwasetde)
Date: 2004-11-10 09:36

Message:
Logged In: YES 
user_id=1131807

The patch breaks test_binhex on 32 and 64 bit architectures 
because Lib/binhex.py is using struct.pack('>h' ...) to pack 
an unsigned short (which is  wrong but does work with the 
current version of Modules/structmodule.c because of the 
lack of range checking).  

The patch breaks test_gzip (and test_tarfile) on 64 bit 
architectures because Lib/gzip.py is using write32 (which 
uses to pack('<l',...) ) to write an unsigned 32 bit checksum.  

This should be fixed in Lib/gzip.py and Lib/binhex.py. 

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

Comment By: Neal Norwitz (nnorwitz)
Date: 2004-11-08 04:15

Message:
Logged In: YES 
user_id=33168

This patch breaks 3 tests:  test_binhex test_gzip test_tarfile.
tarfile breaks because of gzip.

Run on opterons.  Can you update the patch so these tests pass?

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

Comment By: Stefan Grundmann (sgatwasetde)
Date: 2004-10-08 08:07

Message:
Logged In: YES 
user_id=1131807

The attached patch will fix the range-checking-code of the
integer pack functions for 64 and 32 bit architectures
(tested on i386 and Sparc64, 64-bit little-endian was not
tested 'cause of lack of hardware). All test cases worked as
expected, there is no more need for BUGGY_RANGE_CHECK.

I'm a bit unsure about the used method to get an unsigned
long from a Python_Long object with overflow checking... 
PyLong_AsUnsignedLong(PyLong_FromLong(PyInt_AS_LONG(v))) 
looks a rather excessive.

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

Comment By: Stefan Grundmann (sgatwasetde)
Date: 2004-10-06 08:31

Message:
Logged In: YES 
user_id=1131807

I removed the attached patch since it only handles overflows
for np_int/np_uint. bp_int/bp_uint and lp_int/lp_unit have
different issues (no overflow checking at all, unneccessary
loops - that will not be optimized by 
the  compiler). I'm working on a new patch that fixes these
problems 
(and the rest of  BUGGY_RANGE_CHECK ;) ).

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

Comment By: Armin Rigo (arigo)
Date: 2004-10-05 15:31

Message:
Logged In: YES 
user_id=4771

At a first glance, it appears that there is the same problem
in bp_int/bp_uint and lp_int/lp_unit.  Can you check that
pack('<I',...) and pack('>I',...) fail to detect the
overflow in the same way?  If so, can you also provide the
patch for these 4 other routines?  Finally, it would be nice
to add a test case in Lib/test/test_struct.py.

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

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