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: should checkptrArithmetic accept a uintptr instead of unsafe.Pointer? #35379

Open
ianlancetaylor opened this issue Nov 5, 2019 · 19 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@ianlancetaylor
Copy link
Contributor

Two instances of

runtime: bad pointer in frame runtime_test.testSetPanicOnFault at 0xc000206de0: 0x1
fatal error: invalid pointer found on stack

darwin-amd64-race: https://build.golang.org/log/05f53140b89337ef848d51e83ffb8e19aabd27e0
linux-amd64-race (trybot): https://storage.googleapis.com/go-build-log/7828558a/linux-amd64-race_de2789c5.log

@ianlancetaylor ianlancetaylor added this to the Go1.14 milestone Nov 5, 2019
@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 5, 2019
@ianlancetaylor
Copy link
Contributor Author

@bcmills
Copy link
Contributor

bcmills commented Nov 6, 2019

@danscales, could this be related to open-coded defers?

@ianlancetaylor
Copy link
Contributor Author

CC @mdempsky

The stack traces all show a call to checkptrArithmetic, so this could also be related to the checkptr work.

@ianlancetaylor
Copy link
Contributor Author

It took 106 tries, but I was able to recreate the problem with

go test -gcflags=-d=checkptr -test.run=TestSetPanicOnFault -test.count=1 runtime
runtime: bad pointer in frame runtime_test.testSetPanicOnFault at 0xc0000f4ec8: 0x1
fatal error: invalid pointer found on stack

runtime stack:
runtime.throw(0x66b41e, 0x1e)
	/home/iant/go/src/runtime/panic.go:1106 +0x72 fp=0x7f8259184730 sp=0x7f8259184700 pc=0x433832
runtime.adjustpointers(0xc0000f4ec8, 0x7f8259184830, 0x7f8259184bc0, 0x7e7110, 0x814100)
	/home/iant/go/src/runtime/stack.go:599 +0x220 fp=0x7f8259184790 sp=0x7f8259184730 pc=0x44d120
runtime.adjustframe(0x7f8259184ad0, 0x7f8259184bc0, 0x814100)
	/home/iant/go/src/runtime/stack.go:641 +0x34e fp=0x7f8259184860 sp=0x7f8259184790 pc=0x44d47e
runtime.gentraceback(0xffffffffffffffff, 0xffffffffffffffff, 0x0, 0xc000082480, 0x0, 0x0, 0x7fffffff, 0x674630, 0x7f8259184bc0, 0x0, ...)
	/home/iant/go/src/runtime/traceback.go:334 +0x1101 fp=0x7f8259184b38 sp=0x7f8259184860 pc=0x45a361
runtime.copystack(0xc000082480, 0x1000)
	/home/iant/go/src/runtime/stack.go:888 +0x291 fp=0x7f8259184cf0 sp=0x7f8259184b38 pc=0x44db81
runtime.newstack()
	/home/iant/go/src/runtime/stack.go:1042 +0x20b fp=0x7f8259184e80 sp=0x7f8259184cf0 pc=0x44de4b
runtime.morestack()
	/home/iant/go/src/runtime/asm_amd64.s:449 +0x8f fp=0x7f8259184e88 sp=0x7f8259184e80 pc=0x467b1f

goroutine 19 [copystack]:
runtime.mmap(0x0, 0x15f8f8, 0x2200000003, 0xffffffff, 0x0, 0x0)
	/home/iant/go/src/runtime/cgo_mmap.go:23 +0x16a fp=0xc0000f4b08 sp=0xc0000f4b00 pc=0x4048ca
runtime.sysAlloc(0x15f8f8, 0x84d3b0, 0x42beb6)
	/home/iant/go/src/runtime/mem_linux.go:21 +0x3e fp=0xc0000f4b58 sp=0xc0000f4b08 pc=0x41a74e
runtime.stkbucket(0x1, 0x20, 0xc0000f4c00, 0x5, 0x20, 0x101, 0x7f825a387fff)
	/home/iant/go/src/runtime/mprof.go:207 +0x2c8 fp=0xc0000f4bb8 sp=0xc0000f4b58 pc=0x42baa8
runtime.mProf_Malloc(0xc0000ee000, 0x20)
	/home/iant/go/src/runtime/mprof.go:344 +0xd6 fp=0xc0000f4d30 sp=0xc0000f4bb8 pc=0x42bf16
runtime.profilealloc(0xc000048a80, 0xc0000ee000, 0x20)
	/home/iant/go/src/runtime/malloc.go:1183 +0x59 fp=0xc0000f4d50 sp=0xc0000f4d30 pc=0x40f2f9
