Re: 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 <ab@php.net> wrote: >> >> >> >>> Commit: 836fd73cce8d0550baf5477bfb0ea0edbfae455a >>> Author: Anatol Belski <ab@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=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. > Yeah, just to clarify, looks like - on 64 bit there's no issue as we cannot exceed INT64_MAX on any system obviously for some longer time - on 32 bit there can be an issue, so need more check logic Regards Anatol