Currently, newstack loads gp.stackguard0 twice to check for different
poison values. The race window between these two checks can lead to
unintentional stack doubling, and ultimately to stack overflows.
Specifically, newstack checks if stackguard0 is stackPreempt first,
then it checks if it's stackForceMove. If stackguard0 is set to
stackForceMove on entry, but changes to stackPreempt between the two
checks, newstack will incorrectly double the stack allocation.
Fix this by loading stackguard0 exactly once and then checking it
against different poison values.
The effect of this is relatively minor because stackForceMove is only
used by a small number of runtime tests. I found this because
mayMorestackMove uses stackForceMove aggressively, which makes this
failure mode much more likely.
Change-Id: I1f8b6a6744e45533580a3f45d7030ec2ec65a5fb
Reviewed-on: https://go-review.googlesource.com/c/go/+/361775
Trust: Austin Clements <austin@google.com>
Run-TryBot: Austin Clements <austin@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Michael Pratt <mpratt@google.com>
// NOTE: stackguard0 may change underfoot, if another thread
// is about to try to preempt gp. Read it just once and use that same
// value now and below.
- preempt := atomic.Loaduintptr(&gp.stackguard0) == stackPreempt
+ stackguard0 := atomic.Loaduintptr(&gp.stackguard0)
// Be conservative about where we preempt.
// We are interested in preempting user Go code, not runtime code.
// If the GC is in some way dependent on this goroutine (for example,
// it needs a lock held by the goroutine), that small preemption turns
// into a real deadlock.
+ preempt := stackguard0 == stackPreempt
if preempt {
if !canPreemptM(thisg.m) {
// Let the goroutine keep running for now.
}
}
- if gp.stackguard0 == stackForceMove {
+ if stackguard0 == stackForceMove {
// Forced stack movement used for debugging.
// Don't double the stack (or we may quickly run out
// if this is done repeatedly).