runtime.mallocgc(0x20, 0x63c040, 0xc0000ec001, 0xc0000824a8)
	/home/iant/go/src/runtime/malloc.go:1102 +0x471 fp=0xc0000f4df0 sp=0xc0000f4d50 pc=0x40e9d1
runtime.convT2E(0x63c040, 0xc0000f4e58, 0x0, 0x1)
	/home/iant/go/src/runtime/iface.go:324 +0x38 fp=0xc0000f4e28 sp=0xc0000f4df0 pc=0x40c3d8
runtime.checkptrArithmetic(0x1, 0x0, 0x0, 0x0)
	/home/iant/go/src/runtime/checkptr.go:47 +0x14c fp=0xc0000f4e88 sp=0xc0000f4e28 pc=0x40833c
runtime_test.testSetPanicOnFault(0xc0000da120, 0x1, 0xc000040748)
	/home/iant/go/src/runtime/runtime_test.go:210 +0x73 fp=0xc0000f4f10 sp=0xc0000f4e88 pc=0x5b8513
runtime_test.TestSetPanicOnFault(0xc0000da120)
	/home/iant/go/src/runtime/runtime_test.go:188 +0xa1 fp=0xc0000f4f80 sp=0xc0000f4f10 pc=0x5b8411
testing.tRunner(0xc0000da120, 0x676140)
	/home/iant/go/src/testing/testing.go:954 +0xdc fp=0xc0000f4fd0 sp=0xc0000f4f80 pc=0x4e0f8c
runtime.goexit()
	/home/iant/go/src/runtime/asm_amd64.s:1375 +0x1 fp=0xc0000f4fd8 sp=0xc0000f4fd0 pc=0x469ae1
created by testing.(*T).Run
	/home/iant/go/src/testing/testing.go:1005 +0x357

goroutine 1 [chan receive, locked to thread]:
runtime.gopark(0x6746c8, 0xc00007e298, 0x170e, 0x2)
	/home/iant/go/src/runtime/proc.go:304 +0xe0 fp=0xc000052b20 sp=0xc000052b00 pc=0x4360d0
runtime.chanrecv(0xc00007e240, 0xc000052c37, 0xc000000101, 0x4e1357)
	/home/iant/go/src/runtime/chan.go:563 +0x338 fp=0xc000052bb0 sp=0xc000052b20 pc=0x407858
runtime.chanrecv1(0xc00007e240, 0xc000052c37)
	/home/iant/go/src/runtime/chan.go:433 +0x2b fp=0xc000052be0 sp=0xc000052bb0 pc=0x4074cb
testing.(*T).Run(0xc0000da120, 0x66522c, 0x13, 0x676140, 0x4b0301)
	/home/iant/go/src/testing/testing.go:1006 +0x37e fp=0xc000052c90 sp=0xc000052be0 pc=0x4e137e
testing.runTests.func1(0xc0000da000)
	/home/iant/go/src/testing/testing.go:1247 +0x78 fp=0xc000052ce0 sp=0xc000052c90 pc=0x4e5198
testing.tRunner(0xc0000da000, 0xc000052dc0)
	/home/iant/go/src/testing/testing.go:954 +0xdc fp=0xc000052d30 sp=0xc000052ce0 pc=0x4e0f8c
testing.runTests(0xc00009c0a0, 0x82a3e0, 0x13b, 0x13b, 0x0)
	/home/iant/go/src/testing/testing.go:1245 +0x2a7 fp=0xc000052df0 sp=0xc000052d30 pc=0x4e2887
testing.(*M).Run(0xc0000ce000, 0x0)
	/home/iant/go/src/testing/testing.go:1162 +0x15f fp=0xc000052ed0 sp=0xc000052df0 pc=0x4e17df
runtime_test.TestMain(0xc0000ce000)
	/home/iant/go/src/runtime/crash_test.go:28 +0x2f fp=0xc000052f20 sp=0xc000052ed0 pc=0x577cdf
main.main()
	_testmain.go:1144 +0x135 fp=0xc000052f88 sp=0xc000052f20 pc=0x5e65b5
runtime.main()
	/home/iant/go/src/runtime/proc.go:203 +0x212 fp=0xc000052fe0 sp=0xc000052f88 pc=0x435cf2
runtime.goexit()
	/home/iant/go/src/runtime/asm_amd64.s:1375 +0x1 fp=0xc000052fe8 sp=0xc000052fe0 pc=0x469ae1

