On Mon, September 15, 2014 13:13, Nikita Popov wrote: > On Mon, Sep 15, 2014 at 12:58 PM, Anatol Belski <a...@php.net> wrote: > > >> Commit: 836fd73cce8d0550baf5477bfb0ea0edbfae455a >> Author: Anatol Belski <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=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 -- PHP Internals - PHP Runtime Development Mailing List To unsubscribe, visit: http://www.php.net/unsub.php