Ticket #233 (closed bug: fixed)

Opened 6 years ago

Last modified 6 years ago

[BUG] check_named reads past the end of an array

Reported by: cotto Owned by: whiteknight
Priority: normal Milestone: 0.9.1
Component: core Version: trunk
Severity: high Keywords:
Cc: Language:
Patch status: Platform: all

Description

In src/inter_call.c check_named behaves badly at line 1395. The code that reads element i+1 of st->dest.u.op.signature's array reads past the end of that array on the last pass through the loop. I noticed this because when I changed SIG_ITEM to a vtable call, the PMC threw an out-of-bounds exception.

The following gdb session run on a clean build of r36047. Note that PMC_int_val == 5 and that the second time I hit the breakpoint, the PMC's array's 6th element (i.e. i[5]) is accessed by the SIG_ITEM macro.

cotto@feather:/usr/src/parrot/parrot-svn-clean\ 1 $ gdb ./parrot
GNU gdb 6.8-debian
Copyright (C) 2008 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "i486-linux-gnu"...
(gdb) break src/inter_call.c:1395
No source file named src/inter_call.c.
Make breakpoint pending on future shared library load? (y or [n]) y
Breakpoint 1 (src/inter_call.c:1395) pending.
(gdb) run t/pmc/pccmethod_test_1.pir 
Starting program: /usr/src/parrot/parrot-svn-clean/parrot t/pmc/pccmethod_test_1.pir
[Thread debugging using libthread_db enabled]
warning: Lowest section in /usr/lib/libicudata.so.36 is .hash at 000000b4
[New Thread 0xb6b6c6c0 (LWP 20945)]
[Switching to Thread 0xb6b6c6c0 (LWP 20945)]

Breakpoint 1, check_named (interp=0x804f040, st=0xbfa7dffc) at src/inter_call.c:1395
1395                    arg_sig = st->dest.sig = SIG_ITEM(st->dest.u.op.signature, i+1);
(gdb) p i
$1 = 2
(gdb) p *st->dest.u.op.signature
$2 = {cache = {_b = {_bufstart = 0x5, _buflen = 0}, _ptrs = 
     {_struct_val = 0x5, _pmc_val = 0x0}, _i = {_int_val = 5,
     _int_val2 = 0}, _num_val = 2.4703282292062327e-323,
     _string_val = 0x5}, flags = 71304704, vtable = 0x8095198, 
     data = 0x811a360, pmc_ext = 0x80d6154}
(gdb) c
Continuing.

Breakpoint 1, check_named (interp=0x804f040, st=0xbfa7dffc) at src/inter_call.c:1395
1395                    arg_sig = st->dest.sig = SIG_ITEM(st->dest.u.op.signature, i+1);
(gdb) p i
$3 = 4
(gdb) p *st->dest.u.op.signature
$4 = {cache = {_b = {_bufstart = 0x5, _buflen = 0}, _ptrs =
     {_struct_val = 0x5, _pmc_val = 0x0}, _i = {_int_val = 5,
     _int_val2 = 0}, _num_val = 2.4703282292062327e-323, 
     _string_val = 0x5}, flags = 71304704, vtable = 0x8095198, 
     data = 0x811a360, pmc_ext = 0x80d6154}
(gdb) c
Continuing.
test_method3
10, 20

Program exited normally.

Change History

Changed 6 years ago by cotto

I got around this in r36053 by manually including src/pmc/pmc_fixedintegerarray.h in src/inter_call.c and using GETATTR to get the array. The code can then continue reading past the end of the array without throwing any pesky exceptions.

This is hacky, but it seemed like the next best option to fixing check_named. Once the code in check_named is fixed, the GETATTR... business can be changed to a nice encapsulated VTABLE function and the extra header can be removed.

Changed 6 years ago by kjs

I'm not sure if I understand this code enough to commit changes, but unless I horribly oversee something, isn't this just a matter of adding a check, along the lines of:

if (i + 1 < st->dest.n) { /* <== this line added */
  INTVAL* int_array;
  GETATTR_FixedIntegerArray_int_array(interp,
             st->dest.u.op.signature, int_array);
  arg_sig = st->dest.sig = int_array[i+1];

  /* skip associated opt flag arg as well */
  if (arg_sig & PARROT_ARG_OPT_FLAG)
     i++;

} /* <== and add closing brace */


Changed 6 years ago by whiteknight

  • status changed from new to assigned
  • owner set to whiteknight
  • component changed from none to core
  • severity changed from medium to high
  • milestone set to 0.9.1

Changed 6 years ago by NotFound

  • status changed from assigned to closed
  • resolution set to fixed

Fixed according kjs suggestion and getting rid of the temporary hack in r36389

Closing ticket.

Note: See TracTickets for help on using tickets.