Hi there,

The nix is upgraded in #18 and #16 . How about releasing a new version to crates.io and make the upgrade alive? Thanks!

This is cleaner than duplicating each FD twice.

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 BorrowedFds or OwnedFds 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();