There are two places I can think of in this library where UB can occur in safe code:
1. CommandFdExt::fd_mappings
The fd_mappings
function has the following warning in the documentation
Warning: Calling this more than once on the same command, or attempting to run the same command more than once after calling this, may result in unexpected behaviour.
This clearly violates the rule put forward in The Rustonomicon:
No matter what, Safe Rust can't cause Undefined Behavior.
This means that the library should either:
- Have runtime checks to prevent calling the
fd_mappings
or spawn
function more than once. Panicking is not considered unsafe
This is analogous to the indexing ([]
) operator of slices - trying to access an out of bounds index causes a panic
- Mark the
fd_mappings
function as unsafe
- this makes it clear that there is a contract that needs to be fulfilled in order to call the function and the user must assert they understand it by using the unsafe {}
block
This is analogous to the unsafe get_unchecked
method of a slice
In both cases the documentation should be updated accordingly (replacing the warning with a Panics
or Safety
section)
2. FdMapping
The other potential source of UB is the FdMapping
struct. As per the documentation for FdMapping
:
The parent_fd must be kept open until after the child is spawned.
This is clearly another contract that must be fulfilled by the crate user and improper usage can and will lead to UB. While getting the RawFd
from an OwnedFd
(or any other type - using the IntoRawFd
trait) is not unsafe, using a RawFd
is. The logic is essentially the same as with raw pointers:
- Getting a raw pointer is safe (see
Vec::as_ptr
)
- Dereferencing a raw pointer is unsafe
Rust's standard library already provides a way to use file descriptors safely: the OwnedFd
and BorrowedFd
types. See the I/O safety section in std::io docs for more details. They can be used to ensure that parent_fd
will in fact be opened while we have a reference to a OwnedFd
or BorrowedFd
.
Fixing this problem is a bit more nuanced. The simple option would be to make either the fd_mappings
function or FdMapping
constructor unsafe
(well, currently there is no FdMapping::new
function, so the interface would need to be changed - thread) and document the safety contract (namely: the user must ensure parent_fd
is not closed until the child is spawned).
A more sound solution could be making it so that fd_mappings
has a safe variant which takes either BorrowedFd
s or OwnedFd
s for the parent_fd
. (I'm not sure how should child_fd
be implemented, as it doesn't refer to an existing fs
- leaving it as a RawFd
might be a valid solution).
The usage could look like this (modified example from the docs):
With BorrowFd
:
let file = File::open("Cargo.toml").unwrap();
let mut command = Command::new("ls");
command.arg("-l").arg("/proc/self/fd");
let fd: BorrowedFd = file.as_fd();
command
.fd_mappings(vec![
FdMapping::from_borrowed_fd(
/* parent_fd: */ fd, // or &fd ?
/* child_fd: */ 3,
),
])
.unwrap();
// We must use lifetime annotations to make sure the borrowed fd is still valid at this point
// This might require creating a wrapper over `Command`
let mut child = command.spawn().unwrap();
// The file is still valid at this point, but two processes have file descriptors to it
// Are there any valid use cases for this?
// The behaviour could be unexpected, as two processes have access to the same file
file.write(/*...*/);
// We can drop the file - this closes the file descriptor owned by file,
// but no the duplicated one, used by the new thread
drop(file);
child.wait().unwrap();
With OwnedFd
:
let file = File::open("Cargo.toml").unwrap();
let mut command = Command::new("ls");
command.arg("-l").arg("/proc/self/fd");
let owned_fd = OwnedFd::from(file);
// OwnedFd::from<File> consumes `file`
// so the variable `file` is no longer valid at this point
command
.fd_mappings(vec![
FdMapping::from_owned_fd(
/* parent_fd: */ owned_fd, // We transfer ownership of owned_fd
/* child_fd: */ 3,
),
])
.unwrap();
let mut child = command.spawn().unwrap();
// OwnedFd (just as File) implements Drop, which closes the owned stream
// The internal implementation should consider when the OwnedFd is dropped
// Of course it should still be valid when we make the `dup2` syscall
// We should probably examine how it is implemented for standard streams in std::process
//
// The documentation should make it clear when the original file descriptor will be closed
// This is especially important when using pipes, where we expect all the descriptors on the writeable end to be closed,
// so that the reading end can end with an EOF
child.wait().unwrap();