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

syscall: permit setting bInheritHandles to false when calling CreateProcess on Windows #36550

Closed
zhaoya881010 opened this issue Jan 14, 2020 · 21 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows
Milestone

Comments

@zhaoya881010
Copy link

How to set this function so that the child process does not inherit the parent process fds?
CreateProcess args:bInheritHandles on windows,subprocess.popen args close_fds by python on linux.
Why doesn't os.Startprocess provide interface.

In some cases, when the main process exits and the exec process is always there, the socket will not be released.

@ianlancetaylor ianlancetaylor changed the title os.startprocess fds inherit os: open file descriptors inherited by child processes on Windows Jan 14, 2020
@ianlancetaylor
Copy link
Contributor

On Unix systems we mark all open files as close-on-exec, meaning that they are automatically closed when starting a new child process. Are we not doing that on Windows? Does Windows have any similar functionality?

Certainly if at all possible we should be doing this in the standard library rather than requiring the program to use some interface to list the descriptors that should be closed.

CC @alexbrainman

@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. OS-Windows labels Jan 14, 2020
@ianlancetaylor ianlancetaylor added this to the Go1.15 milestone Jan 14, 2020
@zhaoya881010
Copy link
Author

zhaoya881010 commented Jan 14, 2020

@ianlancetaylor In the implementation of os.startprocess, binherithandles of CreateProcess is always true. I hope it can be configured in sysprocatt.
the socket from c lib,go call c lib. go main terminated unexpectedly. if other child process is still alive. Socket does not release. I hope that the child process will not inherit it.refer to python subprocess.popen for specific implementation.

@networkimprov
Copy link

cc @zx2c4 @mattn

@mattn
Copy link
Member

mattn commented Jan 15, 2020

As far as I looked some code of os package in short time, os.Startprocess take ProcAttr in third argument. ProcAttr have Files field to pass handles to child process. Do you mean that you don't want that this files is duplicated with DuplicateHandle always?

@zhaoya881010
Copy link
Author

@mattn I can't get the fds, so I hope that the child process doesn't need to inherit the fds of the parent process,Win provides the binherithandles property on CreateProcess API. i not found linux api property. consider that all FDS should be closed before fork exec, except 0 1 2 fd.

@zhaoya881010
Copy link
Author

this is python subprocess.popen for linux:
code:
......
def _close_fds(self, but):
if hasattr(os, 'closerange'):
os.closerange(3, but)
os.closerange(but + 1, MAXFD)
else:
for i in xrange(3, MAXFD):
if i == but:
continue
try:
os.close(i)
except:
pass
......

@alexbrainman
Copy link
Member

Certainly if at all possible we should be doing this in the standard library rather than requiring the program to use some interface to list the descriptors that should be closed.

I believe normal Go program does not leave any parent process file handles opened in child process. Except 3 handles of child stdin, stdout and stderr.

CreateProcess is always true

Yes. CreateProcess bInheritHandles is always set to true by Go. But that is not enough to make a process handle inherited by a child. From

https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-createprocessa

If this parameter is TRUE, each inheritable handle in the calling process is inherited by the new process.

So only inheritable handles are inherited by child process. And Go does not have any inheritable handles. Except the 3 I described above.

@zhaoya881010 do you actually have some problem to show us? Or you just read source code and think that the code is wrong?

Alex

@zhaoya881010
Copy link
Author

zhaoya881010 commented Jan 15, 2020

@alex
My main program is based on golang, the socket creation is based on c ++ in the lib library, the main program call lib and need to start another process A. process A will not exit. When the main program exited, the socket was not released because it was inherited by A.
The program starts again and the socket is occupied.

@zhaoya881010
Copy link
Author

socket handles are inheritable by default

@ianlancetaylor
Copy link
Contributor

@alexbrainman Thanks for the details.

@zhaoya881010 Socket handles are inheritable by default, but sockets created by Go programs are always created with WSA_FLAG_NO_HANDLE_INHERIT. If you create sockets in C, then you are responsible for either creating them with that flag, or explicitly closing them before creating a child process. For what it's worth, the same is true on Unix systems.

I'm going to close this because it sounds like things are working as expected. Please comment if you disagree.

@mattn
Copy link
Member

mattn commented Jan 15, 2020

FYI, As far as I know, socket handles are process-specific-resources on Windows. So when the parent process exits, the socket is closed always. Also you can't pass the handle to another process without duplicating. To duplicate socket handle, you need to call WSADuplicateSocket.

https://docs.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-wsaduplicatesocketa

