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

cmd/compile: ICE on deferred call to syscall.LazyDLL.Call #44415

Closed
NhokCrazy199 opened this issue Feb 19, 2021 · 20 comments
Closed

cmd/compile: ICE on deferred call to syscall.LazyDLL.Call #44415

NhokCrazy199 opened this issue Feb 19, 2021 · 20 comments
Labels
FrozenDueToAge NeedsFix OS-Windows
Milestone

Comments

@NhokCrazy199
Copy link

@NhokCrazy199 NhokCrazy199 commented Feb 19, 2021

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

go version go1.16 windows/amd64

Does this issue reproduce with the latest release?

Yes, it does

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

set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\MyUser\AppData\Local\go-build
set GOENV=C:\Users\MyUser\AppData\Roaming\go\env
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=D:\Workspace\Go\pkg\mod
set GONOPROXY=
set GONOSUMDB=
set GOOS=windows
set GOPATH=D:\Workspace\Go
set GOPRIVATE=
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.16
set GCCGO=gccgo
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=D:\Workspace\Go\src\proj-path\go.mod
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=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\MyUser\AppData\Local\Temp\go-build1529013081=/tmp/go-build -gno-record-gcc-switches

What did you do?

My previous version is Go 1.15.6 and everything is ok.
I updated my Go and my func can not build with the lastest version:

func DPApi(data []byte) ([]byte, error) {
	dllCrypt := syscall.NewLazyDLL("Crypt32.dll")
	dllKernel := syscall.NewLazyDLL("Kernel32.dll")
	procDecryptData := dllCrypt.NewProc("CryptUnprotectData")
	procLocalFree := dllKernel.NewProc("LocalFree")
	var outBlob dataBlob
	r, _, err := procDecryptData.Call(uintptr(unsafe.Pointer(NewBlob(data))), 0, 0, 0, 0, 0, uintptr(unsafe.Pointer(&outBlob)))
	if r == 0 {
		return nil, err
	}
	defer procLocalFree.Call(uintptr(unsafe.Pointer(outBlob.pbData)))
	return outBlob.ToByteArray(), nil
}

What did you expect to see?

My func can run and build with my Go

What did you see instead?

proj-path/core/browsers/decrypt
core\browsers\decrypt\decrypt_windows.go:89:2: internal compiler error: 'wrap·2': Value live at entry. It shouldn't be. func wrap·2, node procLocalFree, value v35

Please file a bug report including a short program that triggers the error.
https://golang.org/issue/new
core\browsers\decrypt\decrypt_windows.go:75:2: internal compiler error: 'wrap·1': Value live at entry. It shouldn't be. func wrap·1, node procLocalFree, value v35

Please file a bug report including a short program that triggers the error.
https://golang.org/issue/new

