add std::os::unix::process::CommandExt::fd #145687

Qelxiros · 2025-08-20T22:52:51Z

ACP: rust-lang/libs-team#623
Tracking issue: #144989

try-job: aarch64-apple
try-job: arm-android
try-job: test-various

rustbot · 2025-08-20T22:52:56Z

rustbot has assigned @tgross35.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

tgross35

Has this code been run? The implementation just duplicates the fd without interacting with self.

Please make sure that all new API, even unstable, always gets document0ation and examples, as well as tests if the functionality can't be covered in a simple example. This is tricky API, make sure to cover the FD_CLOEXEC behavior in tests and include something to the effect of https://rust-lang.zulipchat.com/#narrow/channel/327149-t-libs-api.2Fapi-changes/topic/Extra.20FDs.20in.20CommandExt/near/439547420 in docs.

The tests at https://github.com/google/command-fds/tree/main can probably serve as reference.

View changes since this review

library/std/src/os/fd/process.rs

dominik-korsa · 2025-09-02T23:12:57Z

library/std/src/os/fd/process.rs

+impl CommandExt for Command {
+    fn fd(&mut self, new_fd: RawFd, old_fd: impl Into<OwnedFd>) -> &mut Self {
+        let old = old_fd.into().as_raw_fd();
+        unsafe {
+            self.as_inner_mut().pre_exec(Box::new(move || {
+                cvt_r(|| libc::dup2(old, new_fd))?;
+                let flags = cvt(libc::fcntl(new_fd, F_GETFD))?;
+                cvt(libc::fcntl(new_fd, F_SETFD, flags & !FD_CLOEXEC))?;
+                cvt_r(|| libc::close(old))?;
+                Ok(())
+            }))
+        }
+
+        self
+    }
+}


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:

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

Lines 72 to 74 in 71289c3

if let Some(ret) = self.posix_spawn(&theirs, envp.as_ref())? {

return Ok((ret, ours));

}

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.

library/std/src/os/fd/process.rs

tgross35 · 2025-09-03T08:30:01Z

Please make sure that all new API, even unstable, always gets document0ation and examples, as well as tests if the functionality can't be covered in a simple example. This is tricky API, make sure to cover the FD_CLOEXEC behavior in tests and include something to the effect of https://rust-lang.zulipchat.com/#narrow/channel/327149-t-libs-api.2Fapi-changes/topic/Extra.20FDs.20in.20CommandExt/near/439547420 in docs.

The tests at https://github.com/google/command-fds/tree/main can probably serve as reference.

Please make sure to note this; a single test unfortunately doesn't cover the nuances that can show up here. https://github.com/google/command-fds/blob/38670577b393c5b6f4e17b4854128cf6120a3ca1/src/lib.rs#L206 has a few test cases, I think it would be completely reasonable to port each of these to a version that matches our API.

Edit: realizing that repo is Apache-2.0 so we can't take anything directly. But tests from it that we should include are:

Multiple parent fds mapped to the same child fd
Two fds, swapped between parent and child
Stdin is mapped

It would also be good to test that:

