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

runtime: findObject crashing on pointer to stack #35068

Closed
mdempsky opened this issue Oct 22, 2019 · 7 comments
Closed

runtime: findObject crashing on pointer to stack #35068

mdempsky opened this issue Oct 22, 2019 · 7 comments
Milestone

Comments

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Oct 22, 2019

Splitting this out from #34972:

$ go test -v -short -gcflags=all=-d=checkptr testing/quick -run=TestMutuallyRecursive
=== RUN   TestMutuallyRecursive
runtime: pointer 0xc00011c3e0 to unallocated span span.base()=0xc00011c000 span.limit=0xc00012c000 span.state=3
fatal error: found bad pointer in Go heap (incorrect use of unsafe or cgo?)

goroutine 7 [running]:
runtime.throw(0x596d14, 0x3e)
	/usr/local/google/home/mdempsky/wd/go/src/runtime/panic.go:774 +0x72 fp=0xc00013c308 sp=0xc00013c2d8 pc=0x42ec12
runtime.findObject(0xc00011c3e0, 0x0, 0x0, 0x0, 0x0, 0x0)
	/usr/local/google/home/mdempsky/wd/go/src/runtime/mbitmap.go:397 +0x3b4 fp=0xc00013c358 sp=0xc00013c308 pc=0x413e04
runtime.checkptrBase(...)
	/usr/local/google/home/mdempsky/wd/go/src/runtime/checkptr.go:56
runtime.checkptrAlignment(0xc00013c3e0, 0x566020, 0x1)
	/usr/local/google/home/mdempsky/wd/go/src/runtime/checkptr.go:23 +0x76 fp=0xc00013c3c0 sp=0xc00013c358 pc=0x406306
reflect.packEface(0x561b60, 0x0, 0x19, 0xff00000000000000, 0x0)
	/usr/local/google/home/mdempsky/wd/go/src/reflect/value.go:106 +0x4c fp=0xc00013c400 sp=0xc00013c3c0 pc=0x4b1c3c
reflect.valueInterface(0x561b60, 0x0, 0x19, 0x1, 0x19, 0xc000096590)
	/usr/local/google/home/mdempsky/wd/go/src/reflect/value.go:1023 +0x12b fp=0xc00013c450 sp=0xc00013c400 pc=0x4b670b
reflect.Value.Interface(...)
	/usr/local/google/home/mdempsky/wd/go/src/reflect/value.go:993
testing/quick.sizedValue(0x5be680, 0x561b60, 0xc0000bc330, 0x32, 0xc00000eb10, 0x196, 0x7ff03c946008, 0x0)
	/usr/local/google/home/mdempsky/wd/go/src/testing/quick/quick.go:67 +0x77 fp=0xc00013c5d8 sp=0xc00013c450 pc=0x52dc77
...

Judging by the sp and fp values, it looks like 0xc00011c3e0 is a valid pointer into packEface's stack frame. But findObject says the span's state is 3 (mSpanFree).

Since it's pointing into a stack frame, shouldn't it be mSpanManual?

/cc @aclements

@mdempsky

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Oct 22, 2019

Oops, no, 0xc00011c3e0 is not a valid pointer into packEface's stack frame. The stack frame is from sp=0xc00013c3c0 to fp=0xc00013c400.

The problem seems to be that findObject takes the pointer parameter as a uintptr, but it's not marked as //go:nosplit. So if the pointer is to the local stack and calling findObject happens to require the stack to be reallocated, then we'll call spanOf for the old pointer.

I see a few options:

  1. Mark findObject as //go:nosplit.

  2. Change findObject to take the pointer as unsafe.Pointer instead of uintptr.

  3. Change checkptr.go to use something other than findObject.

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Oct 22, 2019

@mdempsky How about changing findObject to use spanOfHeap instead of spanOf?

@mdempsky

This comment has been minimized.

Copy link
Member Author

@mdempsky mdempsky commented Oct 22, 2019

@cuonglm I don't know, sorry. I'm not really familiar with how the GC routines are organized. I have to defer to @aclements here on identifying the preferred solution.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 22, 2019

Change https://golang.org/cl/202639 mentions this issue: runtime: fix -d=checkptr failure for testing/quick

@aclements

This comment has been minimized.

Copy link
Member

@aclements aclements commented Oct 22, 2019

Interesting.

findObject can't take the pointer as an unsafe.Pointer because it could be an invalid pointer (part of the point of findObject is to validate the pointer). If a stack growth was triggered by the call while an invalid pointer argument was on the stack, stack growth could panic.

Calling spanOfHeap doesn't help because findObject could still have grown the stack, leaving p as a potentially invalid pointer. spanOfHeap wouldn't let findObject do any validity checking.

I would be okay with making findObject nosplit. We may want to pull the debug.invalidptr case into a helper function to keep findObject's frame small.

gopherbot pushed a commit that referenced this issue Oct 22, 2019
This CL extends checkptrBase to recognize pointers into the stack and
data/bss sections. I was meaning to do this eventually anyway, but
it's also an easy way to workaround #35068.

Updates #35068.

Change-Id: Ib47f0aa800473a4fbc249da52ff03bec32c3ebe2
Reviewed-on: https://go-review.googlesource.com/c/go/+/202639
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 23, 2019

Change https://golang.org/cl/202798 mentions this issue: runtime: mark findObject nosplit

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 23, 2019

Change https://golang.org/cl/202797 mentions this issue: runtime: factor out debug.invalidptr case in findObject

gopherbot pushed a commit that referenced this issue Oct 26, 2019
This helps keeping findObject's frame small.

Updates #35068

Change-Id: I1b8c1fcc5831944c86f1a30ed2f2d867a5f2b242
Reviewed-on: https://go-review.googlesource.com/c/go/+/202797
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
@gopherbot gopherbot closed this in 66f78e9 Oct 26, 2019
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.