Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

internal/poll: invalid uintptr conversion in call to windows.SetFileInformationByHandle #58933

Closed
gopherbot opened this issue Mar 8, 2023 · 13 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done. okay-after-rc1 Used by release team to mark a release-blocker issue as okay to resolve either before or after rc1 OS-Windows
Milestone

Comments

@gopherbot
Copy link

gopherbot commented Mar 8, 2023

#!watchflakes
post <- pkg == "os" && test == "TestChmod" && `The parameter is incorrect`

Issue created automatically to collect these failures.

Example (log):

--- FAIL: TestChmod (0.00s)
    os_test.go:1278: chmod C:\Users\gopher\AppData\Local\Temp\1\_Go_TestChmod2722485006 0666: chmod C:\Users\gopher\AppData\Local\Temp\1\_Go_TestChmod2722485006: The parameter is incorrect.

watchflakes

@gopherbot gopherbot added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 8, 2023
@gopherbot
Copy link
Author

Found new dashboard test flakes for:

#!watchflakes
post <- pkg == "os" && test == "TestChmod"
2023-03-07 22:05 windows-amd64-race go@99914db5 os.TestChmod (log)
--- FAIL: TestChmod (0.00s)
    os_test.go:1278: chmod C:\Users\gopher\AppData\Local\Temp\1\_Go_TestChmod2722485006 0666: chmod C:\Users\gopher\AppData\Local\Temp\1\_Go_TestChmod2722485006: The parameter is incorrect.

watchflakes

@bcmills bcmills changed the title os: TestChmod failures os: TestChmod failures with The parameter is incorrect. on Windows Mar 8, 2023
@bcmills bcmills added this to the Backlog milestone Mar 8, 2023
@bcmills
Copy link
Member

bcmills commented Mar 8, 2023

(CC @golang/windows)

@bcmills
Copy link
Member

bcmills commented Mar 8, 2023

This test was enabled on Windows in CL 250077 for #39606 (CC @iwdgo), and I marked it as safe to run in parallel in CL 458015.

It's not at all clear to me why it would fail nondeterministically, although it's curious that the one failure reported here is on the -race builder. 🤔

@alexbrainman
Copy link
Member

I just looked at the internal/poll.FD.Fchmod code again, and it does not look right to me. For example, why do we only set du.FileAttributes and not du.CreationTime and other fields here?

var du windows.FILE_BASIC_INFO
du.FileAttributes = attrs
l := uint32(unsafe.Sizeof(d))
return windows.SetFileInformationByHandle(fd.Sysfd, windows.FileBasicInfo, uintptr(unsafe.Pointer(&du)), l)

Our code appears to set CreationTime (and all time related fields) to 0. This is wrong. No?

We should extend os.TestChmod to compare file times before and after os.Chmod call.

Alex.

@gopherbot
Copy link
Author

Change https://go.dev/cl/490855 mentions this issue: os: set actual length of structure passed to SetFileInformationByHandle

@qmuntal
Copy link
Contributor

qmuntal commented May 2, 2023

I just looked at the internal/poll.FD.Fchmod code again, and it does not look right to me. For example, why do we only set du.FileAttributes and not du.CreationTime and other fields here?

FILE_BASIC_INFO does not document what happens when time fields are set to 0, but SetFileInformationByHandle calls NtSetInformationFile under the hood, which casts the basic info into FILE_BASIC_INFORMATION. This last struct does document what happens in this situation:

If you specify a value of zero for any of the XxxTime members of the FILE_BASIC_INFORMATION structure, the ZwSetInformationFile function keeps a file's current setting for that time.

We should extend os.TestChmod to compare file times before and after os.Chmod call.

Agree. If we rely on an undocumented behavior then we should at least test our assumptions.

@gopherbot

This comment was marked as off-topic.

@bcmills
Copy link
Member

bcmills commented Dec 12, 2023

Just saw one of these on a windows/386 LUCI TryBot:
https://ci.chromium.org/ui/p/golang/builders/try-workers/gotip-windows-386-test_only/b8761882490968913201/test-results?sortby=&groupby=

=== RUN   TestChmod
=== PAUSE TestChmod
=== CONT  TestChmod
    os_test.go:1294: chmod C:\b\s\w\ir\x\t\_Go_TestChmod1719170795 0666: chmod C:\b\s\w\ir\x\t\_Go_TestChmod1719170795: The parameter is incorrect.
--- FAIL: TestChmod (0.01s)

@bcmills
Copy link
Member

bcmills commented Dec 12, 2023

Oh, yeah, here's a bug, at least:
https://cs.opensource.google/go/go/+/master:src/internal/syscall/windows/syscall_windows.go;l=153;drc=693def151adff1af707d82d28f55dba81ceb08e1

The third argument violates the unsafe.Pointer rules by passing a pointer to a Go object as type uintptr.

That can cause the buffer to be garbage-collected and reused while the call is in progress, leading to arbitrary corruption of its contents.

@bcmills bcmills changed the title os: TestChmod failures with The parameter is incorrect. on Windows internal/poll: invalid uintptr conversion in call to windows.SetFileInformationByHandle Dec 12, 2023
@bcmills bcmills added compiler/runtime Issues related to the Go compiler and/or runtime. NeedsFix The path to resolution is known, but the work has not been done. labels Dec 12, 2023
@bcmills bcmills modified the milestones: Backlog, Go1.22 Dec 12, 2023
@bcmills bcmills self-assigned this Dec 12, 2023
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 12, 2023
@gopherbot
Copy link
Author

Change https://go.dev/cl/549256 mentions this issue: internal/syscall/windows: fix the signature of SetFileInformationByHandle

@bcmills bcmills added FixPending Issues that have a fix which has not yet been reviewed or submitted. okay-after-rc1 Used by release team to mark a release-blocker issue as okay to resolve either before or after rc1 labels Dec 12, 2023
ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
…ndle

Also fix its call site in internal/poll to pass the length of the
actual buffer instead of an unrelated variable, and update the
definition of FILE_BASIC_INFO to match the documented field types
and add padding that is empirically needed on the 386 architecture.

Passing a pointer to a Go-allocated buffer as type uintptr violates
the unsafe.Pointer conversion rules, which allow such a conversion
only in the call expression itself for a call to syscall.Syscall or
equivalent. That can allow the buffer to be corrupted arbitrarily if
the Go runtime happens to garbage-collect it while the call to
SetFileInformationByHandle is in progress.

The Microsoft documentation for SetFileInformationByHandle specifies
its third argument type as LPVOID, which corresponds to Go's
unsafe.Pointer, not uintptr.

Fixes golang#58933 (maybe).

Change-Id: If577b57adea9922f5fcca55e46030c703d8f035c
Cq-Include-Trybots: luci.golang.try:gotip-windows-amd64-longtest
Reviewed-on: https://go-review.googlesource.com/c/go/+/549256
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Than McIntosh <thanm@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Quim Muntal <quimmuntal@gmail.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
@bcmills
Copy link
Member

bcmills commented Feb 22, 2024

@gopherbot, please backport to Go 1.21. This was a use-after-free bug and could cause arbitrary heap corruption in programs that call SetFileInformationByHandle, and it also produces nondeterministic test failures on the release-branch.go1.21 builders.

@gopherbot
Copy link
Author

Backport issue(s) opened: #65882 (for 1.21).

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

@gopherbot
Copy link
Author

Change https://go.dev/cl/566155 mentions this issue: [release-branch.go1.21] internal/syscall/windows: fix the signature of SetFileInformationByHandle

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FixPending Issues that have a fix which has not yet been reviewed or submitted. NeedsFix The path to resolution is known, but the work has not been done. okay-after-rc1 Used by release team to mark a release-blocker issue as okay to resolve either before or after rc1 OS-Windows
Projects
Status: Done
Development

No branches or pull requests

4 participants