A nonexistant fd errors on startup
Save the original raw fd. Ensure it remains open until the command is run, then gets closed after spawn (just fcntl(old_fd, F_GETFD) to check if it's still open)

Qelxiros · 2025-09-03T14:06:42Z

fd_test_swap is broken; I'm not confident that swapping fds like that is possible without a dedicated function since they seem to clobber each other. fd_test_close_time is also broken, at least on my machine. I'm not sure why that is, but I'd appreciate any guidance you may have.

tgross35 · 2025-09-04T21:14:18Z

fd_test_swap is broken; I'm not confident that swapping fds like that is possible without a dedicated function since they seem to clobber each other.

That's expected with the current implementation when both fds need to be alive at the same time; this is what we should check for. The goal is to make sure we don't quietly change this behavior by accident.

fd_test_close_time is also broken, at least on my machine. I'm not sure why that is, but I'd appreciate any guidance you may have.

This one is unfortunately trickier. Linux aggressively reuses fds so it's getting mapped to something else after close, so I guess checking flags isn't enough. Instead, checking that the dev+ino are unequal should work:

use std::{fs, io};
use std::os::fd::AsRawFd;
use std::os::unix::fs::MetadataExt;

#[test]
fn whatever() {
    let (_pipe_reader, pipe_writer) = io::pipe().unwrap();

    let fd = pipe_writer.as_raw_fd();
    let fd_path = format!("/dev/fd/{fd}");

    // let mut cmd = Command::new("cat") ...

    // Get the identifier of the fd (metadata follows symlinks)
    let fd_id = md_file_id(&fs::metadata(&fd_path).expect("fd should be open"));

    // stand in for cmd.spawn().unwrap();
    drop(pipe_writer);

    // After the child is spawned, our fd should be closed
    match fs::metadata(&fd_path) {
        // Ok; fd exists but points to a different file
        Ok(md) => assert_ne!(md_file_id(&md), fd_id),
        // Ok; fd does not exist
        Err(_) => ()
    }

    // ...
}

/// Use dev + ino to uniquely identify a file
fn md_file_id(md: &fs::Metadata) -> (u64, u64) {
    (md.dev(), md.ino())
}

library/std/tests/fd_passing.rs

library/std/src/os/unix/process.rs

library/std/tests/fd_passing.rs

Qelxiros · 2025-09-07T19:13:06Z

Even when checking device/inode values, fd_test_close_time fails on my machine. I think that guarantees that the file descriptor is staying open longer than it should, but I have no idea why. I'm a little out of my depth here, but I'll try to look into it. Let me know if you think of anything else.

tgross35 · 2025-10-20T19:18:43Z

Ibraheem has a lot of reviews

r? libs

tgross35 · 2025-10-30T18:06:14Z

@bors2 try

add std::os::unix::process::CommandExt::fd try-job: aarch64-apple try-job: arm-android try-job: test-various

rust-bors · 2025-10-30T19:37:06Z

💔 Test for bb5247b failed: CI. Failed jobs:

try - arm-android (web logs, extended logs)

Qelxiros · 2025-11-04T15:49:27Z

It looks like the behavior of test_swap is platform-dependent. I don't have easy access to non-Linux machines for testing purposes, so I've removed the test.

@rustbot ready

library/std/src/os/unix/process.rs

Mark-Simulacrum · 2025-11-08T15:36:25Z

library/std/src/os/unix/process.rs

+    ///
+    /// [`posix_spawn`]: https://www.man7.org/linux/man-pages/man3/posix_spawn.3.html
+    #[unstable(feature = "command_pass_fds", issue = "144989")]
+    fn last_spawn_was_posix_spawn(&self) -> Option<bool>;


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.

rustbot · 2025-11-17T17:21:13Z

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

Qelxiros · 2025-11-18T01:42:17Z

@rustbot ready

tgross35 · 2025-11-19T12:35:07Z

It looks like the behavior of test_swap is platform-dependent. I don't have easy access to non-Linux machines for testing purposes, so I've removed the test.

One failing platform certainly isn't a reason to remove a completely valid test entirely. Mark it #[cfg_attr(not(target_os = "android"), should_panic)] (and any other platforms that fail) with a FIXME, it will be worth seeing if we can fix this somehow given users are probably going to expect consistent behavior here.

tgross35 · 2025-11-19T12:42:23Z

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

+        pipe_writer.write_all(b"Hello, world!").unwrap();
+        drop(pipe_writer);
+
+        child.wait().unwrap();


Suggested change

child.wait().unwrap();

child.wait().unwrap().exit_ok().unwrap();

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

rustbot assigned tgross35 Aug 20, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 20, 2025

tgross35 requested changes Aug 27, 2025

View reviewed changes

library/std/src/os/fd/process.rs Outdated Show resolved Hide resolved

library/std/src/os/fd/process.rs Outdated Show resolved Hide resolved

library/std/src/os/fd/process.rs Outdated Show resolved Hide resolved

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 27, 2025

Qelxiros force-pushed the fd-passing branch from 8abe160 to 2c3335f Compare August 27, 2025 21:54

Qelxiros requested a review from tgross35 August 27, 2025 21:56

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 27, 2025

This comment has been minimized.

Sign in to view

Qelxiros force-pushed the fd-passing branch from 2c3335f to 47ad5b9 Compare August 27, 2025 22:35

dominik-korsa reviewed Sep 2, 2025

View reviewed changes

tgross35 requested changes Sep 3, 2025

View reviewed changes

library/std/src/os/fd/process.rs Outdated Show resolved Hide resolved

library/std/src/os/fd/process.rs Outdated Show resolved Hide resolved

library/std/src/os/fd/process.rs Outdated Show resolved Hide resolved

library/std/src/os/fd/process.rs Outdated Show resolved Hide resolved

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 3, 2025

Qelxiros force-pushed the fd-passing branch from 47ad5b9 to 905ad3f Compare September 3, 2025 14:05

This comment has been minimized.

Sign in to view

Qelxiros force-pushed the fd-passing branch from 905ad3f to fd8c770 Compare September 3, 2025 17:50

rustbot added the O-unix Operating system: Unix-like label Sep 3, 2025

This comment has been minimized.

Sign in to view

tgross35 reviewed Sep 4, 2025

View reviewed changes

library/std/tests/fd_passing.rs Outdated Show resolved Hide resolved

Qelxiros force-pushed the fd-passing branch from fd8c770 to 48c14e4 Compare September 7, 2025 19:08

Qelxiros force-pushed the fd-passing branch from 48c14e4 to 7dda1a6 Compare September 7, 2025 19:13

rustbot assigned Mark-Simulacrum and unassigned ibraheemdev Oct 20, 2025

Qelxiros force-pushed the fd-passing branch from fe30064 to 4637558 Compare October 30, 2025 14:36

This comment has been minimized.

Sign in to view

Qelxiros force-pushed the fd-passing branch from 4637558 to dd4c41c Compare October 30, 2025 14:37

This comment has been minimized.

Sign in to view

rust-bors bot added a commit that referenced this pull request Oct 30, 2025

Auto merge of #145687 - Qelxiros:fd-passing, r=<try>

bb5247b

add std::os::unix::process::CommandExt::fd try-job: aarch64-apple try-job: arm-android try-job: test-various

This comment has been minimized.

Sign in to view

Qelxiros force-pushed the fd-passing branch from dd4c41c to f620172 Compare November 4, 2025 15:49

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 4, 2025

Mark-Simulacrum reviewed Nov 8, 2025

View reviewed changes

library/std/src/os/unix/process.rs Show resolved Hide resolved

Mark-Simulacrum reviewed Nov 8, 2025

View reviewed changes

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 8, 2025

add std::os::unix::process::CommandExt::fd

1d0161a

Qelxiros force-pushed the fd-passing branch from f620172 to 1d0161a Compare November 17, 2025 17:21

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 18, 2025

tgross35 reviewed Nov 19, 2025

View reviewed changes

Mark-Simulacrum added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 26, 2025

	if let Some(ret) = self.posix_spawn(&theirs, envp.as_ref())? {
	return Ok((ret, ours));
	}

	child.wait().unwrap();
	child.wait().unwrap().exit_ok().unwrap();

add std::os::unix::process::CommandExt::fd #145687

Are you sure you want to change the base?

add std::os::unix::process::CommandExt::fd #145687

Conversation

Qelxiros commented Aug 20, 2025 • edited by tgross35 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Aug 20, 2025

Uh oh!

tgross35 left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dominik-korsa Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tgross35 commented Sep 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Qelxiros commented Sep 3, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

tgross35 commented Sep 4, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Qelxiros commented Sep 7, 2025

Uh oh!

tgross35 commented Oct 20, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

tgross35 commented Oct 30, 2025

Uh oh!

This comment has been minimized.

This comment has been minimized.

rust-bors bot commented Oct 30, 2025

Uh oh!

Qelxiros commented Nov 4, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rustbot commented Nov 17, 2025

Uh oh!

Qelxiros commented Nov 18, 2025

Uh oh!

Qelxiros commented Aug 20, 2025 •

edited by tgross35

Loading

tgross35 left a comment •

edited by rustbot

Loading

dominik-korsa Sep 5, 2025 •

edited

Loading

tgross35 commented Sep 3, 2025 •

edited

Loading