x/sys/windows: LocalAlloc should return an unsafe.Pointer or *byte

[danderson]

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

$ go version
go version go1.15.8 linux/amd64

Does this issue reproduce with the latest release?

N/A, bug report is for x/sys/windows

What did you do?

I'm writing a library to manipulate the Windows firewall at inet.af/wf. As part of calling the API, I need to construct a tree of C structs with various pointer unsafeties within. I'm using windows.LocalAlloc and windows.LocalFree to allocate memory on the non-go-managed heap.

Unfortunately though, windows.LocalAlloc returns the pointer as a uintptr - presumably because LocalAlloc can return either a raw pointer or a Windows handle to movable memory. This means that any time I want to use the allocated memory, I have to do an unsafe conversion that makes go vet mad:

type myStruct { ... }

	mem, err := windows.LocalAlloc(windows.LPTR, uint32(unsafe.Sizeof(myStruct{})))
	if err != nil {
		panic(err)
	}
	ret := (*myStruct)(unsafe.Pointer(mem))

Even though these are famous last words, I believe that this unsafe conversion is actually safe, because the uintptr isn't representing a pointer into the Go heap. It would be nice to be able to use LocalAlloc without triggering a false positive in go vet.

The two ways I can think of to achieve this are:

• Make LocalAlloc return an unsafe.Pointer instead of a uintptr, to express that it can be cast to whatever pointer type you wish, subject to unsafety rules. That includes converting it to a uintptr, which is the underlying type of windows.Handle, for the case where you allocate a movable chunk of memory.
• Break LocalAlloc into two functions, one returning an unsafe.Pointer for fixed allocs, and one returning a windows.Handle for movable allocs. That would make it easier to use movable allocs in Go (although I cannot think why you'd want to, so my preference would be for the prior option).

[ericlagergren]

Make LocalAlloc return an unsafe.Pointer instead of a uintptr

AFAIK the stdlib does not return unsafe.Pointer outside the unsafe package because it shouldn't be possible to get an unsafe.Pointer without importing the unsafe package.

Perhaps the API could return windows.Pointer instead? Then you could convert it to with unsafe yourself.

[danderson]

Ah, good point. Yeah, a windows.Pointer would work, and arguably be slightly safer in that you can't access any of the allocated bytes at all without importing unsafe to do a conversion or pointer arithmetic.

[tmthrgd]

AFAIK that’s not really an issue anymore, see #40592 (comment) and previous comments.

[networkimprov]

cc @alexbrainman @zx2c4
@gopherbot add OS-Windows

[ericlagergren]

@tmthrgd oh yeah, that’s true. I’d forgotten about that issue. (It’s an unfortunate choice imo, but oh well.)

…

Le 6 mars 2021 à 22:12, Tom Thorogood ***@***.***> a écrit :  AFAIK that’s not really an issue anymore, see #40592 (comment) and previous comments. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[danderson]

I'm fine with either, honestly. No matter what, if you have Windows allocate memory for you, you're going to unsafely cast it to whatever you need. I'd just like to be able to do that without tripping pointer unsafety warnings.

[zx2c4]

I'll fix.

[gopherbot]

Change https://golang.org/cl/299492 mentions this issue: windows: correct definitions of Local* functions

[alexbrainman]

I have to do an unsafe conversion that makes go vet mad

@danderson

I just tried to run go vet on top of current tip (https://github.com/golang/sys/tree/8fe3ee5dd75b278632199a2614e4eac8235af0d0/windows) of golang.org/x/sys/windows, and vet is mad about that code too. I see this:

$ GOOS=windows go vet
# golang.org/x/sys/windows
./env_windows.go:42:39: possible misuse of unsafe.Pointer
./syscall_windows.go:1666:11: possible misuse of unsafe.Pointer
./zsyscall_windows.go:759:40: possible misuse of unsafe.Pointer
./zsyscall_windows.go:765:27: possible misuse of unsafe.Pointer
./zsyscall_windows.go:771:19: possible misuse of unsafe.Pointer
./zsyscall_windows.go:1217:27: possible misuse of unsafe.Pointer
./zsyscall_windows.go:1234:30: possible misuse of unsafe.Pointer
./zsyscall_windows.go:1240:27: possible misuse of unsafe.Pointer
./zsyscall_windows.go:1249:24: possible misuse of unsafe.Pointer
./zsyscall_windows.go:1258:34: possible misuse of unsafe.Pointer
./zsyscall_windows.go:1267:25: possible misuse of unsafe.Pointer
./zsyscall_windows.go:1853:18: possible misuse of unsafe.Pointer
./zsyscall_windows.go:1926:19: possible misuse of unsafe.Pointer
./zsyscall_windows.go:3118:15: possible misuse of unsafe.Pointer
./zsyscall_windows.go:3250:32: possible misuse of unsafe.Pointer
./zsyscall_windows.go:3475:17: possible misuse of unsafe.Pointer
./zsyscall_windows.go:3501:18: possible misuse of unsafe.Pointer
./zsyscall_windows.go:3524:17: possible misuse of unsafe.Pointer
$

From what I know, if you build your program or test with -race flag, and it runs without any crashes, then your code is safe.

I don't know enough about garbage collector and unsafe rules (and they change) to judge if go vet is wrong or not.

Leaving for unsafe experts @ianlancetaylor and @mdempsky to decide,

Thank you.

Alex

[danderson]

Yeah, any conversion from a raw uintptr to an unsafe.Pointer will anger go vet, because that's not safe to do if the uintptr represents a pointer into the Go heap. go vet can't prove that the uintptr is safe to use, so it flags this warning.

I'm not sure how to adjust the syscall definitions to not use the flagged pattern. Arguably it's fine to just say "go vet is imprecise, it's okay to ignore warnings if you're sure"... But it'd be nice to avoid the false positives if there's an easy API tweak that can make it happen.

[alexbrainman]

But it'd be nice to avoid the false positives

Agree.

if there's an easy API tweak that can make it happen.

Changing external API will break existing users. Perhaps it is OK price to pay in this situation. Perhaps there is better solution here.

And we should also fix other go vet warning if possible.

So I would like to hear from others before changing API.

Alex

[ericlagergren]

For others who visit this issue later: just in case this doesn't get fixed somehow, you can use the following to trick go vet into not flagging the conversion (for uintptrs that "point" to system memory only).

var fake windows.Pointer
*(*uintptr)(unsafe.Pointer(&fake)) = addr
return (*T)(unsafe.Pointer(fake))

[timothy-king]

Even though these are famous last words, I believe that this unsafe conversion is actually safe, because the uintptr isn't representing a pointer into the Go heap. It would be nice to be able to use LocalAlloc without triggering a false positive in go vet.

If these are truly false positives, one option would be to suppress this warning in go vet when mem is exactly the return result of windows.LocalAlloc.

[zx2c4]

False positive or not, https://golang.org/cl/299492 provides a much nicer API anyway. So let's just get this fixed up.

[zx2c4]

I've abandoned my CL, and probably this issue can be closed. Doesn't seem like this is worth fixing.

Also, LocalAlloc can return non-pointer handles depending on its arguments...

[danderson]

Yeah, fair 'nuf. The API that can return handles or pointers makes it a bit strange anyway, and it's easy to get around the vet check. Thanks for considering!