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

proposal: syscall: deprecate SysProcAttr.NoInheritHandles #61003

Open
qmuntal opened this issue Jun 26, 2023 · 2 comments
Open

proposal: syscall: deprecate SysProcAttr.NoInheritHandles #61003

qmuntal opened this issue Jun 26, 2023 · 2 comments

Comments

@qmuntal
Copy link
Contributor

qmuntal commented Jun 26, 2023

(Extracted from #60942 (comment) by @bcmills)

Background

On Windows, the NoInheritHandles property of syscall.SysProcAttr makes that child processes created via syscall.StartProcess (and the corresponding os and os/exec wrappers) to not inherit any handle, even those marked as inheritable.

This knob was added in #42098, in a time where all inheritable handles were implicitly inherited. A bit latter, #44011 and CL 288298 refactored how handles were inherited by child processes so that all inheritable handles that needed to be inherited would have to be explicitly listed as such.

With this last change, NoInheritHandles became obsolete and redundant, as one can achieve the same behavior by setting all standard handles (stored in ProcAttr.Files) to os.DevNull and not adding any valid handle into SysProcAttr.AdditionalInheritedHandles. Note that this is the os/exec default's behavior.

The new behavior can led to confusion, like in the following gist, where cmd.StdOut is not inherited even when explicitly marked as such.

cmd := exec.Command("./foo.exe")
cmd.Stdout = os.Stdout
cmd.SysProcAttr = &syscall.SysProcAttr{
	NoInheritHandles: false,
}
cmd.Run()

I've also seen the following redundant pattern in the wild:

cmd := exec.Command(os.Args[0], rebuiltArgs...)
cmd.Stdin = nil
cmd.Stdout = nil
cmd.Stderr = nil
cmd.SysProcAttr = &windows.SysProcAttr{
    NoInheritHandles: true,
}
cmd.Run()

Proposal

Given that NoInheritHandles is redundant and adds cognitive load to the os/exec API, mark it as deprecated and better document the alternative solution.

The next step after deprecating it would be to error out in case NoInheritHandles is true and a non-null handle is set to be inherited via the aforementioned APIs. I prefer to leave this for a future proposal, as doing so might break some currently valid usages.

@bcmills
Copy link
Contributor

bcmills commented Jun 26, 2023

(CC @golang/windows)

@alexbrainman
Copy link
Member

SGTM.

Alex

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Incoming
Development

No branches or pull requests

3 participants