goroutine 2 [force gc (idle)]:
runtime.gopark(0x6749c0, 0x82cf40, 0x1411, 0x1)
	/home/iant/go/src/runtime/proc.go:304 +0xe0 fp=0xc000044fb0 sp=0xc000044f90 pc=0x4360d0
runtime.goparkunlock(...)
	/home/iant/go/src/runtime/proc.go:310
runtime.forcegchelper()
	/home/iant/go/src/runtime/proc.go:253 +0xb7 fp=0xc000044fe0 sp=0xc000044fb0 pc=0x435f87
runtime.goexit()
	/home/iant/go/src/runtime/asm_amd64.s:1375 +0x1 fp=0xc000044fe8 sp=0xc000044fe0 pc=0x469ae1
created by runtime.init.6
	/home/iant/go/src/runtime/proc.go:242 +0x35

goroutine 3 [GC sweep wait]:
runtime.gopark(0x6749c0, 0x82d340, 0x140c, 0x1)
	/home/iant/go/src/runtime/proc.go:304 +0xe0 fp=0xc0000457a8 sp=0xc000045788 pc=0x4360d0
runtime.goparkunlock(...)
	/home/iant/go/src/runtime/proc.go:310
runtime.bgsweep(0xc000022070)
	/home/iant/go/src/runtime/mgcsweep.go:70 +0x9c fp=0xc0000457d8 sp=0xc0000457a8 pc=0x425b6c
runtime.goexit()
	/home/iant/go/src/runtime/asm_amd64.s:1375 +0x1 fp=0xc0000457e0 sp=0xc0000457d8 pc=0x469ae1
created by runtime.gcenable
	/home/iant/go/src/runtime/mgc.go:214 +0x5c

goroutine 4 [GC scavenge wait]:
runtime.gopark(0x6749c0, 0x82d500, 0x140d, 0x1)
	/home/iant/go/src/runtime/proc.go:304 +0xe0 fp=0xc000045f40 sp=0xc000045f20 pc=0x4360d0
runtime.goparkunlock(...)
	/home/iant/go/src/runtime/proc.go:310
runtime.bgscavenge(0xc000022070)
	/home/iant/go/src/runtime/mgcscavenge.go:302 +0xe1 fp=0xc000045fd8 sp=0xc000045f40 pc=0x4251c1
runtime.goexit()
	/home/iant/go/src/runtime/asm_amd64.s:1375 +0x1 fp=0xc000045fe0 sp=0xc000045fd8 pc=0x469ae1
created by runtime.gcenable
	/home/iant/go/src/runtime/mgc.go:215 +0x7e

goroutine 18 [finalizer wait]:
runtime.gopark(0x6749c0, 0x849708, 0x81410, 0x1)
	/home/iant/go/src/runtime/proc.go:304 +0xe0 fp=0xc000044758 sp=0xc000044738 pc=0x4360d0
runtime.goparkunlock(...)
	/home/iant/go/src/runtime/proc.go:310
runtime.runfinq()
	/home/iant/go/src/runtime/mfinal.go:175 +0xa3 fp=0xc0000447e0 sp=0xc000044758 pc=0x41afd3
runtime.goexit()
	/home/iant/go/src/runtime/asm_amd64.s:1375 +0x1 fp=0xc0000447e8 sp=0xc0000447e0 pc=0x469ae1
created by runtime.createfing
	/home/iant/go/src/runtime/mfinal.go:156 +0x61
FAIL	runtime	0.028s
FAIL

@ianlancetaylor
Copy link
Contributor Author

Looks like with -d=checkptr we will routinely get a checkptr panic, which lets the test pass, but occasionally the checkptr panic will trigger a stack copy, and the stack copy will throw a bad pointer error. This is inherent to the test. There really is a bad pointer. That's kind of the point.

@bcmills
Copy link
Contributor

bcmills commented Nov 6, 2019

Could we isolate the bad pointer to a subprocess of the test, and have the test accept either the checkptr failure report or the bad pointer throw as valid outputs?

@mdempsky
Copy link
Member

mdempsky commented Nov 6, 2019

I think the issue here is that checkptrArithmetic takes the converted pointer as an unsafe.Pointer instead of uintptr. In the case of something like unsafe.Pointer(uintptr(1)) that means there's an invalid pointer on the stack.

Normally this causes checkptrArithmetic to panic; but if it's interrupted by GC, then the runtime will fatal error instead.

Two possible fixes:

  1. Just mark the test as //go:nocheckptr, since it's creating invalid pointers anyway.
  2. Change checkptrArithmetic to accept the arithmetic result as uintptr instead of unsafe.Pointer. (We might need/want to mark it and checkptrBase as nosplit too?)

