Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runtime: signal handling: runtime bypasses TSAN/MSAN sigaction interceptors #17753

Closed
bcmills opened this issue Nov 2, 2016 · 9 comments

Comments

Projects
None yet
7 participants
@bcmills
Copy link
Member

commented Nov 2, 2016

go version devel +09bb643 Wed Nov 2 20:49:58 2016 +0000 linux/amd64

What did you do?

$ cat tsan8.go
package main

/*
#cgo CFLAGS: -fsanitize=thread
#cgo LDFLAGS: -fsanitize=thread

#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

struct sigaction prev_sa;

void forwardSignal(int signo, siginfo_t *info, void *context) {
	// One of sa_sigaction and/or sa_handler
	if ((prev_sa.sa_flags&SA_SIGINFO) != 0) {
		prev_sa.sa_sigaction(signo, info, context);
		return;
	}
	if (prev_sa.sa_handler != SIG_IGN && prev_sa.sa_handler != SIG_DFL) {
		prev_sa.sa_handler(signo);
		return;
	}

	fprintf(stderr, "No Go handler to forward to!\n");
	abort();
}

void registerSegvFowarder() {
	struct sigaction sa;
	memset(&sa, 0, sizeof(sa));
	sigemptyset(&sa.sa_mask);
	sa.sa_flags = SA_SIGINFO | SA_ONSTACK;
	sa.sa_sigaction = forwardSignal;

	if (sigaction(SIGSEGV, &sa, &prev_sa) != 0) {
		perror("failed to register SEGV forwarder");
		exit(EXIT_FAILURE);
	}
}
*/
import "C"

func main() {
	C.registerSegvFowarder()

	defer func() {
		recover()
	}()
	var nilp *int
	*nilp = 42
}
$ CC=/usr/lib/llvm-3.8/bin/clang go run tsan8.go

What did you expect to see?

The test should pass (as it does without the -fsanitize=thread flags).

What did you see instead?

$ CC=/usr/lib/llvm-3.8/bin/clang go run tsan8.go
No Go handler to forward to!
SIGABRT: abort
PC=0x7f4b364cfc37 m=0

goroutine 1 [running]:

goroutine 17 [syscall, locked to thread]:
runtime.goexit()
        /usr/lib/google-golang/src/runtime/asm_amd64.s:2086 +0x1

rax    0x0
rbx    0x0
rcx    0xffffffffffffffff
rdx    0x6
rdi    0x7436
rsi    0x7436
rbp    0xb
rsp    0xc420009948
r8     0x7f4b375a1440
r9     0x524e23
r10    0x8
r11    0x206
r12    0xc
r13    0x7f4b375387c0
r14    0xc420009bc0
r15    0xc420009cf0
rip    0x7f4b364cfc37
rflags 0x206
cs     0x33
fs     0x0
gs     0x0
exit status 2

It appears that TSAN and MSAN replace the libc sigaction function with their own versions, and when sigaction is called they return the remembered struct sigaction previously passed to the libc function rather than calling all the way down to the OS.

One possible fix would be to make TSAN and MSAN make a real sigaction syscall to verify the actual handler before returning the cached one.

Another would be to make the Go runtime use the libc sigaction function (instead of making the system call directly from Go) when the sanitizers are in use (e.g. as we do today for mmap via x_cgo_mmap).

@bcmills

This comment has been minimized.

Copy link
Member Author

commented Nov 2, 2016

@quentinmit quentinmit added this to the Go1.8Maybe milestone Nov 4, 2016

@bcmills

This comment has been minimized.

Copy link
Member Author

commented Nov 9, 2016

Thinking about this some more, I think the right fix is to do something along the lines of x_cgo_mmap so that the Go runtime calls through the *SAN libc interceptors when they're present.

Here's my reasoning: I expect that a lot of Go users have more control over which Go version they're using than over their C compiler, and it's easier to build a fresh-from-source Go toolchain than a fresh-from-source C toolchain if they're in an environment that tends to significantly lag upstream releases. Hooking the runtime potentially fixes it across all C toolchains in a way that Go users can control more easily.

@dvyukov

This comment has been minimized.

Copy link
Member

commented Nov 9, 2016

Sounds good to me.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2016

I agree that routing through the C library sigaction is probably correct for cgo programs.

Note: on GNU/Linux the sigaction library function fails with EINVAL for any attempt to do anything with signals 32 and 33. I think we mostly ignore the return value of sigaction anyhow so that may not matter.

@minux

This comment has been minimized.

Copy link
Member

commented Nov 10, 2016

@bcmills

This comment has been minimized.

Copy link
Member Author

commented Nov 10, 2016

@minux One point to draw the line would be between "bug" and "feature". The sigaction libc call is needed to avoid crashes in otherwise-correct programs compiled with -fsanitize=thread, whereas the socket functions only enable a feature (SOCKS proxying) that can be obtained in other ways.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Nov 11, 2016

Is someone going to work on this for Go 1.8?
If not let's move it to the Go 1.9 milestone.
Thanks.

@bcmills

This comment has been minimized.

Copy link
Member Author

commented Nov 11, 2016

I've got a change pending, but I've hit a couple snags during testing. (I may need some help sorting out a bad interaction between systemstack, //go:nosplit, and anonymous functions.)

@gopherbot

This comment has been minimized.

Copy link

commented Nov 11, 2016

CL https://golang.org/cl/33142 mentions this issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.