go icon indicating copy to clipboard operation
go copied to clipboard

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

Open abennett opened this issue 9 months ago • 1 comments

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
}

abennett avatar May 16 '24 21:05 abennett

CC @golang/runtime, @alexbrainman.

dmitshur avatar May 16 '24 22:05 dmitshur

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.

ianlancetaylor avatar May 18 '24 20:05 ianlancetaylor

@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.

mguy-huntress avatar May 20 '24 17:05 mguy-huntress

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

gopherbot avatar Jun 08 '24 07:06 gopherbot

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

qmuntal avatar Jun 08 '24 08:06 qmuntal

@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

alexbrainman avatar Jun 08 '24 09:06 alexbrainman