Re: [PHP-DEV] Re: [PHP-CVS] com php-src: fix signed/unsigned mismatch: Zend/zend_execute.c

Hi Nikita,

On Mon, September 15, 2014 22:36, Nikita Popov wrote:
> On Mon, Sep 15, 2014 at 2:47 PM, Anatol Belski <[email protected]
> <mailto:[email protected]> > 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 <[email protected]
>>> <mailto:[email protected]> > wrote:
>>>
>>>
>>>
>>>
>>>> Commit:    836fd73cce8d0550baf5477bfb0ea0edbfae455a
>>>> Author:    Anatol Belski <[email protected] <mailto:[email protected]> >
>>>> 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