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
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@mdempsky
Copy link
Member

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 The path to resolution is known, but the work has not been done. 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
Copy link
Member Author

Thanks to @wI2L for the report.

@mdempsky
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

@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
Copy link
Member

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
Copy link
Member Author

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

@gopherbot
Copy link

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

@gopherbot
Copy link

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

@wI2L
Copy link
Contributor

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.

@mdempsky
Copy link
Member Author

@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
Copy link
Member Author

Nevermind, I can reproduce the failure locally. Investigating.

@mdempsky mdempsky reopened this Oct 21, 2019
@mdempsky
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

Happy to help. Thanks again for the report.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

4 participants