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

Jump to bottom

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
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.14

Comments

@mdempsky
Copy link
Member

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 mdempsky added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 22, 2019
@mdempsky mdempsky added this to the Go1.14 milestone Oct 22, 2019
@mdempsky mdempsky mentioned this issue Oct 22, 2019
Closed
@mdempsky
Copy link
Member Author

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

cuonglm commented Oct 22, 2019

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

@mdempsky
Copy link
Member Author

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

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

@aclements
Copy link
Member

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
@mdempsky
runtime: fix -d=checkptr failure for testing/quick
5a5854c 
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
Copy link

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

@gopherbot
Copy link

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
@cuonglm @aclements
runtime: factor out debug.invalidptr case in findObject
813d8e8 
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>
@golang golang locked and limited conversation to collaborators Oct 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.14
Development

No branches or pull requests
4 participants
@mdempsky @aclements @cuonglm @gopherbot