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

  100180
August 11, 2017 11:10 newaltgroup@bk.ru (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!
  100182
August 11, 2017 12:53 newaltgroup@bk.ru (Andrew Nester)
> 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
  100204
August 13, 2017 18:34 newaltgroup@bk.ru (Andrew Nester)
> 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
  100205
August 13, 2017 18:39 andrew.nester.dev@gmail.com (Andrew Nester)
> 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
  100206
August 13, 2017 19:02 cmbecker69@gmx.de ("Christoph M. Becker")
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
  100246
August 17, 2017 15:03 andrew.nester.dev@gmail.com (Andrew Nester)
> 13 àâã. 2017 ã., â 21:39, 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!
  100248
August 17, 2017 17:01 david.proweb@gmail.com (David Rodrigues)
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 dev@gmail.com>:
> > > > 13 авг. 2017 г., в 21:39, 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