On Sat Dec 08 18:24:17 2007, petdance wrote: > > In intlist_get(), we call list_get() which can return a NULL. > > Then, the result is checked against -1, and then > dereferenced. > > I suspect that check against -1 should actually be a > check against NULL, but don't know enough to prove it > and write the test case.
According to make cover, the case where the return was -1 was never hit. A simple test case caused a segfault, which definitely isn't the right thing. The attached patch fixes and refactors intlist_get() to make the coverage more obvious in the output of the coverage tests. It also adds a test case to exercise the previously neglected path. I'd like comments on whether intlist_get does the right thing with this patch and if it's exercised correctly by the test.
Index: src/intlist.c =================================================================== --- src/intlist.c (revision 29733) +++ src/intlist.c (working copy) @@ -306,10 +306,13 @@ INTVAL intlist_get(PARROT_INTERP, ARGMOD(IntList *list), INTVAL idx) { - /* XXX list_get can return NULL RT #48367 */ void * const ret = list_get(interp, (List *)list, idx, enum_type_INTVAL); - const INTVAL retval = ret == (void *)-1 ? 0 : *(INTVAL *)ret; - return retval; + if (ret == NULL) { + return (INTVAL)0; + } + else { + return *(INTVAL *)ret; + } } /* Index: t/pmc/intlist.t =================================================================== --- t/pmc/intlist.t (revision 29733) +++ t/pmc/intlist.t (working copy) @@ -6,7 +6,7 @@ use warnings; use lib qw( . lib ../lib ../../lib ); use Test::More; -use Parrot::Test tests => 8; +use Parrot::Test tests => 9; =head1 NAME @@ -168,6 +168,15 @@ I need a shower. OUTPUT +pasm_output_is( <<'CODE', <<'OUTPUT', "out of bounds" ); +#not sure if this is the right thing to do, but it doesn't segfault + new P0, 'IntList' + set P0, 1 + set I0, P0[25] + end +CODE +OUTPUT + pasm_output_is( <<'CODE', <<'OUTPUT', "direct access 2" ); new P0, 'IntList' set I10, 1100000