go: cmd/compile: -d=checkptr doesn't handle reflect.{Slice,String}Header.Data

This program is valid, yet -d=checkptr flags it as unsafe pointer arithmetic:

package main

import (
	"reflect"
	"unsafe"
)

var s []int

func main() {
	s = make([]int, 10)
	h := (*reflect.SliceHeader)(unsafe.Pointer(&s))
	println(unsafe.Pointer(h.Data))
}

This is because I forgot to implement logic to handle rule 6 for -d=checkptr.

About this issue

Original URL
State: closed
Created 5 years ago
Reactions: 1
Comments: 16 (14 by maintainers)

Most upvoted comments

@mdempsky I messed with my dev toolchain at first, I was using an outdated binary. With the latest changes at master and the change you suggested regarding shdr := *(*reflect.SliceHeader)(v) it passes with -d=checkptr.

I was aware of the issues reported by vet but I had no idea what was wrong, until I stumbled upon your recent CL, so I gave it another try.

Anyway, I appreciate you took some time to help me address these issues with this project as part of your ongoing changes to the toolchain, so thank you.

wI2L on Oct 21, 2019

These changes make TestCompositeTypes pass under -d=checkptr for me:

diff --git a/instructions.go b/instructions.go
index b7f95fb..5de1cbf 100644
--- a/instructions.go
+++ b/instructions.go
@@ -972,7 +972,7 @@ func newSliceInstr(t reflect.Type) (Instruction, error) {
                esz = t.Elem().Size()
        }
        return func(v unsafe.Pointer, w Writer, es *encodeState) error {
-               shdr := *(*reflect.SliceHeader)(v)
+               shdr := (*reflect.SliceHeader)(v)
                if shdr.Data == zero {
                        if es.opts.nilSliceEmpty {
                                _, err := w.WriteString("[]")
@@ -994,9 +994,9 @@ func newSliceInstr(t reflect.Type) (Instruction, error) {
                                        return err
                                }
                        }
-                       var p unsafe.Pointer
+                       p := unsafe.Pointer(shdr.Data)
                        if isPtr {
-                               p = unsafe.Pointer(*(*uintptr)(unsafe.Pointer(shdr.Data + (i * esz))))
+                               p = unsafe.Pointer(*(*uintptr)(unsafe.Pointer(uintptr(p) + (i * esz))))
                                if p == nil {
                                        if _, err := w.WriteString("null"); err != nil {
                                                return err
@@ -1004,7 +1004,7 @@ func newSliceInstr(t reflect.Type) (Instruction, error) {
                                        continue
                                }
                        } else {
-                               p = unsafe.Pointer(shdr.Data + (i * esz))
+                               p = unsafe.Pointer(uintptr(p) + (i * esz))
                        }
                        // Encode the nth element of the slice.
                        if err := eins(p, w, es); err != nil {

There were two issues:

You can’t write *(*reflect.SliceHeader)(v), because that makes an unsafe copy of the slice header. You’re only allowed to use a *reflect.SliceHeader that points to an actual slice-typed variable. (It looks like a similar issue is in byteSliceInstr.)
As mentioned before, the shdr.Data has to be converted to unsafe.Pointer before it can be converted back to uintptr for use in pointer arithmetic.

mdempsky on Oct 21, 2019

Thanks, but I already have a patch. I just haven’t uploaded it yet.

mdempsky on Oct 21, 2019

@cuonglm I mean that cmd/compile does safely compile that program, but package unsafe doesn’t guarantee it to be safe. So it’s okay for that program that to fail with -d=checkptr.

mdempsky on Oct 21, 2019