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

[thepudds]

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

[gopherbot]

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

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

[gopherbot]

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

[dmitshur]

CC @golang/windows.

[gopherbot]

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

[thepudds]

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

[gopherbot]

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

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