Skip to content

Commit 3560cf0

Browse files
prattmicjoedian
authored andcommitted
[release-branch.go1.22] runtime: always update stack bounds on cgocallback
callbackUpdateSystemStack contains a fast path to exit early without update if SP is already within the g0.stack bounds. This is not safe, as a subsequent call may have new stack bounds that only partially overlap the old stack bounds. In this case it is possible to see an SP that is in the old stack bounds, but very close to the bottom of the bounds due to the partial overlap. In that case we're very likely to "run out" of space on the system stack. We only need to do this on extra Ms, as normal Ms have precise bounds defined when we allocated the stack. TSAN annotations are added to x_cgo_getstackbounds because bounds is a pointer into the Go stack. The stack can be reused when an old thread exits and a new thread starts, but TSAN can't see the synchronization there. This isn't a new case, but we are now calling more often. For #62440. Fixes #67298. Cq-Include-Trybots: luci.golang.try:go1.22-linux-amd64-longtest Change-Id: I5389050494987b7668d0b317fb92f85e61d798ac Reviewed-on: https://go-review.googlesource.com/c/go/+/584597 Reviewed-by: Cherry Mui <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> (cherry picked from commit 1ffc296) Reviewed-on: https://go-review.googlesource.com/c/go/+/585935 Run-TryBot: Joedian Reid <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
1 parent 5159a71 commit 3560cf0

File tree

3 files changed

+33
-7
lines changed

3 files changed

+33
-7
lines changed

src/runtime/cgo/gcc_stack_darwin.c

+5
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,11 @@ x_cgo_getstackbound(uintptr bounds[2])
1515
p = pthread_self();
1616
addr = pthread_get_stackaddr_np(p); // high address (!)
1717
size = pthread_get_stacksize_np(p);
18+
19+
// bounds points into the Go stack. TSAN can't see the synchronization
20+
// in Go around stack reuse.
21+
_cgo_tsan_acquire();
1822
bounds[0] = (uintptr)addr - size;
1923
bounds[1] = (uintptr)addr;
24+
_cgo_tsan_release();
2025
}

src/runtime/cgo/gcc_stack_unix.c

+4
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,10 @@ x_cgo_getstackbound(uintptr bounds[2])
3636
#endif
3737
pthread_attr_destroy(&attr);
3838

39+
// bounds points into the Go stack. TSAN can't see the synchronization
40+
// in Go around stack reuse.
41+
_cgo_tsan_acquire();
3942
bounds[0] = (uintptr)addr;
4043
bounds[1] = (uintptr)addr + size;
44+
_cgo_tsan_release();
4145
}

src/runtime/cgocall.go

+24-7
Original file line numberDiff line numberDiff line change
@@ -214,15 +214,18 @@ func cgocall(fn, arg unsafe.Pointer) int32 {
214214
//go:nosplit
215215
func callbackUpdateSystemStack(mp *m, sp uintptr, signal bool) {
216216
g0 := mp.g0
217-
if sp > g0.stack.lo && sp <= g0.stack.hi {
218-
// Stack already in bounds, nothing to do.
219-
return
220-
}
221217

222-
if mp.ncgo > 0 {
218+
inBound := sp > g0.stack.lo && sp <= g0.stack.hi
219+
if mp.ncgo > 0 && !inBound {
223220
// ncgo > 0 indicates that this M was in Go further up the stack
224-
// (it called C and is now receiving a callback). It is not
225-
// safe for the C call to change the stack out from under us.
221+
// (it called C and is now receiving a callback).
222+
//
223+
// !inBound indicates that we were called with SP outside the
224+
// expected system stack bounds (C changed the stack out from
225+
// under us between the cgocall and cgocallback?).
226+
//
227+
// It is not safe for the C call to change the stack out from
228+
// under us, so throw.
226229

227230
// Note that this case isn't possible for signal == true, as
228231
// that is always passing a new M from needm.
@@ -240,12 +243,26 @@ func callbackUpdateSystemStack(mp *m, sp uintptr, signal bool) {
240243
exit(2)
241244
}
242245

246+
if !mp.isextra {
247+
// We allocated the stack for standard Ms. Don't replace the
248+
// stack bounds with estimated ones when we already initialized
249+
// with the exact ones.
250+
return
251+
}
252+
243253
// This M does not have Go further up the stack. However, it may have
244254
// previously called into Go, initializing the stack bounds. Between
245255
// that call returning and now the stack may have changed (perhaps the
246256
// C thread is running a coroutine library). We need to update the
247257
// stack bounds for this case.
248258
//
259+
// N.B. we need to update the stack bounds even if SP appears to
260+
// already be in bounds. Our "bounds" may actually be estimated dummy
261+
// bounds (below). The actual stack bounds could have shifted but still
262+
// have partial overlap with our dummy bounds. If we failed to update
263+
// in that case, we could find ourselves seemingly called near the
264+
// bottom of the stack bounds, where we quickly run out of space.
265+
249266
// Set the stack bounds to match the current stack. If we don't
250267
// actually know how big the stack is, like we don't know how big any
251268
// scheduling stack is, but we assume there's at least 32 kB. If we

0 commit comments

Comments
 (0)