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: os/exec, syscall: allow child processes to inherit all inheritable handles from the parent process #60942

Open
qmuntal opened this issue Jun 22, 2023 · 10 comments

Comments

@qmuntal
Copy link
Contributor

qmuntal commented Jun 22, 2023

On Windows, one must explicitly list the handles that will be inherited by a new process when calling syscall.StartProcess and os.StartProcess. This is problematic for applications that don't know which handles they have inherited from their parent process and want to spawn new child processes with the same inherited handles, as in #53652.

Windows does not provide an API to list all inherited handles, but suggest to use some form of process communication (source), so these applications are left behind without a solution other than reimplementing syscall.StartProcess.

In fact, before go1.17 we did support this use case, all inheritable handles were passed explicitly. #44011 changed syscall.StartProcess to use the more robust approach of passing them implicitly. What I'm proposing here is to allow users to opt into the old behavior by adding a new property to syscall.SysProcAttr property, named InheritAllInheritableHandles.

It would look like this:

type SysProcAttr struct {
        // ...
        // Existing props
	NoInheritHandles           bool                // if set, each inheritable handle in the calling process is not inherited by the new process
	AdditionalInheritedHandles []Handle            // a list of additional handles, already marked as inheritable, that will be inherited by the new process
	ParentProcess              Handle              // if non-zero, the new process regards the process given by this handle as its parent process, and AdditionalInheritedHandles, if set, should exist in this parent process
	// New props
	InheritAllInheritableHandles bool // if set and NoInheritHandles is unset, each inheritable handle in the calling process is inherited by the new process
}

If InheritAllInheritableHandles is set and NoInheritHandles is unset, then syscall.StartProcess would ignore AdditionalInheritedHandles and call the relevant windows APIs as before CL 288297. We could also fail in case both InheritAllInheritableHandles and NoInheritHandles are set, I'm still not sold on this.

Notice that handles passed in AdditionalInheritedHandles will still be inherited when InheritAllInheritableHandles is set, as they must be marked as inheritable to be usable in AdditionalInheritedHandles.

@golang/windows @alexbrainman @zx2c4

@bcmills
Copy link
Contributor

bcmills commented Jun 22, 2023

I don't really understand what the meaning of the NoInheritHandles field is if inheritable handles are not inherited by default. Its doc comment currently talks about handles that are not inherited, but that seems to be the current default behavior anyway.

Can we update the documentation for NoInheritHandles to describe which handles it actually affects? (That is, which handles would change from inherited to not because of that field being set?)

@qmuntal
Copy link
Contributor Author

qmuntal commented Jun 23, 2023

I don't really understand what the meaning of the NoInheritHandles field is if inheritable handles are not inherited by default. Its doc comment currently talks about handles that are not inherited, but that seems to be the current default behavior anyway.

The doc comment is outdated. syscall.StartProcess always adds stdin, stdout and stderr to the list of inherited handles regardless of the AdditionalInheritedHandles content. NoInheritHandles is a knob that disables handle inheritance, so in practice the only reason to set is to not inherit the standard handlers, other handlers are already not inherited unless they are added to AdditionalInheritedHandles.

Can we update the documentation for NoInheritHandles to describe which handles it actually affects? (That is, which handles would change from inherited to not because of that field being set?)

Yep, will submit a CL improving the doc comments.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/505515 mentions this issue: syscall: clarify which handles are affected by SysProcAttr.NoInheritHandles

@qmuntal
Copy link
Contributor Author

qmuntal commented Jun 23, 2023

Huh, I've just seen that pipes returned by os.Pipe are marked as inheritable since CL 288299, so applications using InheritAllInheritableHandles would inadvertently leak all file descriptors created using os.Pipe(). This is bad, because leaking a file descriptor can cause hangs or security issues when spawning malicious processes.

