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
Closed
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills 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
Copy link
Contributor Author

bcmills commented Nov 2, 2016

@ianlancetaylor @dvyukov

@quentinmit quentinmit added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 4, 2016
@quentinmit quentinmit added this to the Go1.8Maybe milestone Nov 4, 2016
@bcmills
Copy link
Contributor Author

bcmills 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
Copy link
Member

dvyukov commented Nov 9, 2016

Sounds good to me.

@ianlancetaylor
Copy link
Contributor

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
Copy link
Member

minux commented Nov 10, 2016 via email

@bcmills
Copy link
Contributor Author

bcmills 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
Copy link
Contributor

rsc 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
Copy link
Contributor Author

bcmills 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
Copy link

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

@rsc rsc added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Nov 14, 2016
@golang golang locked and limited conversation to collaborators Nov 16, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants