Hi Jakub,
On Sat, January 11, 2014 17:08, Jakub Zelenka wrote:
> On Sat, Jan 11, 2014 at 3:29 PM, Anatol Belski <[email protected]> wrote:
>
>
>>
>> yep, that's exactly how it performs till now in the mainstream, just
>> replace ZEND_INT_MAX with LONG_MAX. Adding a warning to every such case
>> would cause a warning flood in many PHP apps, guaranteed :) Whereby it
>> might be ok for debug mode maybe, I wouldn't do it as I can't remember
>> any WTFs about the behavior.
>>
>>
>>
> I meant adding warning only for overflow cases. Values in existing apps
> can't be bigger than LONG_MAX so I don't see how it could cause warning
> flood in the current PHP apps. I don't even think that users will be
> often using such a big values after the 64bit changes.
>
> The thing is that if I use value bigger than LONG_MAX after 64bit changes
> and I pass it to the function defined in extension that does not support
> it (use "l" in zpp), then I rather see warning than unexpected rounding to
> LONG_MAX... There is no way how to find out (except looking to the ext
> sources) that the big values are not supported. If I get warning, I can
> fix it in the code straight away...
>
But isn't it the exact situation now on ILP32 vs LP64 systems? Like 32 vs
64 bit Linux? I haven't suggested 'l' to handle the param the old way,
just it to be an alias for 'i'. In the new code that would be as dynamic
as php_int_t is, however 'l' as alias would retain zpp compatibility to
the older PHP.
For instance how much sense had such a centralized warding for the snippet
is_int(PHP_INT_MAX+1) ? There is a lot of functions accepting an arbitrary
numeric value and not depending on any library, why they should throw a
warning by default? Why should we throw a warning about 32 bit in the 64
bit build? Furtehrmore, LONG_MAX is 64 bit on LP64 systems, that would be
the same as PHP_INT_MAX.
> The another thing is that this checking needs to be done by other
> libraries wrappers after getting parameters because the most libraries
> using smaller types. That was exactly what I did when I worked on openssl
> ext for 64bit and I will need to do it in my fann and crypto extension.
> There are bunch
> of other extensions that will need to do it too (for example imagick). The
> difference with openssl changes is that the BC will need to be kept. I
> am sure that if warnings were already in zend_parse_arg_impl, then the
> work for supporting 64bit branch would much easier for me and other PECL
> extension maintainers...
>
Yes, that's the case - the range checks have to be done locally according
to what the wrapped library needs. If you haven't done them till now, you
might have an issue on 64 bit linux anyway. Like say the lib uses int, but
64 bit long is passed. If that checks are already in place in your ext,
then it's fine.
The range checks you've done for openssl are the obligatory part, that's
what I was doing as well for all the other extensions. Some are still
pending as on the progress wiki page. And of course this is to mention in
the porting guide. The libraries are different, for instance iconv uses
size_t for string length, libxml uses int for string length. zpp cannot
know, which library an extension uses. On the other hand - an extension
without any library deps but with some math functions should be kept in
mind, too.
Regards
anatol