Skip to content

Commit f4870b7

Browse files
aclementsandybons
authored andcommitted
[release-branch.go1.9] runtime: restore the Go-allocated signal stack in unminit
Currently, when we minit on a thread that already has an alternate signal stack (e.g., because the M was an extram being used for a cgo callback, or to handle a signal on a C thread, or because the platform's libc always allocates a signal stack like on Android), we simply drop the Go-allocated gsignal stack on the floor. This is a problem for Ms on the extram list because those Ms may later be reused for a different thread that may not have its own alternate signal stack. On tip, this manifests as a crash in sigaltstack because we clear the gsignal stack bounds in unminit and later try to use those cleared bounds when we re-minit that M. On 1.9 and earlier, we didn't clear the bounds, so this manifests as running more than one signal handler on the same signal stack, which could lead to arbitrary memory corruption. This CL fixes this problem by saving the Go-allocated gsignal stack in a new field in the m struct when overwriting it with a system-provided signal stack, and then restoring the original gsignal stack in unminit. This CL is designed to be easy to back-port to 1.9. It won't quite cherry-pick cleanly, but it should be sufficient to simply ignore the change in mexit (which didn't exist in 1.9). Now that we always have a place to stash the original signal stack in the m struct, there are some simplifications we can make to the signal stack handling. We'll do those in a later CL. Fixes #22930. Change-Id: I55c5a6dd9d97532f131146afdef0b216e1433054 Reviewed-on: https://go-review.googlesource.com/88315 Run-TryBot: Andrew Bonventre <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Ian Lance Taylor <[email protected]>
1 parent 2308c9c commit f4870b7

File tree

7 files changed

+131
-5
lines changed

7 files changed

+131
-5
lines changed

src/runtime/crash_cgo_test.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -411,3 +411,16 @@ func TestCgoNumGoroutine(t *testing.T) {
411411
t.Errorf("expected %q got %v", want, got)
412412
}
413413
}
414+
415+
func TestSigStackSwapping(t *testing.T) {
416+
switch runtime.GOOS {
417+
case "plan9", "windows":
418+
t.Skip("no sigaltstack on %s", runtime.GOOS)
419+
}
420+
t.Parallel()
421+
got := runTestProg(t, "testprogcgo", "SigStack")
422+
want := "OK\n"
423+
if got != want {
424+
t.Errorf("expected %q got %v", want, got)
425+
}
426+
}

src/runtime/os3_plan9.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,3 +153,6 @@ func setThreadCPUProfiler(hz int32) {
153153
// TODO: Enable profiling interrupts.
154154
getg().m.profilehz = hz
155155
}
156+
157+
// gsignalStack is unused on Plan 9.
158+
type gsignalStack struct{}

src/runtime/os_nacl.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -285,6 +285,9 @@ func sigenable(uint32) {}
285285
func sigignore(uint32) {}
286286
func closeonexec(int32) {}
287287

288+
// gsignalStack is unused on nacl.
289+
type gsignalStack struct{}
290+
288291
var writelock uint32 // test-and-set spin lock for write
289292

