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

[mdempsky]

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.

[mdempsky]

Thanks to @wI2L for the report.

[mdempsky]

Also, I'll note that @wI2L's report actually involved unsafe.Pointer(h.Data + x). While I think cmd/compile does allow that today, my reading of the unsafe.Pointer safety rules is that is not guaranteed. I think that computation has to be written as unsafe.Pointer(uintptr(unsafe.Pointer(h.Data)) + x) (i.e., first apply rule 6 to convert h.Data to unsafe.Pointer; then apply rule 3 to convert to uintptr, apply arithmetic, and convert back to unsafe.Pointer).

[cuonglm]

Also, I'll note that @wI2L's report actually involved unsafe.Pointer(h.Data + x). While I think cmd/compile does allow that today, my reading of the unsafe.Pointer safety rules is that is _not_ guaranteed. I think that computation has to be written as unsafe.Pointer(uintptr(unsafe.Pointer(h.Data)) + x) (i.e., first apply rule 6 to convert h.Data to unsafe.Pointer; then apply rule 3 to convert to uintptr, apply arithmetic, and convert back to unsafe.Pointer).

This one:

    s = make([]int, 10)
    h := (*reflect.StringHeader)(unsafe.Pointer(&s))
    println(unsafe.Pointer(h.Data + 1))

fails with -d=checkptr, or you mean different things when saying "cmd/compile does allow that"?

[mdempsky]

@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.

[cuonglm]

@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.

I see you assigned yourself after I have a fix in my local. Do you want me to send the patch?

[mdempsky]

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

[gopherbot]

Change https://golang.org/cl/202560 mentions this issue: cmd/compile: recognize reflect.{Slice,String}Header for -d=checkptr

[gopherbot]

Change https://golang.org/cl/202558 mentions this issue: cmd/compile: simplify isReflectHeaderDataField

[wI2L]

@mdempsky I cherry picked the two commits (CL 202558 then CL 202560) and applied to clean dev tree and rebuilt to test against my codebase with the change you suggested to convert the Data field to unsafe.Pointer then to uintprt before the arithmetic operation
p = unsafe.Pointer(uintptr(unsafe.Pointer(shdr.Data)) + (i * esz))
and the same panic is till there.
I am reporting it because I found out that the trybots log doesn't show any failure for the test you added in test/fixedbugs/issue35027.go (I can only assume that it stopped prematurely after the failure in issue19168.go).

EDIT: Just saw you're active on making changes, sorry for the noise, I'll give it another try once you are done.

[mdempsky]

@wI2L Hm, that's weird. Can you try at master? I think that should be good.

If it's still failing with the extra conversions, can you help me figure out how to reproduce the failure locally so I can investigate? Thanks!

[mdempsky]

Nevermind, I can reproduce the failure locally. Investigating.

[mdempsky]

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:

1. 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.)

2. 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]

Finally, unsafe.Pointer(*(*uintptr)(p)) technically isn't safe either. I'd probably write:

p := unsafe.Pointer(shdr.Data)
p = unsafe.Pointer(uintptr(p) + i * esz)
if isPtr {
    p = *(*unsafe.Pointer)(p)
    if p == nil { ... }
}

[mdempsky]

Also, it looks like cmd/vet was already flagging several of these issues:

$ go vet .
# github.com/wI2L/jettison
./instructions.go:924:9: possible misuse of unsafe.Pointer
./instructions.go:999:9: possible misuse of unsafe.Pointer
./instructions.go:999:36: possible misuse of unsafe.Pointer
./instructions.go:1007:9: possible misuse of unsafe.Pointer
./struct.go:213:9: possible misuse of unsafe.Pointer

[wI2L]

@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.

[mdempsky]

Happy to help. Thanks again for the report.