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

gopherbot · 2023-03-08T18:18:13Z

#!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

The text was updated successfully, but these errors were encountered:

gopherbot · 2023-03-08T18:18:49Z

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 · 2023-03-08T20:25:15Z

(CC @golang/windows)

bcmills · 2023-03-08T20:32:36Z

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 · 2023-03-08T22:49:26Z

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?

go/src/internal/poll/fd_windows.go

Lines 1027 to 1030 in 618fb4a

    
           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 · 2023-05-01T14:24:33Z

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

qmuntal · 2023-05-02T13:32:02Z

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.

bcmills · 2023-12-12T21:07:55Z

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 · 2023-12-12T21:18:06Z

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.

gopherbot · 2023-12-12T21:38:17Z

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

…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 · 2024-02-22T19:16:46Z

@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 · 2024-02-22T19:17:09Z

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 · 2024-02-22T19:24:48Z

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

…f SetFileInformationByHandle 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 #65882. Updates #58933. Change-Id: If577b57adea9922f5fcca55e46030c703d8f035c Cq-Include-Trybots: luci.golang.try:go1.21-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> (cherry picked from commit a709724) Reviewed-on: https://go-review.googlesource.com/c/go/+/566155 Reviewed-by: Bryan Mills <bcmills@google.com>

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

bcmills changed the title ~~os: TestChmod failures~~ os: TestChmod failures with The parameter is incorrect. on Windows Mar 8, 2023

bcmills added the OS-Windows label Mar 8, 2023

bcmills added this to the Backlog milestone Mar 8, 2023

This comment was marked as off-topic.

Sign in to view

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 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 modified the milestones: Backlog, Go1.22 Dec 12, 2023

bcmills self-assigned this Dec 12, 2023

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

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

gopherbot closed this as completed in a709724 Dec 18, 2023

gopherbot mentioned this issue Feb 22, 2024

internal/poll: invalid uintptr conversion in call to windows.SetFileInformationByHandle [1.21 backport] #65882

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gopherbot commented Mar 8, 2023 •

edited by bcmills

gopherbot commented Mar 8, 2023

bcmills commented Mar 8, 2023

bcmills commented Mar 8, 2023

alexbrainman commented Mar 8, 2023

gopherbot commented May 1, 2023

qmuntal commented May 2, 2023

This comment was marked as off-topic.

bcmills commented Dec 12, 2023

bcmills commented Dec 12, 2023 •

edited

gopherbot commented Dec 12, 2023

bcmills commented Feb 22, 2024

gopherbot commented Feb 22, 2024

gopherbot commented Feb 22, 2024

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

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

Comments

gopherbot commented Mar 8, 2023 • edited by bcmills

gopherbot commented Mar 8, 2023

bcmills commented Mar 8, 2023

bcmills commented Mar 8, 2023

alexbrainman commented Mar 8, 2023

gopherbot commented May 1, 2023

qmuntal commented May 2, 2023

This comment was marked as off-topic.

bcmills commented Dec 12, 2023

bcmills commented Dec 12, 2023 • edited

gopherbot commented Dec 12, 2023

bcmills commented Feb 22, 2024

gopherbot commented Feb 22, 2024

gopherbot commented Feb 22, 2024

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

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

gopherbot commented Mar 8, 2023 •

edited by bcmills

bcmills commented Dec 12, 2023 •

edited