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: crash with C sigaltstacks #22930

Closed
aclements opened this issue Nov 29, 2017 · 10 comments
Closed

runtime: crash with C sigaltstacks #22930

aclements opened this issue Nov 29, 2017 · 10 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge release-blocker
Milestone

Comments

@aclements
Copy link
Member

What version of Go are you using (go version)?

go version devel +0c14345c96 Wed Nov 29 16:54:25 2017 +0000 linux/amd64

Does this issue reproduce with the latest release?

No. This started with commit 44d9e96.

What operating system and processor architecture are you using (go env)?

linux/amd64

What did you do?

https://play.golang.org/p/JkOKEJ9kQk (not runnable on the playground)

What did you expect to see?

Successful exit with no output.

What did you see instead?

Segfault.

The problem is that the sigaltstack syscall is failing (which we handle unceremoniously with a segfault) because we're passing stack bounds (0, 0). The happens because I incorrectly made minit/unminit assume they would each only be called once per M. This is true for regular Go threads, but not true for calls from C threads into Go, or signals delivered on C threads.

Here's a specific way this can happen:

  1. oneNewExtraM creates an M with a Go-allocated gsignal stack and put it on the extram list.
  2. A C-created thread with a C-created alternate signal stack calls a Go function. (Or a signal handled by the runtime arrives on such a thread.)
    1. cgocallback_gofunc (or badsignal) calls needm. needm gets the extra M created above off the list and calls minit -> minitSignals -> minitSignalStack.
    2. minitSignalStack sees that a signal stack is already set for this thread and sets m.newSigstack = false.
    3. The Go signal handler returns, which calls dropm -> unminit -> unminitSignals.
    4. unminitSignals sees that m.newSigstack is false, so it sets m.gsignal.stack = stack{}
    5. dropm returns the M to the extram list.
  3. A C-created thread without an alternate signal stack calls a Go function (or gets a signal)
    1. needm gets the same M from the extra M list. But this time minitSignalStack sees that there is no signal stack for the current thread, so it calls signalstack(&_g_.m.gsignal.stack).
    2. Since we cleared m.gsignal.stack in step 2.4 above, the syscall fails and we crash.
@aclements aclements added this to the Go1.10 milestone Nov 29, 2017
@aclements aclements self-assigned this Nov 29, 2017
@aclements
Copy link
Member Author

I think this same problem has existed in an even worse form for many releases. Prior to 44d9e96, we didn't clear m.gsignal.stack, so it didn't crash when it tried to use that gsignal stack. Instead, step 3(i) would reuse the signal stack from the C thread in step 2, meaning that both C-created threads could potentially be running with the same signal stack.

It seems like we must track the Go-allocated gsignal stack and the "active" signal stack separately so that when we use an extra M we can re-instate the Go-allocated gsignal stack when we dropm.

/cc @ianlancetaylor

@dsnet dsnet changed the title runtime: crash with C sigaltstacks runtime: crash with C signaltstacks Nov 29, 2017
@dsnet dsnet changed the title runtime: crash with C signaltstacks runtime: crash with C sigaltstacks Nov 30, 2017
@ianlancetaylor
Copy link
Contributor

Yikes, I think you're right. When the signal handler changes signal stacks it calls restoreGsignalStack, but nothing does that when needm calls minit.

@gopherbot
Copy link

Change https://golang.org/cl/81476 mentions this issue: runtime: restore the Go-allocated signal stack in unminit

@aclements aclements modified the milestones: Go1.10, Go1.9.3 Dec 1, 2017
@aclements
Copy link
Member Author

Re-opening for 1.9. I have a cherry-pick locally that resolves the conflicts if it's helpful.

@gopherbot
Copy link

Change https://golang.org/cl/81615 mentions this issue: runtime: use MAP_ANON in sigstack check

gopherbot pushed a commit that referenced this issue Dec 1, 2017
MAP_ANON is the deprecated but more portable spelling of
MAP_ANONYMOUS. Use MAP_ANON to un-break the Darwin 10.10 builder.

Updates #22930.

Change-Id: Iedd6232b94390b3b2a7423c45cdcb25c1a5b3323
Reviewed-on: https://go-review.googlesource.com/81615
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@ianlancetaylor
Copy link
Contributor

Really reopening.

@andybons
Copy link
Member

andybons commented Jan 18, 2018

CL 88315 OK for Go 1.9.3.
CL 88316 OK for Go 1.9.3.

@gopherbot
Copy link

Change https://golang.org/cl/88316 mentions this issue: [release-branch.go1.9] runtime: use MAP_ANON in sigstack check

@gopherbot
Copy link

Change https://golang.org/cl/88315 mentions this issue: [release-branch.go1.9] runtime: restore the Go-allocated signal stack in unminit

@andybons andybons added the CherryPickApproved Used during the release process for point releases label Jan 18, 2018
gopherbot pushed a commit that referenced this issue Jan 22, 2018
… in unminit

Currently, when we minit on a thread that already has an alternate
signal stack (e.g., because the M was an extram being used for a cgo
callback, or to handle a signal on a C thread, or because the
platform's libc always allocates a signal stack like on Android), we
simply drop the Go-allocated gsignal stack on the floor.

This is a problem for Ms on the extram list because those Ms may later
be reused for a different thread that may not have its own alternate
signal stack. On tip, this manifests as a crash in sigaltstack because
we clear the gsignal stack bounds in unminit and later try to use
those cleared bounds when we re-minit that M. On 1.9 and earlier, we
didn't clear the bounds, so this manifests as running more than one
signal handler on the same signal stack, which could lead to arbitrary
memory corruption.

This CL fixes this problem by saving the Go-allocated gsignal stack in
a new field in the m struct when overwriting it with a system-provided
signal stack, and then restoring the original gsignal stack in
unminit.

This CL is designed to be easy to back-port to 1.9. It won't quite
cherry-pick cleanly, but it should be sufficient to simply ignore the
change in mexit (which didn't exist in 1.9).

Now that we always have a place to stash the original signal stack in
the m struct, there are some simplifications we can make to the signal
stack handling. We'll do those in a later CL.

Fixes #22930.

Change-Id: I55c5a6dd9d97532f131146afdef0b216e1433054
Reviewed-on: https://go-review.googlesource.com/88315
Run-TryBot: Andrew Bonventre <andybons@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit that referenced this issue Jan 22, 2018
MAP_ANON is the deprecated but more portable spelling of
MAP_ANONYMOUS. Use MAP_ANON to un-break the Darwin 10.10 builder.

Updates #22930.

Change-Id: Iedd6232b94390b3b2a7423c45cdcb25c1a5b3323
Reviewed-on: https://go-review.googlesource.com/88316
Run-TryBot: Andrew Bonventre <andybons@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@andybons
Copy link
Member

go1.9.3 has been packaged and includes:

  • CL 88315 [release-branch.go1.9] runtime: restore the Go-allocated signal stack in unminit
  • CL 88316 [release-branch.go1.9] runtime: use MAP_ANON in sigstack check

The release is posted at golang.org/dl.

— golang.org/x/build/cmd/releasebot, Jan 22 21:02:57 UTC

@golang golang locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge release-blocker
Projects
None yet
Development

No branches or pull requests

4 participants