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

runtime: native callback on Windows only returns 32 bit result on 64 bit platform #29331

Open
rixtox opened this Issue Dec 19, 2018 · 8 comments

Comments

Projects
None yet
6 participants
@rixtox
Copy link

rixtox commented Dec 19, 2018

What version of Go are you using?

$ go version
go version go1.11.4 windows/amd64

Problem

I'm calling a DLL function, that takes a callback function, where the callback function returns a pointer to some data structure, and the DLL will perform some action on the data structure.

package main

import (
    "golang.org/x/sys/windows"
    "unsafe"
)

type A struct {}
var a = &A{}

func Callback() uintptr {
    ptr := uintptr(unsafe.Pointer(a))
    fmt.Printf("Address: 0x%X\n", ptr)
    return ptr
}

func main() {
    dll := windows.MustLoadDLL("some.dll")
    init := dll.MustFindProc("init")
    init.Call(windows.NewCallback(Callback))
    fmt.Printf("This never get prints\n")
}

I'm running Go on Windows 64-bit machine, the DLL is also 64-bit. The above code will print the address of the pointer, which is 0xC000080018, but the call to the DLL will fail silently.

Cause

I don't have the source code to the DLL so I can't make it print the value it gets. So I used IDA pro to dynamically debug the program, and I found that the Go callback function is actually returning 32-bit value instead of 64-bit value. This is the final code of the Go callback routine before it returns back to the DLL:

main.exe:00000000004537E7     mov     eax, [rcx+rdx-8]
main.exe:00000000004537EB     pop     qword ptr [rcx+rdx-8]
main.exe:00000000004537EF     retn

As you can see Go is writing the return value in eax instead of rax, which will discard the higher address, therefore the DLL will only get 0x80018 and caused errors.

Possible fix?

I've done some digging into Go's implementation of the callback, and found the assembly code at src/runtime/sys_windows_amd64.s. I noticed that in runtime·callbackasm1, the return value is set using MOVL instead of MOVQ. So I changed it to MOVQ and everything worked.

I'm not sure if it's a bug or actually intended, so I'm opening an issue first.

@bcmills bcmills changed the title Windows native callback only returns 32 bit result on 64 bit platform x/sys/windows: native callback only returns 32 bit result on 64 bit platform Dec 19, 2018

@gopherbot gopherbot added this to the Unreleased milestone Dec 19, 2018

@bcmills

This comment has been minimized.

Copy link
Member

bcmills commented Dec 19, 2018

@alexbrainman

This comment has been minimized.

Copy link
Member

alexbrainman commented Dec 29, 2018

@rixtox

Thank you for reporting this issue. I am pretty sure, it is a bug to use MOVL instead of MOVQ in that code. windows/386 was first version of Go, and then windows/amd64 was created later. I suspect 386 code was copied onto amd64, and we did not adjusted that line. We also obviously do not have the test for that code.

I will try and write new test, and change this code, and see if anything gets affected in a bad way. We won't be able to make this change until after go1.12 is released. It has been broken forever, so it cannot be urgent.

If you want to try and implement the change yourself, let me know. This https://golang.org/doc/contribute.html is how to contribute.

Alex

@OliverZou

This comment has been minimized.

Copy link

OliverZou commented Jan 16, 2019

o no,I met the like problem when go64exe load 64dll which call a callback function imp in go under windows10 x64.after c function call the callback ,the go program stop at asm_amd64.s cgocallback_gofunc.i hope these bugs are fiexed as soon as possible.thanks. (by the workround mentioned here ,i modified file and rebuilt a go.exe,but it dont work for me)

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Jan 25, 2019

Change https://golang.org/cl/159579 mentions this issue: runtime: allow for syscall.NewCallback function return uintptr values

@alexbrainman

This comment has been minimized.

Copy link
Member

alexbrainman commented Jan 25, 2019

@rixtox and @OliverZou please try https://golang.org/cl/159579 see if it fixes your problem. https://golang.org/cl/159579 fixes syscall package, not golang.org/x/sys/windows. So just replace golang.org/x/sys/windows with syscall in your test program. If syscall fix works, I will copy that code into golang.org/x/sys/windows.

Thank you.

Alex

@rixtox

This comment has been minimized.

Copy link
Author

rixtox commented Jan 26, 2019

@alexbrainman The fix works for me.

@alexbrainman

This comment has been minimized.

Copy link
Member

alexbrainman commented Jan 27, 2019

The fix works for me.

Thanks for confirming. Now we need for someone to review my change (I made a note there to wait until go1.12 is released).

Alex

@odeke-em odeke-em changed the title x/sys/windows: native callback only returns 32 bit result on 64 bit platform runtime: native callback on Windows only returns 32 bit result on 64 bit platform Jan 30, 2019

@odeke-em

This comment has been minimized.

Copy link
Member

odeke-em commented Jan 30, 2019

Thanks for confirming. Now we need for someone to review my change (I made a note there to wait until go1.12 is released).

Cool, thanks for the CL @alexbrainman! @randall77 and I reviewed and approved your CL, and there is a small pending update to the test.
I've also updated this issue's title to reflect where the change is to be made, i.e. in the runtime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment