Skip to content

syscall: SyscallN always escapes the variadic argument #70197

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

Closed
qmuntal opened this issue Nov 5, 2024 · 10 comments
Closed

syscall: SyscallN always escapes the variadic argument #70197

qmuntal opened this issue Nov 5, 2024 · 10 comments
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Critical A critical problem that affects the availability or correctness of production systems built using Go NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Milestone

Comments

@qmuntal
Copy link
Member

qmuntal commented Nov 5, 2024

Go version

go version devel go1.24-bea9b91f0f Tue Nov 5 01:16:03 2024 +0000 windows/amd64

What did you do?

Run go build -gcflags=-m .

package main

import (
	"syscall"
	"unsafe"
)

var sink byte
var procProcessPrng = syscall.NewLazyDLL("bcryptprimitives.dll").NewProc("ProcessPrng").Addr()

func main() {
	var buf [10]byte
	syscall.SyscallN(procProcessPrng, uintptr(unsafe.Pointer(&buf[0])), uintptr(len(buf)))
	sink ^= buf[0]
}

What did you see happen?

The syscall.SyscallN variadic argument escaped to the heap.

./main.go:13:18: ... argument escapes to heap

What did you expect to see?

The syscall.SyscallN variadic argument didn't escape to the heap, just as other syscall.SyscalX functions.

./main.go:13:18: ... argument does not escape

Note that syscall.SyscallN is implemented by runtime.syscall_syscalln, which makes sure that the variadic argument doesn't escape:

c.args = uintptr(noescape(unsafe.Pointer(&args[0])))

@golang/compiler @golang/windows

@qmuntal qmuntal added OS-Windows NeedsFix The path to resolution is known, but the work has not been done. labels Nov 5, 2024
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Nov 5, 2024
@qmuntal
Copy link
Member Author

qmuntal commented Nov 5, 2024

This issue is blocking the use of syscall.SyscallN in x/sys/windows/mkwinsyscall, see CL 623821.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/625297 mentions this issue: syscall: mark SyscallN as noescape

@qmuntal
Copy link
Member Author

qmuntal commented Nov 5, 2024

@dmitshur @ianlancetaylor would it make sense to backport CL 623821 (if finally merged) to Go 1.22 and Go 1.23? This issue exists since the introduction of syscall.SyscallN in Go 1.17, but the alternatives (syscall.Syscall and friends) have been deprecated since then. I wouldn't want users to start switching back to the deprecated functions, as they are easier to misuse.

@dmitshur
Copy link
Contributor

dmitshur commented Nov 5, 2024

@qmuntal I'm a bit confused by the specifics in your question (maybe you got the wrong CL number mentioned? that one pulls in many x/sys CLs all at once), but in general, if there is a serious issue without a workaround and we can be confident the fix is narrow and safe, please create the backport issues with the rationale and we will consider it. See https://go.dev/wiki/MinorReleases#cherry-pick-cls-for-vendored-golangorgx-packages for details on a single x/sys CL can be backported to the vendored packages on 1.23/1.22 release branches.

@qmuntal
Copy link
Member Author

qmuntal commented Nov 5, 2024

I'm a bit confused by the specifics in your question (maybe you got the wrong CL number mentioned?

Wops, I linked the wrong CL. I was referring to 625297.

I'll create the backport issue, thanks for the pointer.

@qmuntal
Copy link
Member Author

qmuntal commented Nov 5, 2024

@gopherbot please open the backport tracking issues. syscall.SyscallN alternatives are deprecated, and there is the risk of users switching back to them if syscall.SyscallN is less performant.

@gopherbot
Copy link
Contributor

Backport issue(s) opened: #70201 (for 1.22), #70202 (for 1.23).

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

@qmuntal qmuntal self-assigned this Nov 5, 2024
@dmitshur dmitshur added this to the Go1.24 milestone Nov 5, 2024
@dmitshur dmitshur added the Critical A critical problem that affects the availability or correctness of production systems built using Go label Nov 20, 2024
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/630215 mentions this issue: [release-branch.go1.22]syscall: mark SyscallN as noescape

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/630196 mentions this issue: [release-branch.go1.23] syscall: mark SyscallN as noescape

gopherbot pushed a commit that referenced this issue Nov 20, 2024
syscall.SyscallN is implemented by runtime.syscall_syscalln, which makes
sure that the variadic argument doesn't escape.

There is no need to worry about the lifetime of the elements of the
variadic argument, as the compiler will keep them live until the
function returns.

For #70197
Fixes #70202

Change-Id: I12991f0be12062eea68f2b103fa0a794c1b527eb
Reviewed-on: https://go-review.googlesource.com/c/go/+/625297
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Reviewed-by: David Chase <drchase@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
(cherry picked from commit 7fff741)
Reviewed-on: https://go-review.googlesource.com/c/go/+/630196
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Quim Muntal <quimmuntal@gmail.com>
gopherbot pushed a commit that referenced this issue Nov 20, 2024
syscall.SyscallN is implemented by runtime.syscall_syscalln, which makes
sure that the variadic argument doesn't escape.

There is no need to worry about the lifetime of the elements of the
variadic argument, as the compiler will keep them live until the
function returns.

For #70197
Fixes #70201

Change-Id: I12991f0be12062eea68f2b103fa0a794c1b527eb
Reviewed-on: https://go-review.googlesource.com/c/go/+/625297
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Reviewed-by: David Chase <drchase@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
(cherry picked from commit 7fff741)
Reviewed-on: https://go-review.googlesource.com/c/go/+/630215
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Quim Muntal <quimmuntal@gmail.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
mpminardi pushed a commit to tailscale/go that referenced this issue Jan 28, 2025
syscall.SyscallN is implemented by runtime.syscall_syscalln, which makes
sure that the variadic argument doesn't escape.

There is no need to worry about the lifetime of the elements of the
variadic argument, as the compiler will keep them live until the
function returns.

For golang#70197
Fixes golang#70202

Change-Id: I12991f0be12062eea68f2b103fa0a794c1b527eb
Reviewed-on: https://go-review.googlesource.com/c/go/+/625297
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Reviewed-by: David Chase <drchase@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
(cherry picked from commit 7fff741)
Reviewed-on: https://go-review.googlesource.com/c/go/+/630196
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Quim Muntal <quimmuntal@gmail.com>
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. Critical A critical problem that affects the availability or correctness of production systems built using Go NeedsFix The path to resolution is known, but the work has not been done. OS-Windows
Projects
None yet
Development

No branches or pull requests

4 participants