Golang x/sys/windows: some syscalls are creating dangling pointers

CL 653856 is an optimization that increases how often make slice results can be stored on the stack.

@dmitshur noticed that the x/sys/windows.TestBuildSecurityDescriptor test started failing on Windows after that CL was merged.

As a diagnostic step, I was able to get the test to pass again by forcing some make results in sys/windows/security_windows.go to be heap allocated in (*SECURITY_DESCRIPTOR).ToAbsolute.

I also then commented on the CL that from the Windows docs, it looked like the MakeAbsoluteSD Windows API was being used in a way that was creating dangling pointers:

It looks like MakeAbsoluteSD is an example of a syscall that asks the user pass in memory that is then filled in with the results, and it looks like the results end up containing pointers to each other for some of the filled in arguments:

https://learn.microsoft.com/en-us/windows/win32/api/securitybaseapi/nf-securitybaseapi-makeabsolutesd [...] Previously, this would all be heap memory, but with the change in this CL, it can be stack allocated memory.

I think that might mean there is a dangling pointer in absoluteSD pointing to the ToAbsolute stack after ToAbsolute returns, which might be why the test then fails.

Keith responded that there are probably other related problems as well, including in some of the related functions:

So it is allocating an object that contains pointers using a method that marks none of its fields as pointers. (And with a size which is Windows' idea of what the size is, which may or may not match what Go thinks the size is.)

Then it calls into Windows which is writing those pointer fields without any write barrier notification to the GC.

So once this thing returns, nothing is keeping the 4 things referenced from this struct alive. The next GC (or maybe the current GC, given the missing write barriers) will collect all the referenced objects, leading to dangling pointers.

That's even before this CL. After this CL things just fall apart more quickly 😊 [...] Some of the other functions in this area have similar problems, like (*SECURITY_DESCRIPTOR).SetDACL.

Opening this issue to help track the scope of the problem and resolution.

CC @randall77

Comment From: gopherbot

Change https://go.dev/cl/663355 mentions this issue: windows: fix dangling pointers in (*SECURITY_DESCRIPTOR).ToAbsolute

Comment From: gabyhelp

Related Code Changes

• cmd/compile: stack allocate variable-sized makeslice
• syscall: revert security_windows.go change of CL 201877

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

Comment From: gopherbot

Change https://go.dev/cl/663575 mentions this issue: windows: fix dangling pointers in (*SECURITY_DESCRIPTOR).Set{DACL,SACL,Owner,Group}

Comment From: dmitshur

CC @golang/windows.

Comment From: gopherbot

Change https://go.dev/cl/663795 mentions this issue: cmd/compile/internal/escape: add debug flag to disable stack allocate variable-sized makeslice

Comment From: thepudds

Related: https://go.dev/cl/663715.

Comment From: gopherbot

Change https://go.dev/cl/663715 mentions this issue: syscall: fix dangling pointers in Windows' process attributes

Comment From: thepudds

Hi @qmuntal, does this look like it could be a related bug, where pointer to buffer is stored via NtSetInformationFile?

    bufferSize := int(unsafe.Offsetof(dummyFileRenameInfo.FileName)) + fileNameLen
    buffer := make([]byte, bufferSize)
    typedBufferPtr := (*fileRenameInformation)(unsafe.Pointer(&buffer[0]))
    typedBufferPtr.ReplaceIfExists = windows.FILE_RENAME_REPLACE_IF_EXISTS | windows.FILE_RENAME_POSIX_SEMANTICS
    typedBufferPtr.FileNameLength = uint32(fileNameLen)
    copy((*[windows.MAX_LONG_PATH]uint16)(unsafe.Pointer(&typedBufferPtr.FileName[0]))[:fileNameLen/2:fileNameLen/2], newNameUTF16)
    err = windows.NtSetInformationFile(fileHandle, &iosb, &buffer[0], uint32(bufferSize), windows.FileRenameInformation)

(From x/sys/windows/syscall_windows_test.go).

If it is a bug, it's just affecting a test I think. (I only took a quick look, so maybe not a bug. I'll post more context here in a bit).

Comment From: thepudds

With https://go.dev/cl/663355 submitted as of a few minutes ago, the x/sys Windows builders now seem to have flipped back to green with that commit:

https://ci.chromium.org/p/golang/g/x-sys-gotip-by-go/console

Comment From: mknyszek

Hey @thepudds, in triage, we're wondering whether your landed fix is enough for the issue, or there needs to be more work done here. I see you've got 2 changes sent out, one merged, one WIP. Or maybe this is a broader problem with x/sys/windows? Thanks.

Comment From: thepudds

Hi @mknyszek, I would guess there are still some problems in x/sys/windows, including the possible issue ~3 comments back in https://github.com/golang/go/issues/73199#issuecomment-2787471345 that I don't know if anyone has opined on yet.

The problems that https://go.dev/cl/663575 is attempting to address I think are probably real.

Maybe nothing else pops up, but it's probably worth keeping this open for a bit longer. I was also trying out a stress test just now, though I don't know how fruitful that will be.

Comment From: qmuntal

Hi @qmuntal, does this look like it could be a related bug, where pointer to buffer is stored via NtSetInformationFile?

Hmm, the pointer to the buffer is not retained by NtSetInformationFile, and the buffer itself doesn't contain pointers to Go objects, so that call seems safe to me.

Comment From: gopherbot

Change https://go.dev/cl/666116 mentions this issue: cmd/compile: align stack-allocated backing stores higher than required

Comment From: randall77

Just to note here, the problem with NtSetInformationFile is one of alignment. It does its allocation using make([]byte, ...) but then casts it to something that needs higher alignment (a *fileRenameInformation). If the byte slice is very unaligned, accesses through that pointer will also be unaligned. Normally on amd64, not a problem. But this pointer gets passed in a syscall to a windows function that complains about alignment. It would also be a problem to use some instruction that required alignment (something in sync/atomic, or SSE instructions in assembly, ...). CL 666116 forces pointer-alignment for these stack-allocated backing stores, which better matches what malloc used to do. It won't be perfect, but it will probably fix a lot of potential pitfalls like this.

Comment From: randall77

The godoc for NtSetInformationFile describes its argument at just *byte, with no indication that it needs a larger alignment. The Windows docs do say it must be of a certain type, depending on the final parameter. We don't have any way in the Go type system to enforce that, unfortunately. It would be nice to have an example usage that doesn't have this alignment issue.

There might be a similar potential problem for SetFileInformationByHandle, DeviceIoControl, and GetFileInformationByHandleEx. Probably others.

It is particularly tricky for NtSetInformationFile because the size of the argument is variable length.

Comment From: alexbrainman

Here https://learn.microsoft.com/en-us/cpp/build/x64-calling-convention?view=msvc-170&viewFallbackFrom=vs-2017#alignment are the windows/amd64 alignment rules, if others need them.

I did not find anything interesting there.

Alex