Set O_CLOEXEC on received file descriptors#537
Merged
djs55 merged 7 commits intomoby:masterfrom May 10, 2021
Merged
Conversation
By default Go sets O_CLOEXEC on file descriptors it creates. This ensures that they aren't accidentally inherited over fork+exec. Unfortunately when we `recvmsg` a file descriptor, there is no flag on Darwin to set the close on exec flag atomically. We must do it ourselves and tolerate the small race window where the fd could be inherited. Signed-off-by: David Scott <dave@recoil.org>
This avoids having to create lots of wrapper structs. Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
We use `Close()` directly everywhere. Signed-off-by: David Scott <dave@recoil.org>
We now rely on Go to "adopt" the file descriptors via `net.FileListner` and `net.FilePacketConn`. Go sets flags like `O_CLOEXEC` and takes care of blocking modes. Signed-off-by: David Scott <dave@recoil.org>
| if controlErr != nil { | ||
| return | ||
| } | ||
| // Go normally opens file descriptors with O_CLOEXEC. This prevents races where |
Collaborator
There was a problem hiding this comment.
I doubt syscall.Dup2 would set the O_CLOEXEC on the new fd. Do you have a reference for this comment?
Have you considered syscall.Dup3 ? Though I have no idea how it behaves on mac.
Collaborator
Author
There was a problem hiding this comment.
Ah sorry I should have squashed this commit -- the code is deleted in a later patch. I had a look on Mac for Dup3 but unfortunately they don't have it :(
fredericdalleau
approved these changes
May 7, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
By default Go sets
O_CLOEXECon file descriptors it creates. This ensures that they aren't accidentally inherited over fork+exec.When binding a port on the host we first try
net.Listen. On modern macOS this works forINADDR_ANY(all IPs)Unfortunately it doesn't work for privileged ports on specific IPs, such as
127.0.0.1:80. For these cases we contact a helper process which runs as root, which binds the port and then usessendmsgto transmit it to us. Unfortunately when werecvmsga file descriptor, the resulting descriptor does not haveO_CLOEXECset, so it is captured by future subprocesses. Even if the parent process callsclose(), the port is still in use by the subprocesses and can't be re-bound. This caused the failure seen in docker/for-mac#5649The solution is to ensure all file descriptors have
O_CLOEXECset. On Linux there's asendmsgflag to set the flag atomically on received fds, but this is missing on Darwin. The best we can do is to callfcntlto set the flag ourselves after receiving the fd. Unfortunately this means there's a small race window where a fork would inherit the fd, but this shouldn't happen very often in practice.Go will call
fcntlfor us if we "adopt" the file descriptor usingos.NewFileand then usenet.FileListenerandnet.FilePacketConn. This allows us to remove a lot of complicated wrapping code and allows us to remove a workaround forClosehanging when blocked inAccept, caused by the state of the file descriptor not being correct. This required some minor changes to types (net.Listenerrather than*net.TCPListener) but it's worth it.It's possible to work around this problem in other ways in the client code, for example:
forkbut beforeexec. This can be expensive with highulimitvalues, if there is no API to enumerate open fds. I'm not sure it can be done easily in Go where theexec.CommandAPI is high-level.Signed-off-by: David Scott dave@recoil.org