code review 5266050: reflect: store unsafe.Pointer in {Slice,String}Header (issue 5266050)

[hect...@gmail.com]

Reviewers: rsc,

Message:
Hello r...@golang.org (cc: brad...@golang.org,
golan...@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

Description:
reflect: store unsafe.Pointer in {Slice,String}Header

This is in order to mark these types as containing pointers, so the gc
won't free the memory behind the Data field from under us.

Please review this at http://codereview.appspot.com/5266050/

Affected files:
M src/pkg/crypto/tls/root_windows.go
M src/pkg/gob/decode.go
M src/pkg/gob/encode.go
M src/pkg/reflect/value.go

Index: src/pkg/crypto/tls/root_windows.go
===================================================================
--- a/src/pkg/crypto/tls/root_windows.go
+++ b/src/pkg/crypto/tls/root_windows.go
@@ -26,7 +26,7 @@

var asn1Slice []byte
hdrp := (*reflect.SliceHeader)(unsafe.Pointer(&asn1Slice))
- hdrp.Data = cert.EncodedCert
+ hdrp.Data = unsafe.Pointer(cert.EncodedCert)
hdrp.Len = int(cert.Length)
hdrp.Cap = int(cert.Length)

Index: src/pkg/gob/decode.go
===================================================================
--- a/src/pkg/gob/decode.go
+++ b/src/pkg/gob/decode.go
@@ -662,11 +662,11 @@
// Always write a header at p.
hdrp := (*reflect.SliceHeader)(unsafe.Pointer(p))
if hdrp.Cap < n {
- hdrp.Data = uintptr(unsafe.NewArray(atyp.Elem(), n))
+ hdrp.Data = unsafe.NewArray(atyp.Elem(), n)
hdrp.Cap = n
}
hdrp.Len = n
- dec.decodeArrayHelper(state, hdrp.Data, elemOp, elemWid, n, elemIndir,
ovfl)
+ dec.decodeArrayHelper(state, uintptr(hdrp.Data), elemOp, elemWid, n,
elemIndir, ovfl)
}

// ignoreSlice skips over the data for a slice value with no destination.
Index: src/pkg/gob/encode.go
===================================================================
--- a/src/pkg/gob/encode.go
+++ b/src/pkg/gob/encode.go
@@ -560,7 +560,7 @@
return
}
state.update(i)
- state.enc.encodeArray(state.b, slice.Data, *elemOp, t.Elem().Size(),
indir, int(slice.Len))
+ state.enc.encodeArray(state.b, uintptr(slice.Data), *elemOp,
t.Elem().Size(), indir, int(slice.Len))
}
case reflect.Array:
// True arrays have size in the type.
Index: src/pkg/reflect/value.go
===================================================================
--- a/src/pkg/reflect/value.go
+++ b/src/pkg/reflect/value.go
@@ -841,7 +841,7 @@
panic("reflect: slice index out of range")
}
typ := iv.typ.Elem()
- addr := unsafe.Pointer(s.Data + uintptr(i)*typ.Size())
+ addr := unsafe.Pointer(uintptr(s.Data) + uintptr(i)*typ.Size())
return valueFromAddr(flag, typ, addr)
}

@@ -1170,7 +1170,7 @@
}
return uintptr(iv.word)
case Slice:
- return (*SliceHeader)(iv.addr).Data
+ return uintptr((*SliceHeader)(iv.addr).Data)
}
panic(&ValueError{"reflect.Value.Pointer", iv.kind})
}
@@ -1423,10 +1423,10 @@
base = uintptr(iv.addr)
case Slice:
typ = iv.typ.toType()
- base = (*SliceHeader)(iv.addr).Data
+ base = uintptr((*SliceHeader)(iv.addr).Data)
}
s := new(SliceHeader)
- s.Data = base + uintptr(beg)*typ.Elem().Size()
+ s.Data = unsafe.Pointer(base + uintptr(beg)*typ.Elem().Size())
s.Len = end - beg
s.Cap = cap - beg
return valueFromAddr(iv.flag&flagRO, typ, unsafe.Pointer(s))
@@ -1522,14 +1522,14 @@
// StringHeader is the runtime representation of a string.
// It cannot be used safely or portably.
type StringHeader struct {
- Data uintptr
+ Data unsafe.Pointer
Len int
}