I see three options to overcome this issue:

  1. Add a warning to InheritAllInheritableHandles docs says that when used in a process that also calls os.Pipe(), all pipes that shouldn't be inherited should be marked an such, possibly using a lock that ensures that another thread doens't call syscall.StartProcess in between os.Pipe() and syscall.SetHandleInformation(h, syscall.HANDLE_FLAG_INHERIT, 0).
  2. Change os.Pipe() on Windows so the pipe returned is not inheritable, and find another way to solve os: Pipe can't be inherited on windows #21085.
  3. Don't accept this proposal.

I like the second one, if that's feasible and can mitigate possible breaking changes.

@bcmills
Copy link
Contributor

bcmills commented Jun 23, 2023

I am still very confused. os/exec by default directs Stdin, Stdout, and Stderr to os.DevNull anyway. Are the parent's os.Stdin, os.Stdout, and os.Stderr inherited by default even if they are not being used as the corresponding streams for the child process?

On the other hand, if the user explicitly sets cmd.Stdout = os.Stdout or similar, then it seems like it would be confusing for NoInheritHandles to undo that. Similarly, if the user calls cmd.StdoutPipe, then it would be confusing for NoInheritHandles to immediately break that pipe.

(exec.TestNoInheritHandles unfortunately doesn't shed any light on the situation, because it doesn't test the behavior with a child process with nontrivial I/O at all. 😵‍💫)


Perhaps NoInheritHandles should cause syscall.StartProcess to error out if attr.Files contains any handles that refer to files other than NUL?

Along those same lines, perhaps NoInheritHandles should be deprecated, and we should set bInheritHandles to false by default whenever attr.Files contains only NUL files?


It looks like the field was added in the first place in https://go.dev/cl/261917 for #42098, but I don't see any tests associated with that proposal or CL at all.

(CC @ianlancetaylor @JohanKnutzen).

@bcmills
Copy link
Contributor

bcmills commented Jun 23, 2023

https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessasuserw says (emphasis mine):

By default, passing TRUE as the value of the bInheritHandles parameter causes all inheritable handles to be inherited by the new process. This can be problematic for applications which create processes from multiple threads simultaneously yet desire each process to inherit different handles.

That problematic case seems like what essentially always happens in a Go program. In particular, it seems like allowing InheritAllInheritableHandles would be reintroducing a problem exactly analogous to the one that forces the use of syscall.ForkLock on Unix platforms.

...actually, come to think of it: it appears that syscall.ForkLock is still used in net.sysSocket on Windows, but since CL 288297 it is no longer used in syscall.StartProcess on Windows, so it currently has no effect except to cause a little extra cache contention in net.sysSocket.

If we were to add InheritAllInheritableHandles, would we also need to restore the use of syscall.ForkLock in syscall.StackProcess? (Or would we just accept and document that it is inherently racy if inheritable handles may be opened concurrently?)

@qmuntal
Copy link
Contributor Author

qmuntal commented Jun 23, 2023

I am still very confused. os/exec by default directs Stdin, Stdout, and Stderr to os.DevNull anyway. Are the parent's os.Stdin, os.Stdout, and os.Stderr inherited by default even if they are not being used as the corresponding streams for the child process?

Will investigate more next week. I'll have to run some tests to understand this.

...actually, come to think of it: it appears that syscall.ForkLock is still used in net.sysSocket on Windows, but since CL 288297 it is no longer used in syscall.StartProcess on Windows, so it currently has no effect except to cause lock contention in net.sysSocket.

In fact, it is no longer necessary to cover the case where WSA_FLAG_NO_HANDLE_INHERIT is not supported, as it supported since Windows 7, so the use of syscall.ForkLock can be removed. Therefore, InheritAllInheritableHandles wouldn't need to lock neither. Will submit a CL later with this cleanup (targeting go1.22).

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/506136 mentions this issue: net: remove sysSocket fallback for Windows 7

@qmuntal
Copy link
Contributor Author

qmuntal commented Jun 26, 2023

