From 78074f6850c34a955d69f578e363d1d3f3e00e25 Mon Sep 17 00:00:00 2001 From: Elias Naur Date: Sun, 29 Jan 2017 15:34:50 +0100 Subject: [PATCH] runtime: handle SIGPIPE in c-archive and c-shared programs Before this CL, Go programs in c-archive or c-shared buildmodes would not handle SIGPIPE. That leads to surprising behaviour where writes on a closed pipe or socket would raise SIGPIPE and terminate the program. This CL changes the Go runtime to handle SIGPIPE regardless of buildmode. In addition, SIGPIPE from non-Go code is forwarded. This is a refinement of CL 32796 that fixes the case where a non-default handler for SIGPIPE is installed by the host C program. Fixes #17393 Change-Id: Ia41186e52c1ac209d0a594bae9904166ae7df7de Reviewed-on: https://go-review.googlesource.com/35960 Run-TryBot: Elias Naur TryBot-Result: Gobot Gobot Reviewed-by: Ian Lance Taylor --- misc/cgo/testcarchive/carchive_test.go | 19 ++++++++++++ misc/cgo/testcarchive/main2.c | 35 +++++++++++++++++++++- misc/cgo/testcarchive/main3.c | 33 ++++++++++++++++++++ misc/cgo/testcarchive/main5.c | 18 +++++++++++ misc/cgo/testcarchive/src/libgo2/libgo2.go | 30 +++++++++++++++++++ misc/cgo/testcarchive/src/libgo3/libgo3.go | 12 ++++++++ src/os/signal/doc.go | 9 +++--- src/runtime/cgocall.go | 4 +++ src/runtime/runtime2.go | 1 + src/runtime/signal_unix.go | 15 ++++++---- 10 files changed, 165 insertions(+), 11 deletions(-) diff --git a/misc/cgo/testcarchive/carchive_test.go b/misc/cgo/testcarchive/carchive_test.go index 4999929775..3c768a0ef3 100644 --- a/misc/cgo/testcarchive/carchive_test.go +++ b/misc/cgo/testcarchive/carchive_test.go @@ -265,6 +265,25 @@ func TestSignalForwarding(t *testing.T) { t.Logf("%s", out) t.Errorf("got %v; expected SIGSEGV", ee) } + + // Test SIGPIPE forwarding + cmd = exec.Command(bin[0], append(bin[1:], "3")...) + + out, err = cmd.CombinedOutput() + + if err == nil { + t.Logf("%s", out) + t.Error("test program succeeded unexpectedly") + } else if ee, ok := err.(*exec.ExitError); !ok { + t.Logf("%s", out) + t.Errorf("error (%v) has type %T; expected exec.ExitError", err, err) + } else if ws, ok := ee.Sys().(syscall.WaitStatus); !ok { + t.Logf("%s", out) + t.Errorf("error.Sys (%v) has type %T; expected syscall.WaitStatus", ee.Sys(), ee.Sys()) + } else if !ws.Signaled() || ws.Signal() != syscall.SIGPIPE { + t.Logf("%s", out) + t.Errorf("got %v; expected SIGPIPE", ee) + } } func TestSignalForwardingExternal(t *testing.T) { diff --git a/misc/cgo/testcarchive/main2.c b/misc/cgo/testcarchive/main2.c index 774e014a16..769cd497e6 100644 --- a/misc/cgo/testcarchive/main2.c +++ b/misc/cgo/testcarchive/main2.c @@ -17,6 +17,7 @@ #include #include #include +#include #include "libgo2.h" @@ -26,6 +27,7 @@ static void die(const char* msg) { } static volatile sig_atomic_t sigioSeen; +static volatile sig_atomic_t sigpipeSeen; // Use up some stack space. static void recur(int i, char *p) { @@ -37,6 +39,10 @@ static void recur(int i, char *p) { } } +static void pipeHandler(int signo, siginfo_t* info, void* ctxt) { + sigpipeSeen = 1; +} + // Signal handler that uses up more stack space than a goroutine will have. static void ioHandler(int signo, siginfo_t* info, void* ctxt) { char a[1024]; @@ -106,6 +112,10 @@ static void init() { die("sigaction"); } + sa.sa_sigaction = pipeHandler; + if (sigaction(SIGPIPE, &sa, NULL) < 0) { + die("sigaction"); + } } int main(int argc, char** argv) { @@ -167,7 +177,30 @@ int main(int argc, char** argv) { nanosleep(&ts, NULL); i++; if (i > 5000) { - fprintf(stderr, "looping too long waiting for signal\n"); + fprintf(stderr, "looping too long waiting for SIGIO\n"); + exit(EXIT_FAILURE); + } + } + + if (verbose) { + printf("provoking SIGPIPE\n"); + } + + GoRaiseSIGPIPE(); + + if (verbose) { + printf("waiting for sigpipeSeen\n"); + } + + // Wait until the signal has been delivered. + i = 0; + while (!sigpipeSeen) { + ts.tv_sec = 0; + ts.tv_nsec = 1000000; + nanosleep(&ts, NULL); + i++; + if (i > 5000) { + fprintf(stderr, "looping too long waiting for SIGPIPE\n"); exit(EXIT_FAILURE); } } diff --git a/misc/cgo/testcarchive/main3.c b/misc/cgo/testcarchive/main3.c index 0a6c0d3f74..5a1a60d4cd 100644 --- a/misc/cgo/testcarchive/main3.c +++ b/misc/cgo/testcarchive/main3.c @@ -25,6 +25,31 @@ static void ioHandler(int signo, siginfo_t* info, void* ctxt) { sigioSeen = 1; } +// Set up the SIGPIPE signal handler in a high priority constructor, so +// that it is installed before the Go code starts. + +static void pipeHandler(int signo, siginfo_t* info, void* ctxt) { + const char *s = "unexpected SIGPIPE\n"; + write(2, s, strlen(s)); + exit(EXIT_FAILURE); +} + +static void init(void) __attribute__ ((constructor (200))); + +static void init() { + struct sigaction sa; + + memset(&sa, 0, sizeof sa); + sa.sa_sigaction = pipeHandler; + if (sigemptyset(&sa.sa_mask) < 0) { + die("sigemptyset"); + } + sa.sa_flags = SA_SIGINFO; + if (sigaction(SIGPIPE, &sa, NULL) < 0) { + die("sigaction"); + } +} + int main(int argc, char** argv) { int verbose; struct sigaction sa; @@ -34,6 +59,14 @@ int main(int argc, char** argv) { verbose = argc > 2; setvbuf(stdout, NULL, _IONBF, 0); + if (verbose) { + printf("raising SIGPIPE\n"); + } + + // Test that the Go runtime handles SIGPIPE, even if we installed + // a non-default SIGPIPE handler before the runtime initializes. + ProvokeSIGPIPE(); + if (verbose) { printf("calling sigaction\n"); } diff --git a/misc/cgo/testcarchive/main5.c b/misc/cgo/testcarchive/main5.c index 9fadf0801e..2437bf07c5 100644 --- a/misc/cgo/testcarchive/main5.c +++ b/misc/cgo/testcarchive/main5.c @@ -68,6 +68,24 @@ int main(int argc, char** argv) { break; } + case 3: { + if (verbose) { + printf("attempting SIGPIPE\n"); + } + + int fd[2]; + if (pipe(fd) != 0) { + printf("pipe(2) failed\n"); + return 0; + } + // Close the reading end. + close(fd[0]); + // Expect that write(2) fails (EPIPE) + if (write(fd[1], "some data", 9) != -1) { + printf("write(2) unexpectedly succeeded\n"); + return 0; + } + } default: printf("Unknown test: %d\n", test); return 0; diff --git a/misc/cgo/testcarchive/src/libgo2/libgo2.go b/misc/cgo/testcarchive/src/libgo2/libgo2.go index fbed493b93..19c8e1a6dc 100644 --- a/misc/cgo/testcarchive/src/libgo2/libgo2.go +++ b/misc/cgo/testcarchive/src/libgo2/libgo2.go @@ -4,6 +4,30 @@ package main +/* +#include +#include +#include +#include + +// Raise SIGPIPE. +static void CRaiseSIGPIPE() { + int fds[2]; + + if (pipe(fds) == -1) { + perror("pipe"); + exit(EXIT_FAILURE); + } + // Close the reader end + close(fds[0]); + // Write to the writer end to provoke a SIGPIPE + if (write(fds[1], "some data", 9) != -1) { + fprintf(stderr, "write to a closed pipe succeeded\n"); + exit(EXIT_FAILURE); + } + close(fds[1]); +} +*/ import "C" import ( @@ -46,5 +70,11 @@ func TestSEGV() { func Noop() { } +// Raise SIGPIPE. +//export GoRaiseSIGPIPE +func GoRaiseSIGPIPE() { + C.CRaiseSIGPIPE() +} + func main() { } diff --git a/misc/cgo/testcarchive/src/libgo3/libgo3.go b/misc/cgo/testcarchive/src/libgo3/libgo3.go index 94e5d21c14..e276a3c347 100644 --- a/misc/cgo/testcarchive/src/libgo3/libgo3.go +++ b/misc/cgo/testcarchive/src/libgo3/libgo3.go @@ -40,5 +40,17 @@ func SawSIGIO() C.int { } } +// ProvokeSIGPIPE provokes a kernel-initiated SIGPIPE. +//export ProvokeSIGPIPE +func ProvokeSIGPIPE() { + r, w, err := os.Pipe() + if err != nil { + panic(err) + } + r.Close() + defer w.Close() + w.Write([]byte("some data")) +} + func main() { } diff --git a/src/os/signal/doc.go b/src/os/signal/doc.go index 73b01a2764..16f49c7ab8 100644 --- a/src/os/signal/doc.go +++ b/src/os/signal/doc.go @@ -181,10 +181,11 @@ If the Go runtime sees an existing signal handler for the SIGCANCEL or SIGSETXID signals (which are used only on GNU/Linux), it will turn on the SA_ONSTACK flag and otherwise keep the signal handler. -For the synchronous signals, the Go runtime will install a signal -handler. It will save any existing signal handler. If a synchronous -signal arrives while executing non-Go code, the Go runtime will invoke -the existing signal handler instead of the Go signal handler. +For the synchronous signals and SIGPIPE, the Go runtime will install a +signal handler. It will save any existing signal handler. If a +synchronous signal arrives while executing non-Go code, the Go runtime +will invoke the existing signal handler instead of the Go signal +handler. Go code built with -buildmode=c-archive or -buildmode=c-shared will not install any other signal handlers by default. If there is an diff --git a/src/runtime/cgocall.go b/src/runtime/cgocall.go index 879e786231..755269ebd2 100644 --- a/src/runtime/cgocall.go +++ b/src/runtime/cgocall.go @@ -110,6 +110,7 @@ func cgocall(fn, arg unsafe.Pointer) int32 { mp := getg().m mp.ncgocall++ mp.ncgo++ + mp.incgo = true // Reset traceback. mp.cgoCallers[0] = 0 @@ -151,6 +152,7 @@ func cgocall(fn, arg unsafe.Pointer) int32 { //go:nosplit func endcgo(mp *m) { + mp.incgo = false mp.ncgo-- if raceenabled { @@ -180,9 +182,11 @@ func cgocallbackg(ctxt uintptr) { savedsp := unsafe.Pointer(gp.syscallsp) savedpc := gp.syscallpc exitsyscall(0) // coming out of cgo call + gp.m.incgo = false cgocallbackg1(ctxt) + gp.m.incgo = true // going back to cgo call reentersyscall(savedpc, uintptr(savedsp)) diff --git a/src/runtime/runtime2.go b/src/runtime/runtime2.go index 1ceab0ad8c..c164c0f7b4 100644 --- a/src/runtime/runtime2.go +++ b/src/runtime/runtime2.go @@ -429,6 +429,7 @@ type m struct { inwb bool // m is executing a write barrier newSigstack bool // minit on C thread called sigaltstack printlock int8 + incgo bool // m is executing a cgo call fastrand uint32 ncgocall uint64 // number of cgo calls in total ncgo int32 // number of cgo calls currently in progress diff --git a/src/runtime/signal_unix.go b/src/runtime/signal_unix.go index 49c7579f27..0bf5a752a9 100644 --- a/src/runtime/signal_unix.go +++ b/src/runtime/signal_unix.go @@ -111,8 +111,8 @@ func sigInstallGoHandler(sig uint32) bool { } // When built using c-archive or c-shared, only install signal - // handlers for synchronous signals. - if (isarchive || islibrary) && t.flags&_SigPanic == 0 { + // handlers for synchronous signals and SIGPIPE. + if (isarchive || islibrary) && t.flags&_SigPanic == 0 && sig != _SIGPIPE { return false } @@ -518,16 +518,19 @@ func sigfwdgo(sig uint32, info *siginfo, ctx unsafe.Pointer) bool { return true } - // Only forward synchronous signals. c := &sigctxt{info, ctx} - if c.sigcode() == _SI_USER || flags&_SigPanic == 0 { + // Only forward synchronous signals and SIGPIPE. + // Unfortunately, user generated SIGPIPEs will also be forwarded, because si_code + // is set to _SI_USER even for a SIGPIPE raised from a write to a closed socket + // or pipe. + if (c.sigcode() == _SI_USER || flags&_SigPanic == 0) && sig != _SIGPIPE { return false } // Determine if the signal occurred inside Go code. We test that: // (1) we were in a goroutine (i.e., m.curg != nil), and - // (2) we weren't in CGO (i.e., m.curg.syscallsp == 0). + // (2) we weren't in CGO. g := getg() - if g != nil && g.m != nil && g.m.curg != nil && g.m.curg.syscallsp == 0 { + if g != nil && g.m != nil && g.m.curg != nil && !g.m.incgo { return false } // Signal not handled by Go, forward it. -- 2.50.0