290293
/*

src/runtime/runtime2.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -386,10 +386,11 @@ type m struct {
386386
divmod uint32 // div/mod denominator for arm - known to liblink
387387

388388
// Fields not known to debuggers.
389-
procid uint64 // for debuggers, but offset not hard-coded
390-
gsignal *g // signal-handling g
391-
sigmask sigset // storage for saved signal mask
392-
tls [6]uintptr // thread-local storage (for x86 extern register)
389+
procid uint64 // for debuggers, but offset not hard-coded
390+
gsignal *g // signal-handling g
391+
goSigStack gsignalStack // Go-allocated signal handling stack
392+
sigmask sigset // storage for saved signal mask
393+
tls [6]uintptr // thread-local storage (for x86 extern register)
393394
mstartfn func()
394395
curg *g // current running goroutine
395396
caughtsig guintptr // goroutine running during fatal signal

src/runtime/signal_unix.go

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -702,7 +702,7 @@ func minitSignalStack() {
702702
signalstack(&_g_.m.gsignal.stack)
703703
_g_.m.newSigstack = true
704704
} else {
705-
setGsignalStack(&st, nil)
705+
setGsignalStack(&st, &_g_.m.goSigStack)
706706
_g_.m.newSigstack = false
707707
}
708708
}
@@ -732,6 +732,14 @@ func unminitSignals() {
732732
if getg().m.newSigstack {
733733
st := stackt{ss_flags: _SS_DISABLE}
734734
sigaltstack(&st, nil)
735+
} else {
736+
// We got the signal stack from someone else. Restore
737+
// the Go-allocated stack in case this M gets reused
738+
// for another thread (e.g., it's an extram). Also, on
739+
// Android, libc allocates a signal stack for all
740+
// threads, so it's important to restore the Go stack
741+
// even on Go-created threads so we can free it.
742+
restoreGsignalStack(&getg().m.goSigStack)
735743
}
736744
}
737745

src/runtime/signal_windows.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -223,3 +223,6 @@ func crash() {
223223
// It's okay to leave this empty for now: if crash returns
224224
// the ordinary exit-after-panic happens.
225225
}
226+
227+
// gsignalStack is unused on Windows.
228+
type gsignalStack struct{}
Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
// Copyright 2017 The Go Authors. All rights reserved.
2+
// Use of this source code is governed by a BSD-style
3+
// license that can be found in the LICENSE file.
4+
5+
// +build !plan9,!windows
6+
7+
// Test handling of Go-allocated signal stacks when calling from
8+
// C-created threads with and without signal stacks. (See issue
9+
// #22930.)
10+
11+
package main
12+
13+
/*
14+
#include <pthread.h>
15+
#include <signal.h>
16+
#include <stdio.h>
17+
#include <stdlib.h>
18+
#include <sys/mman.h>
19+
20+
#ifndef MAP_STACK
21+
#define MAP_STACK 0
22+
#endif
23+
24+
extern void SigStackCallback();
25+
26+
static void* WithSigStack(void* arg __attribute__((unused))) {
27+
// Set up an alternate system stack.
28+
void* base = mmap(0, SIGSTKSZ, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS|MAP_STACK, -1, 0);
29+
if (base == MAP_FAILED) {
30+
perror("mmap failed");
31+
abort();
32+
}
33+
stack_t st = {}, ost = {};
34+
st.ss_sp = (char*)base;
35+
st.ss_flags = 0;
36+
st.ss_size = SIGSTKSZ;
37+
if (sigaltstack(&st, &ost) < 0) {
38+
perror("sigaltstack failed");
39+
abort();
40+
}
41+
42+
// Call Go.
43+
SigStackCallback();
44+
45+
// Disable signal stack and protect it so we can detect reuse.
46+
if (ost.ss_flags & SS_DISABLE) {
47+
// Darwin libsystem has a bug where it checks ss_size
48+
// even if SS_DISABLE is set. (The kernel gets it right.)
49+
ost.ss_size = SIGSTKSZ;
50+
}
51+
if (sigaltstack(&ost, NULL) < 0) {
52+
perror("sigaltstack restore failed");
53+
abort();
54+
}
55+
mprotect(base, SIGSTKSZ, PROT_NONE);
56+
return NULL;
57+
}
58+
59+
static void* WithoutSigStack(void* arg __attribute__((unused))) {
60+
SigStackCallback();
61+
return NULL;
62+
}
63+
64+
static void DoThread(int sigstack) {
65+
pthread_t tid;
66+
if (sigstack) {
67+
pthread_create(&tid, NULL, WithSigStack, NULL);
68+
} else {
69+
pthread_create(&tid, NULL, WithoutSigStack, NULL);
70+
}
71+
pthread_join(tid, NULL);
72+
}
73+
*/
74+
import "C"
75+
76+
func init() {
77+
register("SigStack", SigStack)
78+
}
79+
80+
func SigStack() {
81+
C.DoThread(0)
82+
C.DoThread(1)
83+
C.DoThread(0)
84+
C.DoThread(1)
85+
println("OK")
86+
}
87+
88+
var BadPtr *int
89+
90+
//export SigStackCallback
91+
func SigStackCallback() {
92+
// Cause the Go signal handler to run.
93+
defer func() { recover() }()
94+
*BadPtr = 42
95+
}

0 commit comments

Comments
 (0)