ID: 51023 Comment by: seanius at debian dot org Reported By: geissert at debian dot org Status: Open Bug Type: Filter related Operating System: * PHP Version: 5.3SVN-2010-02-12 New Comment:
Here's the patch i've cobbled together. in case it doesn't cut/paste okay, it's also available at: http://git.debian.org/?p=pkg-php/php.git;a=commitdiff;h=3061d111de130df7388cc78e26b63cc105574775 From: Sean Finney <[email protected]> Subject: Fix improper signed overflow detection in filter extension The existing filter code relied on detecting invalid long integers by examining computed values for wraparound. This is not defined behavior in any C standard, and in fact recent versions of gcc will optimize out such checks resulting in invalid code. This patch therefore changes how the overflow/underflow conditions are detected, using more reliable arithmetic. It also fixes another bug, that the minimum integer value (-PHP_INT_MAX)-1 could not be detected properly. This patch also includes an update to the test case that detects such overflows, adding much more thorough and descriptive checking. Bug: http://bugs.php.net/bug.php?id=51023 Bug-Debian: http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=570287 --- php.orig/ext/filter/logical_filters.c +++ php/ext/filter/logical_filters.c @@ -68,7 +68,7 @@ static int php_filter_parse_int(const char *str, unsigned int str_len, long *ret TSRMLS_DC) { /* {{{ */ long ctx_value; - int sign = 0; + int sign = 0, digit = 0; const char *end = str + str_len; switch (*str) { @@ -82,7 +82,7 @@ static int php_filter_parse_int(const ch /* must start with 1..9*/ if (str < end && *str >= '1' && *str <= '9') { - ctx_value = ((*(str++)) - '0'); + ctx_value = ((sign)?-1:1) * ((*(str++)) - '0'); } else { return -1; } @@ -95,19 +95,18 @@ static int php_filter_parse_int(const ch while (str < end) { if (*str >= '0' && *str <= '9') { - ctx_value = (ctx_value * 10) + (*(str++) - '0'); + digit = (*(str++) - '0'); + if ( (!sign) && ctx_value <= (LONG_MAX-digit)/10 ) { + ctx_value = (ctx_value * 10) + digit; + } else if ( sign && ctx_value >= (LONG_MIN+digit)/10) { + ctx_value = (ctx_value * 10) - digit; + } else { + return -1; + } } else { return -1; } } - if (sign) { - ctx_value = -ctx_value; - if (ctx_value > 0) { /* overflow */ - return -1; - } - } else if (ctx_value < 0) { /* overflow */ - return -1; - } *ret = ctx_value; return 1; --- php.orig/ext/filter/tests/046.phpt +++ php/ext/filter/tests/046.phpt @@ -4,16 +4,46 @@ Integer overflow <?php if (!extension_loaded("filter")) die("skip"); ?> --FILE-- <?php -$s = sprintf("%d", PHP_INT_MAX); -var_dump(is_long(filter_var($s, FILTER_VALIDATE_INT))); +$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; +} -$s = sprintf("%.0f", PHP_INT_MAX+1); -var_dump(filter_var($s, FILTER_VALIDATE_INT)); +function test_validation($val, $msg) { + $f = filter_var($val, FILTER_VALIDATE_INT); + echo "$msg filtered: "; var_dump($f); // filtered value (or false) + echo "$msg is_long: "; var_dump(is_long($f)); // test validation + echo "$msg equal: "; var_dump($val == $f); // test equality of result +} -$s = sprintf("%d", -PHP_INT_MAX); -var_dump(is_long(filter_var($s, FILTER_VALIDATE_INT))); +// PHP_INT_MAX +test_validation($max, "max"); +test_validation($overflow, "overflow"); +test_validation($min, "min"); +test_validation($underflow, "underflow"); ?> ---EXPECT-- -bool(true) -bool(false) -bool(true) +--EXPECTF-- +max filtered: int(%d) +max is_long: bool(true) +max equal: bool(true) +overflow filtered: bool(false) +overflow is_long: bool(false) +overflow equal: bool(false) +min filtered: int(-%d) +min is_long: bool(true) +min equal: bool(true) +underflow filtered: bool(false) +underflow is_long: bool(false) +underflow equal: bool(false) Previous Comments: ------------------------------------------------------------------------ [2010-02-23 13:04:48] [email protected] See also bug #51008 ------------------------------------------------------------------------ [2010-02-20 20:56:44] [email protected] Further investigation revealed that the bug occurs with gcc 4.4 and optimisation -02. Without optimisation the code still works. ------------------------------------------------------------------------ [2010-02-11 23:31:02] geissert at debian dot org Description: ------------ The filter fails to detect an integer overflow and passes the FILTER_VALIDATE_INT test. The problem is caused because php_filter_parse_int uses a long to detect the overflow, which of course doesn't have the same size of an integer. This can be fixed by making ctx_value an integer in both php_filter_parse_int and php_filter_int (and for correctness, not setting Z_TYPE_P(value) to IS_LONG). Reproduce code: --------------- // the current test: $s = sprintf("%d", PHP_INT_MAX); var_dump(is_long(filter_var($s, FILTER_VALIDATE_INT))); $s = sprintf("%.0f", PHP_INT_MAX+1); var_dump(filter_var($s, FILTER_VALIDATE_INT)); $s = sprintf("%d", -PHP_INT_MAX); var_dump(is_long(filter_var($s, FILTER_VALIDATE_INT))); Expected result: ---------------- bool(true) bool(false) bool(true) Actual result: -------------- bool(true) int(-2147483648) bool(true) ------------------------------------------------------------------------ -- Edit this bug report at http://bugs.php.net/?id=51023&edit=1
