Skip to content
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

fix: allow safer PosixSpawnFileActions usage #2491

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

CameronNemo
Copy link
Contributor

Many functions used for PosixSpawnFileActions were demanding fds passed implement the AsFd trait, but because these actions are meant to be taken in the child process, that trait doesn't offer much benefit and actually often leads to the caller needing to do an unsafe operation: instantiating an OwnedFd from a RawFd. All of these functions need a RawFd anyway, so just let the caller pass a RawFd directly rather than have to unsafely create an OwnedFd first, which itself could have unintended side effects like closing the FD in the parent when no parent-side actions were intended.

What does this PR do

Followup to #2475, which added the API and has not been in a released version yet.

Checklist:

  • I have read CONTRIBUTING.md
  • I have written necessary tests and rustdoc comments
  • A change log has been added if this PR modifies nix's API (NOTE: changelog was added in Add wrappers for posix_spawn related functions #2475, and the changelog does not need any updates)

@CameronNemo
Copy link
Contributor Author

CameronNemo commented Sep 8, 2024

For some context, right now I am being steered toward writing this code:

    let mut file_actions = spawn::PosixSpawnFileActions::init()?;
    let dev_null = CString::new("/dev/null")?;
    let dev_null = dev_null.as_c_str();
    let fd0 = unsafe { std::os::fd::OwnedFd::from_raw_fd(0) };
    let fd1 = unsafe { std::os::fd::OwnedFd::from_raw_fd(1) };
    file_actions.add_open(fd0, dev_null, OFlag::O_RDONLY, Mode::empty())?;
    file_actions.add_dup2(fd0, fd1)?;

Besides the unsafe usage, this code is wrong -- it will close the stdin and stdout file descriptors in the parent when the fd0 and fd1 variables get dropped. That is not at all what we are trying to accomplish; we only want actions to be taken in the child, thus I think AsRawFds are a better API choice for these functions.

@SteveLauC
Copy link
Member

Thanks for the feedback!

I agree that I/O-safe interfaces do not make sense here.

Regarding your example, you can actually use the Stdin and Stdout from std as they have AsFd implemented, though I think we should use RawFd in these interfaces.

src/spawn.rs Outdated
@@ -281,16 +277,16 @@ impl PosixSpawnFileActions {
/// Add a [dup2](https://pubs.opengroup.org/onlinepubs/9699919799/functions/dup2.html) action. See
/// [posix_spawn_file_actions_adddup2](https://pubs.opengroup.org/onlinepubs/9699919799/functions/posix_spawn_file_actions_adddup2.html).
#[doc(alias("posix_spawn_file_actions_adddup2"))]
pub fn add_dup2<Fd1: AsFd, Fd2: AsFd>(
pub fn add_dup2<Fd1: AsRawFd, Fd2: AsRawFd>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Taking a look at the types that implement AsRawFd, they are mostly owned file descriptors, e.g., File, passing them by value would have them closed within these add_xxx() functions, and we don't have an impl like:

impl<T: AsRawFd> AsRawFd for &T

what about:

add_xxx<Fd: AsRawFd>(_: &AsRawFd)

Though one drawback with this interface is that when passing RawFds, we still need to add a layer of reference to it:

let stdin_raw_fd: RawFd = 0;

add_xxx(&stdin_raw_fd);

thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about removing the generic type and just using RawFd?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah just using RawFd might be simpler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright pushed with the direct RawFd type.

Many functions used for PosixSpawnFileActions were demanding fds passed
implement the AsFd trait, but because these actions are meant to be
taken in the child process, that trait doesn't offer much benefit and
actually often leads to the caller needing to do an unsafe operation:
instantiating an OwnedFd from a RawFd. All of these functions need a
RawFd anyway, so just let the caller pass a RawFd directly rather than
have to unsafely create an OwnedFd first, which itself could have
unintended side effects like closing the FD in the parent when no
parent-side actions were intended.
@SteveLauC SteveLauC enabled auto-merge September 10, 2024 02:15
@SteveLauC SteveLauC added this pull request to the merge queue Sep 10, 2024
github-merge-queue bot pushed a commit that referenced this pull request Sep 10, 2024
Many functions used for PosixSpawnFileActions were demanding fds passed
implement the AsFd trait, but because these actions are meant to be
taken in the child process, that trait doesn't offer much benefit and
actually often leads to the caller needing to do an unsafe operation:
instantiating an OwnedFd from a RawFd. All of these functions need a
RawFd anyway, so just let the caller pass a RawFd directly rather than
have to unsafely create an OwnedFd first, which itself could have
unintended side effects like closing the FD in the parent when no
parent-side actions were intended.

Co-authored-by: Cameron Nemo <[email protected]>
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 10, 2024
@SteveLauC SteveLauC added this pull request to the merge queue Sep 10, 2024
Merged via the queue into nix-rust:master with commit e5ac667 Sep 10, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants