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
