From ddb503be96dd9a10c6591a2e6806548f9ddbac62 Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Wed, 31 Jan 2018 12:05:24 -0500 Subject: [PATCH] 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 . 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 TryBot-Result: Gobot Gobot Reviewed-by: Ian Lance Taylor --- src/runtime/panic.go | 7 ++++++- src/runtime/traceback.go | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/src/runtime/panic.go b/src/runtime/panic.go index e1477e2486..55ecb67654 100644 --- a/src/runtime/panic.go +++ b/src/runtime/panic.go @@ -805,7 +805,12 @@ func shouldPushSigpanic(gp *g, pc, lr uintptr) bool { // the link register as code, then this assumes the panic was // caused by a call to non-code. In this case, we want to // ignore this call to make unwinding show the context. - if findfunc(pc).valid() { + // + // If we running C code, we're not going to recognize pc as a + // Go function, so just assume it's good. Otherwise, traceback + // may try to read a stale LR that looks like a Go code + // pointer and wander into the woods. + if gp.m.incgo || findfunc(pc).valid() { // This wasn't a bad call, so use PC as sigpanic's // return PC. return true diff --git a/src/runtime/traceback.go b/src/runtime/traceback.go index 747176c278..2261942ab4 100644 --- a/src/runtime/traceback.go +++ b/src/runtime/traceback.go @@ -287,7 +287,7 @@ func gentraceback(pc0, sp0, lr0 uintptr, gp *g, skip int, pcbuf *uintptr, max in // But if callback is set, we're doing a garbage collection and must // get everything, so crash loudly. doPrint := printing - if doPrint && gp.m.incgo { + if doPrint && gp.m.incgo && f.entry == sigpanicPC { // We can inject sigpanic // calls directly into C code, // in which case we'll see a C