Skip to content

Commit eb3c6a9

Browse files
mknyszekmknyszek
committed
runtime: disable stack shrinking in activeStackChans race window
Currently activeStackChans is set before a goroutine blocks on a channel operation in an unlockf passed to gopark. The trouble is that the unlockf is called *after* the G's status is changed, and the G's status is what is used by a concurrent mark worker (calling suspendG) to determine that a G has successfully been suspended. In this window between the status change and unlockf, the mark worker could try to shrink the G's stack, and in particular observe that activeStackChans is false. This observation will cause the mark worker to *not* synchronize with concurrent channel operations when it should, and so updating pointers in the sudog for the blocked goroutine (which may point to the goroutine's stack) races with channel operations which may also manipulate the pointer (read it, dereference it, update it, etc.). Fix the problem by adding a new atomically-updated flag to the g struct called parkingOnChan, which is non-zero in the race window above. Then, in isShrinkStackSafe, check if parkingOnChan is zero. The race is resolved like so: * Blocking G sets parkingOnChan, then changes status in gopark. * Mark worker successfully suspends blocking G. * If the mark worker observes parkingOnChan is non-zero when checking isShrinkStackSafe, then it's not safe to shrink (we're in the race window). * If the mark worker observes parkingOnChan as zero, then because the mark worker observed the G status change, it can be sure that gopark's unlockf completed, and gp.activeStackChans will be correct. The risk of this change is low, since although it reduces the number of places that stack shrinking is allowed, the window here is incredibly small. Essentially, every place that it might crash now is replaced with no shrink. This change adds a test, but the race window is so small that it's hard to trigger without a well-placed sleep in park_m. Also, this change fixes stackGrowRecursive in proc_test.go to actually allocate a 128-byte stack frame. It turns out the compiler was destructuring the "pad" field and only allocating one uint64 on the stack. Fixes golang#40641. Change-Id: I7dfbe7d460f6972b8956116b137bc13bc24464e8 Reviewed-on: https://go-review.googlesource.com/c/go/+/247050 Run-TryBot: Michael Knyszek <[email protected]> TryBot-Result: Go Bot <[email protected]> Reviewed-by: Michael Pratt <[email protected]> Trust: Michael Knyszek <[email protected]>
1 parent b4ea672 commit eb3c6a9

6 files changed

Lines changed: 122 additions & 2 deletions

File tree

‎src/runtime/chan.go‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -250,6 +250,11 @@ func chansend(c *hchan, ep unsafe.Pointer, block bool, callerpc uintptr) bool {
250250
gp.waiting = mysg
251251
gp.param = nil
252252
c.sendq.enqueue(mysg)
253+
// Signal to anyone trying to shrink our stack that we're about
254+
// to park on a channel. The window between when this G's status
255+
// changes and when we set gp.activeStackChans is not safe for
256+
// stack shrinking.
257+
atomic.Store8(&gp.parkingOnChan, 1)
253258
gopark(chanparkcommit, unsafe.Pointer(&c.lock), waitReasonChanSend, traceEvGoBlockSend, 2)
254259
// Ensure the value being sent is kept alive until the
255260
// receiver copies it out. The sudog has a pointer to the
@@ -568,6 +573,11 @@ func chanrecv(c *hchan, ep unsafe.Pointer, block bool) (selected, received bool)
568573
mysg.c = c
569574
gp.param = nil
570575
c.recvq.enqueue(mysg)
576+
// Signal to anyone trying to shrink our stack that we're about
577+
// to park on a channel. The window between when this G's status
578+
// changes and when we set gp.activeStackChans is not safe for
579+
// stack shrinking.
580+
atomic.Store8(&gp.parkingOnChan, 1)
571581
gopark(chanparkcommit, unsafe.Pointer(&c.lock), waitReasonChanReceive, traceEvGoBlockRecv, 2)
572582

573583
// someone woke us up
@@ -646,7 +656,19 @@ func recv(c *hchan, sg *sudog, ep unsafe.Pointer, unlockf func(), skip int) {
646656
func chanparkcommit(gp *g, chanLock unsafe.Pointer) bool {
647657
// There are unlocked sudogs that point into gp's stack. Stack
648658
// copying must lock the channels of those sudogs.
659+
// Set activeStackChans here instead of before we try parking
660+
// because we could self-deadlock in stack growth on the
661+
// channel lock.
649662
gp.activeStackChans = true
663+
// Mark that it's safe for stack shrinking to occur now,
664+
// because any thread acquiring this G's stack for shrinking
665+
// is guaranteed to observe activeStackChans after this store.
666+
atomic.Store8(&gp.parkingOnChan, 0)
667+
// Make sure we unlock after setting activeStackChans and
668+
// unsetting parkingOnChan. The moment we unlock chanLock
669+
// we risk gp getting readied by a channel operation and
670+
// so gp could continue running before everything before
671+
// the unlock is visible (even to gp itself).
650672
unlock((*mutex)(chanLock))
651673
return true
652674
}

‎src/runtime/chan_test.go‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -623,6 +623,62 @@ func TestShrinkStackDuringBlockedSend(t *testing.T) {
623623
<-done
624624
}
625625

626+
func TestNoShrinkStackWhileParking(t *testing.T) {
627+
// The goal of this test is to trigger a "racy sudog adjustment"
628+
// throw. Basically, there's a window between when a goroutine
629+
// becomes available for preemption for stack scanning (and thus,
630+
// stack shrinking) but before the goroutine has fully parked on a
631+
// channel. See issue 40641 for more details on the problem.
632+
//
633+
// The way we try to induce this failure is to set up two
634+
// goroutines: a sender and a reciever that communicate across
635+
// a channel. We try to set up a situation where the sender
636+
// grows its stack temporarily then *fully* blocks on a channel
637+
// often. Meanwhile a GC is triggered so that we try to get a
638+
// mark worker to shrink the sender's stack and race with the
639+
// sender parking.
640+
//
641+
// Unfortunately the race window here is so small that we
642+
// either need a ridiculous number of iterations, or we add
643+
// "usleep(1000)" to park_m, just before the unlockf call.
644+
const n = 10
645+
send := func(c chan<- int, done chan struct{}) {
646+
for i := 0; i < n; i++ {
647+
c <- i
648+
// Use lots of stack briefly so that
649+
// the GC is going to want to shrink us
650+
// when it scans us. Make sure not to
651+
// do any function calls otherwise
652+
// in order to avoid us shrinking ourselves
653+
// when we're preempted.
654+
stackGrowthRecursive(20)
655+
}
656+
done <- struct{}{}
657+
}
658+
recv := func(c <-chan int, done chan struct{}) {
659+
for i := 0; i < n; i++ {
660+
// Sleep here so that the sender always
661+
// fully blocks.
662+
time.Sleep(10 * time.Microsecond)
663+
<-c
664+
}
665+
done <- struct{}{}
666+
}
667+
for i := 0; i < n*20; i++ {
668+
c := make(chan int)
669+
done := make(chan struct{})
670+
go recv(c, done)
671+
go send(c, done)
672+
// Wait a little bit before triggering
673+
// the GC to make sure the sender and
674+
// reciever have gotten into their groove.
675+
time.Sleep(50 * time.Microsecond)
676+
runtime.GC()
677+
<-done
678+
<-done
679+
}
680+
}
681+
626682
func TestSelectDuplicateChannel(t *testing.T) {
627683
// This test makes sure we can queue a G on
628684
// the same channel multiple times.

‎src/runtime/proc_test.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -523,9 +523,17 @@ func BenchmarkPingPongHog(b *testing.B) {
523523
<-done
524524
}
525525

526+
var padData [128]uint64
527+
526528
func stackGrowthRecursive(i int) {
527529
var pad [128]uint64
528-
if i != 0 && pad[0] == 0 {
530+
pad = padData
531+
for j := range pad {
532+
if pad[j] != 0 {
533+
return
534+
}
535+
}
536+
if i != 0 {
529537
stackGrowthRecursive(i - 1)
530538
}
531539
}

‎src/runtime/runtime2.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -453,6 +453,10 @@ type g struct {
453453
// copying needs to acquire channel locks to protect these
454454
// areas of the stack.
455455
activeStackChans bool
456+
// parkingOnChan indicates that the goroutine is about to
457+
// park on a chansend or chanrecv. Used to signal an unsafe point
458+
// for stack shrinking. It's a boolean value, but is updated atomically.
459+
parkingOnChan uint8
456460

457461
raceignore int8 // ignore race detection events
458462
sysblocktraced bool // StartTrace has emitted EvGoInSyscall about this goroutine

‎src/runtime/select.go‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ package runtime
77
// This file contains the implementation of Go select statements.
88

99
import (
10+
"runtime/internal/atomic"
1011
"unsafe"
1112
)
1213

@@ -61,7 +62,20 @@ func selunlock(scases []scase, lockorder []uint16) {
6162
func selparkcommit(gp *g, _ unsafe.Pointer) bool {
6263
// There are unlocked sudogs that point into gp's stack. Stack
6364
// copying must lock the channels of those sudogs.
65+
// Set activeStackChans here instead of before we try parking
66+
// because we could self-deadlock in stack growth on a
67+
// channel lock.
6468
gp.activeStackChans = true
69+
// Mark that it's safe for stack shrinking to occur now,
70+
// because any thread acquiring this G's stack for shrinking
71+
// is guaranteed to observe activeStackChans after this store.
72+
atomic.Store8(&gp.parkingOnChan, 0)
73+
// Make sure we unlock after setting activeStackChans and
74+
// unsetting parkingOnChan. The moment we unlock any of the
75+
// channel locks we risk gp getting readied by a channel operation
76+
// and so gp could continue running before everything before the
77+
// unlock is visible (even to gp itself).
78+
6579
// This must not access gp's stack (see gopark). In
6680
// particular, it must not access the *hselect. That's okay,
6781
// because by the time this is called, gp.waiting has all
@@ -305,6 +319,11 @@ func selectgo(cas0 *scase, order0 *uint16, pc0 *uintptr, nsends, nrecvs int, blo
305319

306320
// wait for someone to wake us up
307321
gp.param = nil
322+
// Signal to anyone trying to shrink our stack that we're about
323+
// to park on a channel. The window between when this G's status
324+
// changes and when we set gp.activeStackChans is not safe for
325+
// stack shrinking.
326+
atomic.Store8(&gp.parkingOnChan, 1)
308327
gopark(selparkcommit, nil, waitReasonSelect, traceEvGoBlockSelect, 1)
309328
gp.activeStackChans = false
310329

‎src/runtime/stack.go‎

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -862,6 +862,13 @@ func copystack(gp *g, newsize uintptr) {
862862
// Adjust sudogs, synchronizing with channel ops if necessary.
863863
ncopy := used
864864
if !gp.activeStackChans {
865+
if newsize < old.hi-old.lo && atomic.Load8(&gp.parkingOnChan) != 0 {
866+
// It's not safe for someone to shrink this stack while we're actively
867+
// parking on a channel, but it is safe to grow since we do that
868+
// ourselves and explicitly don't want to synchronize with channels
869+
// since we could self-deadlock.
870+
throw("racy sudog adjustment due to parking on channel")
871+
}
865872
adjustsudogs(gp, &adjinfo)
866873
} else {
867874
// sudogs may be pointing in to the stack and gp has
@@ -1105,7 +1112,11 @@ func isShrinkStackSafe(gp *g) bool {
11051112
// We also can't copy the stack if we're at an asynchronous
11061113
// safe-point because we don't have precise pointer maps for
11071114
// all frames.
1108-
return gp.syscallsp == 0 && !gp.asyncSafePoint
1115+
//
1116+
// We also can't *shrink* the stack in the window between the
1117+
// goroutine calling gopark to park on a channel and
1118+
// gp.activeStackChans being set.
1119+
return gp.syscallsp == 0 && !gp.asyncSafePoint && atomic.Load8(&gp.parkingOnChan) == 0
11091120
}
11101121

11111122
// Maybe shrink the stack being used by gp.

0 commit comments

Comments
 (0)