syscall: (*LazyProc).Call does not keep arguments alive (anymore) #34474
Comments
bcmills
added
NeedsInvestigation
|
CC @mdempsky This is implemented via the magic comment |
|
FWIW, below is the output of EDIT: Unrelated to DispatchMessage, I don't see anything about |
|
I could reproduce this problem on current tip. But I could not reproduce this problem on bbe5da4. So I bisected the problem to: Alex |
|
CC @randall77 |
|
@alexbrainman Are you sure that's the right bisect? That was three years ago. Seems like |
That is what I did after I have found the culprit Alex |
|
Sorry, I don't doubt that it failed there, it's just that I would think that it was fixed, and then failed again more recently. Has this really been broken for over three years and nobody noticed? |
Maybe our Windows users are just still using Go 1.7. ;[ |
I used git bisect command between bbe5da4 and current tip - that is how I discovered broken ca4089a commit. I suspect all commits after ca4089a are broken. I do not know how to prove that. If you have suggestion, I am all years. Also you need to be aware that test above sometimes succeeds even with broken commit - it is not 100% test. So sometimes I have to run test multiple times to make sure.
I would not be surprised, if it was broken for that long. The test above uses callbacks. And callbacks are manly used in Windows GUI. I don't think there are many Go Windows GUI programs / packages. I believe @zx2c4 built / supports Windows Go GUI program. Maybe he had some unexplained crashes. And I am sure, we have some test around this area, but obviously they are not enough. And as Go runtime changed over time, maybe tests became useless. Alex |
Note that I saw premature freeing even for calls without callbacks. For example, the CreateWindowEx call results in a garbled window title if Related, my edit above said:
|
Actually, yes. I wound up allocating certain objects explicitly on the Win32 heap in a fix to the lxn/walk package. I wouldn't be surprised if Alex's three year old commit was the culprit. When I showed up relatively recently, @lxn gave me the impression that the bug I was fixing (i.e. hacking around) had plagued him and his programs for quite a long time. |
|
Here's the commit in question: lxn/walk@c0622b1 The analysis in that commit message is likely wrong, but it does directly correspond with the page permissions I was seeing in WinDbg at the time of the crash. |
|
Here's a more minimal example: package main
import (
"runtime"
"syscall"
"unsafe"
)
func main() {
finalizerCalled := false
v := &struct{ b [9]byte }{}
runtime.SetFinalizer(v, func(v *struct{ b [9]byte }) {
finalizerCalled = true
})
(*syscall.Proc)(unsafe.Pointer(&struct {
dll uintptr
name string
addr uintptr
}{addr: syscall.NewCallback(func(v uintptr) uintptr {
runtime.GC()
if finalizerCalled {
println("UaF")
}
// Assume that rather than a Go-managed callback, this is
// actually some system function that dereferences v.
return 0
})})).Call(uintptr(unsafe.Pointer(v)))
// Uncomment this next line to remove the UaF.
//runtime.KeepAlive(v)
} |
I am pretty sure (I did not check), that windowProc is called by the time CreateWindowEx returns.
Which commit are you referring to? Which Alex are you referring to?
@lxn you should always come and complain here at the source. We don't always help, but we always try. And you will get more eyes here.
Sure. I can reproduce this on current tip too. Nice test we could add to the repo. Thank you. Alex |
|
Hey @randall77 - could you take a look at the PoC in #34474 (comment) failing due to your commit ca4089a? I've started to hack up the compiler to fix this and I'm sure I'll get it working eventually but it occurred to me that somebody who actually works on the Go compiler might be better suited to formulate a proper fix. |
|
I'm not sure what the fix would be. Somehow find OpConvert ops that convert unsafe.Pointer to uintptr and are then used in a call. Extend the lifetime of the arg of OpConvert to include the callsite. There is related code in cmd/compile/internal/gc/plive.go, (*Liveness).markUnsafePoints. But maybe there's a better way to make this happen. Maybe introduce OpKeepAlive(x) ops after calls for every argument which is OpConvert(x) where x is an unsafe.Pointer type. |
|
I thought order.go already tries to insert KeepAlive for temps introduced for calls to //go:uintprescapes involving unsafe.Pointer=>uintptr conversions? |
|
Indeed, that code looks like it already exists. Why is it not working? |
|
Not sure. I can take a look later this week if no one beats me to it. |
|
Had a little bit of time to look into it. It's just because Order.call only checks OCALLFUNC, and LazyProc.Call uses OCALLMETH. |
|
Change https://golang.org/cl/198043 mentions this issue: |
|
If Windows folks could test CL 198043, that would be great. I think it should address the issues with LazyProc reported here, but I don't have a Windows computer to test with. There are some more edge cases I want to consider before submitting it, but I don't think those are likely to affect real programs. I'm also curious whether this merits backport to Go 1.13? It's a regression from an older Go release, but much older, so maybe not? |
|
Thanks. If the fix is simple and obvious (in hindsight), I had hoped it would qualify for a 1.13 backport. The crashes and workarounds are subtle, and my work (Gio on Window) requires Go 1.13 anyway. |
|
@eliasnaur Great, thanks for the confirmation! |
|
@gopherbot please backport this, because mis-compilations are dangerous and the fix appears to be a trivial single line fix. |
|
Backport issue(s) opened: #34641 (for 1.12), #34642 (for 1.13). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases. |
|
I'm marking this for Go 1.14 because I think some fix should make the release. If nothing else, I'd like the CL 198043 fix for regular LazyProc.Calls to go in. |
What version of Go are you using (
go version)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
On my Windows 10, run the program from https://play.golang.org/p/2JzHDalGN7Q.
What did you expect to see?
No errors.
What did you see instead?
Which means that the
m *msgargument to dispatchMessage was finalized before the call to the window procedure callback returned, indicating that the_DispatchMessage.Calldidn't keep its argument alive.On my machine, I can comment out the panic and get a native crash instead:
This issue seems like a variant of #16035. And sure enough, adding
runtime.KeepAlive(m)to dispatchMessage does fix the native crash above.The text was updated successfully, but these errors were encountered: