[2010-02-20 20:56 UTC] geissert@php.net

Further investigation revealed that the bug occurs with gcc 4.4 and optimisation -02. Without optimisation the code still works.

 [2010-02-25 21:53 UTC] seanius at debian dot org

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 <seanius@debian.org>
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)

 [2010-03-06 19:56 UTC] geissert@php.net

This bug has been fixed in SVN.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.
 
Thank you for the report, and for helping us make PHP better.