Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mdempsky opened this issue Oct 21, 2019 · 16 comments
Closed

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

mdempsky opened this issue Oct 21, 2019 · 16 comments
Assignees
Labels
Milestone

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Oct 21, 2019

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 mdempsky added the NeedsFix label Oct 21, 2019
@mdempsky mdempsky added this to the Go1.14 milestone Oct 21, 2019
@mdempsky mdempsky self-assigned this Oct 21, 2019
@mdempsky

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Oct 21, 2019

Thanks to @wI2L for the report.

@mdempsky

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Oct 21, 2019

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

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Oct 21, 2019

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

This comment has been minimized.

Copy link
Member Author

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

@cuonglm

This comment has been minimized.

Copy link
Contributor

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

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

@mdempsky

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Oct 21, 2019

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 21, 2019

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 21, 2019

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

@wI2L

This comment has been minimized.

Copy link
Contributor

@wI2L wI2L commented Oct 21, 2019

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

@gopherbot gopherbot closed this in b282efa Oct 21, 2019
@mdempsky

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Oct 21, 2019

@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

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Oct 21, 2019

Nevermind, I can reproduce the failure locally. Investigating.

@mdempsky mdempsky reopened this Oct 21, 2019
@mdempsky

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented 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:

  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 mdempsky closed this Oct 21, 2019
@mdempsky

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Oct 21, 2019

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

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Oct 21, 2019

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

This comment has been minimized.

Copy link
Contributor

@wI2L wI2L commented Oct 21, 2019

@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

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Oct 21, 2019

Happy to help. Thanks again for the report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.