On Mon, Sep 15, 2014 at 2:47 PM, Anatol Belski <a...@php.net> 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 <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=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
>
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.
Nikita
