Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [PATCH] For Bug #38770 »

[PATCH] For Bug #38770

php.internals

David Soria Parra

19 years ago
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

David Soria Parra

19 years ago
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.

David Soria Parra

19 years ago
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; }

Richard Lynch

19 years ago
On Wed, November 15, 2006 4:50 pm, David Soria Parra wrote:
> 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?