[Request][Discussion] Double value as array key improvement

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 написал(а):

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 andrew.nester.dev@gmail.com
wrote:

13 авг. 2017 г., в 21:39, Andrew Nester andrew.nester.dev@gmail.com
написал(а):

11 авг. 2017 г., в 15:53, Andrew Nester newaltgroup@bk.ru написал(а):

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