Welcome! Log In Create A New Profile

Advanced

[PHP-DEV] [Request][Discussion] Double value as array key improvement

Posted by Andrew Nester 
Hello everyone!

I was working on following request https://bugs.php.net/bug.php?id=75053 https://bugs.php.net/bug.php?id=75053 which resulted in following pull request https://github.com/php/php-src/pull/2676 https://github.com/php/php-src/pull/2676

The problem here is following: when we’re using large numbers as array index when adding new elements it could overwrite already existing value.
Assume we have 2 indexes 5076964154930102272 and 999999999999999999999999999999 with different value set for them.

Because 999999999999999999999999999999 is larger than maximum long int number for 64-bit systems, it will be converted to double. (corresponding code here https://github.com/php/php-src/blob/master/Zend/zend_language_scanner.l#L1648 <https://github.com/php/php-src/blob/master/Zend/zend_language_scanner.l#L1648>;)
But when double value is used as array indexes, it is converted to long integer. (f.e., code is here https://github.com/php/php-src/blob/master/Zend/zend_execute.c#L1573 <https://github.com/php/php-src/blob/master/Zend/zend_execute.c#L1573>;)
At this case it causes overflow and we’ve got index equal to 5076964154930102272 and as a result - we’re overwriting previously set value.

My suggestion is following:
1) when double key is less than maximum possible long integer - convert it to integer
2) if it’s larger - convert it to string.

That’s what implemented in proposed PR.

Another possible option is just to throw warning in this case (proposed by Nikita Popov)

I would happy to hear any feedback and suggestions about this solution.
Thanks!
> On Aug 11, 2017, at 2:10 PM, Andrew Nester <newaltgroup@bk.ru> wrote:
>
> Hello everyone!
>
> I was working on following request https://bugs.php.net/bug.php?id=75053 https://bugs.php.net/bug.php?id=75053 which resulted in following pull request https://github.com/php/php-src/pull/2676 https://github.com/php/php-src/pull/2676
>
> The problem here is following: when we’re using large numbers as array index when adding new elements it could overwrite already existing value.
> Assume we have 2 indexes 5076964154930102272 and 999999999999999999999999999999 with different value set for them.
>
> Because 999999999999999999999999999999 is larger than maximum long int number for 64-bit systems, it will be converted to double. (corresponding code here https://github.com/php/php-src/blob/master/Zend/zend_language_scanner.l#L1648 <https://github.com/php/php-src/blob/master/Zend/zend_language_scanner.l#L1648>;)
> But when double value is used as array indexes, it is converted to long integer. (f.e., code is here https://github.com/php/php-src/blob/master/Zend/zend_execute.c#L1573 <https://github.com/php/php-src/blob/master/Zend/zend_execute.c#L1573>;)
> At this case it causes overflow and we’ve got index equal to 5076964154930102272 and as a result - we’re overwriting previously set value.
>
> My suggestion is following:
> 1) when double key is less than maximum possible long integer - convert it to integer
> 2) if it’s larger - convert it to string.
>
> That’s what implemented in proposed PR.
>
> Another possible option is just to throw warning in this case (proposed by Nikita Popov)
>
> I would happy to hear any feedback and suggestions about this solution.
> Thanks!

Here is the alternative solution which emits E_WARNING in case of integer array index overflow.
https://github.com/php/php-src/pull/2677
> 11 авг. 2017 г., в 15:53, Andrew Nester <newaltgroup@bk.ru> написал(а):
>
>
>> On Aug 11, 2017, at 2:10 PM, Andrew Nester <newaltgroup@bk.ru> wrote:
>>
>> Hello everyone!
>>
>> I was working on following request https://bugs.php.net/bug.php?id=75053 which resulted in following pull request https://github.com/php/php-src/pull/2676
>>
>> The problem here is following: when we’re using large numbers as array index when adding new elements it could overwrite already existing value.
>> Assume we have 2 indexes 5076964154930102272 and 999999999999999999999999999999 with different value set for them.
>>
>> Because 999999999999999999999999999999 is larger than maximum long int number for 64-bit systems, it will be converted to double. (corresponding code here https://github.com/php/php-src/blob/master/Zend/zend_language_scanner.l#L1648)
>> But when double value is used as array indexes, it is converted to long integer. (f.e., code is here https://github.com/php/php-src/blob/master/Zend/zend_execute.c#L1573)
>> At this case it causes overflow and we’ve got index equal to 5076964154930102272 and as a result - we’re overwriting previously set value.
>>
>> My suggestion is following:
>> 1) when double key is less than maximum possible long integer - convert it to integer
>> 2) if it’s larger - convert it to string.
>>
>> That’s what implemented in proposed PR.
>>
>> Another possible option is just to throw warning in this case (proposed by Nikita Popov)
>>
>> I would happy to hear any feedback and suggestions about this solution.
>> Thanks!
>
> Here is the alternative solution which emits E_WARNING in case of integer array index overflow.
> https://github.com/php/php-src/pull/2677

