Hi,
While working on bug #51023, Sean and I were wondering why they were written
in the first place, instead of using strtol; which can handle the three
cases and properly detect overflows and underflows (and doesn't risk being
optimised-away like with the current implementations).
If there's no objection, I would like to remove them entirely and use strtol
in php_filter_int.
Cheers,
Raphael Geissert
Raphael Geissert wrote:
If there's no objection, I would like to remove them entirely and use
strtol in php_filter_int.
By the way, I don't think that the fact that strtol is locale-aware should
be a reason to make the change. Since scripts are expected to use the value
returned by filter_var() there should be no problem with the fact that
strings such as "100'000" are accepted as a valid integer, under certain
locales.
Cheers,
Raphael Geissert
hi,
Please don't commit this change. I remember some issues with strtol
(portability, BC breaks, etc.). Please wait until we figured them out,
or find back in the archives why strtol could be a bad idea.
Raphael Geissert wrote:
If there's no objection, I would like to remove them entirely and use
strtol in php_filter_int.By the way, I don't think that the fact that strtol is locale-aware should
be a reason to make the change. Since scripts are expected to use the value
returned byfilter_var()there should be no problem with the fact that
strings such as "100'000" are accepted as a valid integer, under certain
locales.Cheers,
Raphael Geissert
--
--
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
Raphael Geissert wrote:
If there's no objection, I would like to remove them entirely and use
strtol in php_filter_int.By the way, I don't think that the fact that strtol is locale-aware should
be a reason to make the change. Since scripts are expected to use the value
returned byfilter_var()there should be no problem with the fact that
strings such as "100'000" are accepted as a valid integer, under certain
locales.Please don't commit this change. I remember some issues with strtol
(portability, BC breaks, etc.). Please wait until we figured them out,
or find back in the archives why strtol could be a bad idea.
Poratability was indeed the reason, as well as the locale issues that
you mention (POSIX locales==useless). Please don't commit this.
regards,
Derick
--
http://derickrethans.nl | http://xdebug.org
twitter: @derickr
Derick Rethans wrote:
Please don't commit this change. I remember some issues with strtol
(portability, BC breaks, etc.). Please wait until we figured them out,
or find back in the archives why strtol could be a bad idea.Poratability was indeed the reason, as well as the locale issues that
you mention (POSIX locales==useless). Please don't commit this.
Ok. Sean came up with a different patch which is being tested right now.
If all the tests pass then I'm going to commit it.
Cheers,
Raphael Geissert
Derick Rethans wrote:
Please don't commit this change. I remember some issues with strtol
(portability, BC breaks, etc.). Please wait until we figured them out,
or find back in the archives why strtol could be a bad idea.Poratability was indeed the reason, as well as the locale issues that
you mention (POSIX locales==useless). Please don't commit this.Ok. Sean came up with a different patch which is being tested right now.
If all the tests pass then I'm going to commit it.
What patch? Please do not commit anything there without first posting
to the list. There were enough breakage in this area, no need to
introduce new ones again.
Thanks for your understanding,
Cheers,
Pierre
@pierrejoye | http://blog.thepimp.net | http://www.libgd.org
Pierre Joye wrote:
What patch? Please do not commit anything there without first posting
to the list. There were enough breakage in this area, no need to
introduce new ones again.
Here it is:
http://git.debian.org/?p=pkg-
php/php.git;a=blob;f=debian/patches/filter_validate_int.patch;hb=3061d111de130df7388cc78e26b63cc105574775
Like Sean stated on his post to the list, the engine is also affected.
Somebody would need to apply the patch there.
Cheers,
Raphael Geissert
Raphael Geissert wrote:
Pierre Joye wrote:
What patch? Please do not commit anything there without first posting
to the list. There were enough breakage in this area, no need to
introduce new ones again.
No objection then?
Here it is:
http://git.debian.org/?p=pkg-
php/php.git;a=blob;f=debian/patches/filter_validate_int.patch;hb=3061d111de130df7388cc78e26b63cc105574775Like Sean stated on his post to the list, the engine is also affected.
Somebody would need to apply the patch there.
Cheers,
Raphael Geissert
Derick Rethans wrote:
Please don't commit this change. I remember some issues with strtol
(portability, BC breaks, etc.). Please wait until we figured them out,
or find back in the archives why strtol could be a bad idea.Poratability was indeed the reason, as well as the locale issues that
you mention (POSIX locales==useless). Please don't commit this.Ok. Sean came up with a different patch which is being tested right now.
If all the tests pass then I'm going to commit it.
What exactly are you trying to fix here?
Derick
--
http://derickrethans.nl | http://xdebug.org
twitter: @derickr
Derick Rethans wrote:
What exactly are you trying to fix here?
gcc 4.4's optimiser removes the overflow check present in
php_filter_parse_int and ZEND_HANDLE_NUMERIC (but I can't touch that part of
the code anyway...) which prevents the overflow from being detected.
Cheers,
Raphael Geissert
Derick Rethans wrote:
What exactly are you trying to fix here?
gcc 4.4's optimiser removes the overflow check present in
php_filter_parse_int and ZEND_HANDLE_NUMERIC (but I can't touch that part of
the code anyway...) which prevents the overflow from being detected.
Doesn't this sound like a GCC bug to you then?
with kind regards,
Derick
--
http://derickrethans.nl | http://xdebug.org
twitter: @derickr
Derick Rethans wrote:
gcc 4.4's optimiser removes the overflow check present in
php_filter_parse_int and ZEND_HANDLE_NUMERIC (but I can't touch that part
of the code anyway...) which prevents the overflow from being detected.Doesn't this sound like a GCC bug to you then?
Sorry for the late response. No, it is not a bug, please refer to Sean's
message.
Cheers,
Raphael Geissert