code review 9372044: runtime: fix heap corruption

runtime: fix GC scanning of slices
If a slice points to an array embedded in a struct,
the whole struct can be incorrectly scanned as the slice buffer.
Fixes issue 5443.

[dvyukov, 2013-05-15 17:15:59]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://dvyukov%40google.com@code.google.com/p/go/

[bradfitz, 2013-05-15 17:25:56]

Nice test!



On Wed, May 15, 2013 at 10:15 AM, <dvyukov@google.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
>
https://dvyukov%40google.com@**code.google.com/p/go/<http://40google.com@code...
>
>
> Description:
> runtime: fix heap corruption
> Fixes issue 5443.
>
> Please review this at
https://codereview.appspot.**com/9372044/<https://codereview.appspot.com/9372...
>
> Affected files:
>   M src/pkg/runtime/gc_test.go
>   M src/pkg/runtime/mgc0.c
>
>
> Index: src/pkg/runtime/gc_test.go
> ==============================**==============================**=======
> --- a/src/pkg/runtime/gc_test.go
> +++ b/src/pkg/runtime/gc_test.go
> @@ -97,3 +97,27 @@
>                 m[a] = T{}
>         }
>  }
> +
> +func TestGcArraySlice(t *testing.T) {
> +       type X struct {
> +               buf     [1]byte
> +               nextbuf []byte
> +               next    *X
> +       }
> +       var head *X
> +       for i := 0; i < 10; i++ {
> +               p := &X{}
> +               p.buf[0] = 42
> +               p.next = head
> +               if head != nil {
> +                       p.nextbuf = head.buf[:]
> +               }
> +               head = p
> +               runtime.GC()
> +       }
> +       for p := head; p != nil; p = p.next {
> +               if p.buf[0] != 42 {
> +                       t.Fatal("corrupted heap")
> +               }
> +       }
> +}
> Index: src/pkg/runtime/mgc0.c
> ==============================**==============================**=======
> --- a/src/pkg/runtime/mgc0.c
> +++ b/src/pkg/runtime/mgc0.c
> @@ -799,7 +799,10 @@
>                         sliceptr = (Slice*)(stack_top.b + pc[1]);
>                         if(sliceptr->cap != 0) {
>                                 obj = sliceptr->array;
> -                               objti = pc[2] | PRECISE | LOOP;
> +                               // Can't use slice element type for
> scanning,
> +                               // because if it points to an array embed
> in a struct,
> +                               // we will scan the whole struct as the
> slice.
> +                               // So just obtain type info from heap.
>                         }
>                         pc += 3;
>                         break;
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[gobot, 2013-05-15 17:34:14]

R=cshapiro (assigned by r)

[iant, 2013-05-15 18:01:33]

LGTM

Thanks.

[r, 2013-05-15 18:16:37]

https://codereview.appspot.com/9372044/diff/5001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/9372044/diff/5001/src/pkg/runtime/mgc0.c#newco...
src/pkg/runtime/mgc0.c:803: // because if it points to an array embed in a
struct,
s/embed/embedded/

[cshapiro1, 2013-05-15 18:23:05]

https://codereview.appspot.com/9372044/diff/5001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/9372044/diff/5001/src/pkg/runtime/mgc0.c#newco...
src/pkg/runtime/mgc0.c:802: // Can't use slice element type for scanning,
I am a little bit confused by this comment.  Is the value at pc[2] bad?  Where
is it being generated?  Why isn't the fix there?

[dvyukov, 2013-05-15 18:30:28]

https://codereview.appspot.com/9372044/diff/5001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/9372044/diff/5001/src/pkg/runtime/mgc0.c#newco...
src/pkg/runtime/mgc0.c:802: // Can't use slice element type for scanning,
On 2013/05/15 18:23:05, cshapiro1 wrote:
> I am a little bit confused by this comment.  Is the value at pc[2] bad?  Where
> is it being generated?  Why isn't the fix there?

It is correct, but it is correct only for the array. But when we scan a slice we
don't know it's size, so we scan the whole memory block using that type.
If the array sits in the beginning of a struct, we will scan the whole struct as
if it an array.
So the problem is actually with the LOOP flag.

https://codereview.appspot.com/9372044/diff/5001/src/pkg/runtime/mgc0.c#newco...
src/pkg/runtime/mgc0.c:803: // because if it points to an array embed in a
struct,
On 2013/05/15 18:16:37, r wrote:
> s/embed/embedded/

Done.

[cshapiro1, 2013-05-15 18:39:01]

https://codereview.appspot.com/9372044/diff/5001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/9372044/diff/5001/src/pkg/runtime/mgc0.c#newco...
src/pkg/runtime/mgc0.c:802: // Can't use slice element type for scanning,
Can we get a better commit log description?

If I understand you correctly, the type of the slice is assumed to be the type
of the entire memory block pointed to by the slice?

If this is true, then if my slice points to an array that is *anywhere* inside
of a struct, the entire struct is assumed to be of the same type as the slice. 
Correct?

It sounds like the logic for LOOP is incorrect.  There should be (at least) a
TODO at the site where it is generated referring to this issue.  Do we have a
bug for this?

[dvyukov, 2013-05-15 19:23:10]

https://codereview.appspot.com/9372044/diff/5001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/9372044/diff/5001/src/pkg/runtime/mgc0.c#newco...
src/pkg/runtime/mgc0.c:802: // Can't use slice element type for scanning,
On 2013/05/15 18:39:01, cshapiro1 wrote:
> If I understand you correctly, the type of the slice is assumed to be the type
> of the entire memory block pointed to by the slice?

Yes.

> If this is true, then if my slice points to an array that is *anywhere* inside

No, if it is *somewhere* then flushptrbuf() will see that's a pointer into
middle of a block and drop type info. So it will magically work.

> of a struct, the entire struct is assumed to be of the same type as the slice.

> Correct?

Correct. But it happens iff the slice points at the beginning of the memory
block.


> It sounds like the logic for LOOP is incorrect.  There should be (at least) a
> TODO at the site where it is generated referring to this issue.  Do we have a
> bug for this?

The LOOP is not generated, it was added right here.

[dvyukov, 2013-05-15 19:24:34]

On 2013/05/15 18:39:01, cshapiro1 wrote:
> https://codereview.appspot.com/9372044/diff/5001/src/pkg/runtime/mgc0.c
> File src/pkg/runtime/mgc0.c (right):
> 
>
https://codereview.appspot.com/9372044/diff/5001/src/pkg/runtime/mgc0.c#newco...
> src/pkg/runtime/mgc0.c:802: // Can't use slice element type for scanning,
> Can we get a better commit log description?

Done

[cshapiro1, 2013-05-15 19:34:27]

LGTM, thanks.

[minux1, 2013-05-15 19:46:54]

I think it's worth mentioning that this only happens iff
the array is at the beginning of the struct.

[dvyukov, 2013-05-15 19:50:44]

*** Submitted as https://code.google.com/p/go/source/detail?r=1abed5873071 ***

runtime: fix GC scanning of slices
If a slice points to an array embedded in a struct,
the whole struct can be incorrectly scanned as the slice buffer.
Fixes issue 5443.

R=cshapiro, iant, r, cshapiro, minux.ma
CC=bradfitz, gobot, golang-dev
https://codereview.appspot.com/9372044

[dvyukov, 2013-05-15 19:51:06]

On 2013/05/15 19:46:54, minux wrote:
> I think it's worth mentioning that this only happens iff
> the array is at the beginning of the struct.

Done