add std::os::unix::process::CommandExt::fd rust-lang/rust#145687

Can this just reuse the existing CommandExt rather than creating a new one?

The existing CommandExt implementations are in other modules of std::os. According to the ACP, this trait should live in std::os::fd.

I did notice it said that, but do you know why? Having two CommandExts (one of which isn't in a process module like Command itself is) doesn't make any sense to me so I am thinking this is just an oversight.

I'm not sure what you mean about process modules. There are (prior to this PR) three documented instances of CommandExt, each of which is contained in a process module. This PR adds a fourth such instance and a process module for std::os::fd.

I mean specifically the OS process module, of which there is only one per platform (where the existing CommandExts live), not just any module named process. Can you explain why it makes more sense to add a new trait in fd rather than just adding a new method to the existing Unix CommandExt?

As I understand it, the std::os::fd module exists for fd-related operations. It includes WASI (https://github.com/rust-lang/rust/blob/master/library/std/src/os/mod.rs#L185-L186) because fds are handled similarly to unix. If this API works on WASI, why not expose it there?

I think that if there is interest on wasi, it would be easy enough to add a std::os::wasi::CommandExt. This would also mean pre_exec can be provided.

Does the current impl work on WASI? Afaik our libc doesn't have dup/dup2 or fnctl for that platform, so it would need a fallback anyway.

We shouldn't have any errors here since old_fd should be valid. It would still be a good idea for consistency to check the return values and turn them into a panic (probably just .expecting the result of cvt_nz)

Is it correct to rely on 0 here? dup2 doesn't set FD_CLOEXEC but doesn't really specify what would happen with other flags if they get added. It might be better to F_GETFD the flags and explicitly unset only FD_CLOEXEC. In any case, it could use a comment.

@the8472 may know better

Note also rust-lang/libs-team#623 (comment).

Using pre_exec for this can reduce the performance of starting the child process.
See my Zulip thread on this:
#t-libs-api/api-changes > Extra FDs in CommandExt @ 💬
also mentioned in this comment in the tracking issue:
#144989 (comment)

Using pre_exec switches the spawn implementation from posix_spawn to fork + execve syscalls. Forking is slower than posix_spawn, because it needs to copy the programs memory (in practice copy on write optimizations are used, but they are still somewhat costly - see my benchmarks also linked in the Zulip message). posix_spawn supports setting the passed FDs, so this feature should be used if possible.

Do you happen to have a sketch of what a better interface would look like?

No, but the issue I've described here is not a problem with the signature of fd. The extension should not use the public pre_exec method, the implementation needs to be modified instead.

I believe this file is of interest:

Note that there are multiple structs called Command because of how the target-specific functionality is implemented.

Command.spawn first tries to run the posix_spawn method, but its implementations return None if an attribute which the limited posix_spawn syscall cannot handle is set - see the implementations of the posix_spawn method.

Please keep this behavior in mind when adding new Command attributes and creating tests - cases when spawn uses the posix_spawn syscall and when it does not should both be tested. I'm not sure if there is a way to check which spawn variant was chosen in the tests, it is an implementation detail after all.

The extension should not use the public pre_exec method, the implementation needs to be modified instead.

See how other methods in the CommandExt trait are implemented in the library/std/src/os/fd/process.rs file that you modified - they all call the actual implementation using .as.inner_mut()

It would be reasonably straightforward to add a Vec<(OwnedFd, RawFd)> to Command that could be used either with something based on posix_spawn_file_actions_adddup2 or with the exec fallback (also avoiding 1 closure per fd). But aside from being slow, is there any correctness problem with the current implementation?

As long as there isn't anything explicitly wrong, I think it would be reasonable to get the current implementation over the line first so we have some tests. Any chance you would be interested in submitting a followup changing the implementation, since you have been looking into this a lot?

I'm not sure if there is a way to check which spawn variant was chosen in the tests, it is an implementation detail after all.

Adding a last_spawn_was_posix_spawn field to Command for debugging would be fine. That will be accessible from tests once they're moved to within std/src.

But aside from being slow, is there any correctness problem with the current implementation?

No, I'm not claiming this implementation is incorrect - I also haven't reviewed it thoroughly though.

Any chance you would be interested in submitting a followup changing the implementation, since you have been looking into this a lot?

If I have the time, sure.

Are the eprintln!s here intended to be kept?

I guess the current impl may or may not change based on #145687 (comment), but can't this just use self.pre_exec(...)? Which just handles the as_inner_mut + Box

That method exists in unix::process::CommandExt. I could add it here, but that might fall outside the scope of the ACP.

procfs isn't cross-platform, /dev is though. Exhaustively enumerating all fds also seems potentially unreliable without closing excess descriptors first, imagining an OS could have more default descriptors.

Suggestion to avoid this and also change the named file for a pipe to help avoid conflicts:

// Create a pipe between this process and the child
let (pipe_reader, mut pipe_writer) = io::pipe()?;

let fd_num = 123;

let mut cmd = Command::new("cat");
// `cat` will print the contents of a fd to its stdout
cmd.arg(format!("/dev/fd/{fd_num}"))
    .stdout(Stdio::piped())
    // And we connect the pipe to that fd
    .fd(fd_num, pipe_reader);

// Launch the process
let mut child = cmd.spawn()?;
let mut stdout = child.stdout.take().unwrap();

// And write to the pipe
pipe_writer.write_all(b"Hello, world!")?;
drop(pipe_writer);

// `cat` should read what we sent and send it back via stdout
child.wait()?;
assert_eq!(io::read_to_string(&mut stdout)?, "Hello, world!");

Ok(())

I think I am missing something here; old_fd.into() creates an OwnedFd that isn't bound to anything, then only the integer raw fd is moved into the closure. Why doesn't the file immediately get closed when the OwnedFd is dropped, before spawn happens?

I think old_fd also needs to be moved into the child and closed there after dup. Please see #145687 (comment) (also linked above)

Yeah, the current test is a false positive; it happens to work because close seems to remap the fd to /dev/null, which shows up if you write something to the file and try to read it. Poking things slightly breaks things, e.g. using status rather than output or opening more files to change the number of existing descriptors https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=be191ed9866ec5390fbc64225564ea07

These can go in library/std/src/sys/process/unix/unix/tests.rs which already has the right cfg settings

Move this to above the second example, since that is where it is relevant

Nit, the error handling is hidden to make the example more compact:

    /// # fn main() -> io::Result<()> {
    /// ...
    /// # Ok(()) <- also remove the empty line before this one
    /// # }

The block can then be dedented (same for the second test)

Nit, usually we keep it obvious what comes from libc (also lets us gate in a single place if needed)

Avoiding magic numbers

If cat is given multiple files it will print their output back-to-back, so the output from both can be checked (one will be clobbered, add a comment saying that is expected)

The test here isn't the most obvious, could you add a comment about what we are checking?

When checking system errors always prefer comparing with >/< rather than ==. More robust if some specific system returns an error code rather than specifically "-1"

Also add a test that checks for errors if a garbage fd is passed. Something like

// Check that an invalid fd causes an error
fn whatever() {
    let mut cmd = Command::new("echo");
    
    // Pick a fd that is unlikely to be used, and sanity check that it is indeed closed
    // NOTE: This is not TOCTOU-safe, technically the OS could assign `fd` before we try to spawn
    // the command. However, this is unlikely and there isn't really a better way to ensure a fd
    // remains unassigned.
    let fd = c_int::MAX / 2;
    assert!(unsafe { fcntl(fd, F_GETFD) } < 0);

    cmd.fd(fd, fd);
    assert!(cmd.spawn().is_err());
}

This test doesn't compile because the second parameter of fd is of type OwnedFd. I could construct a garbage one with unsafe, but at that point it's up to the user to determine that the fd is valid, right?

You're right! Ignore me here

After thinking about this a bit more, I think I overlooked a pretty obvious answer; this gets run after fork (and before exec) so it's working with its own copy of old_fd FD rather than the original. IOW the close here is closing the child's copy of the fd, but the parent's still exists and is not accessible from the child, so it winds up leaked. I think there might actually be a double close here because there's the explicit libc::close and the drop of old_fd (usually we warn on this but I'm guessing panics don't work quite right after fork).

I don't think there is a good way to do this in pre_exec because the parent's copy needs to get cleaned up only after a successful call to fork since that's the point where the child has its own fds keeping the resources open.

Would you be up for changing to the alternative discussed around #145687 (comment)? It would roughly look like:

• Add a Vec<(OwnedFd, RawFd)> to the Unix command
• .fd(...) pushes an entry
• Update the default strategy posix_spawn at

  rust/library/std/src/sys/process/unix/unix.rs

  Line 449 in bbcbc78

  fn posix_spawn(

  :
  • Call posix_spawn_file_actions_adddup2 for each vec entry
  • Call posix_spawn_file_actions_addclose for each original
• Update the fork+exec fallback in the same file, basically move what's here
• After spawning, clear the vec so the OwnedFDs get dropped and closed in the parent process
• Run existing tests once for each implementation. Probably easiest to turn the existing #[test]s into regular functions with a use_exec: bool arg, and if true adds a dummy pre_exec closure to force the fork+exec fallback.

I think this should be enhanced by checking what FDs are available from the child too, the same IDs can be obtained from the stat CLI tool. I'll need to think about this a bit.

Maybe name this function close_owned_fds to make it obvious why it’s being called.

I think it would be good to add a last_spawn_was_posix_spawn: Option<bool> field that gets set to None at the start of the spawn call, then Some(true) if posix_spawn succeeds and Some(false) if exec succeeds. Then we can assert that the right strategy was used on platforms that support it (which is more useful than just here).

Could use a comment like

For testing purposes, store Some(true) if the last spawn used posix_spawn, Some(false) if exec was used, and None if this hasn't been spawned yet.

Could use a comment like

A map of parent FDs to child FDs to be inherited during spawn.

This import can go at the top of the posix_spawn function

These can all use .unwrap() rather than unwrap_or right? Since it's unconditionally set when spawning.

Ah we actually also have to account for platforms that don't support posix_spawn. I think it might be easiest to just put this in a separate function.

Taking that config from fn posix_spawn in unix.rs:

#[track_caller]
fn assert_spawn_method(cmd: &Command, use_exec: bool) {
    let used_posix_spawn = cmd.last_spawn_was_posix_spawn().unwrap();
    if use_exec {
        assert!(!used_posix_spawn, "posix_spawn used but exec was expected");
    } else if cfg!(any(
        target_os = "freebsd",
        target_os = "illumos",
        all(target_os = "linux", target_env = "gnu"),
        all(target_os = "linux", target_env = "musl"),
        target_os = "nto",
        target_vendor = "apple",
        target_os = "cygwin",
    )) {
        assert!(used_posix_spawn, "platform supports posix_spawn but it wasn't used");
    } else {
        assert!(!used_posix_spawn);
    }
}

Group these into a module so they can get a combined doc comment:

/// For `Command`'s fd-related tests, we want to be sure they work both with exec
/// and with `posix_spawn`. We test both the default which should use `posix_spawn`
/// on supported platforms, and using `pre_exec` to force spawn using `exec`.
mod fd_impls {
    fn test_stdin(use_exec: bool) { ... }
    ...
}

#[test]
fn fd_tests_posix_spawn() {
    fd_impls:test_stdin(false);
    ...
}

Nit so failures point at the behavior rather than at spawn vs. exec

We should clarify somewhere what the difference between new_fd and old_fd is, right?

(And new_fd is not invalidated in the parent process, only the child, right?)

This doesn't seem mentioned in the ACP or tracking issue, and seems like an odd place to put this. It's also not clear to me why this is on Command vs on Child?

I'm not convinced we want to make this a stable API -- posix_spawn feels like a low level detail rather than something callers should care about. If we do want this exposed we should explain what callers are supposed to do with the information.

This is for testing purposes (see #145687 (comment)). Would it be better to mark it perma-unstable? (I'm not sure exactly how to do that)

I don't think it can be private because tests are treated as separate crates.

It should be possible to make it private. Integration tests in library/std/tests are treated as separate crates, but these are in src/ so can use internal API.

Verify the child didn't run into errors, applies a few places

This is problematic and probably unsound (as it violates I/O safety) if new_fd is accidentally equal to the file descriptor number that is used by the pipe or socket used to return errors. That would lead to spawn inaccurately reporting a success, even if an error occurs (since this half of the pipe will be closed).

Is there a way to check for that? If not, it seems impossible to avoid.

@joboet friendly nudge :)

You could use dup2 (ideally std's try_clone wrapper) to clone the pipe/socket to a different descriptor and then close the original, effectively moving the descriptor.

Maybe I don't understand the issue. If new_fd might collide with the pipe/socket, then it might still collide after I move the pipe/socket, right?

If old_fd == new_fd (which is useful for clearing FD_CLOEXEC), I think the adddup2 should run, but addclose should be skipped.