-
Notifications
You must be signed in to change notification settings - Fork 139
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
std::process::Command and I/O safety #100
Comments
Yes, legacy of Unix design continuing to bite. I've seen this problem pop up everywhere; long ago, Firefox had a bug where it leaked the esound fd to child processes which could break sound. In fact today for me running One approach that some language runtimes have taken is to by default walk over all open FDs and close them, except the ones that are explicitly (via API) passed to the child process. The main one I'm thinking of here is Python's subprocess which notice that
I helped write glib's GSubprocess and it was an explicit choice to do it the same way as Python is doing it; there's an equivalent While we clearly can't switch Rust's In the short term...WDYT about offering this API ostreedev/ostree-rs-ext@89d3727 somewhere in the rustix ecosystem? If we took a hard dependency on |
OK, there is There is one other argument for avoiding |
This Here's a very rough sketch of the kind of thing I'm thinking about: https://github.com/sunfishcode/intercomm In particular, see examples/fun.rs. The goal is to make passing file descriptors as easy as passing strings, where users don't need to manually allocate child file descriptor indices. The code above works with Command which takes a plain path, but I agree that some form of |
Hmm. Some overlap with https://docs.rs/procspawn/0.10.0/procspawn/ (cool crate if you aren't aware of it). It also seems like most users would want at least a I definitely love the idea of making it foolproof to pass the file descriptor and the string value with its number as a single unit from the caller's PoV. I have a small concern that there might be programs that want the fd in a special way. At least for my particular use case it looks like I'm a bit uncertain; this approach clearly has more safety, but there's a lot more logic (and opinions) going on, and it seems to me it could make sense to at least offer something close to my proposed lower level API which is still arguably safe? |
For now I created https://github.com/cgwalters/cap-std-ext - I'm thinking to centralize more things there too as a staging ground. (xref bytecodealliance/cap-std#211 too) |
That makes sense for now. I think I'm getting bogged down here because this whole topic ties into some big-picture goals for me. One off the things that's tricky is that after the user calls |
Yeah. In my case though the child process is Go, not Rust. So...none of that applies unfortunately. (If you are idly curious, this is used by https://github.com/containers/containers-image-proxy-rs/ which is part of my active project in pulling operating system updates via the containers/image stack, and the host process here is increasingly Rust with a large unfortunate swath of C/C++, and I definitely didn't want to add Go into the address space too 😉 ) |
Tangentially related, https://docs.rs/ipc-channel/ supports adding channels into messages over its own channels. I didn't deep dive but I think this works by adding it to an out-of-band queue; since the crate is also responsible for serializing and doing writing, it can handle that too. |
OK https://crates.io/crates/cap-std-ext exists now on crates.io. I also worked on an O_TMPFILE API. What do you think? Basically I (or we, and others) can iterate on some higher level APIs there, and then migrate them down into cap-std where it makes sense. |
Looks good! And yes, I think this makes sense. There are a lot of utility functions that could potentially go in such a crate, so let's see where it goes. |
Following up on a discussion in #97:
std::process::Command
has a safe API for spawning child processes. Its documentation doesn't mention non-O_CLOEXEC
file descriptors, but from some experimenting, child processes do inherit non-O_CLOEXEC
file descriptors.Does this violate I/O safety?
When a program calls
Command::spawn
, all existing memory in the process is replaced with a new program image, so Rust's safety guarantees have typically considered that to be the end of one program and the beginning of a new one. However, this is an area where file descriptors are different from pointers. FIle descriptors not markedO_CLOEXEC
persist into the next program. In effect,spawn
is implicitly obtaining the values of all such file descriptors in the process, even those meant to be encapsulated, and "passing" them to another program where they could be accessed. That has the shape of an I/O safety violation. And in practice, leaking file descriptors to child processes is a real security problem.One possible way out of this is to say that safe code can't create non-
O_CLOEXEC
file descriptors. Rust's std already does open all file descriptors asO_CLOEXEC
whenever it can, and sets them toO_CLOEXEC
even when it can't create them that way. Perhaps then, rustix'sopenat
function should always setO_CLOEXEC
too, and perhaps there should be a special function named something likeunsafe fn dup2_no_cloexec
for creating non-O_CLOEXEC
file descriptors for when those are really needed.One tricky issue is that not all OS's can set
O_CLOEXEC
atomically in all cases. Rust usesioctl
withFIOCLEX
to set theCLOEXEC
flag as soon as it can, but there is a window where another thread could callexec
and capture the file descriptor before it hasO_CLOEXEC
set. A related issue is that users ofstd::process::Command
may already be creating non-O_CLOEXEC
file descriptors within safe functions. A possible long-term path out of this is to say that post-exec
programs accessing file descriptors passed throughexec
is in the same category as using a debugger on the pre-exec
program, so it's out of scope for I/O safety, and then to provide a new safe way to pass file descriptors to child processes and gradually encourage the Rust ecosystem to adopt it.The text was updated successfully, but these errors were encountered: