golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
122.91k stars 17.52k forks source link

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

Closed zhaoya881010 closed 1 year ago

zhaoya881010 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

cc @zx2c4 @mattn

mattn commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

socket handles are inheritable by default

ianlancetaylor commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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.

alexbrainman commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

Possibly related: #24328

zhaoya881010 commented 3 years ago

@odeke-em How is it going?

zx2c4 commented 3 years ago

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

zhaoya881010 commented 3 years ago

@zx2c4 ok,i'll try.

JohanKnutzen commented 3 years ago

@zx2c4 @zhaoya881010 You can set this by:

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

Unfortunately it is broken as of 1.17: https://github.com/golang/go/issues/48040

qmuntal commented 1 year ago

Unfortunately it is broken as of 1.17: https://github.com/golang/go/issues/48040

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