Zend/tests/bug45877.phpt, #51008, and signed integer overflows

15 years ago by sean finney — view source

unread

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