Re: [RFC] 64 bit platform improvements for string length and integer

Hi Jakub, On Mon, January 13, 2014 00:23, Jakub Zelenka wrote: > On Sun, Jan 12, 2014 at 9:18 PM, Jakub Zelenka <bukka@php.net> wrote: > > >> Hi Anatol, >> >> >> Jakub >> On Sat, Jan 11, 2014 at 7:34 PM, Anatol Belski <ab@php.net> wrote: >> >> >>> If a library expects long, in the new code that's the issue on 32 bit >>> windows only. So yes, probably the way you describe is plausible, >>> check PHP_WIN32 and PHP_API_VERSION. Honestly, right at the place >>> where I sit, I can't remember any library working with long (well, >>> timeval struct and so on, not really libs). There are int, size_t, >>> int64_t, ... so while the case you describe is of course possible, >>> it's rather an exception. Usually a simple runtime range check will be >>> good enough, if needed at all. >>> >>> >> Actually there is one big library where you can find it. It's OpenSSL >> which is why I was asking about it.. :) There are quite a few places >> where it's used. The main ones are following: >> >> - ASN.1 API functions (I plan to completely wrap it :) ) where it's >> used for data length. - BIO_ctrl for length parameter (BIO_ctrl is a >> definition result for many macros like BIO_set_mem_buf, >> BIO_set_buffer_size, BIO_set_write_buf_size...) >> - Big numbers - there is a situation a bit more complicated but unsigned >> long is used (there will be necessary some other checks anyway so it's >> not an issue) >> >> I think that would be good to have a look to the other exts and double >> check if used libs have long parameters for data lengths. If not, then I >> agree with you that there is no point to do casting to long with >> warnings just because of OpenSSL. >> That openssl part looks like not implemented in PHP. Talking about the core lib deps as well as a lot of libs for PECL libraries, that's just not the case. The case with BIO_ctrl does therefore not justify such global warnings. >> However it would be good to define in compat header a macro for >> checking that long != php_int_t . I know that it's currently only _WIN64 >> but if you implement enabling 64bit on 32bit platform (future scope in >> the RFC :) ), then there will be an extra definition. >> >> That compat.h header isn't thought to be swiss army knife, once copied into some extensions source, it of course can and should be adapted to the concrete circumstances. > I was thinking a bit more what will be the consequences of asn1 and bio > in my ext for "l" flag and it won't be such a big issue regarding to zpp. > There are just few cases in asn1 (ASN1_INTEGER_set for example) and BIO > stuff will probably require mainly string conversions which is a bit > different issue... > > What's the actually plan for keeping BC for "s" and "p"? Is it gonna > be > zend_str_size from compat header and "s" and "p" aliases of "S" > and "P"? > > Something like... > ... > char *s; zend_str_size len; if (zend_parse_parameters(ZEND_NUM_ARGS() > TSRMLS_CC, "s", &s, &len) == > FAILURE) { return; } > ... > > > If so, I think that that would be a slightly bigger issue (size_t vs > int)... > > There are only two solutions of zpp "s" that I think of: > 1. check range (INT_MAX) and cast it to int (possibly throw warning if > it's out of range) -> that could result too many warning but it's safe 2. > PHP version variable zend_str_size (see above) -> the same as discussed > for long but considerably more places where we will need to find out what > the zend_str_size really is. > Jakub, please read the RFC, there's no zend_str_size anymore. It was used only while porting a lot of stuff. Also, there are now the tools and the doc I was talking about http://git.php.net/?p=php-src.git;a=tree;f=compat;hb=refs/heads/str_size_and_int64 . Yet raw, will improve with usage as there will questions come. Regards Anatol