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

danderson · 2021-03-07T01:12:21Z

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

The text was updated successfully, but these errors were encountered:

ericlagergren · 2021-03-07T03:31:24Z

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 · 2021-03-07T05:24:44Z

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 · 2021-03-07T06:12:27Z

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

networkimprov · 2021-03-07T06:17:06Z

cc @alexbrainman @zx2c4
@gopherbot add OS-Windows

ericlagergren · 2021-03-07T06:23:33Z

@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 · 2021-03-07T07:11:00Z

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 · 2021-03-07T14:43:42Z

I'll fix.

gopherbot · 2021-03-07T14:44:45Z

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

alexbrainman · 2021-03-08T01:57:18Z

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 · 2021-03-08T02:01:23Z

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 · 2021-03-08T02:27:48Z

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 · 2021-03-08T02:37:25Z

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 · 2021-03-10T19:44:42Z

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 · 2021-03-10T20:13:46Z

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

zx2c4 · 2021-03-20T17:10:30Z

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 · 2021-03-20T21:06:23Z

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!

gopherbot added this to the Unreleased milestone Mar 7, 2021

gopherbot added the OS-Windows label Mar 7, 2021

toothrot added the NeedsFix The path to resolution is known, but the work has not been done. label Mar 8, 2021

timothy-king added the Analysis Issues related to static analysis (vet, x/tools/go/analysis) label Mar 10, 2021

danderson closed this as completed Mar 20, 2021

golang locked and limited conversation to collaborators Mar 20, 2022

gopherbot added the FrozenDueToAge label Mar 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

danderson commented Mar 7, 2021

ericlagergren commented Mar 7, 2021

danderson commented Mar 7, 2021

tmthrgd commented Mar 7, 2021

networkimprov commented Mar 7, 2021

ericlagergren commented Mar 7, 2021 via email

danderson commented Mar 7, 2021

zx2c4 commented Mar 7, 2021

gopherbot commented Mar 7, 2021

alexbrainman commented Mar 8, 2021

danderson commented Mar 8, 2021

alexbrainman commented Mar 8, 2021

ericlagergren commented Mar 8, 2021 •

edited

timothy-king commented Mar 10, 2021

zx2c4 commented Mar 10, 2021

zx2c4 commented Mar 20, 2021

danderson commented Mar 20, 2021

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

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

Comments

danderson commented Mar 7, 2021

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

Does this issue reproduce with the latest release?

What did you do?

ericlagergren commented Mar 7, 2021

danderson commented Mar 7, 2021

tmthrgd commented Mar 7, 2021

networkimprov commented Mar 7, 2021

ericlagergren commented Mar 7, 2021 via email

danderson commented Mar 7, 2021

zx2c4 commented Mar 7, 2021

gopherbot commented Mar 7, 2021

alexbrainman commented Mar 8, 2021

danderson commented Mar 8, 2021

alexbrainman commented Mar 8, 2021

ericlagergren commented Mar 8, 2021 • edited

timothy-king commented Mar 10, 2021

zx2c4 commented Mar 10, 2021

zx2c4 commented Mar 20, 2021

danderson commented Mar 20, 2021

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

ericlagergren commented Mar 8, 2021 •

edited