Newsgroups: php.internals Path: news.php.net Xref: news.php.net php.internals:47058 Return-Path: Mailing-List: contact internals-help@lists.php.net; run by ezmlm Delivered-To: mailing list internals@lists.php.net Received: (qmail 3012 invoked from network); 26 Feb 2010 23:39:04 -0000 Received: from unknown (HELO lists.php.net) (127.0.0.1) by localhost with SMTP; 26 Feb 2010 23:39:04 -0000 Authentication-Results: pb1.pair.com smtp.mail=sean@seanius.net; spf=permerror; sender-id=unknown Authentication-Results: pb1.pair.com header.from=seanius@debian.org; sender-id=unknown Received-SPF: error (pb1.pair.com: domain seanius.net from 66.93.22.232 cause and error) X-PHP-List-Original-Sender: sean@seanius.net X-Host-Fingerprint: 66.93.22.232 cobija.connexer.com Received: from [66.93.22.232] ([66.93.22.232:56769] helo=cobija.connexer.com) by pb1.pair.com (ecelerity 2.1.1.9-wez r(12769M)) with ESMTP id 85/00-02681-59B588B4 for ; Fri, 26 Feb 2010 18:39:01 -0500 Received: from rangda.stickybit.se (h-234-204.A189.priv.bahnhof.se [81.170.234.204]) by cobija.connexer.com (Postfix) with ESMTPA id 4E2E830032 for ; Fri, 26 Feb 2010 18:38:58 -0500 (EST) Received: by rangda.stickybit.se (Postfix, from userid 1000) id 3BB2DFEDA; Sat, 27 Feb 2010 00:38:55 +0100 (CET) Date: Sat, 27 Feb 2010 00:38:55 +0100 To: internals@lists.php.net Message-ID: <20100226233854.GA5002@rangda.stickybit.se> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.20 (2009-06-14) Subject: Zend/tests/bug45877.phpt, #51008, and signed integer overflows From: seanius@debian.org (sean finney) 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-- ---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