// SliceHeader is the runtime representation of a slice.
// It cannot be used safely or portably.
type SliceHeader struct {
- Data uintptr
+ Data unsafe.Pointer
Len int
Cap int
}
@@ -1656,7 +1656,7 @@
panic("reflect: MakeSlice of non-slice type")
}
s := &SliceHeader{
- Data: uintptr(unsafe.NewArray(typ.Elem(), cap)),
+ Data: unsafe.NewArray(typ.Elem(), cap),
Len: len,
Cap: cap,
}

[alex.b...@gmail.com]

Can't comment about your change. Do not know enough about gc.

http://codereview.appspot.com/5266050/diff/5001/src/pkg/crypto/tls/root_windows.go
File src/pkg/crypto/tls/root_windows.go (right):

http://codereview.appspot.com/5266050/diff/5001/src/pkg/crypto/tls/root_windows.go#newcode29
src/pkg/crypto/tls/root_windows.go:29: hdrp.Data =
unsafe.Pointer(cert.EncodedCert)
Please, get rid of reflect altogether.

diff -r f6f99bbc5576 src/pkg/crypto/tls/root_windows.go
--- a/src/pkg/crypto/tls/root_windows.go Sun Oct 16 20:50:11 2011 +1100
+++ b/src/pkg/crypto/tls/root_windows.go Mon Oct 17 09:59:57 2011 +1100
@@ -6,7 +6,6 @@

import (
"crypto/x509"
- "reflect"
"syscall"
"unsafe"
)
@@ -23,17 +22,8 @@
if cert == nil {
break
}
-
- var asn1Slice []byte
- hdrp := (*reflect.SliceHeader)(unsafe.Pointer(&asn1Slice))
- hdrp.Data = cert.EncodedCert
- hdrp.Len = int(cert.Length)
- hdrp.Cap = int(cert.Length)
-
- buf := make([]byte, len(asn1Slice))
- copy(buf, asn1Slice)
-
- if cert, err := x509.ParseCertificate(buf); err == nil {
+ p := (*[1<<20]byte)(unsafe.Pointer(cert.EncodedCert))
+ if cert, err := x509.ParseCertificate((*p)[:cert.Length]); err == nil
{
roots.AddCert(cert)
}
}

http://codereview.appspot.com/5266050/

[dvy...@google.com]

Ouch!
Don't we want 6g to consider types containing uintptr's as containing
pointers?

http://codereview.appspot.com/5266050/

[dvy...@google.com]

On 2011/10/17 07:05:05, dvyukov wrote:
> Ouch!
> Don't we want 6g to consider types containing uintptr's as containing
pointers?

I think we do, []uintptr was always dangerous.
I can reproduce it on my machine, please, test the following patch:
http://codereview.appspot.com/5278048/

http://codereview.appspot.com/5266050/

[dvy...@google.com]

On 2011/10/17 07:15:43, dvyukov wrote:
> On 2011/10/17 07:05:05, dvyukov wrote:
> > Ouch!
> > Don't we want 6g to consider types containing uintptr's as
containing
> pointers?

> I think we do, []uintptr was always dangerous.
> I can reproduce it on my machine, please, test the following patch:

/\/\/\/\/\
can *not*

> http://codereview.appspot.com/5278048/

http://codereview.appspot.com/5266050/

[Hector Chu]

On 17 October 2011 08:05, <dvy...@google.com> wrote:
> Ouch!
> Don't we want 6g to consider types containing uintptr's as containing
> pointers?

Maybe. I did consider this solution.
I'll let Russ decide on what's best.

[kra...@gmail.com]

The copy should be kept around. The struct returned from
x509.ParseCertificate contains subslices of the passed-in
data.

Every time we call CertEnumCertificatesInStore, it frees
the previous CertContext.

http://codereview.appspot.com/5266050/

[Russ Cox]

I'd prefer to use Dmitriy's patch for now.
This is a compiler bug introduced only in the
last few days. It may be necessary to make an
API change, but for now I'd prefer to just undo
a little bit of the compiler change and buy more
time to think about any possible API change.

Thanks.
Russ

[hect...@gmail.com]

*** Abandoned ***

http://codereview.appspot.com/5266050/