If WSADuplicateSocket is called, WSAPROTOCOL_INFOA can be extracted as byte data (ex file). Now the server process can close the socket and exit. Then child process can read the bytes from the file, and make socket handle from the WSAPROTOCOL_INFOA with WSASocketA.

https://docs.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-wsasocketa

Below is an example code to do it.

https://github.com/mattn/gospel/blob/master/fd_windows.go#L13-L74

@zhaoya881010
Copy link
Author

zhaoya881010 commented Jan 17, 2020

@ianlancetaylor @mattn @alexbrainman
This is my program flow:

  1. run gomain-->call cgo(listing socket)
  2. gomain-->fork/exec--sleep 10000s
  3. gomain-->exit-->call cgo(close socket)
  4. sleep -->inherit socket
  5. run gomain-->cgo(listing socket failed)

Question:

  1. cgo does not expose fd so there is no way to set FD_CLOEXEC.Pushing third parties to change dll libraries is more difficult because they are no problem.
  2. It is also more difficult to actively close the socket in the exec program. Because it is a process like sleep, I cannot modify it.
    Is there any best solution?I hope to actively close useless fd when fork child in os.Startprocess.

@ianlancetaylor
Copy link
Contributor

It seems a bit sloppy for a library to allocate a socket but not provide any way for you to set HANDLE_FLAG_INHERIT to 0. That seems like it would be a problem for many different users of the library, not just Go programs.

Right now, syscall.StartProcess requires that exactly three files be passed to the child process. If we start a process on Windows with bInheritHandles set to false, what will standard input, output, and error be?

If we can resolve that, I guess it would be OK with me if we added a field to syscall.SysProcAttr to request calling CreateProcess with bInheritHandles set to false. Of course in that case it would be an error if there was anything in the SysProcAttr.Files field.

@ianlancetaylor ianlancetaylor changed the title os: open file descriptors inherited by child processes on Windows syscall: permit setting bInheritHandles to false when calling CreateProcess on Windows Jan 17, 2020
@alexbrainman
Copy link
Member

Right now, syscall.StartProcess requires that exactly three files be passed to the child process. If we start a process on Windows with bInheritHandles set to false, what will standard input, output, and error be?

From STARTUPINFOW doco

https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/ns-processthreadsapi-startupinfow

hStdInput

If dwFlags specifies STARTF_USESTDHANDLES, this member is the standard input handle for the process. If STARTF_USESTDHANDLES is not specified, the default for standard input is the keyboard buffer.

and

STARTF_USESTDHANDLES 0x00000100 The hStdInput, hStdOutput, and hStdError members contain additional information.If this flag is specified when calling one of the process creation functions, the handles must be inheritable and the function's bInheritHandles parameter must be set to TRUE.

So, if CreateProcess bInheritHandles is false, then STARTF_USESTDHANDLES cannot be used, and lpStartupInfo.hStdInput cannot be used, so child process input will be keyboard.

Same for stdout and stderr.

But, we could, probably, use new STARTUPINFOEX to overcome this problem.

https://devblogs.microsoft.com/oldnewthing/?p=8873

Alex

@zhaoya881010
Copy link
Author

zhaoya881010 commented Jan 22, 2020

@alexbrainman

This is a very common problem:https://bugs.python.org/issue19764
I try to use STARTUPINFOEX and PROC_THREAD_ATTRIBUTE_HANDLE_LIST in func StartProcess.at least it solved the socket inheritance problem and can get standard input and output.I'm not sure if this is the best way.
on Linux,no STARTUPINFOEX. can try to traverse the fds and then close the unused fd.

@networkimprov
Copy link

Possibly related: #24328

@odeke-em odeke-em modified the milestones: Go1.15, Backlog May 25, 2020
@zhaoya881010
Copy link
Author

@odeke-em How is it going?

@zx2c4
Copy link
Contributor

zx2c4 commented Mar 25, 2021

Isn't this already fixed? SysProcAttrs has a field for it.

@zhaoya881010
Copy link
Author

@zx2c4 ok,i'll try.

@JohanKnutzen
Copy link

JohanKnutzen commented Aug 28, 2021

@zx2c4 @zhaoya881010 You can set this by:

cmd.SysProcAttr = &syscall.SysProcAttr{
	NoInheritHandles: true,
}

Unfortunately it is broken as of 1.17:
#48040

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 7, 2022
@qmuntal
Copy link
Contributor

qmuntal commented Jun 26, 2023

Unfortunately it is broken as of 1.17:
#48040

This has fixed in https://golang.org/cl/350416. Closing as there is nothing else to do here.

@qmuntal qmuntal closed this as completed Jun 26, 2023
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. 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

No branches or pull requests

10 participants