resizablepmcarray splice heap-buffer-overflow with negative count arg #766

Closed
rurban opened this Issue May 5, 2012 · 7 comments

Projects

None yet

4 participants

@rurban
Member
rurban commented May 5, 2012

perl6 -e'my @a=splice([], 1); (latest git versions both)
splice at src/pmc/resizablepmcarray.pmc:673
reading 8 byte left of a 64byte buffer, but in which? A PMC has 32 AFAIK (64-bit)
8 byte in user-data looks pretty security-relevant to me.

There must be strange logical error in the memmove(item + offset + elems1, ...

I added

        if (tail > 0 && count > elems1) {
Parrot_io_printf(interp, "XXX splice item(%x,%d)+offset(%d)+elems1(%d)=%x, count=%d, tail=%d, from=0x%x,%d\n",
                 item,PMC_size(SELF), offset,elems1,item + offset + elems1,count,tail,from,sizeof(PMC));
            /* we're shrinking the array, so first move the tail left */
            memmove(item + offset + elems1, item + offset + count,
                 tail * sizeof (PMC *));
        }

and get:

$ PERL6LIB=lib ./perl6 -e'my @a=splice([], 1);'
XXX splice item(d0d5fc80,2)+offset(0)+elems1(0)=d0d5fc80, count=1, tail=1, from=0xd04709d0,32    (pass!)
XXX splice item(d0da0080,2)+offset(0)+elems1(0)=d0da0080, count=1, tail=1, from=0xd0470d90,32 (fail!)

==31516== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7f555ce78a78 at pc 0x7f556ea75394 bp 0x7fffd8fa57f0 sp 0x7fffd8fa57e8
READ of size 1 at 0x7f555ce78a78 thread T0

    #0  0000000000ff8394 <Parrot_ResizablePMCArray_splice_orig+0x1e14>:
    VTABLE_set_integer_native(interp, _self, offset + elems1 + tail);
    item = PMC_array(_self);
    if (tail > 0 && count < elems1) {
        /* the array grew, so move the tail to the right */
        memmove(item + offset + elems1, item + offset + count,
  ff8394:   48 8b 44 24 40          mov    0x40(%rsp),%rax
    #1  Parrot_ResizablePMCArray_splice+0x2fb
    #2  Parrot_splice_p_p_i_i+0x717
    #3  runops_fast_core+0x4ad
    #4  runops_int+0x6b5
    #5  runops+0xe1f
    #6  Parrot_pcc_invoke_from_sig_object+0xa76
    #7  Parrot_ext_call+0x765
    #8  Parrot_Task_invoke+0xc2b
    #9  Parrot_pcc_invoke_from_sig_object+0x812
    #10  Parrot_ext_call+0x765
    #11  Parrot_cx_next_task+0x8d0
    #12  Parrot_cx_outer_runloop+0x581
    #13  Parrot_cx_begin_execution+0x8fd
    #14  Parrot_pf_execute_bytecode_program+0x9a4
    #15  Parrot_api_run_bytecode+0xa96
    #16  main+0x841
    #17  __libc_start_main+0xfd
0x7f555ce78a78 is located 8 bytes to the left of 64-byte region [0x7f555ce78a80,0x7f555ce78ac0)
@Whiteknight Whiteknight was assigned May 5, 2012
Owner

There are a least two issues here... Rakudo's splice function is sending incorrect values to Parrot's splice opcode, and there's a problem with Parrot's splice in handling strange offsets and counts. I can easily fix these in Rakudo and NQP, but I probably need some better idea of Parrot's desired splice opcode semantics for negative counts and offsets beyond the end of the array. Do we basically want to follow p5 semantics?

Pm

@pmichaud pmichaud was assigned Aug 16, 2012
Owner
leto commented Aug 16, 2012

I think following p5 semantics for splicing + counts/offsets is a good starting point.

Contributor

Parrot's splice opcode is not frequently used, and I don't think there's any clear description of what it's semantics are supposed to be (especially for "strange offsets and counts"). I'm inclined, if there are ambiguities, to default to whatever the desired p6 semantics are supposed to be. Where we don't have coherent designs, I'm not against stealing them from people who have real, current needs.

Owner
leto commented Aug 16, 2012

+1 for splice semantics closest to what Rakudo/NQP need. I can help with writing tests, just let me know what you need

Owner
leto commented Dec 11, 2012

Has there been any progress on this? Does the bug still exist?

Member
rurban commented Dec 11, 2012

The bug still exists, but there is not even a parrot testcase for it.
I've talked to pmichaud about it. The code in question is super tricky, he wrote it. He kindly offered to fix it, as soon as we spec'd the semantics.

We agreed to follow p5 splicing + negative counts/offsets semantics.

@rurban rurban added this to the 6.10.0 milestone Oct 20, 2014
@rurban rurban assigned rurban and unassigned pmichaud Oct 20, 2014
Member
rurban commented Nov 9, 2014

.pir testcase to repro the args

    # GH 766 heap-buffer-overflow with asan
    $P1 = new ['ResizablePMCArray']
    $P2 = new ['ResizablePMCArray']
    $P1 = 0
    $P2 = 0
    splice $P1, $P2, 0, -1

Found plenty of more testcases for the asan heap overflow with negative count.
See rurban/rpa-splice-gh766 e490c7c

    not ok 11 - ResizablePMCArray.splice 0, 0, 0, -2
    not ok 12 - ResizablePMCArray.splice 0, 0, 0, -1
    not ok 16 - ResizablePMCArray.splice 0, 0, 1, -2
    not ok 36 - ResizablePMCArray.splice 0, 1, 0, -2
    not ok 37 - ResizablePMCArray.splice 0, 1, 0, -1
    not ok 41 - ResizablePMCArray.splice 0, 1, 1, -2
    not ok 61 - ResizablePMCArray.splice 0, 2, 0, -2
    not ok 62 - ResizablePMCArray.splice 0, 2, 0, -1
    not ok 66 - ResizablePMCArray.splice 0, 2, 1, -2
    not ok 81 - ResizablePMCArray.splice 1, 0, -1, -2
    not ok 82 - ResizablePMCArray.splice 1, 0, -1, -1
    not ok 86 - ResizablePMCArray.splice 1, 0, 0, -2
    not ok 87 - ResizablePMCArray.splice 1, 0, 0, -1
    not ok 91 - ResizablePMCArray.splice 1, 0, 1, -2
    not ok 106 - ResizablePMCArray.splice 1, 1, -1, -2
    not ok 107 - ResizablePMCArray.splice 1, 1, -1, -1
    not ok 111 - ResizablePMCArray.splice 1, 1, 0, -2
    not ok 112 - ResizablePMCArray.splice 1, 1, 0, -1
    not ok 116 - ResizablePMCArray.splice 1, 1, 1, -2
    not ok 131 - ResizablePMCArray.splice 1, 2, -1, -2
    not ok 132 - ResizablePMCArray.splice 1, 2, -1, -1
    not ok 136 - ResizablePMCArray.splice 1, 2, 0, -2
    not ok 137 - ResizablePMCArray.splice 1, 2, 0, -1
    not ok 141 - ResizablePMCArray.splice 1, 2, 1, -2
    not ok 151 - ResizablePMCArray.splice 2, 0, -2, -2
    not ok 152 - ResizablePMCArray.splice 2, 0, -2, -1
    not ok 156 - ResizablePMCArray.splice 2, 0, -1, -2
    not ok 161 - ResizablePMCArray.splice 2, 0, 0, -2
    not ok 162 - ResizablePMCArray.splice 2, 0, 0, -1
    not ok 166 - ResizablePMCArray.splice 2, 0, 1, -2
    not ok 176 - ResizablePMCArray.splice 2, 1, -2, -2
    not ok 177 - ResizablePMCArray.splice 2, 1, -2, -1
    not ok 181 - ResizablePMCArray.splice 2, 1, -1, -2
    not ok 186 - ResizablePMCArray.splice 2, 1, 0, -2
    not ok 187 - ResizablePMCArray.splice 2, 1, 0, -1
    not ok 191 - ResizablePMCArray.splice 2, 1, 1, -2
    not ok 201 - ResizablePMCArray.splice 2, 2, -2, -2
    not ok 202 - ResizablePMCArray.splice 2, 2, -2, -1
    not ok 206 - ResizablePMCArray.splice 2, 2, -1, -2
    not ok 211 - ResizablePMCArray.splice 2, 2, 0, -2
    not ok 212 - ResizablePMCArray.splice 2, 2, 0, -1
    not ok 216 - ResizablePMCArray.splice 2, 2, 1, -2
@rurban rurban pushed a commit that referenced this issue Nov 9, 2014
Reini Urban [test] add testcase for GH #766, not repro anymore
this paniced with asan only. no need for an ok test here
5b127ee
@rurban rurban pushed a commit that referenced this issue Nov 9, 2014
Reini Urban [test] t/stress/rpa-splice.t: reproducible testcases for GH #766
with asan:
t/stress/rpa-splice.t (Wstat: 10752 Tests: 225 Failed: 42)
  Failed tests:  11-12, 16, 36-37, 41, 61-62, 66, 81-82
                86-87, 91, 106-107, 111-112, 116, 131-132
                136-137, 141, 151-152, 156, 161-162, 166
                176-177, 181, 186-187, 191, 201-202, 206
                211-212, 216
e490c7c
@rurban rurban pushed a commit that referenced this issue Nov 9, 2014
Reini Urban [test] t/stress/rpa-splice.t: reproducible testcases for GH #766
with asan:
t/stress/rpa-splice.t (Wstat: 10752 Tests: 225 Failed: 42)
  Failed tests:  11-12, 16, 36-37, 41, 61-62, 66, 81-82
                86-87, 91, 106-107, 111-112, 116, 131-132
                136-137, 141, 151-152, 156, 161-162, 166
                176-177, 181, 186-187, 191, 201-202, 206
                211-212, 216
06c5643
@rurban rurban pushed a commit that referenced this issue Nov 9, 2014
Reini Urban [pmc] fix splice overflows with negative count
Fixes GH #766.
Throw illegal argument with negative count arguments for now.

The perl5 semantics with negative counts makes not much sense as perl5
splices an array into the array, but we just fill the elements with an
value, we do not splice an into another array.
43537c1
@rurban rurban changed the title from resizablepmcarray splice heap-buffer-overflow to resizablepmcarray splice heap-buffer-overflow with negative count arg Nov 10, 2014
@rurban rurban pushed a commit that referenced this issue Nov 10, 2014
Reini Urban [pmc] fix splice overflows with negative count
Fixes GH #766.
Throw illegal argument with negative count arguments for now.

TODO: Implement support for negative count argument for splice.
Add more splice methods.
ec7e1ac
@rurban rurban pushed a commit that referenced this issue Nov 10, 2014
Reini Urban [test] t/stress/rpa-splice.t: reproducible testcases for GH #766
with asan:
t/stress/rpa-splice.t (Wstat: 10752 Tests: 225 Failed: 42)
  Failed tests:  11-12, 16, 36-37, 41, 61-62, 66, 81-82
                86-87, 91, 106-107, 111-112, 116, 131-132
                136-137, 141, 151-152, 156, 161-162, 166
                176-177, 181, 186-187, 191, 201-202, 206
                211-212, 216
b6a87f1
@rurban rurban pushed a commit that referenced this issue Nov 10, 2014
Reini Urban [pmc] fix splice overflows with negative count
Fixes GH #766.
Throw illegal argument with negative count arguments for now.

TODO: Implement support for negative count argument for splice.
Add more splice methods.
f898fa5
@rurban rurban pushed a commit that referenced this issue Nov 10, 2014
Reini Urban [pmc] fix splice overflows with negative count
Fixes GH #766.
Throw illegal argument with negative count arguments for now.

TODO: Implement support for negative count argument for splice.
Add more splice methods.
cc28c94
@rurban rurban closed this Nov 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment