From 3fa54a93ebbfbb5674df145adcb3eb5b4e781c91 Mon Sep 17 00:00:00 2001
From: Fabian Meumertzheim <fabian@meumertzhe.im>
Date: Thu, 13 Feb 2025 08:33:50 +0100
Subject: [PATCH 1/3] Add test
---
src/os/signal/signal_test.go | 117 ++++++++++++++++++++++++++++++++++-
1 file changed, 114 insertions(+), 3 deletions(-)
diff --git a/src/os/signal/signal_test.go b/src/os/signal/signal_test.go
index d54787bc199a3b..d70127e43b0e03 100644
--- a/src/os/signal/signal_test.go
+++ b/src/os/signal/signal_test.go
@@ -217,9 +217,7 @@ func testCancel(t *testing.T, ignore bool) {
// Either way, this should undo both calls to Notify above.
if ignore {
Ignore(syscall.SIGWINCH, syscall.SIGHUP)
- // Don't bother deferring a call to Reset: it is documented to undo Notify,
- // but its documentation says nothing about Ignore, and (as of the time of
- // writing) it empirically does not undo an Ignore.
+ defer Reset(syscall.SIGWINCH, syscall.SIGHUP)
} else {
Reset(syscall.SIGWINCH, syscall.SIGHUP)
}
@@ -349,6 +347,8 @@ func TestStop(t *testing.T) {
for _, sig := range sigs {
sig := sig
t.Run(fmt.Sprint(sig), func(t *testing.T) {
+ defer Reset(sig)
+
// When calling Notify with a specific signal,
// independent signals should not interfere with each other,
// and we end up needing to wait for signals to quiesce a lot.
@@ -913,3 +913,114 @@ func TestSignalTrace(t *testing.T) {
close(quit)
<-done
}
+
+// #46321 test Reset actually undoes the effect of Ignore.
+func TestResetIgnore(t *testing.T) {
+ if os.Getenv("GO_TEST_RESET_IGNORE") != "" {
+ s, err := strconv.Atoi(os.Getenv("GO_TEST_RESET_IGNORE"))
+ if err != nil {
+ t.Fatalf("failed to parse signal: %v", err)
+ }
+ resetIgnoreTestProgram(syscall.Signal(s))
+ }
+
+ sigs := []syscall.Signal{
+ syscall.SIGHUP,
+ syscall.SIGINT,
+ syscall.SIGUSR1,
+ syscall.SIGTERM,
+ syscall.SIGCHLD,
+ syscall.SIGWINCH,
+ }
+
+ for _, notify := range []bool{false, true} {
+ for _, sig := range sigs {
+ t.Run(fmt.Sprintf("%s[notify=%t]", sig, notify), func(t *testing.T) {
+ if Ignored(sig) {
+ t.Fatalf("expected %q to not be ignored initially", sig)
+ }
+
+ Ignore(sig)
+ if notify {
+ c := make(chan os.Signal, 1)
+ Notify(c, sig)
+ defer Stop(c)
+ }
+ Reset(sig)
+
+ if Ignored(sig) {
+ t.Fatalf("expected %q to not be ignored", sig)
+ }
+
+ // Child processes inherit the ignored status of signals, so verify that it
+ // is indeed not ignored.
+ cmd := testenv.Command(t, testenv.Executable(t), "-test.run=^TestResetIgnore$")
+ cmd.Env = append(os.Environ(), "GO_TEST_RESET_IGNORE="+strconv.Itoa(int(sig)))
+ err := cmd.Run()
+ if _, ok := err.(*exec.ExitError); ok {
+ t.Fatalf("expected %q to not be ignored in child process", sig)
+ } else if err != nil {
+ t.Fatalf("child process failed to launch: %v", err)
+ }
+ })
+ }
+ }
+}
+
+func resetIgnoreTestProgram(sig os.Signal) {
+ if Ignored(sig) {
+ os.Exit(1)
+ }
+ os.Exit(0)
+}
+
+// #46321 test Reset correctly undoes the effect of Ignore when the child
+// process is started with a signal ignored.
+func TestInitiallyIgnoredResetIgnore(t *testing.T) {
+ testenv.MustHaveExec(t)
+
+ if os.Getenv("GO_TEST_INITIALLY_IGNORED_RESET_IGNORE") != "" {
+ s, err := strconv.Atoi(os.Getenv("GO_TEST_INITIALLY_IGNORED_RESET_IGNORE"))
+ if err != nil {
+ t.Fatalf("failed to parse signal: %v", err)
+ }
+ initiallyIgnoredResetIgnoreTestProgram(syscall.Signal(s))
+ }
+
+ sigs := []syscall.Signal{
+ syscall.SIGINT,
+ syscall.SIGHUP,
+ }
+
+ for _, sig := range sigs {
+ t.Run(fmt.Sprint(sig), func(t *testing.T) {
+ Ignore(sig)
+ defer Reset(sig)
+
+ cmd := testenv.Command(t, testenv.Executable(t), "-test.run=^TestInitiallyIgnoredResetIgnore$")
+ cmd.Env = append(os.Environ(), "GO_TEST_INITIALLY_IGNORED_RESET_IGNORE="+strconv.Itoa(int(sig)))
+ err := cmd.Run()
+ if _, ok := err.(*exec.ExitError); ok {
+ t.Fatalf("expected %q to be ignored in child process", sig)
+ } else if err != nil {
+ t.Fatalf("child process failed to launch: %v", err)
+ }
+ })
+ }
+}
+
+func initiallyIgnoredResetIgnoreTestProgram(sig os.Signal) {
+ if !Ignored(sig) {
+ os.Exit(1)
+ }
+ Reset(sig)
+ if !Ignored(sig) {
+ os.Exit(1)
+ }
+ Ignore(sig)
+ Reset(sig)
+ if !Ignored(sig) {
+ os.Exit(1)
+ }
+ os.Exit(0)
+}
From a1cecf9e1bc0497dd979f090076bce3a72c13d1d Mon Sep 17 00:00:00 2001
From: Fabian Meumertzheim <fabian@meumertzhe.im>
Date: Thu, 13 Feb 2025 08:33:50 +0100
Subject: [PATCH 2/3] Fix test
---
src/os/signal/signal.go | 4 ++--
src/runtime/os3_plan9.go | 3 ++-
src/runtime/os_wasm.go | 2 +-
src/runtime/signal_unix.go | 38 ++++++++++++++++++++++++++---------
src/runtime/signal_windows.go | 3 ++-
src/runtime/sigqueue.go | 10 ++++++++-
6 files changed, 44 insertions(+), 16 deletions(-)
diff --git a/src/os/signal/signal.go b/src/os/signal/signal.go
index b9fe16baa50b1f..81b4dbbcce3f91 100644
--- a/src/os/signal/signal.go
+++ b/src/os/signal/signal.go
@@ -168,8 +168,8 @@ func Notify(c chan<- os.Signal, sig ...os.Signal) {
}
}
-// Reset undoes the effect of any prior calls to [Notify] for the provided
-// signals.
+// Reset undoes the effect of any prior calls to [Notify] or [Ignore] for the
+// provided signals.
// If no signals are provided, all signal handlers will be reset.
func Reset(sig ...os.Signal) {
cancel(sig, disableSignal)
diff --git a/src/runtime/os3_plan9.go b/src/runtime/os3_plan9.go
index dd1570561803f8..efc5a33af71764 100644
--- a/src/runtime/os3_plan9.go
+++ b/src/runtime/os3_plan9.go
@@ -149,7 +149,8 @@ Exit:
func sigenable(sig uint32) {
}
-func sigdisable(sig uint32) {
+func sigdisable(sig uint32) bool {
+ return false
}
func sigignore(sig uint32) {
diff --git a/src/runtime/os_wasm.go b/src/runtime/os_wasm.go
index 8046caf45ef133..296e0fd860f127 100644
--- a/src/runtime/os_wasm.go
+++ b/src/runtime/os_wasm.go
@@ -142,7 +142,7 @@ func getfp() uintptr { return 0 }
func setProcessCPUProfiler(hz int32) {}
func setThreadCPUProfiler(hz int32) {}
-func sigdisable(uint32) {}
+func sigdisable(uint32) bool { return false }
func sigenable(uint32) {}
func sigignore(uint32) {}
diff --git a/src/runtime/signal_unix.go b/src/runtime/signal_unix.go
index 96628d6baae910..8ac2724332a096 100644
--- a/src/runtime/signal_unix.go
+++ b/src/runtime/signal_unix.go
@@ -202,23 +202,28 @@ func sigenable(sig uint32) {
enableSigChan <- sig
<-maskUpdatedChan
if atomic.Cas(&handlingSig[sig], 0, 1) {
- atomic.Storeuintptr(&fwdSig[sig], getsig(sig))
+ h := getsig(sig)
+ if h != _SIG_IGN {
+ atomic.Storeuintptr(&fwdSig[sig], h)
+ }
setsig(sig, abi.FuncPCABIInternal(sighandler))
}
}
}
-// sigdisable disables the Go signal handler for the signal sig.
+// sigdisable resets the signal handler for the signal sig back to the default
+// at program startup or the last custom handler registered by cgo.
// It is only called while holding the os/signal.handlers lock,
// via os/signal.disableSignal and signal_disable.
-func sigdisable(sig uint32) {
+// Returns true if the signal is ignored after the change.
+func sigdisable(sig uint32) bool {
if sig >= uint32(len(sigtable)) {
- return
+ return false
}
// SIGPROF is handled specially for profiling.
if sig == _SIGPROF {
- return
+ return false
}
t := &sigtable[sig]
@@ -227,14 +232,27 @@ func sigdisable(sig uint32) {
disableSigChan <- sig
<-maskUpdatedChan
- // If initsig does not install a signal handler for a
- // signal, then to go back to the state before Notify
- // we should remove the one we installed.
- if !sigInstallGoHandler(sig) {
+ if sigInstallGoHandler(sig) {
+ if atomic.Cas(&handlingSig[sig], 0, 1) {
+ h := getsig(sig)
+ // Preserve custom signal handlers installed with cgo.
+ if h != _SIG_IGN && h != _SIG_DFL {
+ atomic.Storeuintptr(&fwdSig[sig], h)
+ }
+ setsig(sig, abi.FuncPCABIInternal(sighandler))
+ }
+ return false
+ } else {
+ // If initsig does not install a signal handler for a
+ // signal, then to go back to the state before Notify
+ // we should remove the one we installed.
atomic.Store(&handlingSig[sig], 0)
- setsig(sig, atomic.Loaduintptr(&fwdSig[sig]))
+ fs := atomic.Loaduintptr(&fwdSig[sig])
+ setsig(sig, fs)
+ return fs == _SIG_IGN
}
}
+ return false
}
// sigignore ignores the signal sig.
diff --git a/src/runtime/signal_windows.go b/src/runtime/signal_windows.go
index b0c653ee46dd0b..67d68476513a3f 100644
--- a/src/runtime/signal_windows.go
+++ b/src/runtime/signal_windows.go
@@ -434,7 +434,8 @@ func initsig(preinit bool) {
func sigenable(sig uint32) {
}
-func sigdisable(sig uint32) {
+func sigdisable(sig uint32) bool {
+ return false
}
func sigignore(sig uint32) {
diff --git a/src/runtime/sigqueue.go b/src/runtime/sigqueue.go
index 62a8e8a70226fe..299e5ee87e28ef 100644
--- a/src/runtime/sigqueue.go
+++ b/src/runtime/sigqueue.go
@@ -230,11 +230,19 @@ func signal_disable(s uint32) {
if s >= uint32(len(sig.wanted)*32) {
return
}
- sigdisable(s)
+ ignored := sigdisable(s)
w := sig.wanted[s/32]
w &^= 1 << (s & 31)
atomic.Store(&sig.wanted[s/32], w)
+
+ i := sig.ignored[s/32]
+ if ignored {
+ i |= 1 << (s & 31)
+ } else {
+ i &^= 1 << (s & 31)
+ }
+ atomic.Store(&sig.ignored[s/32], i)
}
// Must only be called from a single goroutine at a time.
From 6dd5b0dbda6b7cdcbc971d5e4fc4f96403661498 Mon Sep 17 00:00:00 2001
From: Fabian Meumertzheim <fabian@meumertzhe.im>
Date: Thu, 20 Feb 2025 22:37:49 +0100
Subject: [PATCH 3/3] Address first round of comments
---
src/os/signal/signal_test.go | 26 +++++++++-----------------
src/runtime/signal_unix.go | 2 +-
2 files changed, 10 insertions(+), 18 deletions(-)
diff --git a/src/os/signal/signal_test.go b/src/os/signal/signal_test.go
index d70127e43b0e03..d56866723489d8 100644
--- a/src/os/signal/signal_test.go
+++ b/src/os/signal/signal_test.go
@@ -921,7 +921,10 @@ func TestResetIgnore(t *testing.T) {
if err != nil {
t.Fatalf("failed to parse signal: %v", err)
}
- resetIgnoreTestProgram(syscall.Signal(s))
+ if Ignored(syscall.Signal(s)) {
+ os.Exit(1)
+ }
+ os.Exit(0)
}
sigs := []syscall.Signal{
@@ -937,7 +940,7 @@ func TestResetIgnore(t *testing.T) {
for _, sig := range sigs {
t.Run(fmt.Sprintf("%s[notify=%t]", sig, notify), func(t *testing.T) {
if Ignored(sig) {
- t.Fatalf("expected %q to not be ignored initially", sig)
+ t.Skipf("expected %q to not be ignored initially", sig)
}
Ignore(sig)
@@ -957,23 +960,14 @@ func TestResetIgnore(t *testing.T) {
cmd := testenv.Command(t, testenv.Executable(t), "-test.run=^TestResetIgnore$")
cmd.Env = append(os.Environ(), "GO_TEST_RESET_IGNORE="+strconv.Itoa(int(sig)))
err := cmd.Run()
- if _, ok := err.(*exec.ExitError); ok {
- t.Fatalf("expected %q to not be ignored in child process", sig)
- } else if err != nil {
- t.Fatalf("child process failed to launch: %v", err)
+ if err != nil {
+ t.Fatalf("expected %q to not be ignored in child process: %v", sig, err)
}
})
}
}
}
-func resetIgnoreTestProgram(sig os.Signal) {
- if Ignored(sig) {
- os.Exit(1)
- }
- os.Exit(0)
-}
-
// #46321 test Reset correctly undoes the effect of Ignore when the child
// process is started with a signal ignored.
func TestInitiallyIgnoredResetIgnore(t *testing.T) {
@@ -1000,10 +994,8 @@ func TestInitiallyIgnoredResetIgnore(t *testing.T) {
cmd := testenv.Command(t, testenv.Executable(t), "-test.run=^TestInitiallyIgnoredResetIgnore$")
cmd.Env = append(os.Environ(), "GO_TEST_INITIALLY_IGNORED_RESET_IGNORE="+strconv.Itoa(int(sig)))
err := cmd.Run()
- if _, ok := err.(*exec.ExitError); ok {
- t.Fatalf("expected %q to be ignored in child process", sig)
- } else if err != nil {
- t.Fatalf("child process failed to launch: %v", err)
+ if err != nil {
+ t.Fatalf("expected %q to be ignored in child process: %v", sig, err)
}
})
}
diff --git a/src/runtime/signal_unix.go b/src/runtime/signal_unix.go
index 8ac2724332a096..c8dcdbc87a1d9f 100644
--- a/src/runtime/signal_unix.go
+++ b/src/runtime/signal_unix.go
@@ -215,7 +215,7 @@ func sigenable(sig uint32) {
// at program startup or the last custom handler registered by cgo.
// It is only called while holding the os/signal.handlers lock,
// via os/signal.disableSignal and signal_disable.
-// Returns true if the signal is ignored after the change.
+// Reports whether the signal is ignored after the change.
func sigdisable(sig uint32) bool {
if sig >= uint32(len(sigtable)) {
return false