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

@dmitshur

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

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

Related Code Changes

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

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

CC @golang/windows.

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

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

syscall: fix dangling pointers in Windows' process attributes

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

@qmuntal

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

windows: fix dangling pointers in (*SECURITY_DESCRIPTOR).ToAbsolute

@mknyszek

Hi @mknyszek, I would guess there are still some problems in x/sys/windows, including the possible issue ~3 comments back in #73199 (comment) 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.

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

cmd/compile/internal/escape: add hash for bisecting stack allocation …

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

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.

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.

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

cmd/compile: align stack-allocated backing stores higher than required

Navigation Menu

Repository navigation

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

7 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

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

Description

Activity

gopherbot commented on Apr 7, 2025

gabyhelp commented on Apr 7, 2025

gopherbot commented on Apr 7, 2025

dmitshur commented on Apr 7, 2025

gopherbot commented on Apr 8, 2025

thepudds commented on Apr 8, 2025

gopherbot commented on Apr 8, 2025

thepudds commented on Apr 8, 2025

7 remaining items

thepudds commented on Apr 10, 2025

qmuntal commented on Apr 10, 2025

gopherbot commented on Apr 17, 2025

randall77 commented on Apr 17, 2025

randall77 commented on Apr 17, 2025

alexbrainman commented on Apr 17, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions