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 <[email protected]> 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 <[email protected]> написал(а):
>
>
>> On Aug 11, 2017, at 2:10 PM, Andrew Nester <[email protected]> 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 <[email protected]> написал(а):
>
>
>> On Aug 11, 2017, at 2:10 PM, Andrew Nester <[email protected]> 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 <[email protected]> написал(а):
>
>> 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 <[email protected].com> написал(а):
>
>
>
>> 11 авг. 2017 г., в 15:53, Andrew Nester <[email protected]> написал(а):
>>
>>
>>> On Aug 11, 2017, at 2:10 PM, Andrew Nester <[email protected]> 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 <[email protected]>:

>
>
> > 13 авг. 2017 г., в 21:39, Andrew Nester <[email protected]>
> написал(а):
> >
> >
> >
> >> 11 авг. 2017 г., в 15:53, Andrew Nester <[email protected]> написал(а):
> >>
> >>
> >>> On Aug 11, 2017, at 2:10 PM, Andrew Nester <[email protected]> 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
On Thu, Aug 17, 2017 at 5:03 PM, Andrew Nester <[email protected]>
wrote:

>
>
> > 13 авг. 2017 г., в 21:39, Andrew Nester <[email protected]>
> написал(а):
> >
> >
> >
> >> 11 авг. 2017 г., в 15:53, Andrew Nester <[email protected]> написал(а):
> >>
> >>
> >>> On Aug 11, 2017, at 2:10 PM, Andrew Nester <[email protected]> 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!


Sounds good to me. Something you might want to consider is to also throw a
warning if the floating point number is not an exact integer. For example
allow a silent cast of 42.0 to 42, but throw a warning if 42.5 is used as
an index (or worse, 42.999999999, in which case it likely isn't doing what
the programmer thinks it's doing).

Nikita
Hi everyone,

Nikita Popov wrote:
>
> Sounds good to me. Something you might want to consider is to also throw a
> warning if the floating point number is not an exact integer. For example
> allow a silent cast of 42.0 to 42, but throw a warning if 42.5 is used as
> an index (or worse, 42.999999999, in which case it likely isn't doing what
> the programmer thinks it's doing).

I wonder about whether this is a good idea. I've previously suggested
something like this myself, but at the present time, I'd be more
concerned about consistent behaviour. We do silent casts from floats to
integers in other places in PHP. Array indices aren't that special.
Moreover, this implicit truncation behaviour is useful in some cases.

--
Andrea Faulds
https://ajf.me/

--
PHP Internals - PHP Runtime Development Mailing List
To unsubscribe, visit: http://www.php.net/unsub.php
Sorry, only registered users may post in this forum.

Click here to login