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: sigpanic in C on openbsd/386 causes "unexpected return PC" #23640

Closed
aclements opened this issue Jan 31, 2018 · 2 comments

Comments

Projects
None yet
2 participants
@aclements
Copy link
Member

commented Jan 31, 2018

What version of Go are you using (go version)?

tip (ebe38b8)

Does this issue reproduce with the latest release?

No

What operating system and processor architecture are you using (go env)?

openbsd-386-60 and openbsd-386-62 builders

What did you do?

Commit ebe38b8 revealed an issue on these two builders:
https://build.golang.org/log/c601a18e2af24794e6c0899e05dddbb08caefc17
https://build.golang.org/log/f96fb991bad94ca0ad27793956f443931e501a54

The problem is that preparePanic sniffs the stack to decide where to put the PC from the signal context. In the cgo case, it will find that !findfunc(pc).valid() because pc is in C code, and then it will check if the top of the stack looks like a Go PC. However, this stack slot is just in a C frame, so it could contain anything, including what looks like a valid Go PC. For example, in the first log, it sees 1c02c23a <runtime.newproc1+682>. When this condition is met, it skips putting the signal PC on the stack at all. As a result, when we later unwind from the sigpanic, we'll "successfully" but incorrectly unwind to whatever PC was in this uninitialized slot and go who knows where from there.

So, we could simply allow this bad unwinding and weaken the condition on the print to suppress it if we're in cgo regardless of whether or not we're looking at sigpanic. Or we could tweak preparePanic somehow to make the unwind work. Perhaps we could consider the PC valid if m.incgo.

I believe this could happen on any GOARCH since they all have similar logic. The fact that it hit openbsd/386 is a coincidence.

@aclements aclements added this to the Go1.10 milestone Jan 31, 2018

@aclements aclements self-assigned this Jan 31, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Jan 31, 2018

Change https://golang.org/cl/91136 mentions this issue: runtime: suppress "unexpected return pc" any time we're in cgo

@gopherbot gopherbot closed this in 3ff41cd Jan 31, 2018

@gopherbot

This comment has been minimized.

Copy link

commented Jan 31, 2018

Change https://golang.org/cl/91137 mentions this issue: runtime: avoid bad unwinding from sigpanic in C code

gopherbot pushed a commit that referenced this issue Feb 13, 2018

runtime: avoid bad unwinding from sigpanic in C code
Currently, if a sigpanic call is injected into C code, it's possible
for preparePanic to leave the stack in a state where traceback can't
unwind correctly past the sigpanic.

Specifically, shouldPushPanic sniffs the stack to decide where to put
the PC from the signal context. In the cgo case, it will find that
!findfunc(pc).valid() because pc is in C code, and then it will check
if the top of the stack looks like a Go PC. However, this stack slot
is just in a C frame, so it could be uninitialized and contain
anything, including what looks like a valid Go PC. For example, in
https://build.golang.org/log/c601a18e2af24794e6c0899e05dddbb08caefc17,
it sees 1c02c23a <runtime.newproc1+682>. When this condition is met,
it skips putting the signal PC on the stack at all. As a result, when
we later unwind from the sigpanic, we'll "successfully" but
incorrectly unwind to whatever PC was in this uninitialized slot and
go who knows where from there.

Fix this by making shouldPushPanic assume that the signal PC is always
usable if we're running C code, so we always make it appear like
sigpanic's caller.

This lets us be pickier again about unexpected return PCs in
gentraceback.

Updates #23640.

Change-Id: I1e8ade24b031bd905d48e92d5e60c982e8edf160
Reviewed-on: https://go-review.googlesource.com/91137
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>

@golang golang locked and limited conversation to collaborators Jan 31, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.