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

windows: SEH and VEH don't play well together #47576

Open
zx2c4 opened this issue Aug 6, 2021 · 9 comments
Open

windows: SEH and VEH don't play well together #47576

zx2c4 opened this issue Aug 6, 2021 · 9 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Milestone

Comments

@zx2c4
Copy link
Contributor

zx2c4 commented Aug 6, 2021

On Windows, there are two types of exception handling: structured exception handling (SEH) and vectored exception handling (VEH).

MSVC generates binaries with SEH, in which individual functions have exception handlers attached. On 64bit, these are stored in a table in the .pdata section of the PE. When a function throws an exception (say, due to an illegal instruction or a divide by zero), the SEH for it, if any, is called.

Go's runtime uses VEH, in which the entire runtime has a series of handlers that VEH jumps through until it reaches a termination point.

When an exception is thrown, ntdll executes VEH before SEH. This is reasonable behavior, but poses a problem for our use of VEH.

Recent MSVC toolchains generate arm64 DLLs that probe for ARMv8.1 atomics by testing some instructions, and using SEH to trap a potential illegal instruction exception indicating that those instructions aren't there. ARM64 DLLs generated by recent MSVC toolchains do this unconditionally during initialization.

When the Go runtime loads one of these recent DLLs (using LoadLibrary like normal), its VEH handler is called for the illegal instruction exception generated by the probing. The Go VEH handler then decides it's fatal, and quits. That's a big problem.

I see a few solutions to this:

  1. We special case illegal instruction exceptions on arm64 that are thrown from outside Go's .text section, as I've done on https://go-review.googlesource.com/c/go/+/340070 . This works fine (and I'm shipping it now) and seems like the least invasive change. This could however, mask other illegal instruction exceptions that are thrown from outside Go's .text section that aren't subsequently caught and handled by SEH, in which case the binary will terminate without spitting out a stacktrace. That seems like an unlikely and insignificant edge case, however.

  2. We always allow our VEH to fall through to SEH (or to nothing) if the exception comes from outside Go's .text section. This would work, but would also mean that legitimate crashes caused by library functions would not result in a Go stacktrace being printed. I sort of like this solution, but I can imagine why others might not. And those stacktraces are probably pretty useful. This would mean deleting the TestRaiseException unit test we have in runtime/.

  3. We allow VEH to fall through to SEH if the exception comes from outside Go's .text section and there does exist an SEH for that function. This would be a more general case of (1), and perhaps a very good solution. I can probably do the reverse engineering necessary to implement this, though it's not easy. However, if an SEH then decided not to continue execution but to crash instead, we wouldn't print a stacktrace. It remains to be seen where that edge case would be hit.

  4. We make our VEH call an SEH directly if it exists, and only crash with a stacktrace if the SEH called doesn't handle it. This is very tricky to do, but would probably give us the best of all worlds.

  5. We do (2), but then install a SEH (using RtlAddFunctionTable) for the IP of syscall.Syscall that prints a stacktrace, so that we do ultimately get the unhandled exceptions that VEH passes on due to (2). This seems simple-ish, though I suspect we would miss stacktraces caused by threads created by non-Go code.

Before I go further in thinking about this, I thought we ought to discuss all this first. https://go-review.googlesource.com/c/go/+/340070 works as a stopgap solution for now, anyhow.

CC @rsc @ianlancetaylor @aclements @cherrymui @bradfitz @alexbrainman @jstarks

@gopherbot
Copy link

Change https://golang.org/cl/340070 mentions this issue: runtime: allow arm64 SEH to be called if illegal instruction

@zx2c4
Copy link
Contributor Author

zx2c4 commented Aug 6, 2021

@jstarks If you've got opinions from a Windows perspective on what behavior seems most compliant, that'd carry a lot of weight. Our current maze of handlers is in https://github.com/golang/go/blob/master/src/runtime/signal_windows.go .

gopherbot pushed a commit that referenced this issue Aug 6, 2021
DLLs built with recent Microsoft toolchains for ARM64 test for ARMv8.1
atomics by potentially calling an illegal instruction, and then trapping
the exception to disable use of them by way of a structured exception
handler. However, vectored exception handlers are always called before
structured exception handlers. When LoadLibrary-ing DLLs that do this
probing during initialization, our lastcontinuehandler winds up being
called, and then crashing, but actually it should give execution back to
the library to handle the exception and fix up the state. So special
case this for arm64 with illegal instructions, and hope that we're not
masking other things in external DLLs that might more fatally trigger an
illegal instruction exception.

Updates #47576.

Change-Id: I341ab99cd8d513ae999b75596749d49779072022
Reviewed-on: https://go-review.googlesource.com/c/go/+/340070
Trust: Jason A. Donenfeld <Jason@zx2c4.com>
Run-TryBot: Jason A. Donenfeld <Jason@zx2c4.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>
@jstarks
Copy link

jstarks commented Aug 6, 2021

I'm not sure yet. But one thing I want to point out is that Go's vectored exception handler (exceptionhandler) is already falling through to SEH. The vectored continue handlers are run unconditionally after VEH and SEH handling until one of them returns EXCEPTION_CONTINUE_EXECUTION.

I think ideally your VCH would only panic if none of the VEH or SEH handlers returned EXCEPTION_CONTINUE_EXECUTION. But I don't yet know if that information is available to you.

It seems like Go really just wants to be called for unhandled exceptions, but the unhandled exception filter isn't run for debugged processes, which is problematic. Is that right?

@zx2c4
Copy link
Contributor Author

zx2c4 commented Aug 6, 2021

Oh, that's interesting. I didn't consider that the continued handlers were running after SEH. The reason is that that last continue handler appears to being called before the arm64 DLL's SEH, despite being "last". That's odd...

@jstarks
Copy link

jstarks commented Aug 6, 2021

Hmm. I don't immediately see how that's possible.

@zx2c4
Copy link
Contributor Author

zx2c4 commented Aug 6, 2021

Bug? Try reverting 70546f6 and LoadLibrary'ing a DLL produced by the latest EWDK 22000 (here's one). You'll see that it dies when probing. Then add that commit back, and you'll see that it hits the interlocked probe exception handler. At least I think that's what's going on?

@jstarks
Copy link

jstarks commented Aug 6, 2021

I'll try to debug this in the next few days.

@seankhliao seankhliao added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows labels Aug 7, 2021
@seankhliao seankhliao added this to the Unplanned milestone Aug 20, 2022
@gopherbot
Copy link

Change https://go.dev/cl/457875 mentions this issue: runtime,cmd/link: allow SEH tramps handle non-Go exception

@qmuntal
Copy link
Contributor

qmuntal commented Dec 15, 2022

I've implemented option 5 in CL 457875. Still WIP but looks promising. Edit: missed exception on non-go threads might render this option unusable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Projects
None yet
Development

No branches or pull requests

5 participants