diff options
author | Cherry Zhang <cherryyz@google.com> | 2019-02-09 23:31:59 -0500 |
---|---|---|
committer | Brad Fitzpatrick <bradfitz@golang.org> | 2019-02-13 16:52:03 +0000 |
commit | 74f0f6939cb4863bde85af64beb490a1b673b0a7 (patch) | |
tree | 617acdb97dae6f809a69807c510d03b53c96df9b | |
parent | 7ab5e0c5e2e2e75da90d18ecb4b8461b0c29c94c (diff) | |
download | go-74f0f6939cb4863bde85af64beb490a1b673b0a7.tar.gz go-74f0f6939cb4863bde85af64beb490a1b673b0a7.zip |
[release-branch.go1.12] runtime: scan gp._panic in stack scan
In runtime.gopanic, the _panic object p is stack allocated and
referenced from gp._panic. With stack objects, p on stack is dead
at the point preprintpanics runs. gp._panic points to p, but
stack scan doesn't look at gp. Heap scan of gp does look at
gp._panic, but it stops and ignores the pointer as it points to
the stack. So whatever p points to may be collected and clobbered.
We need to scan gp._panic explicitly during stack scan.
To test it reliably, we introduce a GODEBUG mode "clobberfree",
which clobbers the memory content when the GC frees an object.
Fixes #30150.
Change-Id: I11128298f03a89f817faa221421a9d332b41dced
Reviewed-on: https://go-review.googlesource.com/c/161778
Run-TryBot: Cherry Zhang <cherryyz@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
(cherry picked from commit af8f4062c24cb36af4dc24fbaffd23aa7f7bde36)
Reviewed-on: https://go-review.googlesource.com/c/162358
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
-rw-r--r-- | src/runtime/crash_test.go | 12 | ||||
-rw-r--r-- | src/runtime/extern.go | 4 | ||||
-rw-r--r-- | src/runtime/mgcmark.go | 6 | ||||
-rw-r--r-- | src/runtime/mgcsweep.go | 14 | ||||
-rw-r--r-- | src/runtime/runtime1.go | 2 | ||||
-rw-r--r-- | src/runtime/testdata/testprog/crash.go | 21 |
6 files changed, 58 insertions, 1 deletions
diff --git a/src/runtime/crash_test.go b/src/runtime/crash_test.go index 6fba4dd91a..03ebf022a6 100644 --- a/src/runtime/crash_test.go +++ b/src/runtime/crash_test.go @@ -728,3 +728,15 @@ func TestG0StackOverflow(t *testing.T) { runtime.G0StackOverflow() } + +// Test that panic message is not clobbered. +// See issue 30150. +func TestDoublePanic(t *testing.T) { + output := runTestProg(t, "testprog", "DoublePanic", "GODEBUG=clobberfree=1") + wants := []string{"panic: XXX", "panic: YYY"} + for _, want := range wants { + if !strings.Contains(output, want) { + t.Errorf("output:\n%s\n\nwant output containing: %s", output, want) + } + } +} diff --git a/src/runtime/extern.go b/src/runtime/extern.go index af858a331f..437406d991 100644 --- a/src/runtime/extern.go +++ b/src/runtime/extern.go @@ -27,6 +27,10 @@ It is a comma-separated list of name=val pairs setting these named variables: allocfreetrace: setting allocfreetrace=1 causes every allocation to be profiled and a stack trace printed on each object's allocation and free. + clobberfree: setting clobberfree=1 causes the garbage collector to + clobber the memory content of an object with bad content when it frees + the object. + cgocheck: setting cgocheck=0 disables all checks for packages using cgo to incorrectly pass Go pointers to non-Go code. Setting cgocheck=1 (the default) enables relatively cheap diff --git a/src/runtime/mgcmark.go b/src/runtime/mgcmark.go index 86416caab5..022cc8d7d7 100644 --- a/src/runtime/mgcmark.go +++ b/src/runtime/mgcmark.go @@ -709,7 +709,13 @@ func scanstack(gp *g, gcw *gcWork) { return true } gentraceback(^uintptr(0), ^uintptr(0), 0, gp, 0, nil, 0x7fffffff, scanframe, nil, 0) + + // Find additional pointers that point into the stack from the heap. + // Currently this includes defers and panics. See also function copystack. tracebackdefers(gp, scanframe, nil) + if gp._panic != nil { + state.putPtr(uintptr(unsafe.Pointer(gp._panic))) + } // Find and scan all reachable stack objects. state.buildIndex() diff --git a/src/runtime/mgcsweep.go b/src/runtime/mgcsweep.go index edb9fcac09..6ac3b03176 100644 --- a/src/runtime/mgcsweep.go +++ b/src/runtime/mgcsweep.go @@ -291,7 +291,7 @@ func (s *mspan) sweep(preserve bool) bool { } } - if debug.allocfreetrace != 0 || raceenabled || msanenabled { + if debug.allocfreetrace != 0 || debug.clobberfree != 0 || raceenabled || msanenabled { // Find all newly freed objects. This doesn't have to // efficient; allocfreetrace has massive overhead. mbits := s.markBitsForBase() @@ -302,6 +302,9 @@ func (s *mspan) sweep(preserve bool) bool { if debug.allocfreetrace != 0 { tracefree(unsafe.Pointer(x), size) } + if debug.clobberfree != 0 { + clobberfree(unsafe.Pointer(x), size) + } if raceenabled { racefree(unsafe.Pointer(x), size) } @@ -446,3 +449,12 @@ retry: traceGCSweepDone() } } + +// clobberfree sets the memory content at x to bad content, for debugging +// purposes. +func clobberfree(x unsafe.Pointer, size uintptr) { + // size (span.elemsize) is always a multiple of 4. + for i := uintptr(0); i < size; i += 4 { + *(*uint32)(add(x, i)) = 0xdeadbeef + } +} diff --git a/src/runtime/runtime1.go b/src/runtime/runtime1.go index c5667e73ad..0c0a31ee6a 100644 --- a/src/runtime/runtime1.go +++ b/src/runtime/runtime1.go @@ -301,6 +301,7 @@ type dbgVar struct { var debug struct { allocfreetrace int32 cgocheck int32 + clobberfree int32 efence int32 gccheckmark int32 gcpacertrace int32 @@ -318,6 +319,7 @@ var debug struct { var dbgvars = []dbgVar{ {"allocfreetrace", &debug.allocfreetrace}, + {"clobberfree", &debug.clobberfree}, {"cgocheck", &debug.cgocheck}, {"efence", &debug.efence}, {"gccheckmark", &debug.gccheckmark}, diff --git a/src/runtime/testdata/testprog/crash.go b/src/runtime/testdata/testprog/crash.go index 4d83132198..c4990cdda9 100644 --- a/src/runtime/testdata/testprog/crash.go +++ b/src/runtime/testdata/testprog/crash.go @@ -11,6 +11,7 @@ import ( func init() { register("Crash", Crash) + register("DoublePanic", DoublePanic) } func test(name string) { @@ -43,3 +44,23 @@ func Crash() { testInNewThread("second-new-thread") test("main-again") } + +type P string + +func (p P) String() string { + // Try to free the "YYY" string header when the "XXX" + // panic is stringified. + runtime.GC() + runtime.GC() + runtime.GC() + return string(p) +} + +// Test that panic message is not clobbered. +// See issue 30150. +func DoublePanic() { + defer func() { + panic(P("YYY")) + }() + panic(P("XXX")) +} |