From 7c2cf4e779a66b212a3c94f2b20ade1c2c275b84 Mon Sep 17 00:00:00 2001 From: Austin Clements Date: Thu, 4 Jan 2018 10:43:29 -0500 Subject: [PATCH] runtime: avoid race on allp in findrunnable findrunnable loops over allp to check run queues *after* it has dropped its own P. This is unsafe because allp can change when nothing is blocking safe-points. Hence, procresize could change allp concurrently with findrunnable's loop. Beyond generally violating Go's memory model, in the best case this could findrunnable to observe a nil P pointer if allp has been grown but the new slots not yet initialized. In the worst case, the reads of allp could tear, causing findrunnable to read a word that isn't even a valid *P pointer. Fix this by taking a snapshot of the allp slice header (but not the backing store) before findrunnable drops its P and iterating over this snapshot. The actual contents of allp are immutable up to len(allp), so this fixes the race. Updates #23098 (may fix). Change-Id: I556ae2dbfffe9fe4a1bf43126e930b9e5c240ea8 Reviewed-on: https://go-review.googlesource.com/86215 Run-TryBot: Austin Clements TryBot-Result: Gobot Gobot Reviewed-by: Brad Fitzpatrick Reviewed-by: Ian Lance Taylor --- src/runtime/proc.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/runtime/proc.go b/src/runtime/proc.go index ff441badde..2e958f7fc5 100644 --- a/src/runtime/proc.go +++ b/src/runtime/proc.go @@ -2299,6 +2299,12 @@ stop: return gp, false } + // Before we drop our P, make a snapshot of the allp slice, + // which can change underfoot once we no longer block + // safe-points. We don't need to snapshot the contents because + // everything up to cap(allp) is immutable. + allpSnapshot := allp + // return P and block lock(&sched.lock) if sched.gcwaiting != 0 || _p_.runSafePointFn != 0 { @@ -2338,7 +2344,7 @@ stop: } // check all runqueues once again - for _, _p_ := range allp { + for _, _p_ := range allpSnapshot { if !runqempty(_p_) { lock(&sched.lock) _p_ = pidleget() -- 2.48.1