File descriptors leak control #27609
Conversation
This trait implementation is accessible through the new "raw_dirfd" feature gate.
Add the `leak_fds() `and `leak_fds_whitelist()` to the UNIX `CommandExt` trait. Linux and Android implementations are optimized with procfs. The `whitelist` argument contains a set of `RawFd` who will intentionally leak through the `Command`'s `execve(2)`. All this file descriptors will lost their `FD_CLOEXEC` flag, but only in the new process. Indeed, the `fcntl(2)` is done after the `fork(2)` to be thread safe. This is useful to pass file descriptors to a child process like the old `extra_io()` was intended to do. This functions are accessible through the new "process_leak_fds" feature gate.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
| } | ||
| }; | ||
| if !cfg.leak_fds { | ||
| walk_open_fd(close_or_leak); |
There was a problem hiding this comment.
This is wrong. You can't close fds as you walk over the /proc/self/fd directory. That's like modifying a list as you iterate over it. Also, allocating memory (with opendir) after forking (in a multithreaded program) is just plain unsafe.
The "correct" way to do this is to use the FD_CLOEXEC flag to mark the fds you want to close to close on exec as you iterate over /proc/self/fd using the raw getdents system call.
There was a problem hiding this comment.
This function is called just after the fork, when there is no other thread nor any open(2) call, so no more FD are created (except the one from opendir which is ignored).
This post-fork code may deadlock for OSX and FreeBSD (as says the child_after_fork comment) but walk_open_fd_proc() is only called on Linux. Not sure it's worth using the raw getdents(2) here.
We could use fcntl/FD_CLOEXEC instead of close(2) but it imply two syscalls (one to get the flags and another to update them) which should be slower.
There was a problem hiding this comment.
You can use the FIOCLEX ioctl to set the flag atomically and in one system call. I'm not sure what you mean about how no more file descriptors are created because there are no threads. I was talking about how other threads that weren't copied over to the new process by the fork may be holding locks deep inside malloc or many other functions and so cause deadlock in the forked process. I'm not sure what you mean about whether or not getdents is worth it. If you want to implement this feature correctly you have to use getdents. If you cannot implement this feature correctly then you should not implement it.
There was a problem hiding this comment.
The FIOCLEX ioctl seems to fit well.
I'm not sure what you mean about how no more file descriptors are created because there are no threads.
The main (parent) process may have multiple threads. After the fork, the child process have only one thread. The parent threads file descriptors should not be impacted by the FD_CLOEXEC operations on the child (post-fork) file descriptors (which are shared between all threads of a same process).
I was talking about how other threads that weren't copied over to the new process by the fork may be holding locks deep inside malloc or many other functions and so cause deadlock in the forked process. I'm not sure what you mean about whether or not getdents is worth it.
As I said before and like the comment said: "this is what pthread_atfork() takes care of".
If you cannot implement this feature correctly then you should not implement it.
Yeah, of course I want to do it right :)
There was a problem hiding this comment.
Unfortunately, the situation with standard libraries using pthread_atfork to correctly unlock locks has always been messy (see https://bugzilla.redhat.com/show_bug.cgi?id=906468, see https://stackoverflow.com/questions/29106483/glibc-malloc-deadlock-during-string-reserve, see https://code.google.com/p/gperftools/issues/detail?id=496, see https://code.google.com/p/android/issues/detail?id=178033, see https://bugzilla.mozilla.org/show_bug.cgi?id=680190). These were just a few of the bugs I was able to quickly search for. Being able to use malloc after a fork in a multi-threaded process isn't supported by standards and not well supported across platforms.
One horrible hack to support this might be to exec a helper binary (which can be the same file as a linked-in shared object).
|
Does someone could check if it's possible to use |
|
This is a pretty significant piece of functionality to add to process spawning which is also very Unix and/or Linux specific, and I'm pretty wary of it as well because it's all turned off by default. We've tried to make If we're adding unix-specific extensions, then I'd much rather just start going the route of "run this closure after the fork" rather than adding all possible options and combinations to the in-tree implementation. |
That would be really good but need a safe async/reentrant function type (cf. FnAsync) to take care of the post-fork constraints. That would also solve the interrupt handler implementation (cf. #11203). Nonetheless, a FD leak control function should be provided to avoid rewriting it any time we want to control FDs leak. |
|
Post-fork isn't quite the same as an async signal handler as you can generally call lots more functions in principle. Either way, yes, it would have stern documentation explaining the limited circumstances its running under, and it may even be
I disagree with this because it's already provided by the standard library in the form of CLOEXEC, if you have to go out of your way to patch up other native libraries then that seems to fine to depend upon as an external crates.io crate. |
The standard library doesn't cover the case of already open file descriptors nor when you want to pass a file descriptor to (only) one specific command (in a multi-threaded program). |
|
@l0kod Many BSDs implement |
alexcrichton
added
the
T-libs-api
Sure, but it's not the responsibility of the standard library to do everything and fix up all possible bugs. Allowing a closure to run post-fork would be simple to implement, extensible, and allow this functionality to exist outside in crates.io. It's somewhat clear from the discussion here that the precise implementation is somewhat in question, and iterating quickly on crates.io is probably much easier than inside the standard library itself. |
|
@alexcrichton As much as I dislike @l0kod 's implementation I have seen this type of code rewritten in a huge amount of places and almost every single implementation gets this code completely wrong so I'd actually really like this code to be written correctly in a standardized place for once. |
|
@sstewartgallus I certainly agree that this should be standardized in one place! This is what crates.io serves us, however, by enabling this to happen not only in the standard library. Large implementations like this typically need to bake outside the standard library first before moving in, and just because it's on crates.io doesn't mean that everyone will start reimplementing it themselves! |
|
Ok the libs team talked about this PR during triage today, and our conclusions were:
As a result I'm going to close this PR as we've decided that it'll need an RFC to accept, but feel free to open a PR for the "run a closure after fork" API! |
|
OK, I will move this code to a crate and open a PR for the post-fork closure. FYI, Python offer this feature by default with Popen/{close,pass}_fds. |
The new
CommandExt::leak_fds(&mut self, on: bool)allow to add a safeguard to not unintentionally leak a file descriptor from a FFI library or inherited from a parent process.The new
CommandExt::leak_fds_whitelist(&mut self, whitelist: HashSet<RawFd>)allow to intentionally and safely pass a file descriptor set to a child process, even if they come from alibstdobject (e.g.File,Socket…) which use theO_CLOEXECflag. This was the purpose of the (unimplemented) old I/Oextra_io()API.I think this two functions are easier to understand and use than a unique generic function.
cc #12148
cc #22678
cc #24237
cc rust-lang/rfcs#941
cc #26478