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

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.

[hector, 2011-10-16 21:55:40]

Hello rsc@golang.org (cc: bradfitz@golang.org, golang-dev@googlegroups.com),

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

[brainman, 2011-10-16 23:05:23]

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

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

http://codereview.appspot.com/5266050/diff/5001/src/pkg/crypto/tls/root_windo...
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)
 		}
 	}

[dvyukov, 2011-10-17 07:05:05]

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

[dvyukov, 2011-10-17 07:15:43]

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/

[dvyukov, 2011-10-17 07:16:19]

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/

[hector, 2011-10-17 07:23:56]

On 17 October 2011 08:05,  <dvyukov@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.

[mkrautz, 2011-10-17 12:22:27]

On 2011/10/16 23:05:23, brainman wrote:
> Can't comment about your change. Do not know enough about gc.
> 
>
http://codereview.appspot.com/5266050/diff/5001/src/pkg/crypto/tls/root_windo...
> File src/pkg/crypto/tls/root_windows.go (right):
> 
>
http://codereview.appspot.com/5266050/diff/5001/src/pkg/crypto/tls/root_windo...
> 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)
>  		}
>  	}

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.

[rsc, 2011-10-17 18:57:49]

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

[hector, 2011-10-17 19:33:42]

*** Abandoned ***