Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [RFC] ZPP Failure On Overflow »

[RFC] ZPP Failure On Overflow

php.internals

Andrew Faulds

11 years ago
Good evening again, Here’s a new RFC: https://wiki.php.net/rfc/zpp_fail_on_overflow Thoughts appreciated, as is help with the patch, though I can probably manage on my own. Thanks!
-- Andrea Faulds http://ajf.me/

Stas Malyshev

11 years ago
Hi!
> Good evening again, > > Here’s a new RFC: https://wiki.php.net/rfc/zpp_fail_on_overflow > > Thoughts appreciated, as is help with the patch, though I can probably manage on my own.
It would be nice to describe why this change is good. So far the motivation is "it is unintuitive" which is a fancy way of saying "I don't like it". Could you list which use cases this functionality improves, which real-life bugs it could fix, etc.? If this is necessary for your BigInt RFC which would not work without it for some reason (I have no idea if it is the case, but if it is) then please state so explicitly and describe why. That may also help to find alternatives in case somebody else sees any other solution that you may have missed. If there are some other arguments for it, please add them to the RFC. Right now it looks kind of thin. I personally don't have any reason to assume what you are proposing is better that what we have now, and BC break is a cost that always must be offset by something that is worth more. Especially a BC break in a form of "it worked before but now it fails" - this can break code in so many hard to catch ways, where you didn't actually care at the least if the function truncates the arg (common situation in proxy/glue libraries, etc. - they'd be completely fine with garbage in - garbage out) but need special code to handle situations where the function fails to run altogether.
-- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/

Andrew Faulds

11 years ago
On 24 Sep 2014, at 01:22, Stas Malyshev <smalyshev@sugarcrm.com> wrote:
> It would be nice to describe why this change is good. So far the > motivation is "it is unintuitive" which is a fancy way of saying "I > don't like it". Could you list which use cases this functionality > improves, which real-life bugs it could fix, etc.?
Basically, it would mean we fail safe rather than silently mangling data in the unusual case where some large float is passed to a function expecting an integer. There are actually quite a few bugs caused by truncation. Among the tests I have to update, one is for a date/time bug caused by a floating-point timestamp being truncated on 32-bit platforms resulting in a completely different date. Should this RFC pass, in these situations you just get an error rather than having your data mangled. If you want it to continue mangling it, that’s fine, (int) exists. If you want to handle it better, that’s also an option.
> If this is necessary for your BigInt RFC which would not work without it > for some reason (I have no idea if it is the case, but if it is) then > please state so explicitly and describe why. That may also help to find > alternatives in case somebody else sees any other solution that you may > have missed.
It’s not completely necessary, but I think bigints would make more sense with this RFC than without it. This RFC would make floats out of bounds cause an error when passed to functions expecting integers. Naturally, if this passes, the bigint patch would then do the same thing for bigints passed to functions which only support platform-native longs (32-bit or 64-bit). I want this to error, because otherwise bigints would truncate like floats, and I think that’s likely to trip people up. While PHP’s integer type would now have arbitrary precision, for obvious reasons most internal functions don’t need to handle integers larger than 32-bit or 64-bit (e.g. for a bit mask argument, or a string length). I’d much prefer if trying to do, say, str_split(“foobar”, 2 ** 128); would error rather than simply truncate silently, especially since passing such a large value here is almost certainly an error. If we truncate silently, it may seem like nothing has gone wrong. After all, if a function runs normally, raises no errors, and does not return an error value, it is usually safe to assume that everything is working fine.
> If there are some other arguments for it, please add them to the RFC. > Right now it looks kind of thin. I personally don't have any reason to > assume what you are proposing is better that what we have now, and BC > break is a cost that always must be offset by something that is worth > more. Especially a BC break in a form of "it worked before but now it > fails" - this can break code in so many hard to catch ways, where you > didn't actually care at the least if the function truncates the arg > (common situation in proxy/glue libraries, etc. - they'd be completely > fine with garbage in - garbage out) but need special code to handle > situations where the function fails to run altogether.
How do these libraries you speak of handle passing other types of arguments that fail? Surely this isn’t a new phenomenon.
-- Andrea Faulds http://ajf.me/

Rowan Collins

11 years ago
Andrea Faulds wrote (on 24/09/2014):
>> Especially a BC break in a form of "it worked before but now it >> >fails" - this can break code in so many hard to catch ways, where you >> >didn't actually care at the least if the function truncates the arg >> >(common situation in proxy/glue libraries, etc. - they'd be completely >> >fine with garbage in - garbage out) but need special code to handle >> >situations where the function fails to run altogether. > How do these libraries you speak of handle passing other types of arguments that fail? Surely this isn’t a new phenomenon.
I think Stas's point was not that libraries don't need to think about such things *in general*, but that the checking to handle *this particular case* will not currently be in place, and might not be put in place until someone is unfortunate enough to trigger the new behaviour. That said, most cases of "garbage in, garbage out" would presumably remain so, since most ZPP failures result in a return of NULL or FALSE, which would probably end up cast back to the expected type (int(0), string(''), etc) by the surrounding code.
-- Rowan Collins [IMSoP]

Andrew Faulds

11 years ago
On 24 Sep 2014, at 16:04, Rowan Collins <rowan.collins@gmail.com> wrote:
> That said, most cases of "garbage in, garbage out" would presumably remain so, since most ZPP failures result in a return of NULL or FALSE, which would probably end up cast back to the expected type (int(0), string(''), etc) by the surrounding code.
Right. It’s not an E_RECOVERABLE_ERROR, you’d just get an E_WARNING.
-- Andrea Faulds http://ajf.me/

Stas Malyshev

11 years ago
Hi!
>> That said, most cases of "garbage in, garbage out" would presumably >> remain so, since most ZPP failures result in a return of NULL or >> FALSE, which would probably end up cast back to the expected type >> (int(0), string(''), etc) by the surrounding code. > > Right. It’s not an E_RECOVERABLE_ERROR, you’d just get an E_WARNING.
No, you'd get E_WARNING *and* function would not run. Where before it did run. That is the problem, not the warning - you add more cases where the function does not run when it did before, and that can have profound consequences on the code that depends on it.
-- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/

Rowan Collins

11 years ago
On 24/09/2014 18:40, Stas Malyshev wrote:
>>> That said, most cases of "garbage in, garbage out" would presumably >>> remain so, since most ZPP failures result in a return of NULL or >>> FALSE, which would probably end up cast back to the expected type >>> (int(0), string(''), etc) by the surrounding code. >> Right. It’s not an E_RECOVERABLE_ERROR, you’d just get an E_WARNING. > No, you'd get E_WARNING *and* function would not run. Where before it > did run. That is the problem, not the warning - you add more cases where > the function does not run when it did before, and that can have profound > consequences on the code that depends on it.
So, the problem comes with built-in functions, which have some side effect, which can be usefully run with a bogus value for an integer argument. In contrast, any function which has some side effect which is actively harmful given a bogus value would be a beneficiary of the change. A function with no side effects will simply go from "garbage in, garbage out" to "garbage in, NULL out". I wonder how many functions actually fall into each category.
-- Rowan Collins [IMSoP]

Stas Malyshev

11 years ago
Hi!
> So, the problem comes with built-in functions, which have some side > effect, which can be usefully run with a bogus value for an integer > argument. In contrast, any function which has some side effect which is > actively harmful given a bogus value would be a beneficiary of the change.
No bogus value ever gets to a function - it always gets INT_MAX on overflow. If INT_MAX is harmful for this function, this change does not help as you could still pass INT_MAX and this change would not do anything. The thing is relying on this would not really improve your code - it is very rare that INT_MAX+1 is harmful for your function but INT_MAX-1 is not. But it may be useful to know that you can pass any value and it will be capped at INT_MAX.
-- Stanislav Malyshev, Software Architect SugarCRM: http://www.sugarcrm.com/

Andrew Faulds

11 years ago
> On 25 Sep 2014, at 02:17, Stas Malyshev <smalyshev@sugarcrm.com> wrote: > > Hi! > >> So, the problem comes with built-in functions, which have some side >> effect, which can be usefully run with a bogus value for an integer >> argument. In contrast, any function which has some side effect which is >> actively harmful given a bogus value would be a beneficiary of the change. > > No bogus value ever gets to a function - it always gets INT_MAX on > overflow. If INT_MAX is harmful for this function, this change does not > help as you could still pass INT_MAX and this change would not do anything. > > The thing is relying on this would not really improve your code - it is > very rare that INT_MAX+1 is harmful for your function but INT_MAX-1 is > not. But it may be useful to know that you can pass any value and it > will be capped at INT_MAX.
No it won't. Normally it truncates (module), only some functions cap.
-- Andrea Faulds http://ajf.me/

Rowan Collins

11 years ago
Andrea Faulds wrote (on 25/09/2014):
>> No bogus value ever gets to a function - it always gets INT_MAX on >> > overflow. If INT_MAX is harmful for this function, this change
does not
>> > help as you could still pass INT_MAX and this change would not do
anything.
>> > No it won't. Normally it truncates (module), only some functions cap.
I saw that in the RFC, and thought it rather odd that a more sane implementation already exists, but functions have to opt in to use it. I thought perhaps it was a side-effect of something else, but README.PARAMETER_PARSING_API doesn't list any other difference between "l" and "L":
> l - long (long) > L - long, limits out-of-range numbers to LONG_MAX/LONG_MIN (long)
What was the original motivation for functions to have that choice?
-- Rowan Collins [IMSoP]

Andrew Faulds

11 years ago
> On 22 Sep 2014, at 23:42, Andrea Faulds <ajf@ajf.me> wrote: > > Good evening again, > > Here’s a new RFC: https://wiki.php.net/rfc/zpp_fail_on_overflow > > Thoughts appreciated, as is help with the patch, though I can probably manage on my own. > > Thanks!
At long last, we are at the point where I have a complete patch, as I’ve managed to finish fixing tests. This means it can go to a vote sometime soon. It’s been more than two months since this was first proposed, so I’ll wait a bit.
-- Andrea Faulds http://ajf.me/