os/exec: No longer possible to pass all inheritable file handles to subprocess on Windows

[brianryner8]

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

Does this issue reproduce with the latest release? not tested, but there are no recent changes related to this

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

What did you do?

Spun off from #48393 @alexbrainman

This commit (@zx2c4 ) changed process creation on Windows so that only stdin/stdout/stderr, and the handles listed in AdditionalInheritedHandles, can be inherited by subprocesses.

Unfortunately, this interacts badly with a python wrapper tool we use. This wrapper sets up a virtual environment with all necessary packages, then simply shells out to the Python interpreter.

Some callers rely on being able to pass a file handle (i.e. a pipe) through to the Python script. This works fine if Python is invoked directly, but with this change to the golang implementation, our wrapper no longer passes these handles through to the python subprocess.

As the wrapper has no specific knowledge of the file handles that the caller wants to pass through, we can't just add them to AdditionalInheritedHandles.

What did you expect to see?

It would be nice if we could opt back into the old behavior in some way, using os/exec.

What did you see instead?

There doesn't seem to be a way to go back to the old behavior, other than by making all of the low-level Windows calls in our own code.

Thanks,

[alexbrainman]

@brianryner8 thank you for creating the issue.

Can you please provide a small self runnable example that worked before go 1.17.10 but does not work now?

Thank you.

Alex

[brianryner8]

The sample in #48393 demonstrates the issue. The only difference for us is that rather than creating the pipe in our Go application, it is passed as an inheritable handle when our Go binary is executed.

[alexbrainman]

The sample in #48393 demonstrates the issue.

I don't see a problem with that code. That code just need to use AdditionalInheritedHandles now. Go 1.17 added AdditionalInheritedHandles, and now everyone have to use it if you want to pass handles between processes.

Alex

[riannucci]

Completely setting aside that this change breaks go1compat (which, imo, should be sufficient reason BY ITSELF to change the default behavior back to the previous behavior), the problem in this case is that the Go program here does not know which handles should be propagated and so has no easy way to set AdditionalInheritedHandles.

The flow goes:

• parent process (which we don't control), marks some HANDLES as inheritable, and invokes "python" with some python script (which we also don't control).
• our Go binary (which is in PATH as "python") does some setup (creation of a virtualenv) and then exec's (or as close ase we can get on Windows) into the real python interpreter.
• the python interpreter runs, running the user-controlled python script, which expects the HANDLEs to be set the way the parent process set them.

In Go 1.16, this worked fine. In Go 1.17 we have to resort to duplicating a sizable chunk of exec_windows.go (https://chromium-review.googlesource.com/c/infra/luci/luci-go/+/3739699).

It may be a reasonable compromise to provide a function which the program can call that populates AdditionalInheritedHandles with any Handles that would have been present in the older version (though, again, this goes against go1compat... but at least it wouldn't leave users high and dry).

[alexbrainman]

@riannucci thanks for your comments.

Completely setting aside that this change breaks go1compat

Your program assumed that syscall.StartProcess will pass all inheritable handles to the new process, but I consider that is a bug in Go 1.16. I will let Go team resolve our differences.

the problem in this case is that the Go program here does not know which handles should be propagated and so has no easy way to set AdditionalInheritedHandles.

I agree. Your program design assumes that inheritable handles will by passed to the child process.

In Go 1.16, this worked fine. In Go 1.17 we have to resort to duplicating a sizable chunk of exec_windows.go (https://chromium-review.googlesource.com/c/infra/luci/luci-go/+/3739699).

I am sorry you had to add extra code to your program. But perhaps it is a good enough solution here. You were the first to complain about this change. Perhaps Go need to provide some general solution for users like yourself. I don't know.

It may be a reasonable compromise to provide a function which the program can call that populates AdditionalInheritedHandles with any Handles that would have been present in the older version

I don't know of a way to enumerate all inheritable process handles. Perhaps others have some suggestions.

Alex

[gazerro]

@riannucci

Completely setting aside that this change breaks go1compat

There are no guarantees on the syscall package:

It is impossible to guarantee long-term compatibility with operating system interfaces, which are changed by outside parties. The syscall package is therefore outside the purview of the guarantees made here.

[brianryner8]

It is impossible to guarantee long-term compatibility with operating system interfaces, which are changed by outside parties. The syscall package is therefore outside the purview of the guarantees made here.

While it's certainly true that OS interfaces are changed by outside parties,

1. it doesn't appear that this change to syscall behavior was prompted by a breaking change to the underlying Windows API
2. the change surfaces as a behavior change to the os/exec package (i.e. Cmd.Start/Cmd.Run), which would be part of the go1compat guarantee, no?

[qmuntal]

I'm not aware of any reasonable way to retrieve all the process inherited handles, therefore neither os nor syscall currently provide a way to support the flow described by @riannucci in #53652 (comment). IMO this use case is very valid, we should provide a way to enable it.

My proposal is to opt-in to the old behavior, that is having a global lock that guards process creation and don't explicitly specify the list of handles to be inherited by the process. Users could opt-in to this behavior by setting a syscall.SysProcAttr property, named InheritAllInheritableHandles:

type SysProcAttr struct {
        // ...
	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!
	InheritAllInheritableHandles bool // if set and NoInheritHandles is unset, each inheritable handle in the calling process is inherited by the new process
}

I'll submit a new proposal issue as this is adding a new API.