Desc:
decrypt_windows.go:89 is line defer procLocalFree.Call(uintptr(unsafe.Pointer(outBlob.pbData)))
decrypt_windows.go:75 is line func DPApi(data []byte) ([]byte, error) {

@egonelbre
Copy link
Contributor

@egonelbre egonelbre commented Feb 19, 2021

Note that the dll and proc instantiation should probably be outside the function, something like:

var (
	dllCrypt  = syscall.NewLazyDLL("Crypt32.dll")
	dllKernel = syscall.NewLazyDLL("Kernel32.dll")

	procDecryptData = dllCrypt.NewProc("CryptUnprotectData")
	procLocalFree   = dllKernel.NewProc("LocalFree")
)

// chrome < 80 https://chromium.googlesource.com/chromium/src/+/76f496a7235c3432983421402951d73905c8be96/components/os_crypt/os_crypt_win.cc#82
func DPApi(data []byte) ([]byte, error) {
	var outBlob dataBlob
	r, _, err := procDecryptData.Call(uintptr(unsafe.Pointer(NewBlob(data))), 0, 0, 0, 0, 0, uintptr(unsafe.Pointer(&outBlob)))
	if r == 0 {

But, it looks like a compiler bug nevertheless.

@egonelbre
Copy link
Contributor

@egonelbre egonelbre commented Feb 19, 2021

Error message looks the same as #44355.

@toothrot
Copy link
Contributor

@toothrot toothrot commented Feb 19, 2021

Thanks for the detailed issue. I think you're right that this is a duplicate of #44355. I'll close this in lieu of that issue.

I think this is an interesting case to mention there though.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Feb 19, 2021

Issue #44355 is pretty narrow: it should only affect inlinable functions that explicitly name all of their result parameters as blank (i.e., _), and then contain a single return statement. Are there any functions like that in consideration here?

It would be great to have a standalone reproducer for this issue. Ideally one that can be built from Linux (e.g., using GOOS=windows is fine, but without requiring cgo).

(FWIW, "Value live at entry" is a pretty generic catch-all failure that happens later in the compiler when almost anything goes wrong in the frontend and we didn't catch it sooner to give a more descriptive error report. So I don't recommend using that alone to deduplicate issues.)

@egonelbre
Copy link
Contributor

@egonelbre egonelbre commented Feb 20, 2021

Windows reproducer (based on the initial cause in https://github.com/moonD4rk/HackBrowserData/blob/6a11361e1dbf0c3e455ccb06a4f31d073f7d8e8e/core/decrypt/decrypt_windows.go):

package decrypt

import (
	"syscall"
	"unsafe"
)

type dataBlob struct {
	cbData uint32
	pbData *byte
}

func (b *dataBlob) ToByteArray() []byte {
	d := make([]byte, b.cbData)
	copy(d, (*[1 << 30]byte)(unsafe.Pointer(b.pbData))[:])
	return d
}

var dllKernel = syscall.NewLazyDLL("Kernel32.dll")

func Call(data []byte) ([]byte, error) {
	procLocalFree := dllKernel.NewProc("LocalFree")
	var outBlob dataBlob
	defer procLocalFree.Call(uintptr(unsafe.Pointer(outBlob.pbData)))
	return outBlob.ToByteArray(), nil
}

@mdempsky mdempsky changed the title Error when Go call Win API cmd/compile: ICE on deferred call to syscall.LazyDLL.Call Feb 20, 2021
@mdempsky mdempsky added the NeedsFix label Feb 20, 2021
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Feb 20, 2021

Thanks @egonelbre! Further minimized, and confirmed it reproduces on Linux with GOOS=windows:

package decrypt

import (
	"syscall"
	"unsafe"
)

var dllKernel = syscall.NewLazyDLL("Kernel32.dll")

func Call() {
	procLocalFree := dllKernel.NewProc("LocalFree")
	defer procLocalFree.Call(uintptr(unsafe.Pointer(nil)))
}

Notably, it doesn't reproduce with -gcflags=-N.

/cc @cuonglm

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Feb 20, 2021

Oh, in the fix for #24491, we only fixed direct function calls. We forgot that for method calls, there's a receiver argument that needs to be passed through the wrapper function too.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Feb 20, 2021

@gopherbot Backport to Go 1.16. Compiler crash on valid Windows code that compiled with Go 1.15.

@ALTree
Copy link
Member

@ALTree ALTree commented Feb 20, 2021

@mdempsky I think you have to say "please" or the gopherbot won't listen to you. A somewhat questionable design decision...

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 20, 2021

@gopherbot Please backport to Go 1.16.

Compiler crash on valid Windows code that compiled with Go 1.15.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 20, 2021

Backport issue(s) opened: #44463 (for 1.15), #44464 (for 1.16).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 21, 2021

Change https://golang.org/cl/294849 mentions this issue: cmd/compile: fix mishandling of unsafe-uintptr arguments with call method in go/defer

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 25, 2021

Change https://golang.org/cl/296489 mentions this issue: [release-branch.go1.16] cmd/compile: fix mishandling of unsafe-uintptr arguments with call method in go/defer

@mdempsky mdempsky reopened this Feb 25, 2021
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Feb 25, 2021

Reopening because the fix is incomplete in the presence of non-open-coded defers.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Feb 25, 2021

It looks like it only worked for open-coded defers because open-coded defers already keep their arguments alive past the deferred call:

Failing test case using open-coded defer: https://play.golang.org/p/9SEiOeaLmCS

Passing test case using heap-allocated defer: https://play.golang.org/p/BWEQuC5EowK

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Feb 25, 2021

Issue with open-coded defers reported as #44623.

As for CL 296489, I think the issue was that when we rewrite t.M(...) into T.M(t, ...), we lose the escape analysis tags. This wasn't an issue for the existing code, because it happens after escape analysis tags are needed; but with CL 296489, the escape analysis tags were still needed within the generated wrapper so it would know to insert the keepalives.

Two possible options I see are:

  1. When we create the signature type for method expressions (e.g., T.M), we need to preserve the method's escape analysis tags. (I think there's an outstanding issue still about this too.) Caution: We need to be careful about the receiver argument's tags.
  2. Instead of rewriting method calls into a function calls with method expressions when wrapping them, we could keep the original method call. This requires extending the wrapCall logic to also pass the receiver argument through the closure.

I was initially thinking 2 would be the easier and safer option; but if option 1 isn't too invasive, I think that might be better actually.

As for a test case, locally I've just added:

       func() {
               s := &S{}
               for {
                       defer s.test("defer method loop", uintptr(setup()), uintptr(setup()), uintptr(setup()), uintptr(setup()))
                       break
               }
       }()
       <-done

It would probably make sense to add a similar variant for normal deferred calls too. (On the upside, these are at least already handled correctly.)

@cuonglm
Copy link
Member

@cuonglm cuonglm commented Feb 26, 2021

@mdempsky FYI, the fix in CL 296569 is the 2nd.

@cuonglm
Copy link
Member

@cuonglm cuonglm commented Feb 26, 2021

@mdempsky I'm leaning to doing 2nd option for now, so the fix and the backports CL will be in synced. Then we can refactoring to 1st option later.

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 26, 2021

Change https://golang.org/cl/296490 mentions this issue: cmd/compile: fix mishandling of unsafe-uintptr arguments with call method in go/defer

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 26, 2021

Change https://golang.org/cl/296769 mentions this issue: [release-branch.go1.16] cmd/compile: fix mishandling of unsafe-uintptr arguments with call method in go/defer

@dmitshur dmitshur added this to the Go1.17 milestone Mar 1, 2021
gopherbot pushed a commit that referenced this issue Mar 1, 2021
…r arguments with call method in go/defer

In CL 253457, we did the same fix for direct function calls. But for
method calls, the receiver argument also need to be passed through the
wrapper function, which we are not doing so the compiler crashes with
the code in #44415.

It will be nicer if we can rewrite OCALLMETHOD to normal OCALLFUNC, but
that will be for future CL. The passing receiver argument to wrapper
function is easier for backporting to go1.16 branch.

Fixes #44464

Change-Id: I03607a64429042c6066ce673931db9769deb3124
Reviewed-on: https://go-review.googlesource.com/c/go/+/296490
Trust: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Run-TryBot: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-on: https://go-review.googlesource.com/c/go/+/296769
Trust: Bryan C. Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators Mar 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix OS-Windows
Projects
None yet
Development

No branches or pull requests

9 participants