# why not use strtol instead of php_filter_parse_{int,hex,octal}? - Thread: https://externals.io/message/47013 - Original archive: https://news-web.php.net/php.internals/47013 - Started by Raphael Geissert on 2010-02-21T17:27:14+00:00 - Messages: 12 - Reply by email to internals@lists.php.net - More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt --- ## #47013 — Raphael Geissert — 2010-02-21T17:27:14+00:00 URL: https://externals.io/message/47013#47013 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 --- ## #47015 — Raphael Geissert — 2010-02-22T01:21:51+00:00 URL: https://externals.io/message/47013#47015 · In reply to: #47013 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 --- ## #47018 — Pierre Joye — 2010-02-22T08:23:16+00:00 URL: https://externals.io/message/47013#47018 · In reply to: #47015 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: > > […] > > 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 > > -- > > > > -- Pierre @pierrejoye | http://blog.thepimp.net | http://www.libgd.org --- ## #47023 — Derick Rethans — 2010-02-22T14:02:36+00:00 URL: https://externals.io/message/47013#47023 · In reply to: #47018 > > > […] > > 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 --- ## #47031 — Raphael Geissert — 2010-02-22T23:13:30+00:00 URL: https://externals.io/message/47013#47031 · In reply to: #47023 Derick Rethans wrote: > > > […] > > 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 --- ## #47032 — Pierre Joye — 2010-02-23T00:52:28+00:00 URL: https://externals.io/message/47013#47032 · In reply to: #47031 > Derick Rethans wrote: > > […] > > 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 --- ## #47033 — Derick Rethans — 2010-02-23T00:53:08+00:00 URL: https://externals.io/message/47013#47033 · In reply to: #47031 > Derick Rethans wrote: > > […] > > 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 --- ## #47034 — Raphael Geissert — 2010-02-23T01:09:40+00:00 URL: https://externals.io/message/47013#47034 · In reply to: #47033 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 --- ## #47036 — Derick Rethans — 2010-02-23T12:25:20+00:00 URL: https://externals.io/message/47013#47036 · In reply to: #47034 > 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? with kind regards, Derick -- http://derickrethans.nl | http://xdebug.org twitter: @derickr --- ## #47060 — Raphael Geissert — 2010-02-27T17:47:17+00:00 URL: https://externals.io/message/47013#47060 · In reply to: #47036 Derick Rethans wrote: > > > […] > > 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 --- ## #47059 — Raphael Geissert — 2010-02-27T17:50:26+00:00 URL: https://externals.io/message/47013#47059 · In reply to: #47032 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 --- ## #47089 — Raphael Geissert — 2010-03-05T05:20:59+00:00 URL: https://externals.io/message/47013#47089 · In reply to: #47059 Raphael Geissert wrote: > Pierre Joye wrote: > > […] 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=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