runtime: use of AddVectoredExceptionHandler on windows is ill-advised

[billziss-gh]

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

> go version
go version go1.19 windows/arm64

Does this issue reproduce with the latest release?

Yes, with the latest major version (1.19).

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

go env Output

> go env
set GO111MODULE=
set GOARCH=arm64
set GOBIN=
set GOCACHE=C:\Users\billziss\AppData\Local\go-build
set GOENV=C:\Users\billziss\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=arm64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\billziss\go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=C:\Users\billziss\go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_arm64
set GOVCS=
set GOVERSION=go1.19
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=NUL
set GOWORK=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-mthreads -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=C:\Users\billziss\AppData\Local\Temp\go-build481145130=/tmp/go-build -gno-record-gcc-switches

What did you do?

I ran a Golang program that uses a DLL that raises a benign Windows exception in a non-Golang thread. This results in a call to runtime.badsignal2.

What did you expect to see?

I expected the program to run without any problems as the Windows API can handle the benign exception internally.

What did you see instead?

On windows/arm64 I saw a crash because of related problem #56080.

Problem and solution

The initExceptionHandler uses AddVectoredExceptionHandler to add a Vectored Exception Handler. This use looks ill-advised. Windows components (and some third party applications and DLLs) regularly use Structured Exception Handling to report benign errrors, such as "Access Denied". Vectored Exception Handlers run prior to the regular Structured Exception Handlers in Windows. This means that the Golang runtime preempts the regular processing of benign errors and mistakenly thinks that an unrecoverable error has happened.

A likely fix is not to call AddVectoredExceptionHandler.

See also

#56080
rclone/rclone#5828 (comment)

[bcmills]

(CC @golang/windows)

[cagedmantis]

cc @golang/runtime

[alexbrainman]

This use looks ill-advised. Windows components (and some third party applications and DLLs) regularly use Structured Exception Handling to report benign errrors, such as "Access Denied". Vectored Exception Handlers run prior to the regular Structured Exception Handlers in Windows

SEH exceptions from non-Go DLLs should be ignored by firstcontinuehandler. The firstcontinuehandler calls isgoexception function to determine if exception is raised by Go, and if not, then passes exception handling back to non-Go DLL by returning EXCEPTION_CONTINUE_SEARCH.

I suspect the problem that you encountered is that you are having exception on non-Go thread. I don't believe that is supported by Go runtime.

If my memory does not fail me Go introduced use of 2 handlers with AddVectoredExceptionHandler to solve problems that you just described - see #8006 for details.

I am pretty sure we tried to implement SEH handlers on amd64 but failed. Windows amd64 binaries that support SEH handling require complete stack information present in the exe. We could not find enough details to implement that.

Alex

[billziss-gh]

The problem with vectored exception handlers is that they see every exception regardless of whether the exception is one that was going to be handled or not. You are correct that in the original issue that prompted this one, the problem likely happens because we have a Windows exception in a non-Go thread.

However even if this was a Windows exception in a Go thread it is not clear to me why the Go runtime should see the exception. A lot of Windows API's use exceptions to implement complicated control flow and injecting Go code execution in that flow can cause problems.

I propose instead an alternative policy for unhandled exception handling that ensures that no Go code runs unless the exception is genuinely not going to be handled. (Please note that I have a limited understanding of the Go runtime and my proposal may in fact be misguided.)

The following C code illustrates:

static VOID UnhandledException(PEXCEPTION_POINTERS ExceptionInfo)
{
    // ... additional code goes here
    ExitProcess(ExceptionInfo->ExceptionRecord->ExceptionCode);
}

static LONG WINAPI UnhandledHandler(PEXCEPTION_POINTERS ExceptionInfo)
{
    UnhandledException(ExceptionInfo);
    return EXCEPTION_CONTINUE_EXECUTION;
}

static LONG NTAPI ContinueHandler(PEXCEPTION_POINTERS ExceptionInfo)
{
    UnhandledException(ExceptionInfo);
    return EXCEPTION_EXECUTE_HANDLER;
}

VOID InitializeExceptionHandling(VOID)
{
    SetUnhandledExceptionFilter(UnhandledHandler);
    AddVectoredContinueHandler(0, ContinueHandler);
}

This works by setting an "unhandled exception filter" and a "vectored continue handler". There are two cases to consider:

• Not running under a debugger. In this case UnhandledHandler will be called and we can do our unhandled processing here and terminate the process.

• Running under a debugger. In this case UnhandledHandler will not be called. However ContinueHandler will be called and we can do our unhandled processing here and terminate the process.

Notice that we have eliminated the need for AddVectoredExceptionHandler but we can still process unhandled exceptions in all cases, whether being debugged or not. Importantly we never execute any code in UnhandledException unless the exception is not going to be handled. This technique handles all kinds of exceptions, including Access Violations, RaiseException and C++ throw.

I close with my understanding of the Windows exception dispatching mechanism. It proceeds in three phases:

• Vectored exception handlers.
  • If any of these handlers returns EXCEPTION_CONTINUE_EXECUTION subsequent vectored exception handlers and structured exception handlers are not invoked; however vectored continue handlers are invoked.
