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

x/sys/windows: uintptr(unsafe.Pointer) calls outside of syscalls #67437

Closed
abennett opened this issue May 16, 2024 · 6 comments
Closed

x/sys/windows: uintptr(unsafe.Pointer) calls outside of syscalls #67437

abennett opened this issue May 16, 2024 · 6 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FixPending Issues that have a fix which has not yet been reviewed or submitted. help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Milestone

Comments

@abennett
Copy link

abennett commented May 16, 2024

Documentation states:

(4) Conversion of a Pointer to a uintptr when calling syscall.Syscall.
The Syscall functions in package syscall pass their uintptr arguments directly to the operating system, which then may, depending on the details of the call, reinterpret some of them as pointers. That is, the system call implementation is implicitly converting certain arguments back from uintptr to pointer.
If a pointer argument must be converted to uintptr for use as an argument, that conversion must appear in the call expression itself

However, there are calls within /x/sys/windows that do not follow this requirement. For an example, this call is casting an unsafe.Pointer to uintptr then is then passed to here

The generated calls in windows/zsyscall_windows.go should likely accept unsafe.Pointer arguments instead of uintptr.

Examples

// from windows/svc/service.go
handle, err := windows.RegisterServiceCtrlHandlerEx(windows.StringToUTF16Ptr(theService.name), ctlHandlerCallback, uintptr(unsafe.Pointer(&theService)))

// from windows/zsyscall_windows.go
func RegisterServiceCtrlHandlerEx(serviceName *uint16, handlerProc uintptr, context uintptr) (handle Handle, err error) {
	r0, _, e1 := syscall.Syscall(procRegisterServiceCtrlHandlerExW.Addr(), 3, uintptr(unsafe.Pointer(serviceName)), uintptr(handlerProc), uintptr(context))
	handle = Handle(r0)
	if handle == 0 {
		err = errnoErr(e1)
	}
	return
}
@dmitshur dmitshur changed the title /x/sys: uintptr(unsafe.Pointer) calls outside of syscalls x/sys: uintptr(unsafe.Pointer) calls outside of syscalls May 16, 2024
@gopherbot gopherbot added this to the Unreleased milestone May 16, 2024
@dmitshur dmitshur changed the title x/sys: uintptr(unsafe.Pointer) calls outside of syscalls x/sys/windows: uintptr(unsafe.Pointer) calls outside of syscalls May 16, 2024
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label May 16, 2024
@dmitshur dmitshur added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 16, 2024
@dmitshur
Copy link
Contributor

CC @golang/runtime, @alexbrainman.

@ianlancetaylor
Copy link
Contributor

CC @golang/windows

I agree that this looks like a bug in x/sys/windows. I don't know how hard it would be to fix.

@mguy-huntress
Copy link

@ianlancetaylor
In this instance, you have the global theService for the context that's being passed in and converted. That context ends up here. You could ignore this context (and the conversion that could cause errors) and just use the global theService directly in this function.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/591475 mentions this issue: windows/svc: do not pass theService to windows.RegisterServiceCtrlHandlerEx

@qmuntal
Copy link
Contributor

qmuntal commented Jun 8, 2024

This is a duplicated of #59687. IMO @alexbrainman CL supersedes the one submitted in #59687.

@alexbrainman
Copy link
Member

@ianlancetaylor
In this instance, you have the global theService for the context that's being passed in and converted. That context ends up here. You could ignore this context (and the conversion that could cause errors) and just use the global theService directly in this function.

I implemented @mguy-huntress suggestion in

https://go-review.googlesource.com/c/sys/+/591475

Hopefully it is good enough.

Alex

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. FixPending Issues that have a fix which has not yet been reviewed or submitted. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FixPending Issues that have a fix which has not yet been reviewed or submitted. help wanted NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Projects
Development

No branches or pull requests

8 participants