Bumping the discussion because not everyone could see my previous email due to wrong configuration on my side, sorry.

Cheers,
Andrew
> 11 авг. 2017 г., в 15:53, Andrew Nester <newaltgroup@bk.ru> написал(а):
>
>
>> On Aug 11, 2017, at 2:10 PM, Andrew Nester <newaltgroup@bk.ru> wrote:
>>
>> Hello everyone!
>>
>> I was working on following request https://bugs.php.net/bug.php?id=75053 which resulted in following pull request https://github.com/php/php-src/pull/2676
>>
>> The problem here is following: when we’re using large numbers as array index when adding new elements it could overwrite already existing value.
>> Assume we have 2 indexes 5076964154930102272 and 999999999999999999999999999999 with different value set for them.
>>
>> Because 999999999999999999999999999999 is larger than maximum long int number for 64-bit systems, it will be converted to double. (corresponding code here https://github.com/php/php-src/blob/master/Zend/zend_language_scanner.l#L1648)
>> But when double value is used as array indexes, it is converted to long integer. (f.e., code is here https://github.com/php/php-src/blob/master/Zend/zend_execute.c#L1573)
>> At this case it causes overflow and we’ve got index equal to 5076964154930102272 and as a result - we’re overwriting previously set value.
>>
>> My suggestion is following:
>> 1) when double key is less than maximum possible long integer - convert it to integer
>> 2) if it’s larger - convert it to string.
>>
>> That’s what implemented in proposed PR.
>>
>> Another possible option is just to throw warning in this case (proposed by Nikita Popov)
>>
>> I would happy to hear any feedback and suggestions about this solution.
>> Thanks!
>
> Here is the alternative solution which emits E_WARNING in case of integer array index overflow.
> https://github.com/php/php-src/pull/2677

My preferred solution is 2nd one (emitting warning) as it more obvious for users, doesn't break previous behaviour.

Cheers,
Andrew
On 13.08.2017 at 20:39, Andrew Nester wrote:

> 11 авг. 2017 г., в 15:53, Andrew Nester <newaltgroup@bk.ru> написал(а):
>
>> Here is the alternative solution which emits E_WARNING in case of integer array index overflow.
>> https://github.com/php/php-src/pull/2677
>
> My preferred solution is 2nd one (emitting warning) as it more obvious for users, doesn't break previous behaviour.

+1

--
Christoph M. Becker


