[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
  100256
August 19, 2017 13:30 nikita.ppv@gmail.com (Nikita Popov)
On Thu, Aug 17, 2017 at 5:03 PM, Andrew Nester dev@gmail.com>
wrote:

> > > > 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!
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
  100270
August 21, 2017 15:04 ajf@ajf.me (Andrea Faulds)
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/
  100278
August 22, 2017 08:48 andrew.nester.dev@gmail.com (Andrew Nester)
> On Aug 19, 2017, at 4:30 PM, Nikita Popov ppv@gmail.com> wrote: > > On Thu, Aug 17, 2017 at 5:03 PM, Andrew Nester dev@gmail.com <mailto:andrew.nester.dev@gmail.com>> wrote: > > > > 13 авг. 2017 г., в 21:39, Andrew Nester dev@gmail.com <mailto:andrew.nester.dev@gmail.com>> написал(а): > > > > > > > >> 11 авг. 2017 г., в 15:53, Andrew Nester <newaltgroup@bk.ru <mailto:newaltgroup@bk.ru>> написал(а): > >> > >> > >>> On Aug 11, 2017, at 2:10 PM, Andrew Nester <newaltgroup@bk.ru <mailto: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 <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 >
Hey Internals! I am planning to create RFC for this change but my account (andrewnesterdev) doesn’t have enough permissions. Could please someone grant it that I could start creating it? Thanks!
  100330
August 31, 2017 00:57 ajf@ajf.me (Andrea Faulds)
Hi there,

Andrew Nester wrote:
> > > 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! >
It's certainly an improvement over silently truncate-wrapping like at present. But one question I might have is whether there's any benefit to it just being a warning rather than an Error. Surely nobody would rely on this behaviour? I can't imagine a use-case where it doing % 2^64 on your indices is helpful. Who would want the code execution to continue beyond that point? Will someone want to silence this error? Compare how the `int` type declaration on userland functions handles floats like these: it throws a TypeError. Internal functions *do* throw a warning, but they don't execute their body and return NULL instead (they do throw a TypeError in strict mode). If you threw some kind of Error instead, I can't imagine it causing backwards-compatibility problems in practice, and at least personally, I think it would be the better choice. But I wrote the RFC that made passing floats > PHP_INT_MAX or < PHP_INT_MIN to functions with integer parameters produce those errors, so of course I'd say that… Anyway, thanks for bringing this up! It's something that I've wanted to fix since a long time ago and had forgotten about. -- Andrea Faulds https://ajf.me/