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

os: Pipe can't be inherited on windows #21085

Closed
zombiezen opened this issue Jul 19, 2017 · 22 comments · Fixed by mattermost/mattermost#7177
Closed

os: Pipe can't be inherited on windows #21085

zombiezen opened this issue Jul 19, 2017 · 22 comments · Fixed by mattermost/mattermost#7177
Labels
FeatureRequest FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Milestone

Comments

@zombiezen
Copy link
Contributor

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

go version go1.8.3 windows/amd64

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

set GOARCH=amd64
set GOBIN=
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\Users\light\go
set GORACE=
set GOROOT=C:\Go
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0
set CXX=g++
set CGO_ENABLED=1
set PKG_CONFIG=pkg-config
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2

What did you do?

https://play.golang.org/p/r5V40AgLht

What did you expect to see?

Parent process
Child process
Hello, World!

I would like to be able to pass an unnamed piped into a subprocess. According to MSDN docs, this seems like something that Windows is capable of, but it is done using handles, not file descriptors IIUC.

What did you see instead?

Parent process
fork/exec C:\Users\light\AppData\Local\Temp\2\go-build579847834\command-line-arguments\_obj\exe\foo.exe: not supported by windows
exit status 1
@ianlancetaylor
Copy link
Contributor

I don't see how to implement this using the os/exec API, because I don't see how the child process can discover the handle. Your program is assuming that file descriptor 3 corresponds to the file passed in the ExtraFiles field. That is how ExtraFiles is designed to work, but while that works on Unix, it does not work on Windows. To implement this we would have to somehow tell the child process that file descriptor 3 should map to some handle, but that seems difficult to do in general.

@zombiezen
Copy link
Contributor Author

I'm not exactly sure about discovery either. I'm more just observing that this is how os/exec says that things will occur, without much of an indication of how it works on Windows. In particular, from Cmd.ExtraFiles doc:

ExtraFiles specifies additional open files to be inherited by the new process. It does not include standard input, standard output, or standard error. If non-nil, entry i becomes file descriptor 3+i.

It seems unfortunate to be unable to send pipes to subprocesses on Windows. I think we should address that somehow.

@ianlancetaylor
Copy link
Contributor

I believe you can do it today by calling the Fd method to get the handle, calling SetHandleInformation(handle, HANDLE_FLAG_INHERIT, 1) to let it stay open in the child, and then passing the handle to the child as an argument. For the os/exec package, it's the last step that is the tricky part. We could invent some way to pass the files to a Go program, but I don't know of any general purpose mechanism that would work for a non-Go program. As far as I know the program basically has to receive the handle number in a command line argument or environment variable.

@alexbrainman
Copy link
Member

Unfortunately what Ian said is correct. os/exec.Cmd.ExtraFiles does not work on Windows. We use Windows CreateProcess API to start new process. CreateProcess accepts STARTUPINFO struct that has only hStdInput, hStdOutput and hStdError fields. You can pass any file handle from one process to the other (just like on Unix), but you need some way for processes to exchange file handle numbers (stdio or TCP or ...). We could try and do something with os/exec.Cmd.ExtraFiles on Windows, but the problem is the child process - there is no general mechanism to pass that information between processes. Windows file handle do not have specific numbers, for example stdin is not 0, stdout is not 1 and so on.

I believe you can do it today by calling the Fd method to get the handle, calling SetHandleInformation(handle, HANDLE_FLAG_INHERIT, 1) to let it stay open in the child, and then passing the handle to the child as an argument.

Normally you would use DuplicateHandle API to do that. It provides everything you might need just for that job.

Alex

@zombiezen
Copy link
Contributor Author

zombiezen commented Aug 8, 2017

Makes sense. I'd like to send a doc fix to Cmd.ExtraFiles to summarize this discussion.

One more question though: if SetHandleInformation enables inheriting handles, would it make sense to amend the behavior of ExtraFiles on Windows to set this option on all the file handles in the ExtraFiles slice? I understand that there's still an out-of-band mechanism that's required to pass the individual handles, but in some ways, ExtraFiles already requires some arrangement with the child process on Unix systems. Clearly, this would need to be communicated in the doc comment how this works, but there's already difference between Windows and Unixy behavior for this field.

@alexbrainman
Copy link
Member

if SetHandleInformation enables inheriting handles, would it make sense to amend the behavior of ExtraFiles on Windows to set this option on all the file handles in the ExtraFiles slice?

I don't think it is that simple. We use CreateProcess Windows API to start new process. CreateProcess has bInheritHandles parameter that controls which handles are inherited by the child process. Go syscall package sets bInheritHandles to true because it needs to pass stdin, stdout and stderr. But this makes all inheritable handles escaped into child process (see the doco https://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx : "... If this parameter TRUE, each inheritable handle in the calling process is inherited by the new process. ..."). We cannot allow that, so none of file handles inside Go are inheritable. We only make stdin, stdout and stderr for new child inheritable, just before we launch new process. We also have a lock that prevents us from running CreateProcess simultaneously, otherwise this would allow these 3 handles (potentially) end up in the wrong process.