• Structured exceptions handlers.
  • If any of these handlers returns EXCEPTION_CONTINUE_EXECUTION subsequent structured exception handlers are not invoked; however vectored continue handlers are invoked.
  • The system wraps every thread with a catch-all exception handler called UnhandledExceptionFilter. This catch-all handler is only used if no other handler handles the exception. This handler is also responsible for calling the handler set by the SetUnhandledExceptionFilter API, unless a debugger is active in which case it returns EXCEPTION_CONTINUE_EXECUTION (and vectored continue handlers will be invoked).
• Vectored continue handlers.
  • These handlers are only invoked if execution is being continued.

---

EDIT: I attach here a small C++ program that demonstrates that this technique works (at least on my Win11 laptop): veh.zip

[alexbrainman]

I propose instead an alternative policy for unhandled exception handling that ensures that no Go code runs unless the exception is genuinely not going to be handled.

Thank you @billziss-gh for your detailed explanation.

Unfortunately I don't have time and knowledge to even consider your proposal. So leaving it to others.

CC @qmuntal in case you are interested

@qmuntal perhaps you also might be interested to be part of @golang/windows group.

Alex

[qmuntal]

Thanks for the detailed context @billziss-gh, this thread contains good info, I'll need time to process it.

Setting aside the vectored vs structured exception handling discussion, I'm still not convinced that the current approach precludes other DLLs to handle exceptions raised in non-Go threads. runtime.badsignal2 does abort the process without escape hatch, but, if I'm not wrong, windows/amd64 does not call runtime.badsignal2 when there is an exception in a non-Go thread. There might be a bug in windows/arm64 which causes runtime.badsignal2 to be called when it shouldn't.

@billziss-gh could you provide a minimal program that demonstrates the issue, so I want better understand what you need? Something like this: https://github.com/golang/go/tree/master/src/runtime/testdata/testwinlib.

@qmuntal perhaps you also might be interested to be part of https://github.com/orgs/golang/teams/windows group.

@alexbrainman please add me to that group.

[billziss-gh]

@qmuntal

I have created throw-away repo golang-veh which allows for some experimentation with the following scenarios:

• RaiseExcept: Raise and catch an exception in a Go thread.
• RaiseNoExcept: Raise but do not catch an exception in a Go thread.
• ThreadRaiseExcept: Raise and catch an exception in a non-Go thread.
• ThreadRaiseNoExcept: Raise but do not catch an exception in a non-Go thread.

I then tried these scenarios with and without the debugger (WinDbg Preview) on x64 and arm64.

Results for Go threads were good (with or without the debugger):

