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: badsignal2 crash on windows/arm64 #56080

Closed
billziss-gh opened this issue Oct 6, 2022 · 7 comments
Closed

runtime: badsignal2 crash on windows/arm64 #56080

billziss-gh opened this issue Oct 6, 2022 · 7 comments
Assignees
@billziss-gh
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Milestone
Backlog

Comments

@billziss-gh
Copy link
Contributor

billziss-gh commented Oct 6, 2022 •
edited

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 Windows exception in a non-Golang thread. This results in a call to runtime.badsignal2 via runtime.sigtramp.

What did you expect to see?

I expected to see the message runtime: signal received on thread not created by Go.

What did you see instead?

An Access Violation in WriteFile.

Problem and solution

The problem is that WriteFile expects a valid pointer for the lpNumberOfBytesWritten parameter. However badsignal2 seems to initialize the x3 register (which contains the lpNumberOfBytesWritten argument) to a bogus value.

The solution is to properly initialize x3 prior to calling WriteFile.

See also

#50877
rclone/rclone#5828 (comment)
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Oct 6, 2022
@billziss-gh billziss-gh mentioned this issue Oct 6, 2022
Open
@cagedmantis
Copy link
Contributor

cc @golang/windows @golang/runtime

@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 7, 2022
@cagedmantis cagedmantis added this to the Backlog milestone Oct 7, 2022
@alexbrainman
Copy link
Member

The solution is to properly initialize x3 prior to calling WriteFile.

@billziss-gh thanks for report.

I am not familiar enough with arm64 assembler, but I am pretty sure you are correct.

Please feel free to send fix for this issue. Here https://go.dev/doc/contribute is how to contribute. Or you can just create Github PR, and Go bot will automatically create Gerrit CL for you.

Thank you.

Alex

@alexbrainman
Copy link
Member

CC @qmuntal in case you are interested

Alex

@billziss-gh
Copy link
Contributor Author

billziss-gh commented Oct 9, 2022 •
edited

The following one-line patch appears to fix the problem on my Windows/ARM machine (i.e. badsignal2 now prints "runtime: signal received on thread not created by Go" before aborting).

diff --git a/src/runtime/sys_windows_arm64.s b/src/runtime/sys_windows_arm64.s
index 024625f821..0a293ec317 100644
--- a/src/runtime/sys_windows_arm64.s
+++ b/src/runtime/sys_windows_arm64.s
@@ -113,7 +113,7 @@ TEXT runtime·badsignal2(SB),NOSPLIT,$16-0
        MOVD    $runtime·badsignalmsg(SB), R1   // lpBuffer
        MOVD    $runtime·badsignallen(SB), R2   // lpNumberOfBytesToWrite
        MOVD    (R2), R2
-       MOVD    R13, R3         // lpNumberOfBytesWritten
+       ADD     $0x10, RSP, R3          // lpNumberOfBytesWritten
        MOVD    $0, R4                  // lpOverlapped
        MOVD    runtime·_WriteFile(SB), R12
        SUB     $16, RSP        // skip over saved frame pointer below RSP

Please note that I am not familiar with Go's assembly and consequently this patch may be brittle. For example, if there is a change in the frame prolog that the Go assembler outputs, the offset to add to RSP may have to change. (I looked at the actual disassembly in WinDbg in order to understand the emitted prolog and inform my patch.)

@qmuntal
Copy link
Contributor

qmuntal commented Oct 10, 2022 •
edited

I confirm that with @billziss-gh patch badsignal2 does not crash and runtime·_WriteFile prints runtime: signal received on thread not created by Go. Please go ahead and submit a CL with the fix. Someone with more experience in Go assembly will have to step in and resolve the prolog question.

On the other hand, I don't think that the expected message should be runtime: signal received ..., but just exit status 42 (being 42 whatever status code passed to RaiseException), at least this is the windows/amd64 behavior.

Summary:

@billziss-gh billziss-gh mentioned this issue Oct 11, 2022
Closed
@billziss-gh
Copy link
Contributor Author

@qmuntal as per your request I have submitted a PR with the above patch.

@gopherbot
Copy link

Change https://go.dev/cl/442216 mentions this issue: runtime: fix invalid pointer in windows/arm64 badsignal2

romaindoumenc pushed a commit to TroutSoftware/go that referenced this issue Nov 3, 2022
@billziss-gh @romaindoumenc
runtime: fix invalid pointer in windows/arm64 badsignal2
997b18e 
Initializes the R3 register with an available address in the stack. The addressed location is used to receive the number of bytes written by WriteFile.

Fixes golang#56080

Change-Id: I0368eb7a31d2d6a098fa9c26e074eb1114a92704
GitHub-Last-Rev: 23dbdb5
GitHub-Pull-Request: golang#56153
Reviewed-on: https://go-review.googlesource.com/c/go/+/442216
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Cherry Mui <cherryyz@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators Oct 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@billziss-gh billziss-gh

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

runtime: fix invalid pointer in windows/arm64 badsignal2
5 participants
@cagedmantis @qmuntal @gopherbot @alexbrainman @billziss-gh