[PHP-CVS] com php-src: Fixed bug #62477 LimitIterator int overflow: ext/spl/spl_iterators.c ext/spl/spl_iterators.h ext/spl/tests/bug62477_1.phpt ext/spl/tests/bug62477_2.phpt
Commit:b383ddf1e5175abf1d000e887961fdcebae646a0 Author:Anatoliy Belsky Wed, 11 Jul 2012 22:25:31 +0200 Parents: bcf5853eaa8b8be793d4a1bd325eaea68cfe57bb Branches: PHP-5.3 PHP-5.4 master Link: http://git.php.net/?p=php-src.git;a=commitdiff;h=b383ddf1e5175abf1d000e887961fdcebae646a0 Log: Fixed bug #62477 LimitIterator int overflow Bugs: https://bugs.php.net/62477 Changed paths: M ext/spl/spl_iterators.c M ext/spl/spl_iterators.h A ext/spl/tests/bug62477_1.phpt A ext/spl/tests/bug62477_2.phpt Diff: diff --git a/ext/spl/spl_iterators.c b/ext/spl/spl_iterators.c index eecd483..1cbb2e4 100755 --- a/ext/spl/spl_iterators.c +++ b/ext/spl/spl_iterators.c @@ -1380,12 +1380,31 @@ static spl_dual_it_object* spl_dual_it_construct(INTERNAL_FUNCTION_PARAMETERS, z intern->dit_type = dit_type; switch (dit_type) { case DIT_LimitIterator: { + zval *tmp_offset, *tmp_count; intern->u.limit.offset = 0; /* start at beginning */ intern->u.limit.count = -1; /* get all */ - if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "O|ll", &zobject, ce_inner, &intern->u.limit.offset, &intern->u.limit.count) == FAILURE) { + if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "O|zz", &zobject, ce_inner, &tmp_offset, &tmp_count) == FAILURE) { zend_restore_error_handling(&error_handling TSRMLS_CC); return NULL; } + if (tmp_offset && Z_TYPE_P(tmp_offset) != IS_NULL) { + if (Z_TYPE_P(tmp_offset) != IS_LONG) { + zend_throw_exception(spl_ce_OutOfRangeException, "offset param must be of type int", 0 TSRMLS_CC); + zend_restore_error_handling(&error_handling TSRMLS_CC); + return NULL; + } else { + intern->u.limit.offset = Z_LVAL_P(tmp_offset); + } + } + if (tmp_count && Z_TYPE_P(tmp_count) != IS_NULL) { + if (Z_TYPE_P(tmp_count) != IS_LONG) { + zend_throw_exception(spl_ce_OutOfRangeException, "count param must be of type int", 0 TSRMLS_CC); + zend_restore_error_handling(&error_handling TSRMLS_CC); + return NULL; + } else { + intern->u.limit.count = Z_LVAL_P(tmp_count); + } + } if (intern->u.limit.offset < 0) { zend_throw_exception(spl_ce_OutOfRangeException, "Parameter offset must be >= 0", 0 TSRMLS_CC); zend_restore_error_handling(&error_handling TSRMLS_CC); diff --git a/ext/spl/spl_iterators.h b/ext/spl/spl_iterators.h index 525a25c..9494b26 100755 --- a/ext/spl/spl_iterators.h +++ b/ext/spl/spl_iterators.h @@ -128,7 +128,7 @@ typedef struct _spl_dual_it_object { uint str_key_len; ulongint_key; int key_type; /* HASH_KEY_IS_STRING or HASH_KEY_IS_LONG */ - int pos; + long pos; } current; dual_it_type dit_type; union { diff --git a/ext/spl/tests/bug62477_1.phpt b/ext/spl/tests/bug62477_1.phpt new file mode 100644 index 000..1b768a7 --- /dev/null +++ b/ext/spl/tests/bug62477_1.phpt @@ -0,0 +1,12 @@ +--TEST-- +Bug #62477 LimitIterator int overflow when float is passed (1) +--FILE-- +__construct(Object(ArrayIterator), %f) +#1 {main} + thrown in %sbug62477_1.php on line %d diff --git a/ext/spl/tests/bug62477_2.phpt b/ext/spl/tests/bug62477_2.phpt new file mode 100644 index 000..aa3468a --- /dev/null +++ b/ext/spl/tests/bug62477_2.phpt @@ -0,0 +1,12 @@ +--TEST-- +Bug #62477 LimitIterator int overflow when float is passed (2) +--FILE-- +__construct(Object(ArrayIterator), 0, %f) +#1 {main} + thrown in %sbug62477_2.php on line %d -- PHP CVS Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-CVS] com php-src: Fixed bug #62477 LimitIterator int overflow: ext/spl/spl_iterators.c ext/spl/spl_iterators.h ext/spl/tests/bug62477_1.phpt ext/spl/tests/bug62477_2.phpt
Hi Anatoliy! I'm not sure that this change is right. As it is now one could no longer pass in "123" (as a string) or 123.0 (as a float) as limit/offset. This goes against usual PHP semantics. Also I'm not exactly sure what the problem was in the first place. Nikita On Wed, Jul 11, 2012 at 10:25 PM, Anatoliy Belsky wrote: > Commit:b383ddf1e5175abf1d000e887961fdcebae646a0 > Author:Anatoliy Belsky Wed, 11 Jul 2012 22:25:31 > +0200 > Parents: bcf5853eaa8b8be793d4a1bd325eaea68cfe57bb > Branches: PHP-5.3 PHP-5.4 master > > Link: > http://git.php.net/?p=php-src.git;a=commitdiff;h=b383ddf1e5175abf1d000e887961fdcebae646a0 > > Log: > Fixed bug #62477 LimitIterator int overflow > > Bugs: > https://bugs.php.net/62477 > > Changed paths: > M ext/spl/spl_iterators.c > M ext/spl/spl_iterators.h > A ext/spl/tests/bug62477_1.phpt > A ext/spl/tests/bug62477_2.phpt > > > Diff: > diff --git a/ext/spl/spl_iterators.c b/ext/spl/spl_iterators.c > index eecd483..1cbb2e4 100755 > --- a/ext/spl/spl_iterators.c > +++ b/ext/spl/spl_iterators.c > @@ -1380,12 +1380,31 @@ static spl_dual_it_object* > spl_dual_it_construct(INTERNAL_FUNCTION_PARAMETERS, z > intern->dit_type = dit_type; > switch (dit_type) { > case DIT_LimitIterator: { > + zval *tmp_offset, *tmp_count; > intern->u.limit.offset = 0; /* start at beginning */ > intern->u.limit.count = -1; /* get all */ > - if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, > "O|ll", &zobject, ce_inner, &intern->u.limit.offset, &intern->u.limit.count) > == FAILURE) { > + if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, > "O|zz", &zobject, ce_inner, &tmp_offset, &tmp_count) == FAILURE) { > zend_restore_error_handling(&error_handling > TSRMLS_CC); > return NULL; > } > + if (tmp_offset && Z_TYPE_P(tmp_offset) != IS_NULL) { > + if (Z_TYPE_P(tmp_offset) != IS_LONG) { > + > zend_throw_exception(spl_ce_OutOfRangeException, "offset param must be of > type int", 0 TSRMLS_CC); > + > zend_restore_error_handling(&error_handling TSRMLS_CC); > + return NULL; > + } else { > + intern->u.limit.offset = > Z_LVAL_P(tmp_offset); > + } > + } > + if (tmp_count && Z_TYPE_P(tmp_count) != IS_NULL) { > + if (Z_TYPE_P(tmp_count) != IS_LONG) { > + > zend_throw_exception(spl_ce_OutOfRangeException, "count param must be of type > int", 0 TSRMLS_CC); > + > zend_restore_error_handling(&error_handling TSRMLS_CC); > + return NULL; > + } else { > + intern->u.limit.count = > Z_LVAL_P(tmp_count); > + } > + } > if (intern->u.limit.offset < 0) { > > zend_throw_exception(spl_ce_OutOfRangeException, "Parameter offset must be >= > 0", 0 TSRMLS_CC); > zend_restore_error_handling(&error_handling > TSRMLS_CC); > diff --git a/ext/spl/spl_iterators.h b/ext/spl/spl_iterators.h > index 525a25c..9494b26 100755 > --- a/ext/spl/spl_iterators.h > +++ b/ext/spl/spl_iterators.h > @@ -128,7 +128,7 @@ typedef struct _spl_dual_it_object { > uint str_key_len; > ulongint_key; > int key_type; /* HASH_KEY_IS_STRING or > HASH_KEY_IS_LONG */ > - int pos; > + long pos; > } current; > dual_it_type dit_type; > union { > diff --git a/ext/spl/tests/bug62477_1.phpt b/ext/spl/tests/bug62477_1.phpt > new file mode 100644 > index 000..1b768a7 > --- /dev/null > +++ b/ext/spl/tests/bug62477_1.phpt > @@ -0,0 +1,12 @@ > +--TEST-- > +Bug #62477 LimitIterator int overflow when float is passed (1) > +--FILE-- > + + > +$it = new LimitIterator(new ArrayIterator(array(42)), 1000); > +--EXPECTF-- > +Fatal error: Uncaught exception 'OutOfRangeException' with message 'offset > param must be of type int' in %sbug62477_1.php:%d > +Stack trace: > +#0 %sbug62477_1.php(%d): LimitIterator->__construct(Object(ArrayIterator), > %f) > +#1 {main} > + thrown in %sbug62477_1.php on line %d > diff --git a/ext/spl/tests/bug62477_2.phpt b/ext/spl/tests/bug62477_2.phpt > new file mode 100644 > index 000..aa3468a > --- /dev/null > +++
Re: [PHP-CVS] com php-src: Fixed bug #62477 LimitIterator int overflow: ext/spl/spl_iterators.c ext/spl/spl_iterators.h ext/spl/tests/bug62477_1.phpt ext/spl/tests/bug62477_2.phpt
Hi! > I'm not sure that this change is right. As it is now one could no > longer pass in "123" (as a string) or 123.0 (as a float) as > limit/offset. This goes against usual PHP semantics. I agree, this: >> - if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, >> "O|ll", &zobject, ce_inner, &intern->u.limit.offset, &intern->u.limit.count) >> == FAILURE) { >> + if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, >> "O|zz", &zobject, ce_inner, &tmp_offset, &tmp_count) == FAILURE) { Is wrong, especially added in 5.3/5.4. I'm getting complaints here about changes that only theoretically affect some future extensions, but here we got API change in stable version that would deny things like "1" in numeric context, which is completely contrary to how PHP does things. Please revert this change. -- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/ (408)454-6900 ext. 227 -- PHP CVS Mailing List (http://www.php.net/) To unsubscribe, visit: http://www.php.net/unsub.php
Re: [PHP-CVS] com php-src: Fixed bug #62477 LimitIterator int overflow: ext/spl/spl_iterators.c ext/spl/spl_iterators.h ext/spl/tests/bug62477_1.phpt ext/spl/tests/bug62477_2.phpt
Hi guys, as it stands the ticket, the user has implemented an iterator which easily accepts floats for seek and other iterator methods. Normal iterator can only work with integers as all the storage for positions is long or int. As result a statement like this new LimitIterator(new ArrayIterator(array(42)), 1000); will just convert that big float to some integer, so the original iterator wouldn't work properly. This is of course not the case with iterators implemented internally in SPL as they only work with long (ulong for instance with the ArrayIterator). But any user space iterators might break this. Just like in the ticket the iterator itself works as expected and gives 100 101 102 Where passing it to the LimitIterator gives back 1410065408 1410065409 1410065410 Also please consider the fact that the documentation states both start and offset params of the LimitIterator as int. I'm on the way to revert this, but the issue stays. May be the patch should be just extended to check if it's a numeric string and try to convert it. The same for a float - check if it overflows the integer range. Or how would you solve this? Regards Anatoliy Am Mi, 11.07.2012, 23:25 schrieb Nikita Popov: > Hi Anatoliy! > > I'm not sure that this change is right. As it is now one could no > longer pass in "123" (as a string) or 123.0 (as a float) as > limit/offset. This goes against usual PHP semantics. > > Also I'm not exactly sure what the problem was in the first place. > > Nikita > > On Wed, Jul 11, 2012 at 10:25 PM, Anatoliy Belsky wrote: >> Commit:b383ddf1e5175abf1d000e887961fdcebae646a0 >> Author:Anatoliy Belsky Wed, 11 Jul 2012 >> 22:25:31 +0200 >> Parents: bcf5853eaa8b8be793d4a1bd325eaea68cfe57bb >> Branches: PHP-5.3 PHP-5.4 master >> >> Link: >> http://git.php.net/?p=php-src.git;a=commitdiff;h=b383ddf1e5175abf1d000e887961fdcebae646a0 >> >> Log: >> Fixed bug #62477 LimitIterator int overflow >> >> Bugs: >> https://bugs.php.net/62477 >> >> Changed paths: >> M ext/spl/spl_iterators.c >> M ext/spl/spl_iterators.h >> A ext/spl/tests/bug62477_1.phpt >> A ext/spl/tests/bug62477_2.phpt >> >> >> Diff: >> diff --git a/ext/spl/spl_iterators.c b/ext/spl/spl_iterators.c >> index eecd483..1cbb2e4 100755 >> --- a/ext/spl/spl_iterators.c >> +++ b/ext/spl/spl_iterators.c >> @@ -1380,12 +1380,31 @@ static spl_dual_it_object* >> spl_dual_it_construct(INTERNAL_FUNCTION_PARAMETERS, z >> intern->dit_type = dit_type; >> switch (dit_type) { >> case DIT_LimitIterator: { >> + zval *tmp_offset, *tmp_count; >> intern->u.limit.offset = 0; /* start at >> beginning */ >> intern->u.limit.count = -1; /* get all */ >> - if (zend_parse_parameters(ZEND_NUM_ARGS() >> TSRMLS_CC, "O|ll", &zobject, ce_inner, &intern->u.limit.offset, >> &intern->u.limit.count) == FAILURE) { >> + if (zend_parse_parameters(ZEND_NUM_ARGS() >> TSRMLS_CC, "O|zz", &zobject, ce_inner, &tmp_offset, &tmp_count) == >> FAILURE) { >> zend_restore_error_handling(&error_handling >> TSRMLS_CC); >> return NULL; >> } >> + if (tmp_offset && Z_TYPE_P(tmp_offset) != >> IS_NULL) { >> + if (Z_TYPE_P(tmp_offset) != IS_LONG) { >> + >> zend_throw_exception(spl_ce_OutOfRangeException, "offset param must be >> of type int", 0 TSRMLS_CC); >> + >> zend_restore_error_handling(&error_handling TSRMLS_CC); >> + return NULL; >> + } else { >> + intern->u.limit.offset = >> Z_LVAL_P(tmp_offset); >> + } >> + } >> + if (tmp_count && Z_TYPE_P(tmp_count) != IS_NULL) >> { >> + if (Z_TYPE_P(tmp_count) != IS_LONG) { >> + >> zend_throw_exception(spl_ce_OutOfRangeException, "count param must be of >> type int", 0 TSRMLS_CC); >> + >> zend_restore_error_handling(&error_handling TSRMLS_CC); >> + return NULL; >> + } else { >> + intern->u.limit.count = >> Z_LVAL_P(tmp_count); >> + } >> + } >> if (intern->u.limit.offset < 0) { >> >> zend_throw_exception(spl_ce_OutOfRangeException, >> "Parameter offset must be >= 0", 0 >> TSRMLS_CC); >> zend_restore_error_handling(&error_handling >> TSRMLS_CC); >> diff --git a/ext/spl/spl_iterators.h b/ext/spl/spl_iterators.h >> index 525a25c..9494b26 100755 >> --- a/ext/spl/spl_iterators.h >> +++ b/ext/spl/spl_iterators.h >> @@ -128,7 +128,7 @@ typedef struct _spl_dual_it_object {
Re: [PHP-CVS] com php-src: Fixed bug #62477 LimitIterator int overflow: ext/spl/spl_iterators.c ext/spl/spl_iterators.h ext/spl/tests/bug62477_1.phpt ext/spl/tests/bug62477_2.phpt
Truncating the number is the right way to do it (as far as PHP semantics goes). It's consistent across all PHP functions that take integers as input. Nuno -Original Message- From: Anatoliy Belsky Sent: Thursday, July 12, 2012 8:50 AM To: Nikita Popov ; Stas Malyshev Cc: php-cvs@lists.php.net Subject: Re: [PHP-CVS] com php-src: Fixed bug #62477 LimitIterator int overflow: ext/spl/spl_iterators.c ext/spl/spl_iterators.h ext/spl/tests/bug62477_1.phpt ext/spl/tests/bug62477_2.phpt Hi guys, as it stands the ticket, the user has implemented an iterator which easily accepts floats for seek and other iterator methods. Normal iterator can only work with integers as all the storage for positions is long or int. As result a statement like this new LimitIterator(new ArrayIterator(array(42)), 1000); will just convert that big float to some integer, so the original iterator wouldn't work properly. This is of course not the case with iterators implemented internally in SPL as they only work with long (ulong for instance with the ArrayIterator). But any user space iterators might break this. Just like in the ticket the iterator itself works as expected and gives 100 101 102 Where passing it to the LimitIterator gives back 1410065408 1410065409 1410065410 Also please consider the fact that the documentation states both start and offset params of the LimitIterator as int. I'm on the way to revert this, but the issue stays. May be the patch should be just extended to check if it's a numeric string and try to convert it. The same for a float - check if it overflows the integer range. Or how would you solve this? Regards Anatoliy Am Mi, 11.07.2012, 23:25 schrieb Nikita Popov: Hi Anatoliy! I'm not sure that this change is right. As it is now one could no longer pass in "123" (as a string) or 123.0 (as a float) as limit/offset. This goes against usual PHP semantics. Also I'm not exactly sure what the problem was in the first place. Nikita On Wed, Jul 11, 2012 at 10:25 PM, Anatoliy Belsky wrote: Commit:b383ddf1e5175abf1d000e887961fdcebae646a0 Author:Anatoliy Belsky Wed, 11 Jul 2012 22:25:31 +0200 Parents: bcf5853eaa8b8be793d4a1bd325eaea68cfe57bb Branches: PHP-5.3 PHP-5.4 master Link: http://git.php.net/?p=php-src.git;a=commitdiff;h=b383ddf1e5175abf1d000e887961fdcebae646a0 Log: Fixed bug #62477 LimitIterator int overflow Bugs: https://bugs.php.net/62477 Changed paths: M ext/spl/spl_iterators.c M ext/spl/spl_iterators.h A ext/spl/tests/bug62477_1.phpt A ext/spl/tests/bug62477_2.phpt Diff: diff --git a/ext/spl/spl_iterators.c b/ext/spl/spl_iterators.c index eecd483..1cbb2e4 100755 --- a/ext/spl/spl_iterators.c +++ b/ext/spl/spl_iterators.c @@ -1380,12 +1380,31 @@ static spl_dual_it_object* spl_dual_it_construct(INTERNAL_FUNCTION_PARAMETERS, z intern->dit_type = dit_type; switch (dit_type) { case DIT_LimitIterator: { + zval *tmp_offset, *tmp_count; intern->u.limit.offset = 0; /* start at beginning */ intern->u.limit.count = -1; /* get all */ - if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "O|ll", &zobject, ce_inner, &intern->u.limit.offset, &intern->u.limit.count) == FAILURE) { + if (zend_parse_parameters(ZEND_NUM_ARGS() TSRMLS_CC, "O|zz", &zobject, ce_inner, &tmp_offset, &tmp_count) == FAILURE) { zend_restore_error_handling(&error_handling TSRMLS_CC); return NULL; } + if (tmp_offset && Z_TYPE_P(tmp_offset) != IS_NULL) { + if (Z_TYPE_P(tmp_offset) != IS_LONG) { + zend_throw_exception(spl_ce_OutOfRangeException, "offset param must be of type int", 0 TSRMLS_CC); + zend_restore_error_handling(&error_handling TSRMLS_CC); + return NULL; + } else { + intern->u.limit.offset = Z_LVAL_P(tmp_offset); + } + } + if (tmp_count && Z_TYPE_P(tmp_count) != IS_NULL) { + if (Z_TYPE_P(tmp_count) != IS_LONG) { + zend_throw_exception(spl_ce_OutOfRangeException, "count param must be of type int", 0 TSRMLS_CC); + zend_restore_error_handling(&error_handling TSRMLS_CC); + return NULL; + } else { + intern->u.limit.count = Z_LVAL_P(tmp_count); + } + } if (intern->u.limit.offset < 0) { zend_throw_exception(spl_ce_OutOfRangeException, "Paramet