code review 9988043: encoding/gob: consistently use unsafe.Pointer for point...

encoding/gob: consistently use unsafe.Pointer for pointer values

Fixes issue 5621.

[iant, 2013-06-04 00:01:42]

Hello golang-dev@googlegroups.com,

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

[iant, 2013-06-04 00:02:30]

I'm not sure this fixes issue 5621, it needs testing.

[cshapiro1, 2013-06-04 00:12:18]

https://codereview.appspot.com/9988043/diff/1/src/pkg/encoding/gob/encode.go
File src/pkg/encoding/gob/encode.go (right):

https://codereview.appspot.com/9988043/diff/1/src/pkg/encoding/gob/encode.go#...
src/pkg/encoding/gob/encode.go:376: func (enc *Encoder) encodeArray(b
*bytes.Buffer, p uintptr, op encOp, elemWid uintptr, elemIndir int, length int)
{
I think p needs to be converted to an unsafe.Pointer as well.

[r, 2013-06-04 00:24:54]

https://codereview.appspot.com/9988043/diff/1/src/pkg/encoding/gob/encode.go
File src/pkg/encoding/gob/encode.go (right):

https://codereview.appspot.com/9988043/diff/1/src/pkg/encoding/gob/encode.go#...
src/pkg/encoding/gob/encode.go:376: func (enc *Encoder) encodeArray(b
*bytes.Buffer, p uintptr, op encOp, elemWid uintptr, elemIndir int, length int)
{
On 2013/06/04 00:12:18, cshapiro1 wrote:
> I think p needs to be converted to an unsafe.Pointer as well.

i think so too

[iant, 2013-06-04 05:46:33]

On 2013/06/04 00:12:18, cshapiro1 wrote:
> https://codereview.appspot.com/9988043/diff/1/src/pkg/encoding/gob/encode.go
> File src/pkg/encoding/gob/encode.go (right):
> 
>
https://codereview.appspot.com/9988043/diff/1/src/pkg/encoding/gob/encode.go#...
> src/pkg/encoding/gob/encode.go:376: func (enc *Encoder) encodeArray(b
> *bytes.Buffer, p uintptr, op encOp, elemWid uintptr, elemIndir int, length
int)
> {
> I think p needs to be converted to an unsafe.Pointer as well.


Done.

Ian

[albert.strasheim, 2013-06-04 06:38:31]

LGTM

On 2013/06/04 00:02:30, iant wrote:
> I'm not sure this fixes issue 5621, it needs testing.

Test passes with the latest patch.

Could someone explain a bit more about what this bug is? If it happened in gob,
I'm guessing we've made this mistake in at least one place in our codebase.

Regards

Albert

[r, 2013-06-04 10:30:55]

LGTM

[iant, 2013-06-04 13:20:34]

On 2013/06/04 06:38:31, albert.strasheim wrote:
> 
> Could someone explain a bit more about what this bug is? If it happened in
gob,
> I'm guessing we've made this mistake in at least one place in our codebase.

The bug arises because Carl has improved the GC so that it does precise tracking
of function arguments.  That means that if there is a function pointer that
holds a pointer value, but does not have a pointer type, that the GC will not
see the reference to the memory.  The encoding/gob package was making this
mistake: it was holding pointer values in function arguments with the type
uintptr.  This patch changes those to unsafe.Pointer, so that the GC can see
that they are pointers.

So this problem can arise in packages that import unsafe, use unsafe pointers,
and then convert those unsafe pointers to uintptr and pass them around.

In later patches this is going to be come an issue with local variables as well.

Ian

[iant, 2013-06-04 13:21:03]

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

encoding/gob: consistently use unsafe.Pointer for pointer values

Fixes issue 5621.

R=golang-dev, cshapiro, r, fullung
CC=golang-dev
https://codereview.appspot.com/9988043