Skip to content

Commit

Permalink
to extract a pidfd we must consume the child
Browse files Browse the repository at this point in the history
As long as a pidfd is on a child it can be safely reaped. Taking it
would mean the child would now have to be awaited through its pid, but could also
be awaited through the pidfd. This could then suffer from a recycling race.
  • Loading branch information
the8472 committed Jun 21, 2024
1 parent f7cf777 commit c2ec99b
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 12 deletions.
22 changes: 15 additions & 7 deletions std/src/os/linux/process.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ struct InnerPidFd;
///
/// A `PidFd` can be obtained by setting the corresponding option on [`Command`]
/// with [`create_pidfd`]. Subsequently, the created pidfd can be retrieved
/// from the [`Child`] by calling [`pidfd`] or [`take_pidfd`].
/// from the [`Child`] by calling [`pidfd`] or [`into_pidfd`].
///
/// Example:
/// ```no_run
Expand All @@ -33,7 +33,7 @@ struct InnerPidFd;
/// .expect("Failed to spawn child");
///
/// let pidfd = child
/// .take_pidfd()
/// .into_pidfd()
/// .expect("Failed to retrieve pidfd");
///
/// // The file descriptor will be closed when `pidfd` is dropped.
Expand All @@ -44,7 +44,7 @@ struct InnerPidFd;
/// [`create_pidfd`]: CommandExt::create_pidfd
/// [`Child`]: process::Child
/// [`pidfd`]: fn@ChildExt::pidfd
/// [`take_pidfd`]: ChildExt::take_pidfd
/// [`into_pidfd`]: ChildExt::into_pidfd
/// [`pidfd_open(2)`]: https://man7.org/linux/man-pages/man2/pidfd_open.2.html
#[derive(Debug)]
#[repr(transparent)]
Expand Down Expand Up @@ -159,18 +159,26 @@ pub trait ChildExt: Sealed {
/// [`Child`]: process::Child
fn pidfd(&self) -> Result<&PidFd>;

/// Takes ownership of the [`PidFd`] created for this [`Child`], if available.
/// Returns the [`PidFd`] created for this [`Child`], if available.
/// Otherwise self is returned.
///
/// A pidfd will only be available if its creation was requested with
/// [`create_pidfd`] when the corresponding [`Command`] was created.
///
/// Taking ownership of the PidFd consumes the Child to avoid pid reuse
/// races. Use [`pidfd`] and [`BorrowedFd::try_clone_to_owned`] if
/// you don't want to disassemble the Child yet.
///
/// Even if requested, a pidfd may not be available due to an older
/// version of Linux being in use, or if some other error occurred.
///
/// [`Command`]: process::Command
/// [`create_pidfd`]: CommandExt::create_pidfd
/// [`pidfd`]: ChildExt::pidfd
/// [`Child`]: process::Child
fn take_pidfd(&mut self) -> Result<PidFd>;
fn into_pidfd(self) -> crate::result::Result<PidFd, Self>
where
Self: Sized;
}

/// Os-specific extensions for [`Command`]
Expand All @@ -181,7 +189,7 @@ pub trait CommandExt: Sealed {
/// spawned by this [`Command`].
/// By default, no pidfd will be created.
///
/// The pidfd can be retrieved from the child with [`pidfd`] or [`take_pidfd`].
/// The pidfd can be retrieved from the child with [`pidfd`] or [`into_pidfd`].
///
/// A pidfd will only be created if it is possible to do so
/// in a guaranteed race-free manner. Otherwise, [`pidfd`] will return an error.
Expand All @@ -195,7 +203,7 @@ pub trait CommandExt: Sealed {
/// [`Command`]: process::Command
/// [`Child`]: process::Child
/// [`pidfd`]: fn@ChildExt::pidfd
/// [`take_pidfd`]: ChildExt::take_pidfd
/// [`into_pidfd`]: ChildExt::into_pidfd
fn create_pidfd(&mut self, val: bool) -> &mut process::Command;
}

Expand Down
5 changes: 2 additions & 3 deletions std/src/sys/pal/unix/linux/pidfd/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,13 @@ fn test_pidfd() {
return;
}

let mut child = Command::new("sleep")
let child = Command::new("sleep")
.arg("1000")
.create_pidfd(true)
.spawn()
.expect("executing 'sleep' failed");

let fd = child.take_pidfd().unwrap();
drop(child);
let fd = child.into_pidfd().unwrap();

assert_matches!(fd.try_wait(), Ok(None));
fd.kill().expect("kill failed");
Expand Down
4 changes: 2 additions & 2 deletions std/src/sys/pal/unix/process/process_unix.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1098,12 +1098,12 @@ mod linux_child_ext {
.ok_or_else(|| io::Error::new(ErrorKind::Uncategorized, "No pidfd was created."))
}

fn take_pidfd(&mut self) -> io::Result<os::PidFd> {
fn into_pidfd(mut self) -> Result<os::PidFd, Self> {
self.handle
.pidfd
.take()
.map(|fd| <os::PidFd as FromInner<imp::PidFd>>::from_inner(fd))
.ok_or_else(|| io::Error::new(ErrorKind::Uncategorized, "No pidfd was created."))
.ok_or_else(|| self)
}
}
}
Expand Down

0 comments on commit c2ec99b

Please sign in to comment.