What you are proposing - calling SetHandleInformation - will make that handle inheritable, and it can possibly escape into wrong process. We could use DuplicateHandle API instead (just before we launch the process), and then close new handle immediately. But DuplicateHandle will create new file handle with new handle number. How are you going to pass that number to the user of Cmd.ExtraFiles?

ExtraFiles already requires some arrangement with the child process on Unix systems

Not true. I could be wrong because I do not know much about Unix, but whatever file is in ExtraFiles[0] will have file handle of 3 in the child process, ExtraFiles[1] will be 4, and so on. So everyone knows how it works, and you don't need to pass these numbers around.

Alex

@mattn
Copy link
Member

mattn commented Aug 9, 2017

This is simple implementation that passing socket into external process which works on Windows.

https://github.com/mattn/gospel

You can test this like below.

gospel echo-server

This pass the socket handle via environment variable GOSPEL_FD (in really, it's not handle, but file). To accept using the handle for the target process (limited), you should use DuplicateHandle.

@alexbrainman
Copy link
Member

https://github.com/mattn/gospel

This actually uses WSADuplicateSocket and WSAStartup to pass network socket between processes. These functions use WSAPROTOCOL_INFO struct (instead of file handles) to do the job.

Alex

@ccbrown
Copy link

ccbrown commented Aug 12, 2017

@alexbrainman

What you are proposing - calling SetHandleInformation - will make that handle inheritable, and it can possibly escape into wrong process.

Seems to me like if you use SetHandleInformation to make the handle inheritable, create the process, then use SetHandleInformation to make the handle uninheritable again, all while holding ForkLock, you wouldn't have this risk. Am I missing something?

Then you could just pass the same handles that you gave to ExtraFiles to the child process via arguments, environment variables, etc.

ExtraFiles already requires some arrangement with the child process on Unix systems

Not true. I could be wrong because I do not know much about Unix, but whatever file is in ExtraFiles[0] will have file handle of 3 in the child process, ExtraFiles[1] will be 4, and so on. So everyone knows how it works, and you don't need to pass these numbers around.

I would say there's still an out-of-band arrangement between the processes, it can just usually be made at compile-time for Unix since the file descriptors are predictable. But that's neither here nor there. This field already acts in non-portable ways, so surely allowing the behavior to be "on Windows, file descriptors are unchanged, and on Unix, entry i becomes file descriptor 3+i" would be reasonable.

@alexbrainman
Copy link
Member

Seems to me like if you use SetHandleInformation to make the handle inheritable, create the process, then use SetHandleInformation to make the handle uninheritable again, all while holding ForkLock, you wouldn't have this risk. Am I missing something?

I think what you are suggesting should work.

surely allowing the behavior to be "on Windows, file descriptors are unchanged, and on Unix, entry i becomes file descriptor 3+i" would be reasonable.

Sure we could do that. Perhaps we should say more than "file descriptors are unchanged". It is very important that child deals with these file handles - at least close them, if it won't use them. Otherwise unpredictable things will happen.

Alex

@glasser
Copy link
Contributor

glasser commented Feb 26, 2018

I am by no means an expert on Windows, but it looks like an answer involves the _get_osfhandle system call.

I'm trying to spawn a Go binary from a Node program with an extra file. I found that Node supports this (parent and child) and that it called the _get_osfhandle syscall in the child (passing, say, 3 as an argument) to translate into something that works with underlying calls. Turned out doing the same in Go works OK too:

	// Strangely, it always returns non-nil error, so we ignore it. If we got an
	// error, the next step (writing to the fd) will fail anyway.
	value, _, _ := syscall.NewLazyDLL("msvcrt").NewProc("_get_osfhandle").Call(fd)
	return uintptr(value)

Calling this with fd = 3 returns something that can be passed to os.NewFile and seems to work properly, at least when the pipe is set up by Node.

I don't know if this will lead to a more generic solution for os/exec, but maybe this will help others dealing with this issue!

@alexbrainman
Copy link
Member

I found that Node supports this (parent and child) and that it called the _get_osfhandle syscall in the child (passing, say, 3 as an argument) to translate into something that works with underlying calls.

Yes, like I said before, if parent and child agree on how to pass file handles, that should work.

syscall.NewLazyDLL("msvcrt").NewProc("_get_osfhandle").Call(fd)

Yes that should work. Unless some computers do not have msvcrt.dll.

Alex

@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 30, 2018
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Mar 30, 2018
@zhangyoufu
Copy link
Contributor

It is possible to pass file/pipe handles and some internal flags to child process using STARTUPINFO.lpReserved2. This is an undocumented feature inside MSVC C runtime. I have an unbuffer.py to demonstrate how to unbuffer stdout of child process using this idea. Maybe we can mimic this undocumented feature and implement ExtraFiles on Windows.

@alexbrainman
Copy link
Member

Maybe we can mimic this undocumented feature and implement ExtraFiles on Windows.

I don't see how that is possible. How would that work?

Alex

@tv42
Copy link

tv42 commented Feb 15, 2019

Your program is assuming that file descriptor 3 corresponds to the file passed in the ExtraFiles field. That is how ExtraFiles is designed to work, but while that works on Unix, it does not work on Windows.

What's _get_osfhandle, then?
https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/get-osfhandle?view=vs-2017

I haven't even attempted to read a significant fraction of Chrome's source code, and I'm definitely not a Windows developer, but.. Chrome's --remote-debugging-pipe flag seems to use fds 3 and 4 to communicate with the parent process, and it seems like it's implementation uses _get_osfhandle on Windows.

https://cs.chromium.org/search/?q=_get_osfhandle&type=cs

@tv42
Copy link

tv42 commented Feb 15, 2019

Here's evidence of a node.js program happily passing fds 3 & 4 to a chrome --remote-debugging-pipe subprocess, and this project claims to work on Windows: https://github.com/GoogleChrome/puppeteer/blob/7001042f796426163f13ca9716408578f1118bf3/lib/Launcher.js#L124-L135

@alexbrainman
Copy link
Member

What's _get_osfhandle, then?

I do not know.

Alex

@joesis
Copy link

joesis commented Aug 26, 2019

Came across this when tackling a similar issue. In short, starting Windows Vista, STARTUPINFOEX is supported which adds lpAttributeList to pass handles.

@gopherbot
Copy link

Change https://golang.org/cl/288272 mentions this issue: syscall: introduce SysProcAttr.ProcThreadAttributeList

@gopherbot
Copy link

Change https://golang.org/cl/288298 mentions this issue: syscall: introduce SysProcAttr.AdditionalInheritedHandles on Windows

@gopherbot
Copy link

Change https://golang.org/cl/288299 mentions this issue: os: mark pipes returned by os.Pipe() as inheritable by default

gopherbot pushed a commit that referenced this issue Feb 26, 2021
This allows users to specify handles that they explicitly want to be
inherited by the new process. These handles must already be marked as
inheritable.

Updates #44011.
Updates #21085.

Change-Id: Ib18322e7dc2909e68c4209e80385492804fa15d3
Reviewed-on: https://go-review.googlesource.com/c/go/+/288298
Trust: Jason A. Donenfeld <Jason@zx2c4.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
@ZekeLu
Copy link
Contributor

ZekeLu commented Jun 11, 2021

@zx2c4 This issue was closed by https://golang.org/cl/288299, I think these lines should be removed too:

go/src/os/exec/exec.go

Lines 121 to 122 in 770f1de

//
// ExtraFiles is not supported on Windows.

if len(attr.Files) > 3 {
return 0, 0, EWINDOWS
}

But it seems that the CL only takes care of the parent process. It's still not clear how to make it work for the child process. I have tried the demo in the OP (https://play.golang.org/p/r5V40AgLht) with these changes, and the output is:

Parent process
Child process
read <pipe>: The handle is invalid.
exit status 1

Then I modified the demo as below, and it works.

 func child() error {
    fmt.Fprintln(os.Stderr, "Child process")
-   r := os.NewFile(3, "<pipe>")
+   r := os.NewFile(os.Stderr.Fd()+4, "<pipe>")
    _, err := io.Copy(os.Stdout, r)
    return err
 }

I understand that this is not a reliable way to get the fd in the child process, I just want to prove that the CL works for the parent process somehow.

Then I tried to use the ExtraFiles to communicate with Chrome (with the --remote-debugging-pipe flag). But it did not work. As pointed out in the previous comments, Chrome use _get_osfhandle to get the handle on Windows (I'm not familiar with the source code of Chrome, but I think it's here). Maybe we should change the Go implementation to support _get_osfhandle? If we do this, we will have a reliable way to get fd in the child process.

FWIW, some of the tests in os/exec/exec_test.go are still skipped on Windows. So there is something more to do here. Should we reopen this issue or file a new one?

ZekeLu added a commit to ZekeLu/chromedp that referenced this issue Jun 26, 2021
Note that pipe connection is just supported on non-windows as of
now. see golang/go#21085.
ZekeLu added a commit to ZekeLu/chromedp that referenced this issue Jun 26, 2021
Note that pipe connection is just supported on non-windows as of
now. see golang/go#21085.
ZekeLu added a commit to ZekeLu/chromedp that referenced this issue Jul 13, 2021
Note that pipe connection is just supported on non-windows as of
now. see golang/go#21085.
@golang golang locked and limited conversation to collaborators Jun 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Projects
None yet
Development

Successfully merging a pull request may close this issue.