Skip to content

/ go

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

Open
ianlancetaylor opened this issue Nov 5, 2019 · 16 comments
Open

runtime: should checkptrArithmetic accept a uintptr instead of unsafe.Pointer? #35379

ianlancetaylor opened this issue Nov 5, 2019 · 16 comments
Labels
NeedsInvestigation
Milestone
Go1.15

Comments

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 5, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Nov 6, 2019

Also on FreeBSD race:

2019-11-05T18:37:06-c3cef0b/freebsd-amd64-race
2019-11-05T18:58:41-f07cbc7/freebsd-amd64-race
2019-11-06T17:03:51-a2b1dc8/freebsd-amd64-race
@bcmills

This comment has been minimized.

Sign in to view
Copy link
Member

@bcmills bcmills commented Nov 6, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Nov 6, 2019

CC @mdempsky

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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Nov 6, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Nov 6, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member

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

This comment has been minimized.

Sign in to view
Copy link
Member

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

This comment has been minimized.

Sign in to view
Copy link

@gopherbot gopherbot commented Nov 6, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Nov 6, 2019

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

This comment has been minimized.

Sign in to view
Copy link

@gopherbot gopherbot commented Nov 6, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member

@mdempsky 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
@gopherbot gopherbot closed this in 6ce4384 Nov 6, 2019
@mdempsky

This comment has been minimized.

Sign in to view
Copy link
Member

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

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

@ianlancetaylor ianlancetaylor commented Nov 16, 2019

Ping @aclements See question above.
@cherrymui

This comment has been minimized.

Sign in to view
Copy link
Contributor

@cherrymui cherrymui commented Nov 19, 2019

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

This comment has been minimized.

Sign in to view
Copy link
Member

@aclements aclements commented Dec 19, 2019

@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

This comment has been minimized.

Sign in to view
Copy link
Member

@odeke-em odeke-em commented Feb 11, 2020

@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation
Projects
None yet
Milestone
  Go1.15
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
@ianlancetaylor @bcmills @mdempsky @gopherbot @cherrymui @aclements @odeke-em @josharian
You can’t perform that action at this time.