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

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=836fd73cce8d0550baf5477
>> 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.

Regards

Anatol