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

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]> wrote:
>>
>>
>>
>>> Commit:    836fd73cce8d0550baf5477bfb0ea0edbfae455a
>>> Author:    Anatol Belski <[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=836fd73cce8d0550baf54
>>> 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