-
Notifications
You must be signed in to change notification settings - Fork 672
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
Conversation
For some context, right now I am being steered toward writing this code:
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 |
Thanks for the feedback! I agree that I/O-safe interfaces do not make sense here. Regarding your example, you can actually use the |
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>( |
There was a problem hiding this comment.
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 RawFd
s, 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?
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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]>
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:
CONTRIBUTING.md