• RaiseExcept: Raising and catching an exception in a Go thread works as expected. (Although I confirmed under WinDbg that sigtramp is called, because of Golang's use of vectored exception handlers.)

• RaiseNoExcept: Raising and not catching the exception in a Go thread also works as expected, in that the Golang runtime catches the exception and prints a reasonable stack trace.

Results for non-Go threads were not good:

• ThreadRaiseExcept:

  • x64: Raising and catching an exception in a non-Go thread works as expected in x64 (with or without the debugger). (Again I confirmed that sigtramp is called, because of Golang's use of vectored exception handlers.)
  • ARM64: Raising and catching an exception in a non-Go thread does not work on ARM64.
    • Without the debugger: The program crashed in badsignal2 (runtime: badsignal2 crash on windows/arm64 #56080)
    • With the debugger: The program should have crashed in badsignal2, but by chance the R3 register is initialized to 0, which allows the program to print runtime: signal received on thread not created by Go endlessly. (The endless printing is because the exception handling mechanism is reentered because of how runtime.abort tries to abort the process by causing another exception.)

• ThreadRaiseNoExcept:

  • x64: Behavior differs with and without the debugger:
    • Without the debugger: Program simply dies without any diagnostic, no error dialogs or WER, etc.
    • With the debugger: Program returns from ThreadRaiseNoExcept. It should not since we raised an exception that is not being caught. (This is likely because the debugger is able to continue exceptions.)
  • ARM64: This never executes under ARM64, because we have already bailed when running the previous test ThreadRaiseExcept.

Overall it looks to me that the exception handling code for Windows needs some TLC.

---

EDIT: My tests were done with:

• x64: stock Go 1.18.1
• ARM64: stock Go 1.19

[gopherbot]

Change https://go.dev/cl/442896 mentions this issue: runtime: ignore exceptions from non-Go threads on windows arm/arm64

[qmuntal]

Thanks for the sample repo. It helped me understand the problem and (hopefully) come up with a solution for arm64, see CL 442896.

I still don't know what you would expect from ThreadRaiseNoExcept. AMD64, and soon ARM64, ignores the exception and dies with exit status 42 (being 42 the error code in RaiseException.

How could we do a better job? We could print stack traces, but IMO they would be meaningless as the exception would have happened in a non-Go thread which we have little information. Handling the exception at the end of the chain with SetUnhandledExceptionFilter wouldn't make it much better. At most we could print (if possible) something like: unhandled exception 42.

[billziss-gh]

@qmuntal I appreciate your work on this and it looks like it fixes the important problem with ThreadRaiseExcept on ARM64.

I still don't know what you would expect from ThreadRaiseNoExcept... How could we do a better job?

My primary point when I opened this issue was that the Go runtime should not inject code in the control flow of non-Go code (e.g. a Windows API) that experiences an exception that is going to be handled. This is not just pedantic: the issue with ThreadRaiseExcept would not have happened if the Go runtime had a different exception policy.

There were some additional issues that came up during subsequent experimentation. For example, ThreadRaiseNoExcept has inconsistent behavior with and without the debugger.

We could print stack traces, but IMO they would be meaningless as the exception would have happened in a non-Go thread which we have little information.

I agree, but this is also why I ask the question: why does the Go runtime catch these exceptions?

---

FYI, I did experiment with a patch along the lines of:

diff --git a/src/runtime/signal_windows.go b/src/runtime/signal_windows.go
index 0f1929e09a..df536688f1 100644
--- a/src/runtime/signal_windows.go
+++ b/src/runtime/signal_windows.go
@@ -28,14 +28,8 @@ func firstcontinuetramp()
 func lastcontinuetramp()

 func initExceptionHandler() {
-       stdcall2(_AddVectoredExceptionHandler, 1, abi.FuncPCABI0(exceptiontramp))
-       if _AddVectoredContinueHandler == nil || GOARCH == "386" {
-               // use SetUnhandledExceptionFilter for windows-386 or
-               // if VectoredContinueHandler is unavailable.
-               // note: SetUnhandledExceptionFilter handler won't be called, if debugging.
-               stdcall1(_SetUnhandledExceptionFilter, abi.FuncPCABI0(lastcontinuetramp))
-       } else {
-               stdcall2(_AddVectoredContinueHandler, 1, abi.FuncPCABI0(firstcontinuetramp))
+       stdcall1(_SetUnhandledExceptionFilter, abi.FuncPCABI0(lastcontinuetramp))
+       if _AddVectoredContinueHandler != nil {
                stdcall2(_AddVectoredContinueHandler, 0, abi.FuncPCABI0(lastcontinuetramp))
        }
 }

Unfortunately this does not pass all tests and I did not have enough time to research why.

In any case I do not have a strong opinion on this matter as I am only an interested user and not a stakeholder of this project. I am happy to close this issue if you do not believe that any further changes are warranted.

[alexbrainman]

why does the Go runtime catch these exceptions?

@billziss-gh as I understand it, there are 2 things that Go exception is trying to achieve.

Go needs to be able to recover from CPU exceptions raised in Go code - like division by 0 or reading or writing memory pointed by nil pointer. See

go/src/runtime/signal_windows.go

Lines 65 to 89 in 61f0409

	func isgoexception(info *exceptionrecord, r *context) bool {
	// Only handle exception if executing instructions in Go binary
	// (not Windows library code).
	// TODO(mwhudson): needs to loop to support shared libs
	if r.ip() < firstmoduledata.text || firstmoduledata.etext < r.ip() {
	return false
	}
	// Go will only handle some exceptions.
	switch info.exceptioncode {
	default:
	return false
	case _EXCEPTION_ACCESS_VIOLATION:
	case _EXCEPTION_INT_DIVIDE_BY_ZERO:
	case _EXCEPTION_INT_OVERFLOW:
	case _EXCEPTION_FLT_DENORMAL_OPERAND:
	case _EXCEPTION_FLT_DIVIDE_BY_ZERO:
	case _EXCEPTION_FLT_INEXACT_RESULT:
	case _EXCEPTION_FLT_OVERFLOW:
	case _EXCEPTION_FLT_UNDERFLOW:
	case _EXCEPTION_BREAKPOINT:
	case _EXCEPTION_ILLEGAL_INSTRUCTION: // breakpoint arrives this way on arm64
	}
	return true
	}

And any go program crash should end with stack trace printed. The stack trace is not printed if go program is running under debugger, but otherwise, stack trace must always be displayed. As far as I remember, #8006 (that I mentioned above) is about stack trace was missing in some situations.

If you can still meet these 2 requirements with new exception handler code, I think it should be good enough.

CC @aarzilli in case this discussion affects Delve

Alex

[aarzilli]

CC @aarzilli in case this discussion affects Delve

AFAIK it shouldn't, if a CL for this happens I'll be happy to check that it works with delve.

[qmuntal]

In any case I do not have a strong opinion on this matter as I am only an interested user and not a stakeholder of this project. I am happy to close this issue if you do not believe that any further changes are warranted.

Don't get me wrong @billziss-gh, it would be okay for me to switch to SetUnhandledExceptionFilter if by this Go applications are more compatible with other Windows apps, so I would not close this proposal yet. For example, we could avoid doing this hack: 70546f6.

I've even tried to useSetUnhandledExceptionFilter instead of AddVectoredExceptionHandler, but without much luck for now.

[gopherbot]

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

[qmuntal]

Heads up: I'm trying to instruct Go's runtime to respect SEH in other modules. CL 457875 is an attempt to do so, but still WIP.