@josharian is this a problem in tip too? (I assume it is). If that's the case, this issue should be put in the 1.19 milestone (to track fixing on tip), and the backport issue that you can ask gopherbot to open will be the one in the 1.18.1 milestone (to track the backporting).

@ALTree oh right. thanks.

@gopherbot please open a backport issue for Go 1.18. This is a compilation regression.

Backport issue(s) opened: #51728 (for 1.18).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

(This bug was minimized from https://github.com/cyolosecurity/certstore/blob/main/certstore_windows.go#L666)

/cc @ianlancetaylor

CL 350070 is based on the valid observation that although the type of Windows HANDLE is void*, the values stored in variables of type HANDLE are normally not pointers. Since Go's GC really wants to know whether something is a pointer or not, telling it that values of type HANDLE are not pointers seems like the right move in general.

Now, perhaps it's true that HLOCAL values happen to be pointers to memory. As far as I can tell the correct way to get an HLOCAL value is to call LocalHandle; I guess that's a pointer to memory? And it appears that new code is supposed to call HeapAlloc which doesn't have this problem.

Anyhow, if we are expected to convert Go pointers to HLOCAL and thus to HANDLE, we are in trouble, because now a HANDLE is sometimes a pointer to memory and sometimes not.

I don't see a problem with the code in cyolosecurity, and the code can easily be changed to compile by writing

	defer C.LocalFree(C.HLOCAL(unsafe.Pointer(cmsg)))

So it seems to me that the Go 1.18 behavior is safer and code that plays fast and loose with pointers should be adjusted. (Note that the Go 1 compatibility guarantee does not apply to cgo code.)

Of course it would be better if that code could continue to work, and I'm certainly open to suggestions.

I'll offer a bit more context here:

The LocalAlloc, LocalFree, GlobalAlloc, GlobalFree and their other adjacent APIs are holdovers from 16-bit Windows. In those days, when we allocated heap memory, those functions would truly return a handle. The programmer would have to then call LocalLock or GlobalLock to convert that handle into a pointer, and then call LocalFree or GlobalFree when done accessing the memory.

Nowadays, for better or worse, the HLOCAL handle is frequently treated as synonymous with the pointer. HGLOBALs often are as well (though my personal preference is not to do so).

In general, I'd suggest that the best rule of thumb is to avoid allocating memory using those functions unless other APIs involved specifically require it -- eg the Win32 clipboard APIs expect to work with HGLOBALs. If some some reason a block of memory must absolutely be allocated via the Windows heap, HeapAlloc should be the default API choice.

@dblohm7 Thanks.

It looks like the code that led to this issue is calling FormatMessage with the FORMAT_MESSAGE_ALLOCATE_BUFFER which does indeed allocate memory using LocalAlloc. It's then casually converting that memory to char*, which I guess is fine today but it should really be calling LocalLock, and should really be passing the original value to LocalFree.

So I think there is a plausible argument that the code in question is technically incorrect and this change caught the error.

OK, sounds like this working as intended. Thanks, @ianlancetaylor and @dblohm7.

Thanks, closing this issue as there currently doesn't seem to be anything to change in the Go tools. Please comment if you disagree.