Open billziss-gh opened 2 years ago
(CC @golang/windows)
cc @golang/runtime
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
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:
EXCEPTION_CONTINUE_EXECUTION
subsequent vectored exception handlers and structured exception handlers are not invoked; however vectored continue handlers are invoked.EXCEPTION_CONTINUE_EXECUTION
subsequent structured exception handlers are not invoked; however vectored continue handlers are invoked.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).EDIT: I attach here a small C++ program that demonstrates that this technique works (at least on my Win11 laptop): veh.zip
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
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.
@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
:
sigtramp
is called, because of Golang's use of vectored exception handlers.)badsignal2
(#56080)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
:
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.)ThreadRaiseExcept
.Overall it looks to me that the exception handling code for Windows needs some TLC.
EDIT: My tests were done with:
Change https://go.dev/cl/442896 mentions this issue: runtime: ignore exceptions from non-Go threads on windows arm/arm64
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
.
@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.
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 https://github.com/golang/go/blob/61f0409c31cad8729d7982425d353d7b2ea80534/src/runtime/signal_windows.go#L65-L89
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
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.
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: 70546f6404c5927a9868a80ccbf4c6c2beaea671.
I've even tried to useSetUnhandledExceptionFilter
instead of AddVectoredExceptionHandler
, but without much luck for now.
Change https://go.dev/cl/457875 mentions this issue: runtime,cmd/link: allow SEH tramps handle non-Go exception
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.
What version of Go are you using (
go version
)?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
OutputWhat 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
usesAddVectoredExceptionHandler
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
https://github.com/rclone/rclone/issues/5828#issuecomment-1269934140