From c783548cfd10813b8e8dada39b78b1037900f13e Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Wed, 11 Dec 2024 02:23:17 +0000 Subject: [PATCH] mount: Some error handling fixups Motivated by https://github.com/containers/bootc/issues/957 but probably not any kind of fix in reality. Our error handling here was buggy. What we really should do is avoid `fork()` and do an execve here, but that's a larger refactor. - Close the other side of the socket in the forked child so the parent doesn't hang indefinitely if the child dies before sending - Change the parent to return a clean error if the child doesn't write anything instead of an `assert!` - Check the exit code of the child Signed-off-by: Colin Walters --- lib/src/mount.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/lib/src/mount.rs b/lib/src/mount.rs index e2bde3fa..a1ad00e6 100644 --- a/lib/src/mount.rs +++ b/lib/src/mount.rs @@ -169,6 +169,8 @@ pub(crate) fn open_tree_from_pidns( // We're in the child. At this point we know we don't have multiple threads, so we // can safely `setns`. + drop(sock_parent); + // Open up the namespace of the target process as a file descriptor, and enter it. let pidlink = fs::File::open(format!("/proc/{}/ns/mnt", pid.as_raw_nonzero()))?; rustix::thread::move_into_link_name_space( @@ -210,6 +212,7 @@ pub(crate) fn open_tree_from_pidns( n => { // We're in the parent; create a pid (checking that n > 0). let pid = rustix::process::Pid::from_raw(n).unwrap(); + drop(sock_child); // Receive the mount file descriptor from the child let mut cmsg_space = vec![0; rustix::cmsg_space!(ScmRights(1))]; let mut cmsg_buffer = rustix::net::RecvAncillaryBuffer::new(&mut cmsg_space); @@ -224,7 +227,7 @@ pub(crate) fn open_tree_from_pidns( ) .context("recvmsg")? .bytes; - assert_eq!(nread, DUMMY_DATA.len()); + anyhow::ensure!(nread == DUMMY_DATA.len()); assert_eq!(buf, DUMMY_DATA); // And extract the file descriptor let r = cmsg_buffer @@ -236,8 +239,14 @@ pub(crate) fn open_tree_from_pidns( .flatten() .next() .ok_or_else(|| anyhow::anyhow!("Did not receive a file descriptor"))?; - rustix::process::waitpid(Some(pid), WaitOptions::empty())?; - Ok(r) + // SAFETY: Since we're not setting WNOHANG, this will always return Some(). + let st = + rustix::process::waitpid(Some(pid), WaitOptions::empty())?.expect("Wait status"); + if let Some(0) = st.exit_status() { + Ok(r) + } else { + anyhow::bail!("forked helper failed: {st:?}"); + } } } }