FAQ
hi everyone,

sorry to drag something from the bug system onto the ML, but i'm unable
to comment on the closed bug. furthermore, the topic was already brought
up in the "strol for int parsing" thread, so i dare say that it is
relevant :)

afaict the bug in question is closed and marked bogus/duplicate due to
bug #51023. however, these are two different bugs in two different
paths of code; only the symptoms (and the underlying programming error)
are the same. i wouldn't be surprised if this problem were found elsewhere
as well...

the problem in both cases is that you can't rely on the wraparound
behavior in signed integer arithmetic. it's not defined in any C standard
and as mentioned in the previous thread leads to buggy behavior in recent
versions of gcc, since they may be smart enough to optimize out the
conditions that would detect overflow. an example from the affected
code in this case (gcc-4.4 with -O2):

rangda[~/debian/php] php -r 'var_dump(array("9223372036854775808" => 1));'
array(1) {
[-9223372036854775808]=>
int(1)
}

vs. the expected:

rangda[~/debian/php] ./cli-build/sapi/cli/php -r 'var_dump(array("9223372036854775808" => 1));'
array(1) {
["9223372036854775808"]=>
int(1)
}

(and likewise for the minimum extreme).

The problem can be solved by calculating when an overflow *will* occur,
instead of when it *did* occur. i.e. instead of

val = val * 10 + digit;
if (val < 0) /* overflow */

do:

if (val <= (LONG_MAX-digit)/10)
val = val * 10 + digit;
else /* overflow */

(and likewise, something similar for underflow detection past LONG_MIN)

so, here's a patch against zend_hash.h which fixes the problem in the
location reported in #51008. i wasn't able to figure out what
the convention was for tabs before the backslash escape, so i kinda
winged it. i also took the liberty of improving the test to more fully
illustrate the failures, similar to what i did in the patch for #51023.

i haven't tested this patch extensively but it does pass both the old
and new version of the test. thoughts and review appreciated. we also
have the patch in our git repo:

http://git.debian.org/?p=pkg-php/php.git;a=blob;f=debian/patches/zend_int_overflow.patch;h=3df7fa0853af0f04b3427e361a3eccfe66c4c3ae;hb=0fe497525d46b2fa8353e37106b47c05ef804cc5


sean

--- php.orig/Zend/zend_hash.h
+++ php/Zend/zend_hash.h
@@ -306,9 +306,11 @@ END_EXTERN_C()

#define ZEND_HANDLE_NUMERIC(key, length, func) do { \
register const char *tmp = key; \
+ int negative = 0; \
\
if (*tmp == '-') { \
tmp++; \
+ negative = 1; \
} \
if (*tmp >= '0' && *tmp <= '9') { /* possibly a numeric index */ \
const char *end = key + length - 1; \
@@ -322,19 +324,19 @@ END_EXTERN_C()
*tmp > '2')) { /* overflow */ \
break; \
} \
- idx = (*tmp - '0'); \
+ idx = ((negative)?-1:1) * (*tmp - '0'); \
while (++tmp != end && *tmp >= '0' && *tmp <= '9') { \
- idx = (idx * 10) + (*tmp - '0'); \
+ int digit = (*tmp - '0'); \
+ if ( (!negative) && idx <= (LONG_MAX-digit)/10 ) { \
+ idx = (idx * 10) + digit; \
+ } else if ( (negative) && idx >= (LONG_MIN+digit)/10 ) { \
+ idx = (idx * 10) - digit; \
+ } else { \
+ --tmp; /* overflow or underflow, make sure tmp != end */ \
+ break; \
+ } \
} \
if (tmp == end) { \
- if (*key == '-') { \
- idx = -idx; \
- if (idx > 0) { /* overflow */ \
- break; \
- } \
- } else if (idx < 0) { /* overflow */ \
- break; \
- } \
return func; \
} \
} \
--- php.orig/Zend/tests/bug45877.phpt
+++ php/Zend/tests/bug45877.phpt
@@ -1,23 +1,40 @@
--TEST--
Bug #45877 (Array key '2147483647' left as string)
---INI--
-precision=20
--FILE--
<?php
-$keys = array(PHP_INT_MAX,
- (string) PHP_INT_MAX,
- (string) (-PHP_INT_MAX - 1),
- -PHP_INT_MAX - 1,
- (string) (PHP_INT_MAX + 1));
+$max = sprintf("%d", PHP_INT_MAX);
+switch($max) {
+case "2147483647": /* 32-bit systems */
+ $min = "-2147483648";
+ $overflow = "2147483648";
+ $underflow = "-2147483649";
+ break;
+case "9223372036854775807": /* 64-bit systems */
+ $min = "-9223372036854775808";
+ $overflow = "9223372036854775808";
+ $underflow = "-9223372036854775809";
+ break;
+default:
+ die("failed: unknown value for PHP_MAX_INT");
+ break;
+}

-var_dump(array_fill_keys($keys, 1));
-?>
---EXPECTF--
-array(3) {
- [%d7]=>
- int(1)
- [-%d8]=>
- int(1)
- ["%d8"]=>
- int(1)
+function test_value($val, $msg) {
+ $a = array($val => 1);
+ $keys = array_keys($a);
+ if ($val == $keys[0]) $result = "ok";
+ else $result = "failed ($val != $keys[0])";
+ echo "$msg: $result\n";
}
+
+test_value($max, "max");
+test_value($overflow, "overflow");
+test_value($min, "min");
+test_value($underflow, "underflow");
+
+?>
+--EXPECT--
+max: ok
+overflow: ok
+min: ok
+underflow: ok

Search Discussions

Related Discussions

Discussion Navigation
viewthread | post
posts ‹ prev | 1 of 1 | next ›
Discussion Overview
groupphp-internals @
categoriesphp
postedFeb 26, '10 at 11:39p
activeFeb 26, '10 at 11:39p
posts1
users1
websitephp.net

1 user in discussion

Sean finney: 1 post

People

Translate

site design / logo © 2022 Grokbase