@gopherbot
Copy link

Change https://golang.org/cl/205699 mentions this issue: runtime: skip TestPanicOnFault in race mode

@ianlancetaylor
Copy link
Contributor Author

Using //go:nocheckptr sounds like a better idea than CL 205699, so I'll try that.

@gopherbot
Copy link

Change https://golang.org/cl/205717 mentions this issue: runtime: mark testSetPanicOnFault as go:nocheckptr

@mdempsky
Copy link
Member

mdempsky commented Nov 6, 2019

@aclements I'm curious if you have any thoughts on the best code generation for the compiler to emit for checkptr.

For something like:

q := unsafe.Pointer(uintptr(p) + 1)

this is currently instrumented as:

tmp := unsafe.Pointer(uintptr(p) + 1)
checkptrArithmetic(tmp, []unsafe.Pointer{p})
q := tmp

This is okay when the pointer arithmetic is correct (and I think necessary in case p points to the stack, and checkptrArithmetic causes it to grow). But when the pointer arithmetic is invalid, there's a chance that the runtime sees the bad pointer on the stack and throws instead (e.g., this issue).

Probably checkptrArithmetic and checkptrBase should be //go:nosplit? Also, maybe they should take uintptr parameters and instrument as either:

tmp := uintptr(p) + 1
checkptrArithmetic(tmp, []unsafe.Pointer{p})
q := unsafe.Pointer(tmp)  // trust that checkptrArithmetic didn't invalidate tmp

or

checkptrArithmetic(uintptr(p) + 1, []unsafe.Pointer{p})
q := unsafe.Pointer(uintptr(p) + 1)  // recalculate arithmetic just to be safe

@mdempsky
Copy link
Member

mdempsky commented Nov 6, 2019

[Reopening to ensure the open questions above get answered.]

@mdempsky mdempsky reopened this Nov 6, 2019
@bcmills bcmills changed the title runtime: bad pointer in frame runtime_test.testSetPanicOnFault runtime: should checkptrArithmetic accept a uintptr instead of unsafe.Pointer? Nov 7, 2019
@ianlancetaylor
Copy link
Contributor Author

Ping @aclements See question above.

@cherrymui
Copy link
Member

Is runtime throw really a bad thing? When there is a bad pointer, the point of checkptrArithmetic is to crash the program (I don't think we expect the user to catch such panic and do something with it). So it crashes either way, although the error message is less clear.

Maybe the runtime could emit a different throw if the GC finds a bad pointer in checkptrArithmetic's frame?

@aclements
Copy link
Member

@aclements I'm curious if you have any thoughts on the best code generation for the compiler to emit for checkptr.

I'd be inclined to make checkptrArithmetic take a uintptr, since unsafe.Pointer should really only be used for "real", known-good pointers. But, of course, then we need to ensure the underlying pointer remains live. For that I'd lean toward your first option, as long as we can ensure checkptrArithmetic is recursively non-preemptible, and we can mark the whole region in the caller as non-preemptible (to prevent asynchronous preemption).

But I don't think this is a big deal, either, and I think the current state is fine for 1.14. As @cherrymui pointed out, it's going to throw one way or the other, it could just be a nicer throw.

@odeke-em
Copy link
Member

@mdempsky, thank you for the questions and others too thanks for chiming in. @aclements on his last response acknowledged that the current state of affairs is fine for Go1.14, but with a recommendation/inclination to go with a runtime throw as @cherrymui recommended, perhaps we can explore these ideas for Go1.15 or later. Austin also removed the “release-blocker” label.

With that, @ianlancetaylor @mdempsky should we perhaps move this issue out of the Go1.14 milestone, perhaps to Backlog? Thank you.

@josharian josharian modified the milestones: Go1.14, Go1.15 Feb 11, 2020
@odeke-em odeke-em modified the milestones: Go1.15, Go1.16 May 31, 2020
@odeke-em
Copy link
Member

Punting to Unreleased/Go1.17.

@odeke-em odeke-em modified the milestones: Go1.16, Go1.17 Dec 25, 2020
@ianlancetaylor
Copy link
Contributor Author

ianlancetaylor commented Apr 21, 2021

@mdempsky Is there anything to do on this issue? I can't tell.

@dmitshur
Copy link
Contributor

This issue was reopened in #35379 (comment) to ensure the open questions above get answered.

Moving to Backlog and assigning to @mdempsky to confirm if there's anything more left, or if it's okay to close this.

@dmitshur dmitshur modified the milestones: Go1.17, Backlog May 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

9 participants