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

Jump to bottom

runtime: signal handling: sigfwd calls C handlers with improper alignment #17641

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

runtime: signal handling: sigfwd calls C handlers with improper alignment #17641

bcmills opened this issue Oct 27, 2016 · 3 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Go1.8

Comments

@bcmills
Copy link
Contributor

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

bcmills commented Oct 28, 2016 •
edited

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

@gopherbot
Copy link

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 The path to resolution is known, but the work has not been done. label Nov 1, 2016
@quentinmit quentinmit added this to the Go1.8 milestone Nov 1, 2016
@bcmills
Copy link
Contributor Author

bcmills 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.

This was referenced Dec 15, 2016
Closed
Open
@golang golang locked and limited conversation to collaborators Nov 1, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
3 participants
@quentinmit @bcmills @gopherbot