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

Open
zhaoya881010 opened this issue Jan 14, 2020 · 13 comments

Comments

@zhaoya881010
Copy link

@zhaoya881010 zhaoya881010 commented Jan 14, 2020

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

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 14, 2020

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

@zhaoya881010

This comment has been minimized.

Copy link
Author

@zhaoya881010 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

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Jan 14, 2020

@mattn

This comment has been minimized.

Copy link
Member

@mattn 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

This comment has been minimized.

Copy link
Author

@zhaoya881010 zhaoya881010 commented Jan 15, 2020

@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

This comment has been minimized.

Copy link
Author

@zhaoya881010 zhaoya881010 commented Jan 15, 2020

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

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jan 15, 2020

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

This comment has been minimized.

Copy link
Author

@zhaoya881010 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

This comment has been minimized.

Copy link
Author

@zhaoya881010 zhaoya881010 commented Jan 15, 2020

socket handles are inheritable by default

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 15, 2020

@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

This comment has been minimized.

Copy link
Member

@mattn 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

This comment has been minimized.

Copy link
Author

@zhaoya881010 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

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 17, 2020

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.