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

2012-07-14 Thread Nuno Lopes 
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 a...@php.net wrote:

Commit:b383ddf1e5175abf1d000e887961fdcebae646a0
Author:Anatoliy Belsky a...@php.net 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

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

2012-07-12 Thread Anatoliy Belsky 
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 a...@php.net wrote:
 Commit:b383ddf1e5175abf1d000e887961fdcebae646a0
 Author:Anatoliy Belsky a...@php.net 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 */

[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

2012-07-11 Thread Anatoliy Belsky 
Commit:b383ddf1e5175abf1d000e887961fdcebae646a0
Author:Anatoliy Belsky a...@php.net 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--
+?php
+
+$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
+++ b/ext/spl/tests/bug62477_2.phpt
@@ -0,0 +1,12 @@
+--TEST--
+Bug #62477 LimitIterator int overflow when float is passed (2)
+--FILE--
+?php
+
+$it = new LimitIterator(new ArrayIterator(array(42)), 0, 1000);
+--EXPECTF--
+Fatal error: Uncaught exception 'OutOfRangeException' with message 'count 
param must be of type int' in %sbug62477_2.php:%d
+Stack trace:
+#0 %sbug62477_2.php(%d): LimitIterator-__construct(Object(ArrayIterator), 0, 
%f)
+#1 {main}
+  thrown in %sbug62477_2.php on line %d


--
PHP CVS Mailing List (http://www.php.net/)
To

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

2012-07-11 Thread 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 a...@php.net wrote:
 Commit:b383ddf1e5175abf1d000e887961fdcebae646a0
 Author:Anatoliy Belsky a...@php.net 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--
 +?php
 +
 +$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
 +++ b/ext/spl/tests/bug62477_2.phpt
 @@ -0,0 +1,12 @@
 +--TEST--
 +Bug #62477 LimitIterator int overflow when float is

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

2012-07-11 Thread Stas Malyshev 
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