--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
> 13 авг. 2017 г., в 21:39, Andrew Nester <andrew.nester.dev@gmail..com> написал(а):
>
>
>
>> 11 авг. 2017 г., в 15:53, Andrew Nester <newaltgroup@bk.ru> написал(а):
>>
>>
>>> On Aug 11, 2017, at 2:10 PM, Andrew Nester <newaltgroup@bk.ru> wrote:
>>>
>>> Hello everyone!
>>>
>>> I was working on following request https://bugs.php.net/bug.php?id=75053 which resulted in following pull request https://github.com/php/php-src/pull/2676
>>>
>>> The problem here is following: when we’re using large numbers as array index when adding new elements it could overwrite already existing value.
>>> Assume we have 2 indexes 5076964154930102272 and 999999999999999999999999999999 with different value set for them.
>>>
>>> Because 999999999999999999999999999999 is larger than maximum long int number for 64-bit systems, it will be converted to double. (corresponding code here https://github.com/php/php-src/blob/master/Zend/zend_language_scanner..l#L1648)
>>> But when double value is used as array indexes, it is converted to long integer. (f.e., code is here https://github.com/php/php-src/blob/master/Zend/zend_execute.c#L1573)
>>> At this case it causes overflow and we’ve got index equal to 5076964154930102272 and as a result - we’re overwriting previously set value.
>>>
>>> My suggestion is following:
>>> 1) when double key is less than maximum possible long integer - convert it to integer
>>> 2) if it’s larger - convert it to string.
>>>
>>> That’s what implemented in proposed PR.
>>>
>>> Another possible option is just to throw warning in this case (proposed by Nikita Popov)
>>>
>>> I would happy to hear any feedback and suggestions about this solution.
>>> Thanks!
>>
>> Here is the alternative solution which emits E_WARNING in case of integer array index overflow.
>> https://github.com/php/php-src/pull/2677
>
> My preferred solution is 2nd one (emitting warning) as it more obvious for users, doesn't break previous behaviour.
>
> Cheers,
> Andrew

Hello internals!

I was working on solution for the problem of double to int conversion for array indices and would like to create an RFC for proposed solution - emitting warning when integer overflow happens during double to int conversion.

Does it look like good idea?

Thanks!
Just reposting to internals:

David wrote:
> If the key is user dependent (eg. Input from form), could it causes a
warning too, right? I think that it should be considered a BC.

Andrew Nester wrote:
> Yes, warning will be emitted in this case as well but actual index will
remain the same.
> I could agree that it's minor BC break as it could affect users


2017-08-17 12:03 GMT-03:00 Andrew Nester <andrew.nester.dev@gmail.com>:

>
>
> > 13 авг. 2017 г., в 21:39, Andrew Nester <andrew.nester.dev@gmail.com>
> написал(а):
> >
> >
> >
> >> 11 авг. 2017 г., в 15:53, Andrew Nester <newaltgroup@bk.ru> написал(а):
> >>
> >>
> >>> On Aug 11, 2017, at 2:10 PM, Andrew Nester <newaltgroup@bk.ru> wrote:
> >>>
> >>> Hello everyone!
> >>>
> >>> I was working on following request https://bugs.php.net/bug.php?
> id=75053 which resulted in following pull request
> https://github.com/php/php-src/pull/2676
> >>>
> >>> The problem here is following: when we’re using large numbers as array
> index when adding new elements it could overwrite already existing value.
> >>> Assume we have 2 indexes 5076964154930102272 and
> 999999999999999999999999999999 with different value set for them.
> >>>
> >>> Because 999999999999999999999999999999 is larger than maximum long int
> number for 64-bit systems, it will be converted to double. (corresponding
> code here https://github.com/php/php-src/blob/master/Zend/zend_
> language_scanner.l#L1648)
> >>> But when double value is used as array indexes, it is converted to
> long integer. (f.e., code is here https://github.com/php/php-
> src/blob/master/Zend/zend_execute.c#L1573)
> >>> At this case it causes overflow and we’ve got index equal to
> 5076964154930102272 and as a result - we’re overwriting previously set
> value.
> >>>
> >>> My suggestion is following:
> >>> 1) when double key is less than maximum possible long integer -
> convert it to integer
> >>> 2) if it’s larger - convert it to string.
> >>>
> >>> That’s what implemented in proposed PR.
> >>>
> >>> Another possible option is just to throw warning in this case
> (proposed by Nikita Popov)
> >>>
> >>> I would happy to hear any feedback and suggestions about this solution.
> >>> Thanks!
> >>
> >> Here is the alternative solution which emits E_WARNING in case of
> integer array index overflow.
> >> https://github.com/php/php-src/pull/2677
> >
> > My preferred solution is 2nd one (emitting warning) as it more obvious
> for users, doesn't break previous behaviour.
> >
> > Cheers,
> > Andrew
>
> Hello internals!
>
> I was working on solution for the problem of double to int conversion for
> array indices and would like to create an RFC for proposed solution -
> emitting warning when integer overflow happens during double to int
> conversion.
>
> Does it look like good idea?
>
> Thanks!




--
David Rodrigues
Sorry, only registered users may post in this forum.

Click here to login