I am still very confused. os/exec by default directs Stdin, Stdout, and Stderr to os.DevNull anyway. Are the parent's os.Stdin, os.Stdout, and os.Stderr inherited by default even if they are not being used as the corresponding streams for the child process?

Got the answer: os/exec does set the standard handles to os.DevNull by default, and this is honored by the OS, child process can't access them.

On the other hand, if the user explicitly sets cmd.Stdout = os.Stdout or similar, then it seems like it would be confusing for NoInheritHandles to undo that. Similarly, if the user calls cmd.StdoutPipe, then it would be confusing for NoInheritHandles to immediately break that pipe.

That's correct. Setting NoInheritHandles makes child processes to not inherit any handle, even the standard ones explicitly set, e.g. using cmd.Stdout = os.Stdout.

Perhaps NoInheritHandles should cause syscall.StartProcess to error out if attr.Files contains any handles that refer to files other than NUL?
Along those same lines, perhaps NoInheritHandles should be deprecated, and we should set bInheritHandles to false by default whenever attr.Files contains only NUL files?

Agree with both, NoInheritHandles behavior can already be achieved by setting Stdin, Stdout, and Stderr to os.DevNull and not adding (or only adding os.DevNull) handles to AdditionalInheritedHandles, which is the os/exec default behavior. Therefore, deprecating NoInheritHandles will reduce the cognitive burden of understanding how to inherit handles.

gopherbot pushed a commit that referenced this issue Jun 27, 2023
…andles

SysProcAttr.NoInheritHandles doc comment is not clear about which
handles are affected by it. This CL clarifies that it not only affects
the ones passed in AdditionalInheritedHandles, but also the ones
passed in ProcAttr.Files, which are required to be stderr, stdin and
stdout when calling syscall.StartProcess.

Updates #60942

Change-Id: I5bc5b3604b6db04b83f6764d5c5ffbdafeeb22fb
Reviewed-on: https://go-review.googlesource.com/c/go/+/505515
Run-TryBot: Quim Muntal <quimmuntal@gmail.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@alexbrainman
Copy link
Member

so these applications are left behind without a solution other than reimplementing syscall.StartProcess.

Allowing process to inherit all its handles is looking for trouble in general. Perhaps this should be hard to do. Perhaps reimplementing syscall.StartProcess is better than allowing user to set new InheritAllInheritableHandles field without considering the consequences.

Agree with both, NoInheritHandles behavior can already be achieved by setting Stdin, Stdout, and Stderr to os.DevNull and not adding (or only adding os.DevNull) handles to AdditionalInheritedHandles, which is the os/exec default behavior. Therefore, deprecating NoInheritHandles will reduce the cognitive burden of understanding how to inherit handles.

Sounds reasonable to me.

Alex

bradfitz pushed a commit to tailscale/go that referenced this issue Jul 15, 2023
…andles

SysProcAttr.NoInheritHandles doc comment is not clear about which
handles are affected by it. This CL clarifies that it not only affects
the ones passed in AdditionalInheritedHandles, but also the ones
passed in ProcAttr.Files, which are required to be stderr, stdin and
stdout when calling syscall.StartProcess.

Updates golang#60942

Change-Id: I5bc5b3604b6db04b83f6764d5c5ffbdafeeb22fb
Reviewed-on: https://go-review.googlesource.com/c/go/+/505515
Run-TryBot: Quim Muntal <quimmuntal@gmail.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit that referenced this issue Jul 20, 2023
`syscall.ForkLock` is not used in `syscall.StartProcess` since
CL 288297, so using it in `sysSocket` makes no sense. This CL
goes a little bit further and removes the `sysSocket` fallback for
Windows 7, since it is not supported since go 1.21 (#57003).

Updates #60942

Change-Id: If14a0d94742f1b80af994f9f69938701ee41b402
Reviewed-on: https://go-review.googlesource.com/c/go/+/506136
Reviewed-by: Heschi Kreinick <heschi@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Quim Muntal <quimmuntal@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
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

4 participants