]> Cypherpunks repositories - gostls13.git/commitdiff
runtime: block signals in needm before allocating M
authorIan Lance Taylor <iant@golang.org>
Tue, 27 Oct 2020 23:09:40 +0000 (16:09 -0700)
committerIan Lance Taylor <iant@golang.org>
Wed, 28 Oct 2020 01:03:23 +0000 (01:03 +0000)
Otherwise, if a signal occurs just after we allocated the M,
we can deadlock if the signal handler needs to allocate an M
itself.

Fixes #42207

Change-Id: I76f44547f419e8b1c14cbf49bf602c6e645d8c14
Reviewed-on: https://go-review.googlesource.com/c/go/+/265759
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
src/runtime/crash_unix_test.go
src/runtime/os_js.go
src/runtime/os_plan9.go
src/runtime/os_windows.go
src/runtime/proc.go
src/runtime/signal_unix.go
src/runtime/testdata/testprogcgo/needmdeadlock.go [new file with mode: 0644]

index fc87f374088868a5728d467fa3fd660eb71869cb..7aba3d48462fdcebe9c6edec83cb99ccdabc3ac0 100644 (file)
@@ -358,3 +358,12 @@ func TestSignalM(t *testing.T) {
                t.Fatalf("signal sent to M %d, but received on M %d", want, got)
        }
 }
+
+// Issue #42207.
+func TestNeedmDeadlock(t *testing.T) {
+       output := runTestProg(t, "testprogcgo", "NeedmDeadlock")
+       want := "OK\n"
+       if output != want {
+               t.Fatalf("want %s, got %s\n", want, output)
+       }
+}
index ff0ee3aa6be8dfd4ae3adfbb82c9eaeaa4f18031..94983b358d4abdc604a84fcb488e3628bc81e397 100644 (file)
@@ -59,7 +59,7 @@ func mpreinit(mp *m) {
 }
 
 //go:nosplit
