From 9ed9df6ca27850704133de1ceb94407c635beb82 Mon Sep 17 00:00:00 2001 From: Michael Anthony Knyszek Date: Tue, 18 Dec 2018 20:04:53 +0000 Subject: [PATCH] runtime: avoid write barrier in startpanic_m startpanic_m could be called correctly in a context where there's a valid G, a valid M, but no P, for example in a signal handler which panics. Currently, startpanic_m has write barriers enabled because write barriers are permitted if a G's M is dying. However, all the current write barrier implementations assume the current G has a P. Therefore, in this change we disable write barriers in startpanic_m, remove the only pointer write which clears g.writebuf, and fix up gwrite to ignore the writebuf if the current G's M is dying, rather than relying on it being nil in the dying case. Fixes #26575. Change-Id: I9b29e6b9edf00d8e99ffc71770c287142ebae086 Reviewed-on: https://go-review.googlesource.com/c/154837 Run-TryBot: Michael Knyszek TryBot-Result: Gobot Gobot Reviewed-by: Austin Clements --- src/runtime/panic.go | 11 +++++++---- src/runtime/print.go | 7 ++++++- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/runtime/panic.go b/src/runtime/panic.go index 81ff21113f..bb83be4715 100644 --- a/src/runtime/panic.go +++ b/src/runtime/panic.go @@ -729,10 +729,13 @@ func fatalpanic(msgs *_panic) { // It returns true if panic messages should be printed, or false if // the runtime is in bad shape and should just print stacks. // -// It can have write barriers because the write barrier explicitly -// ignores writes once dying > 0. +// It must not have write barriers even though the write barrier +// explicitly ignores writes once dying > 0. Write barriers still +// assume that g.m.p != nil, and this function may not have P +// in some contexts (e.g. a panic in a signal handler for a signal +// sent to an M with no P). // -//go:yeswritebarrierrec +//go:nowritebarrierrec func startpanic_m() bool { _g_ := getg() if mheap_.cachealloc.size == 0 { // very early @@ -752,8 +755,8 @@ func startpanic_m() bool { switch _g_.m.dying { case 0: + // Setting dying >0 has the side-effect of disabling this G's writebuf. _g_.m.dying = 1 - _g_.writebuf = nil atomic.Xadd(&panicking, 1) lock(&paniclk) if debug.schedtrace > 0 || debug.scheddetail > 0 { diff --git a/src/runtime/print.go b/src/runtime/print.go index 7b2e4f40ff..e605eb34cb 100644 --- a/src/runtime/print.go +++ b/src/runtime/print.go @@ -89,7 +89,12 @@ func gwrite(b []byte) { } recordForPanic(b) gp := getg() - if gp == nil || gp.writebuf == nil { + // Don't use the writebuf if gp.m is dying. We want anything + // written through gwrite to appear in the terminal rather + // than be written to in some buffer, if we're in a panicking state. + // Note that we can't just clear writebuf in the gp.m.dying case + // because a panic isn't allowed to have any write barriers. + if gp == nil || gp.writebuf == nil || gp.m.dying > 0 { writeErr(b) return }