From 532dee3842298ad242355fd210efbd658cc93196 Mon Sep 17 00:00:00 2001 From: Alan Donovan Date: Tue, 4 Sep 2012 14:40:49 -0400 Subject: [PATCH] runtime: discard SIGPROF delivered to non-Go threads. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signal handlers are global resources but many language environments (Go, C++ at Google, etc) assume they have sole ownership of a particular handler. Signal handlers in mixed-language applications must therefore be robust against unexpected delivery of certain signals, such as SIGPROF. The default Go signal handler runtime·sigtramp assumes that it will never be called on a non-Go thread, but this assumption is violated by when linking in C++ code that spawns threads. Specifically, the handler asserts the thread has an associated "m" (Go scheduler). This CL is a very simple workaround: discard SIGPROF delivered to non-Go threads. runtime.badsignal(int32) now receives the signal number; if it returns without panicking (e.g. sig==SIGPROF) the signal is discarded. I don't think there is any really satisfactory solution to the problem of signal-based profiling in a mixed-language application. It's not only the issue of handler clobbering, but also that a C++ SIGPROF handler called in a Go thread can't unwind the Go stack (and vice versa). The best we can hope for is not crashing. Note: - I've ported this to all POSIX platforms, except ARM-linux which already ignores unexpected signals on m-less threads. - I've avoided tail-calling runtime.badsignal because AFAICT the 6a/6l don't support it. - I've avoided hoisting 'push sig' (common to both function calls) because it makes the code harder to read. - Fixed an (apparently incorrect?) docstring. R=iant, rsc, minux.ma CC=golang-dev https://golang.org/cl/6498057 --- src/pkg/runtime/signal_linux_amd64.c | 2 ++ src/pkg/runtime/sys_darwin_386.s | 9 ++++++--- src/pkg/runtime/sys_darwin_amd64.s | 4 +++- src/pkg/runtime/sys_freebsd_386.s | 5 ++++- src/pkg/runtime/sys_freebsd_amd64.s | 4 +++- src/pkg/runtime/sys_linux_386.s | 5 ++++- src/pkg/runtime/sys_linux_amd64.s | 4 +++- src/pkg/runtime/sys_linux_arm.s | 3 ++- src/pkg/runtime/sys_netbsd_386.s | 5 ++++- src/pkg/runtime/sys_netbsd_amd64.s | 4 +++- src/pkg/runtime/sys_openbsd_386.s | 5 ++++- src/pkg/runtime/sys_openbsd_amd64.s | 4 +++- src/pkg/runtime/thread_darwin.c | 6 +++++- src/pkg/runtime/thread_freebsd.c | 6 +++++- src/pkg/runtime/thread_linux.c | 6 +++++- src/pkg/runtime/thread_netbsd.c | 6 +++++- src/pkg/runtime/thread_openbsd.c | 6 +++++- src/pkg/runtime/thread_plan9.c | 2 +- 18 files changed, 67 insertions(+), 19 deletions(-) diff --git a/src/pkg/runtime/signal_linux_amd64.c b/src/pkg/runtime/signal_linux_amd64.c index 8ff5be78591..96088f781d6 100644 --- a/src/pkg/runtime/signal_linux_amd64.c +++ b/src/pkg/runtime/signal_linux_amd64.c @@ -135,6 +135,8 @@ runtime·setsig(int32 i, void (*fn)(int32, Siginfo*, void*, G*), bool restart) if(restart) sa.sa_flags |= SA_RESTART; sa.sa_mask = ~0ULL; + // TODO(adonovan): Linux manpage says "sa_restorer element is + // obsolete and should not be used". Avoid it here, and test. sa.sa_restorer = (void*)runtime·sigreturn; if(fn == runtime·sighandler) fn = (void*)runtime·sigtramp; diff --git a/src/pkg/runtime/sys_darwin_386.s b/src/pkg/runtime/sys_darwin_386.s index 5f7919dc8cc..c1652090cbe 100644 --- a/src/pkg/runtime/sys_darwin_386.s +++ b/src/pkg/runtime/sys_darwin_386.s @@ -213,8 +213,8 @@ TEXT runtime·sigaction(SB),7,$0 // It is called with the following arguments on the stack: // 0(FP) "return address" - ignored // 4(FP) actual handler -// 8(FP) siginfo style - ignored -// 12(FP) signal number +// 8(FP) signal number +// 12(FP) siginfo style // 16(FP) siginfo // 20(FP) context TEXT runtime·sigtramp(SB),7,$40 @@ -223,8 +223,11 @@ TEXT runtime·sigtramp(SB),7,$40 // check that m exists MOVL m(CX), BP CMPL BP, $0 - JNE 2(PC) + JNE 5(PC) + MOVL sig+8(FP), BX + MOVL BX, 0(SP) CALL runtime·badsignal(SB) + RET // save g MOVL g(CX), DI diff --git a/src/pkg/runtime/sys_darwin_amd64.s b/src/pkg/runtime/sys_darwin_amd64.s index 36e49ebf8bc..69207c8d8ad 100644 --- a/src/pkg/runtime/sys_darwin_amd64.s +++ b/src/pkg/runtime/sys_darwin_amd64.s @@ -173,8 +173,10 @@ TEXT runtime·sigtramp(SB),7,$64 // check that m exists MOVQ m(BX), BP CMPQ BP, $0 - JNE 2(PC) + JNE 4(PC) + MOVL DX, 0(SP) CALL runtime·badsignal(SB) + RET // save g MOVQ g(BX), R10 diff --git a/src/pkg/runtime/sys_freebsd_386.s b/src/pkg/runtime/sys_freebsd_386.s index a72d8972b15..2cfce09f44e 100644 --- a/src/pkg/runtime/sys_freebsd_386.s +++ b/src/pkg/runtime/sys_freebsd_386.s @@ -162,8 +162,11 @@ TEXT runtime·sigtramp(SB),7,$44 // check that m exists MOVL m(CX), BX CMPL BX, $0 - JNE 2(PC) + JNE 5(PC) + MOVL signo+0(FP), BX + MOVL BX, 0(SP) CALL runtime·badsignal(SB) + RET // save g MOVL g(CX), DI diff --git a/src/pkg/runtime/sys_freebsd_amd64.s b/src/pkg/runtime/sys_freebsd_amd64.s index 36e034a8022..3d25db2ce7d 100644 --- a/src/pkg/runtime/sys_freebsd_amd64.s +++ b/src/pkg/runtime/sys_freebsd_amd64.s @@ -138,8 +138,10 @@ TEXT runtime·sigtramp(SB),7,$64 // check that m exists MOVQ m(BX), BP CMPQ BP, $0 - JNE 2(PC) + JNE 4(PC) + MOVQ DI, 0(SP) CALL runtime·badsignal(SB) + RET // save g MOVQ g(BX), R10 diff --git a/src/pkg/runtime/sys_linux_386.s b/src/pkg/runtime/sys_linux_386.s index d9f979f509b..28ae37b8d97 100644 --- a/src/pkg/runtime/sys_linux_386.s +++ b/src/pkg/runtime/sys_linux_386.s @@ -170,8 +170,11 @@ TEXT runtime·sigtramp(SB),7,$44 // check that m exists MOVL m(CX), BX CMPL BX, $0 - JNE 2(PC) + JNE 5(PC) + MOVL sig+0(FP), BX + MOVL BX, 0(SP) CALL runtime·badsignal(SB) + RET // save g MOVL g(CX), DI diff --git a/src/pkg/runtime/sys_linux_amd64.s b/src/pkg/runtime/sys_linux_amd64.s index e0ca6583c65..88810ff74a0 100644 --- a/src/pkg/runtime/sys_linux_amd64.s +++ b/src/pkg/runtime/sys_linux_amd64.s @@ -157,8 +157,10 @@ TEXT runtime·sigtramp(SB),7,$64 // check that m exists MOVQ m(BX), BP CMPQ BP, $0 - JNE 2(PC) + JNE 4(PC) + MOVQ DI, 0(SP) CALL runtime·badsignal(SB) + RET // save g MOVQ g(BX), R10 diff --git a/src/pkg/runtime/sys_linux_arm.s b/src/pkg/runtime/sys_linux_arm.s index 0112cf91585..38bcebfa1ab 100644 --- a/src/pkg/runtime/sys_linux_arm.s +++ b/src/pkg/runtime/sys_linux_arm.s @@ -296,7 +296,8 @@ TEXT runtime·sigaltstack(SB),7,$0 TEXT runtime·sigtramp(SB),7,$24 // this might be called in external code context, // where g and m are not set. - // first save R0, becuase cgo_load_gm will clobber it + // first save R0, because cgo_load_gm will clobber it + // TODO(adonovan): call runtime·badsignal if m=0, like other platforms? MOVW R0, 4(R13) MOVW cgo_load_gm(SB), R0 CMP $0, R0 diff --git a/src/pkg/runtime/sys_netbsd_386.s b/src/pkg/runtime/sys_netbsd_386.s index 75a38f820ed..5f6738ee2d3 100644 --- a/src/pkg/runtime/sys_netbsd_386.s +++ b/src/pkg/runtime/sys_netbsd_386.s @@ -178,8 +178,11 @@ TEXT runtime·sigtramp(SB),7,$44 // check that m exists MOVL m(CX), BX CMPL BX, $0 - JNE 2(PC) + JNE 5(PC) + MOVL signo+0(FP), BX + MOVL BX, 0(SP) CALL runtime·badsignal(SB) + RET // save g MOVL g(CX), DI diff --git a/src/pkg/runtime/sys_netbsd_amd64.s b/src/pkg/runtime/sys_netbsd_amd64.s index f5feb48418f..9fe1ebbc492 100644 --- a/src/pkg/runtime/sys_netbsd_amd64.s +++ b/src/pkg/runtime/sys_netbsd_amd64.s @@ -196,8 +196,10 @@ TEXT runtime·sigtramp(SB),7,$64 // check that m exists MOVQ m(BX), BP CMPQ BP, $0 - JNE 2(PC) + JNE 4(PC) + MOVQ DI, 0(SP) CALL runtime·badsignal(SB) + RET // save g MOVQ g(BX), R10 diff --git a/src/pkg/runtime/sys_openbsd_386.s b/src/pkg/runtime/sys_openbsd_386.s index 0774162f64b..d04b5e653a2 100644 --- a/src/pkg/runtime/sys_openbsd_386.s +++ b/src/pkg/runtime/sys_openbsd_386.s @@ -151,8 +151,11 @@ TEXT runtime·sigtramp(SB),7,$44 // check that m exists MOVL m(CX), BX CMPL BX, $0 - JNE 2(PC) + JNE 5(PC) + MOVL signo+0(FP), BX + MOVL BX, 0(SP) CALL runtime·badsignal(SB) + RET // save g MOVL g(CX), DI diff --git a/src/pkg/runtime/sys_openbsd_amd64.s b/src/pkg/runtime/sys_openbsd_amd64.s index 9df903f74f9..ad7de11f841 100644 --- a/src/pkg/runtime/sys_openbsd_amd64.s +++ b/src/pkg/runtime/sys_openbsd_amd64.s @@ -187,8 +187,10 @@ TEXT runtime·sigtramp(SB),7,$64 // check that m exists MOVQ m(BX), BP CMPQ BP, $0 - JNE 2(PC) + JNE 4(PC) + MOVQ DI, 0(SP) CALL runtime·badsignal(SB) + RET // save g MOVQ g(BX), R10 diff --git a/src/pkg/runtime/thread_darwin.c b/src/pkg/runtime/thread_darwin.c index bfdd9873ea8..aff2b6fd371 100644 --- a/src/pkg/runtime/thread_darwin.c +++ b/src/pkg/runtime/thread_darwin.c @@ -502,7 +502,11 @@ static int8 badsignal[] = "runtime: signal received on thread not created by Go. // This runs on a foreign stack, without an m or a g. No stack split. #pragma textflag 7 void -runtime·badsignal(void) +runtime·badsignal(int32 sig) { + if (sig == SIGPROF) { + return; // Ignore SIGPROFs intended for a non-Go thread. + } runtime·write(2, badsignal, sizeof badsignal - 1); + runtime·exit(1); } diff --git a/src/pkg/runtime/thread_freebsd.c b/src/pkg/runtime/thread_freebsd.c index 1597b1e88b1..4d39f3c8042 100644 --- a/src/pkg/runtime/thread_freebsd.c +++ b/src/pkg/runtime/thread_freebsd.c @@ -211,7 +211,11 @@ static int8 badsignal[] = "runtime: signal received on thread not created by Go. // This runs on a foreign stack, without an m or a g. No stack split. #pragma textflag 7 void -runtime·badsignal(void) +runtime·badsignal(int32 sig) { + if (sig == SIGPROF) { + return; // Ignore SIGPROFs intended for a non-Go thread. + } runtime·write(2, badsignal, sizeof badsignal - 1); + runtime·exit(1); } diff --git a/src/pkg/runtime/thread_linux.c b/src/pkg/runtime/thread_linux.c index f66d2dd4d25..c428ba1b392 100644 --- a/src/pkg/runtime/thread_linux.c +++ b/src/pkg/runtime/thread_linux.c @@ -261,7 +261,11 @@ static int8 badsignal[] = "runtime: signal received on thread not created by Go. // This runs on a foreign stack, without an m or a g. No stack split. #pragma textflag 7 void -runtime·badsignal(void) +runtime·badsignal(int32 sig) { + if (sig == SIGPROF) { + return; // Ignore SIGPROFs intended for a non-Go thread. + } runtime·write(2, badsignal, sizeof badsignal - 1); + runtime·exit(1); } diff --git a/src/pkg/runtime/thread_netbsd.c b/src/pkg/runtime/thread_netbsd.c index be6c205c281..a703e0714ad 100644 --- a/src/pkg/runtime/thread_netbsd.c +++ b/src/pkg/runtime/thread_netbsd.c @@ -261,7 +261,11 @@ static int8 badsignal[] = "runtime: signal received on thread not created by Go. // This runs on a foreign stack, without an m or a g. No stack split. #pragma textflag 7 void -runtime·badsignal(void) +runtime·badsignal(int32 sig) { + if (sig == SIGPROF) { + return; // Ignore SIGPROFs intended for a non-Go thread. + } runtime·write(2, badsignal, sizeof badsignal - 1); + runtime·exit(1); } diff --git a/src/pkg/runtime/thread_openbsd.c b/src/pkg/runtime/thread_openbsd.c index 4e4db747457..c55f25278f0 100644 --- a/src/pkg/runtime/thread_openbsd.c +++ b/src/pkg/runtime/thread_openbsd.c @@ -234,7 +234,11 @@ static int8 badsignal[] = "runtime: signal received on thread not created by Go. // This runs on a foreign stack, without an m or a g. No stack split. #pragma textflag 7 void -runtime·badsignal(void) +runtime·badsignal(int32 sig) { + if (sig == SIGPROF) { + return; // Ignore SIGPROFs intended for a non-Go thread. + } runtime·write(2, badsignal, sizeof badsignal - 1); + runtime.exit(1) } diff --git a/src/pkg/runtime/thread_plan9.c b/src/pkg/runtime/thread_plan9.c index 57d535713d2..9898a65b288 100644 --- a/src/pkg/runtime/thread_plan9.c +++ b/src/pkg/runtime/thread_plan9.c @@ -354,7 +354,7 @@ static int8 badsignal[] = "runtime: signal received on thread not created by Go. // This runs on a foreign stack, without an m or a g. No stack split. #pragma textflag 7 void -runtime·badsignal(void) +runtime·badsignal(int32 sig) { runtime·pwrite(2, badsignal, sizeof badsignal - 1, -1LL); }