-func msigsave(mp *m) {
+func sigsave(p *sigset) {
 }
 
 //go:nosplit
index f3037a750867411b0c73276cfa5a93c76e5b85c2..62aecea06041de4bb65afdf959d9c18526915bff 100644 (file)
@@ -184,7 +184,7 @@ func mpreinit(mp *m) {
        mp.errstr = (*byte)(mallocgc(_ERRMAX, nil, true))
 }
 
-func msigsave(mp *m) {
+func sigsave(p *sigset) {
 }
 
 func msigrestore(sigmask sigset) {
index 9dd140c9520ea1af7c72132bf7917c47b1305ac9..ffb087f9db2794794ab2308dc642f7b57bb3b620 100644 (file)
@@ -873,7 +873,7 @@ func mpreinit(mp *m) {
 }
 
 //go:nosplit
-func msigsave(mp *m) {
+func sigsave(p *sigset) {
 }
 
 //go:nosplit
index 87d4b6e56840cef991eb8c268aa71da626d1d2a2..b335e1184d8ff82eb05aa1fdd65758bdbae7b323 100644 (file)
@@ -598,7 +598,7 @@ func schedinit() {
        typelinksinit() // uses maps, activeModules
        itabsinit()     // uses activeModules
 
-       msigsave(_g_.m)
+       sigsave(&_g_.m.sigmask)
        initSigmask = _g_.m.sigmask
 
        goargs()
@@ -1707,6 +1707,18 @@ func needm() {
                exit(1)
        }
 
+       // Save and block signals before getting an M.
+       // The signal handler may call needm itself,
+       // and we must avoid a deadlock. Also, once g is installed,
+       // any incoming signals will try to execute,
+       // but we won't have the sigaltstack settings and other data
+       // set up appropriately until the end of minit, which will
+       // unblock the signals. This is the same dance as when
+       // starting a new m to run Go code via newosproc.
+       var sigmask sigset
+       sigsave(&sigmask)
+       sigblock()
+
        // Lock extra list, take head, unlock popped list.
        // nilokay=false is safe here because of the invariant above,
        // that the extra list always contains or will soon contain
@@ -1724,14 +1736,8 @@ func needm() {
        extraMCount--
        unlockextra(mp.schedlink.ptr())
 
-       // Save and block signals before installing g.
-       // Once g is installed, any incoming signals will try to execute,
-       // but we won't have the sigaltstack settings and other data
-       // set up appropriately until the end of minit, which will
-       // unblock the signals. This is the same dance as when
-       // starting a new m to run Go code via newosproc.
-       msigsave(mp)
-       sigblock()
+       // Store the original signal mask for use by minit.
+       mp.sigmask = sigmask
 
        // Install g (= m->g0) and set the stack bounds
        // to match the current stack. We don't actually know
@@ -3676,7 +3682,7 @@ func beforefork() {
        // a signal handler before exec if a signal is sent to the process
        // group. See issue #18600.
        gp.m.locks++
-       msigsave(gp.m)
+       sigsave(&gp.m.sigmask)
        sigblock()
 
        // This function is called before fork in syscall package.
index 9318a9b8bc51370aef3c066fda3c24ffadce6b9e..bf4a319b37a7d8d0afdb837c8a97b82352d77fbb 100644 (file)
@@ -1031,15 +1031,15 @@ func sigfwdgo(sig uint32, info *siginfo, ctx unsafe.Pointer) bool {
        return true
 }
 
-// msigsave saves the current thread's signal mask into mp.sigmask.
+// sigsave saves the current thread's signal mask into *p.
 // This is used to preserve the non-Go signal mask when a non-Go
 // thread calls a Go function.
 // This is nosplit and nowritebarrierrec because it is called by needm
 // which may be called on a non-Go thread with no g available.
 //go:nosplit
 //go:nowritebarrierrec
-func msigsave(mp *m) {
-       sigprocmask(_SIG_SETMASK, nil, &mp.sigmask)
+func sigsave(p *sigset) {
+       sigprocmask(_SIG_SETMASK, nil, p)
 }
 
 // msigrestore sets the current thread's signal mask to sigmask.
@@ -1111,7 +1111,7 @@ func minitSignalStack() {
 // thread's signal mask. When this is called all signals have been
 // blocked for the thread.  This starts with m.sigmask, which was set
 // either from initSigmask for a newly created thread or by calling
-// msigsave if this is a non-Go thread calling a Go function. It
+// sigsave if this is a non-Go thread calling a Go function. It
 // removes all essential signals from the mask, thus causing those
 // signals to not be blocked. Then it sets the thread's signal mask.
 // After this is called the thread can receive signals.
diff --git a/src/runtime/testdata/testprogcgo/needmdeadlock.go b/src/runtime/testdata/testprogcgo/needmdeadlock.go
new file mode 100644 (file)
index 0000000..5a9c359
--- /dev/null
@@ -0,0 +1,95 @@
+// Copyright 2020 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// +build !plan9,!windows
+
+package main
+
+// This is for issue #42207.
+// During a call to needm we could get a SIGCHLD signal
+// which would itself call needm, causing a deadlock.
+
+/*
+#include <signal.h>
+#include <pthread.h>
+#include <sched.h>
+#include <unistd.h>
+
+extern void GoNeedM();
+
+#define SIGNALERS 10
+
+static void* needmSignalThread(void* p) {
+       pthread_t* pt = (pthread_t*)(p);
+       int i;
+
+       for (i = 0; i < 100; i++) {
+               if (pthread_kill(*pt, SIGCHLD) < 0) {
+                       return NULL;
+               }
+               usleep(1);
+       }
+       return NULL;
+}
+
+// We don't need many calls, as the deadlock is only likely
+// to occur the first couple of times that needm is called.
+// After that there will likely be an extra M available.
+#define CALLS 10
+
+static void* needmCallbackThread(void* p) {
+       int i;
+
+       for (i = 0; i < SIGNALERS; i++) {
+               sched_yield(); // Help the signal threads get started.
+       }
+       for (i = 0; i < CALLS; i++) {
+               GoNeedM();
+       }
+       return NULL;
+}
+
+static void runNeedmSignalThread() {
+       int i;
+       pthread_t caller;
+       pthread_t s[SIGNALERS];
+
+       pthread_create(&caller, NULL, needmCallbackThread, NULL);
+       for (i = 0; i < SIGNALERS; i++) {
+               pthread_create(&s[i], NULL, needmSignalThread, &caller);
+       }
+       for (i = 0; i < SIGNALERS; i++) {
+               pthread_join(s[i], NULL);
+       }
+       pthread_join(caller, NULL);
+}
+*/
+import "C"
+
+import (
+       "fmt"
+       "os"
+       "time"
+)
+
+func init() {
+       register("NeedmDeadlock", NeedmDeadlock)
+}
+
+//export GoNeedM
+func GoNeedM() {
+}
+
+func NeedmDeadlock() {
+       // The failure symptom is that the program hangs because of a
+       // deadlock in needm, so set an alarm.
+       go func() {
+               time.Sleep(5 * time.Second)
+               fmt.Println("Hung for 5 seconds")
+               os.Exit(1)
+       }()
+
+       C.runNeedmSignalThread()
+       fmt.Println("OK")
+}