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: sigfwd calls C handlers with improper alignment #17641

Closed
bcmills opened this issue Oct 27, 2016 · 3 comments

Comments

Projects
None yet
3 participants
@bcmills
Copy link
Member

commented Oct 27, 2016

go version go1.7.3 linux/amd64

What did you do?

In misc/cgo/testcarchive/main2.c, add a function call within the signal handler which generates an instruction that requires proper alignment. Calling a varargs function with a floating-point parameter seems to suffice:

bcmills:~/src/go.googlesource.com/go/misc/cgo/testcarchive$ git diff main2.c
diff --git a/misc/cgo/testcarchive/main2.c b/misc/cgo/testcarchive/main2.c
index 3726977..c814047 100644
--- a/misc/cgo/testcarchive/main2.c
+++ b/misc/cgo/testcarchive/main2.c
@@ -7,8 +7,10 @@

 #include <setjmp.h>
 #include <signal.h>
+#include <stdarg.h>
 #include <stddef.h>
 #include <stdio.h>
+#include <stdint.h>
 #include <stdlib.h>
 #include <string.h>
 #include <sys/types.h>
@@ -46,11 +48,19 @@ static void ioHandler(int signo, siginfo_t* info, void* ctxt) {
 static jmp_buf jmp;
 static char* nullPointer;

+static void callWithVarargs(void* dummy, ...) {
+       va_list args;
+       va_start(args, dummy);
+       va_end(args);
+}
+
 // Signal handler for SIGSEGV on a C thread.
 static void segvHandler(int signo, siginfo_t* info, void* ctxt) {
        sigset_t mask;
        int i;

+       callWithVarargs("dummy arg", 3.1415);
+
        if (sigemptyset(&mask) < 0) {
                die("sigemptyset");
        }

Enable the race-detector in the corresponding test:

bcmills:~/src/go.googlesource.com/go/misc/cgo/testcarchive$ git diff carchive_test.go
diff --git a/misc/cgo/testcarchive/carchive_test.go b/misc/cgo/testcarchive/carchive_test.go
index 4999929..30334cf 100644
--- a/misc/cgo/testcarchive/carchive_test.go
+++ b/misc/cgo/testcarchive/carchive_test.go
@@ -198,7 +198,7 @@ func TestEarlySignalHandler(t *testing.T) {
                os.RemoveAll("pkg")
        }()

-       cmd := exec.Command("go", "build", "-buildmode=c-archive", "-o", "libgo2.a", "libgo2")
+       cmd := exec.Command("go", "build", "-race", "-buildmode=c-archive", "-o", "libgo2.a", "libgo2")
        cmd.Env = gopathEnv
        if out, err := cmd.CombinedOutput(); err != nil {
                t.Logf("%s", out)

Then, run the test:

bcmills:~/src/go.googlesource.com/go/misc/cgo/testcarchive$ go test -v -test.run=TestEarlySignalHandler ./carchive_test.go
=== RUN   TestEarlySignalHandler
--- FAIL: TestEarlySignalHandler (6.46s)
        carchive_test.go:215:
        carchive_test.go:216: signal: segmentation fault (core dumped)
FAIL
exit status 1
FAIL    command-line-arguments  7.259s

What did you expect to see?

The test should pass.

What did you see instead?

The test fails due to a second SIGSEGV within the first SIGSEGV handler call. The source of the second SIGSEGV is a movaps instruction which requires the stack to be properly 16-byte aligned.

@bcmills

This comment has been minimized.

Copy link
Member Author

commented Oct 28, 2016

As it turns out, at tip it doesn't even require the -race edit: the change to main2.c suffices.

@gopherbot

This comment has been minimized.

Copy link

commented Oct 28, 2016

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

@bcmills bcmills changed the title runtime: signal handling: sigfwd calls C handlers with improper alignment under -race runtime: signal handling: sigfwd calls C handlers with improper alignment Oct 28, 2016

@quentinmit quentinmit added the NeedsFix label Nov 1, 2016

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

@gopherbot gopherbot closed this in 8380de4 Nov 1, 2016

@bcmills

This comment has been minimized.

Copy link
Member Author

commented Nov 1, 2016

There is a related issue that sigtramp and/or cgoSigtramp need to save callee-save registers in case they are themselves forwarded calls from some other signal handler, but I'll file that as a separate 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.