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

x/sys/windows: NewCallback function does not work #21831

Closed
jeet-parekh opened this issue Sep 10, 2017 · 9 comments
Closed

x/sys/windows: NewCallback function does not work #21831

jeet-parekh opened this issue Sep 10, 2017 · 9 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Milestone
Unreleased

Comments

@jeet-parekh
Copy link
Contributor

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"
@ianlancetaylor ianlancetaylor changed the title NewCallback function in x/sys/windows does not work x/sys/windows: NewCallback function does not work Sep 10, 2017
@gopherbot gopherbot added this to the Unreleased milestone Sep 10, 2017
@ianlancetaylor ianlancetaylor added OS-Windows NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 10, 2017
@alexbrainman
Copy link
Member

@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
Copy link
Contributor Author

@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 jeet-parekh reopened this Sep 11, 2017
@jeet-parekh
Copy link
Contributor Author

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

@alexbrainman
Copy link
Member

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
Copy link
Contributor Author

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

@jeet-parekh
Copy link
Contributor Author

jeet-parekh commented Sep 11, 2017 •
edited

@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
Copy link
Member

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
Copy link

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

@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Sep 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
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
Unreleased
Development

No branches or pull requests
4 participants
@ianlancetaylor @gopherbot @alexbrainman @jeet-parekh