x/sys/windows: NewCallback function does not work

[jeet-parekh]

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

go1.9 windows/amd64
on Windows8.1

Does this issue reproduce with the latest release?

yes

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

GOARCH=amd64
GOHOSTARCH=amd64
GOHOSTOS=windows
GOOS=windows

What did you do?

package main

import (
    syswin "golang.org/x/sys/windows"
    // "syscall"
)

var (
    kernel32DLL = syswin.NewLazySystemDLL("kernel32.dll")
    user32DLL = syswin.NewLazySystemDLL("user32.dll")

    procGetCurrentThreadId = kernel32DLL.NewProc("GetCurrentThreadId")
    procSetWindowsHookEx = user32DLL.NewProc("SetWindowsHookExW")
    procCallNextHookEx = user32DLL.NewProc("CallNextHookEx")
)

var hook uintptr

func main() {
    if err := kernel32DLL.Load(); err != nil {
        panic("kernel32.dll not found")
    }
    defer syswin.FreeLibrary(syswin.Handle(kernel32DLL.Handle()))

    if err := user32DLL.Load(); err != nil {
        panic("user32.dll not found")
    }
    defer syswin.FreeLibrary(syswin.Handle(user32DLL.Handle()))

    thread_id, _, _ := procGetCurrentThreadId.Call()

    // use syscall instead of syswin here, and the code will work
    hook, _, _ = procSetWindowsHookEx.Call(2, syswin.NewCallback(callbackFunc), 0 , thread_id)
}

func callbackFunc(code, wparam, lparam uintptr) uintptr {
    ret, _, _ := procCallNextHookEx.Call(hook, code, wparam, lparam)
    return ret
}

What did you expect to see?

NewCallback function in x/sys/windows working correctly as in syscall

What did you see instead?

# command-line-arguments
golang.org/x/sys/windows.NewCallback: call to external function
main.main: relocation target golang.org/x/sys/windows.NewCallback not defined
main.main: undefined: "golang.org/x/sys/windows.NewCallback"

[alexbrainman]

@jeetp96 I don't think golang.org/x/sys/windows.NewCallback ever worked. Did it ever?
Why don't you use syscall.NewCallback instead?

I think we should just delete golang.org/x/sys/windows.NewCallback. What do you think, @ianlancetaylor?

Alex

[jeet-parekh]

@alexbrainman This is the first time I'm using golang.org/x/sys/windows.NewCallback, so I don't know if it ever worked earlier. https://golang.org/pkg/syscall/ says to migrate to golang.org/x/sys, and that's what I was trying to do.

Now that you suggest deleting golang.org/x/sys/windows.NewCallback, and because it's suggested to migrate to golang.org/x/sys, I would like to propose something. Would it be possible to call syscall.NewCallback from golang.org/x/sys/windows.NewCallback and return that as a result?

golang.org/x/sys already imports syscall. By doing what I proposed, developers using golang.org/x/sys would not have to import syscall just for the NewCallback function.

[jeet-parekh]

I'm sorry for that closing and reopening - in the comment box, I thought "Close and comment" meant close the preview and comment.

[alexbrainman]

Would it be possible to call syscall.NewCallback from golang.org/x/sys/windows.NewCallback and return that as a result?

Yes, that should, probably, work. Would you like to try and fix it yourself? This https://golang.org/doc/contribute.html is how to contribute.

Alex

[jeet-parekh]

I would love to try and fix it. I'll read the guidelines and start working on it.

[jeet-parekh]

@alexbrainman, I noticed there is a func newCallback(fn interface{}) (cb uintptr, err error) inside golang.org/x/sys/windows/svc/service.go. It also calls syscall.NewCallback but with an added recover() call. I guess that extra bit of code allows the execution not to panic in case the callback function isn't acceptable? not sure.

I could fix the windows.NewCallback to

1. Call the above function (I know it's an unexported name... I'll make some more changes).
   But then that would change the function signature from func NewCallback(fn interface{}) uintptr to func NewCallback(fn interface{}) uintptr, error. And I guess that would not play well with the Go1 rules?

2. Call syscall.NewCallback directly. This would not change the function signature, and the execution may panic.

What do you suggest?

[alexbrainman]

I guess that extra bit of code allows the execution not to panic in case the callback function isn't acceptable?

Correct. syscall.NewCallback(f) can panic if f is not acceptable function. It was, probably, a mistake to panic instead of returning error from syscall.NewCallback, but it cannot be changed now.

I could fix the windows.NewCallback to
...

I would go for option 2. For consistency sake.

Alex

[gopherbot]

Change https://golang.org/cl/63230 mentions this issue: x/sys/windows: Fix NewCallback function

[gopherbot]

Change https://golang.org/cl/63250 mentions this issue: x/sys/windows: Fix NewCallback function