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.

Sign up for GitHub

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

Jump to bottom

x/sys/windows: EnumProcesses cuts the PIDs buffer size #60223

Closed
roman-mazur opened this issue May 16, 2023 · 3 comments
Closed

x/sys/windows: EnumProcesses cuts the PIDs buffer size #60223

roman-mazur opened this issue May 16, 2023 · 3 comments
Assignees
@roman-mazur
Labels
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. OS-Windows
Milestone
Unreleased

Comments

@roman-mazur
Copy link

roman-mazur commented May 16, 2023 •
edited

Sorry if it's the wrong place to report, but I don't see another place to discuss an issue with golang.org/x/sys.

What version of Go are you using (go version)?

$ go version
go version go1.20.1 windows/amd64

Does this issue reproduce with the latest release?

Yes, it's a problem with a separate module.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
set GO111MODULE=
set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\rmazur\AppData\Local\go-build
set GOENV=C:\Users\rmazur\AppData\Roaming\go\env
set GOEXE=.exe
set GOEXPERIMENT=
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOINSECURE=
set GOMODCACHE=C:\Users\rmazur\go\pkg\mod
set GOOS=windows
set GOPATH=C:\Users\rmazur\go
set GOPROXY=https://proxy.golang.org,direct
set GOROOT=C:\Program Files\Go
set GOSUMDB=sum.golang.org
set GOTMPDIR=
set GOTOOLDIR=C:\Program Files\Go\pkg\tool\windows_amd64
set GOVCS=
set GOVERSION=go1.20.1
set GCCGO=gccgo
set GOAMD64=v1
set AR=ar
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOWORK=
set CGO_CFLAGS=-O2 -g
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-O2 -g
set CGO_FFLAGS=-O2 -g
set CGO_LDFLAGS=-O2 -g
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=C:\Users\rmazur\AppData\Local\Temp\go-build4014544235=/tmp/go-build -gno-record-gcc-switches
gdb --version: �[mGNU gdb (GDB) 10.2

What did you do?

Calling windows.EnumProcesses with a slice [1024]uint32 as the first argument does not result in passing 4096 as the size of the input array. Instead, the len() of the slice is used, cutting the size to 1024 bytes.

The code that generates the windows.EnumProcesses function does not take into account the discrepancy between the input slice type and what the system call expects ([]uint32 vs a byte array).

What did you expect to see?

windows.EnumProcesses should pass len(processIds)*4 as the input array size to the Windows system call.

What did you see instead?

len(processIds) is used instead:
https://github.com/golang/sys/blob/1911637744c199cdad74ee1ee74d19ce61e3d9fa/windows/zsyscall_windows.go#L3524
@roman-mazur roman-mazur changed the title golang.org/x/sys/windows: EnumProcesses cuts the PIDs buffer size x/sys/windows: EnumProcesses cuts the PIDs buffer size May 16, 2023
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label May 16, 2023
@gopherbot gopherbot added this to the Unreleased milestone May 16, 2023
@qmuntal qmuntal added OS-Windows NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 16, 2023
@qmuntal
Copy link
Contributor

qmuntal commented May 16, 2023 •
edited

Thanks for reporting this issue @roman-mazur.

mkwinsyscall does a best effort to generate correct wrappers, always expanding a Go slice to two arguments: a pointer to the slice followed by it's length. IMO this is mostly a sane assumption, e.g. it works for []byte and in more complex cases (see FormatMessage). Unfortunately, EnumProcesses expects the length in bytes, even though it fills a DWORD array. We should fix that.

Do you want to give it a try (see the Contribution Guide)? Else I'll pick it.

A good solution would be to redefine //sys EnumProcesses to generate an unexported function that accepts an explicit length, like this:

//sys	enumProcesses(processIds *uint32, nSize uint32, bytesReturned *uint32) (err error) = psapi.EnumProcesses

Then implement the EnumProcesses exported function in syscall_windows.go with the old parameters (processIds []uint32, bytesReturned *uint32), but calling enumProcesses instead with the appropriate length.

@golang/windows

@qmuntal qmuntal added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 16, 2023
@roman-mazur
Copy link
Author

I was thinking at first about adjusting the generated code to be len()*4 if it's a []uint32 slice, but the logic would not be simple.
I like your idea of treating EnumProcesses as an exception. Will try to do it a bit later this week.

roman-mazur added a commit to roman-mazur/golang-sys that referenced this issue May 17, 2023
@roman-mazur
windows: fix EnumProcesses to pass the correct array size
6546f23 
Implementation generated directly with mkwinsyscall has a wrong
assumption about the expected value for PIDs buffer size.

This change adds some small manual code that converts the input
slice length to the number of bytes of the array backing the slice.

A test is also added. It fails with the previous implementation.

Fixes golang/go#60223

Change-Id: I5e2414acb29c6c949e5e6acd328043f8a8883887
roman-mazur added a commit to roman-mazur/golang-sys that referenced this issue May 17, 2023
@roman-mazur
windows: fix EnumProcesses to pass the correct array size
312c143 
Implementation generated directly with mkwinsyscall has a wrong
assumption about the expected value for PIDs buffer size.

This change adds some small manual code that converts the input
slice length to the number of bytes of the array backing the slice.

A test is also added. It fails with the previous implementation.

Fixes golang/go#60223

Change-Id: I5e2414acb29c6c949e5e6acd328043f8a8883887
@gopherbot
Copy link

Change https://go.dev/cl/495995 mentions this issue: windows: fix EnumProcesses to pass the correct array size

roman-mazur added a commit to roman-mazur/golang-sys that referenced this issue May 18, 2023
@roman-mazur
windows: fix EnumProcesses to pass the correct array size
0f110b0 
Implementation generated directly with mkwinsyscall has a wrong
assumption about the expected value for PIDs buffer size.

This change adds some small manual code that converts the input
slice length to the number of bytes of the array backing the slice.

A test is also added. It fails with the previous implementation.

Fixes golang/go#60223

Change-Id: I5e2414acb29c6c949e5e6acd328043f8a8883887
roman-mazur added a commit to roman-mazur/golang-sys that referenced this issue May 18, 2023
@roman-mazur
windows: fix EnumProcesses to pass the correct array size
4d20601 
Implementation generated directly with mkwinsyscall has a wrong
assumption about the expected value for PIDs buffer size.

This change adds some small manual code that converts the input
slice length to the number of bytes of the array backing the slice.

A test is also added. It fails with the previous implementation.

Fixes golang/go#60223

Change-Id: I5e2414acb29c6c949e5e6acd328043f8a8883887
@roman-mazur roman-mazur mentioned this issue May 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@roman-mazur roman-mazur

Labels
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. OS-Windows
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
3 participants
@roman-mazur @qmuntal @gopherbot