Hi Nikita, On Mon, September 15, 2014 22:36, Nikita Popov wrote: > On Mon, Sep 15, 2014 at 2:47 PM, Anatol Belski <a...@php.net > <mailto:a...@php.net> > wrote: > > > > On Mon, September 15, 2014 13:38, Anatol Belski wrote: > >> On Mon, September 15, 2014 13:13, Nikita Popov wrote: >> >> >>> On Mon, Sep 15, 2014 at 12:58 PM, Anatol Belski <a...@php.net >>> <mailto:a...@php.net> > wrote: >>> >>> >>> >>> >>>> Commit: 836fd73cce8d0550baf5477bfb0ea0edbfae455a >>>> Author: Anatol Belski <a...@php.net <mailto:a...@php.net> > >>>> Mon, >>>> 15 Sep 2014 >>>> 12:12:18 >>>> +0200 >>>> Parents: c8ed0d81e755c700f86cddb74f62eda27bc6e2de >>>> Branches: master >>>> >>>> >>>> >>>> >>>> Link: >>>> http://git.php.net/?p=php-src.git;a=commitdiff;h=836fd73cce8d0550baf >>>> 54 >>>> 77 >>>> bfb0ea0edbfae455a >>>> >>>> Log: >>>> fix signed/unsigned mismatch >>>> >>>> Changed paths: >>>> M Zend/zend_execute.c >>>> >>>> >>>> >>>> >>>> >>>> Diff: >>>> diff --git a/Zend/zend_execute.c b/Zend/zend_execute.c index >>>> 27636fd..5e774e2 100644 >>>> --- a/Zend/zend_execute.c >>>> +++ b/Zend/zend_execute.c >>>> @@ -771,7 +771,7 @@ static void zend_assign_to_string_offset(zval >>>> *str, >>>> zend_long offset, zval *valu } >>>> >>>> >>>> old_str = Z_STR_P(str); - if (offset >= Z_STRLEN_P(str)) { + >>>> if (offset >= (zend_long)Z_STRLEN_P(str)) { zend_long old_len = >>>> Z_STRLEN_P(str); Z_STR_P(str) = >>>> zend_string_realloc(Z_STR_P(str), offset + 1, 0); Z_TYPE_INFO_P(str) >>>> = >>>> IS_STRING_EX; >>>> @@ -1226,7 +1226,7 @@ static zend_always_inline void >>>> zend_fetch_dimension_address_read(zval *result, z offset = >>>> Z_LVAL_P(dim); >>>> } >>>> >>>> >>>> >>>> >>>> - if (UNEXPECTED(offset < 0) || >>>> UNEXPECTED(Z_STRLEN_P(container) <= offset)) { >>>> + if (UNEXPECTED(offset < 0) || >>>> UNEXPECTED((zend_long)Z_STRLEN_P(container) <= offset)) { >>>> if (type != BP_VAR_IS) { zend_error(E_NOTICE, "Uninitialized string >>>> offset: %pd", offset); >>>> } >>>> >>>> >>>> >>>> >>> >>> I wonder if these casts shouldn't happen the other way around, i.e. >>> whether offset should be cast to size_t instead of casting the length >>> to zend_long. >>> >>> The case I'm thinking about is this: If len > ZEND_LONG_MAX, then >>> (zend_long)len will be < 0, so offset will always be >= than >>> (zend_long)len >>> in this case, leading to out of bounds memory access. If we cast the >>> offset to (size_t) instead this shouldn't happen. >>> >> If offset is negative, there were an issue - thus the choice what to >> cast. Probably we can do the offset sign check and handle accordingly, >> that were future proof. >> >> I mean of course zend_string->len itself could exceed INT64_MAX, but >> not the string itself. But that's an issue anyway which would have to be >> looked up at some other place (like say somewhere would be done >> zend_string->len = -1). Hm, an issue could be on 32 bit, if one >> allocates a string not with str_repeat() but in the loop like while(1) >> {$s .= 'x';} >> ... just that we wouldn't be able to pass an offset bigger that INT_MAX >> anyway. Probably have to do more error logic. >> > > The casts look safe for the normal case, memory_limit will not allow to > exceed ZEND_LONG_MAX in PHP. So disregarding what HW does, looks like > nothing to do :) > > Regards > > > Anatol > > > > > Are you sure about this? I can set memory_limit to -1, which is then cast > to size_t, resulting in a limit > ZEND_LONG_MAX. > > > I tried the following in a 32bit VM with -dmemory_limit=-1 and got a > segfault: > > > > <?php > > > $str = str_repeat('x', PHP_INT_MAX); > $str .= 'yyy'; > > > $str[1] = 'a'; > > > > There is no crash if I cast offset to (size_t) instead. > > I redone it by your suggestion. Still I've a crash on Windows x86. It's however already in str_repeat() in allocation
> php7ts_debug.dll!zend_string_safe_alloc(unsigned int n, unsigned int m, unsigned int l, int persistent) Line 117 C php7ts_debug.dll!zif_str_repeat(unsigned int param_count, _zval_struct * return_value, void * * * tsrm_ls) Line 4738 C php7ts_debug.dll!ZEND_DO_FCALL_SPEC_HANDLER(_zend_execute_data * execute_data, void * * * tsrm_ls) Line 593 C php7ts_debug.dll!execute_ex(_zend_execute_data * execute_data, void * * * tsrm_ls) Line 352 C php7ts_debug.dll!zend_execute(_zend_op_array * op_array, _zval_struct * return_value, void * * * tsrm_ls) Line 381 C php7ts_debug.dll!zend_execute_scripts(int type, void * * * tsrm_ls, _zval_struct * retval, int file_count, ...) Line 1345 C php7ts_debug.dll!php_execute_script(_zend_file_handle * primary_file, void * * * tsrm_ls) Line 2560 C If you say you had that fixed with reversing the cast, then I might see a win only issue. Looking further. Regards Anatol -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php