Hi internals,
I tried to fix http://bugs.php.net/bug.php?id=38770.
The pack and unpack function returns different values on different
platform for options like "N".
Example:
print_r(unpack("N", pack("N", -3000)));
prints -3000 und x86
and 4294937296 on x86_64.
That happens because all the 32 bit integers are handled as long in the
zend engine, but long differs from platform (8byte on x86_64 and 4byte on
x86).
If we pack e.g. a integer to big endian and unpack it to little endian,
the long value will be filled by the complete 4byte integer value, so
negative values are keeped. But on x86_64 (little endian) only the lower
4byte are set to the integer value while the highter 4bytes are set to
zero. Thats why the long values that holds the negative integer value is
treaten like a positive long.
Thats why I added a few lines to the pack.c to detect if I get a negative
integer (e.g. highest bit is set on little endian machines). For a
negative integer I fill the higher 4bytes with 1, so that the long value
is treaten as negative.
You may ask why I take a 4byte unsigned 32bit int from pack and test if
the highest bit is set to "cast" it to an signed integer? Thats exactly
what php does on x86 machines as it uses signed long internal.
here is the patch (against 5.2.0 release):
http://sqlbackup.net/data/pack.c.diff
I tested it on x86_64 and x86 linux for various pack options.
It#s not the best way, the best way might be to cast the signed integers
to unsigned in the pack command not in the unpack, but that might be a BC.
What do you think?
Greets
David
Are there no opinions about that?
It's just that if u pack an integer u get an integer and are not
influenced by the convert to the long as it is stored in a zval struct.
It's just about handling an int as an int (which is also 4byte on x64) and
avoid problems if the long is bigger than the int as in x64 but not in
x x84 (on x84 int is 4byte and long is 4byte)
would be great to get some feedback.
I also attached the patch against PHP_5_2 CVS
Index: ext/standard/pack.c
RCS file: /repository/php-src/ext/standard/pack.c,v
retrieving revision 1.57.2.5
diff -u -r1.57.2.5 pack.c
--- ext/standard/pack.c 26 Feb 2006 10:49:50 -0000 1.57.2.5
+++ ext/standard/pack.c 17 Nov 2006 17:54:24 -0000
@@ -756,11 +756,15 @@
long v;
int issigned = 0;
-
if (type == 'i') { issigned = input[inputpos + (machine_little_endian ? (sizeof(int) - 1) : 0)] & 0x80;
-
if (sizeof(long) > sizeof(int) && issigned) { -
v = ~INT_MAX; -
} else { -
v = 0; }
-
v = php_unpack(&input[inputpos], sizeof(int), issigned, int_map);
-
v |= php_unpack(&input[inputpos], sizeof(int), (type=='i')?issigned:0, int_map); add_assoc_long(return_value, n, v); break; }
@@ -781,7 +785,14 @@
map = little_endian_long_map;
}
-
v = php_unpack(&input[inputpos], 4, issigned, map);
-
if (sizeof(long) > 4 && (input[inputpos + map[3]] & 0x80) == 0x80) { -
v = ~INT_MAX; -
} else { -
v = 0; -
} -
v |= php_unpack(&input[inputpos], 4, issigned, map); -
add_assoc_long(return_value, n, v); break; }
It#s not the best way, the best way might be to cast the signed
integers
to unsigned in the pack command not in the unpack, but that might be a
BC.
Assuming it works and does what it's supposed to for 'N' it seems like
a fine patch to me...
BUT:
I would presume that somewhere "out there" in User-land PHP code,
somebody is already working around this issue and their "fix" might be
messed up by this changing. So, to some degree, this is a BC breaking
bugfix. I dunno what policy is in place, but it seems to me that it
would be best to only apply this patch in 6.x to avoid problems...
This would be mitigated if, like, N is a not-often-used pack mode, and
there is already another one that is the obvious work-around for it on
64-bit platforms, or something like that, so that it's super unlikely
that there is any user-land code to work-around N in some way that
this patch will break.
--
Some people have a "gift" link here.
Know what I want?
I want you to buy a CD from some starving artist.
http://cdbaby.com/browse/from/lynch
Yeah, I get a buck. So?