Merge pull request #1001 from cgwalters/wipe-no-xdev

 install: Never traverse mount points with `--wipe`

Author: cgwalters

lib/src/install.rs

@@ -28,6 +28,7 @@ use camino::Utf8Path;
 use camino::Utf8PathBuf;
 use cap_std::fs::{Dir, MetadataExt};
 use cap_std_ext::cap_std;
+use cap_std_ext::cap_std::fs::FileType;
 use cap_std_ext::cap_std::fs_utf8::DirEntry as DirEntryUtf8;
 use cap_std_ext::cap_tempfile::TempDir;
 use cap_std_ext::cmdext::CapStdExtCommandExt;
@@ -56,7 +57,7 @@ use crate::progress_jsonl::ProgressWriter;
 use crate::spec::ImageReference;
 use crate::store::Storage;
 use crate::task::Task;
-use crate::utils::sigpolicy_from_opts;
+use crate::utils::{open_dir_noxdev, sigpolicy_from_opts};
 
 /// The toplevel boot directory
 const BOOT: &str = "boot";
@@ -1558,14 +1559,22 @@ fn require_empty_rootdir(rootfs_fd: &Dir) -> Result<()> {
 }
 
 /// Remove all entries in a directory, but do not traverse across distinct devices.
+/// If mount_err is true, then an error is returned if a mount point is found;
+/// otherwise it is silently ignored.
 #[context("Removing entries (noxdev)")]
-fn remove_all_in_dir_no_xdev(d: &Dir) -> Result<()> {
-    let parent_dev = d.dir_metadata()?.dev();
+fn remove_all_in_dir_no_xdev(d: &Dir, mount_err: bool) -> Result<()> {
     for entry in d.entries()? {
         let entry = entry?;
-        let entry_dev = entry.metadata()?.dev();
-        if entry_dev == parent_dev {
-            d.remove_all_optional(entry.file_name())?;
+        let name = entry.file_name();
+        let etype = entry.file_type()?;
+        if etype == FileType::dir() {
+            if let Some(subdir) = open_dir_noxdev(d, &name)? {
+                remove_all_in_dir_no_xdev(&subdir, mount_err)?;
+            } else if mount_err {
+                anyhow::bail!("Found unexpected mount point {name:?}");
+            }
+        } else {
+            d.remove_file_optional(&name)?;
         }
     }
     anyhow::Ok(())
@@ -1576,13 +1585,15 @@ fn clean_boot_directories(rootfs: &Dir) -> Result<()> {
     let bootdir =
         crate::utils::open_dir_remount_rw(rootfs, BOOT.into()).context("Opening /boot")?;
     // This should not remove /boot/efi note.
-    remove_all_in_dir_no_xdev(&bootdir)?;
+    remove_all_in_dir_no_xdev(&bootdir, false)?;
+    // TODO: Discover the ESP the same way bootupd does it; we should also
+    // support not wiping the ESP.
     if ARCH_USES_EFI {
         if let Some(efidir) = bootdir
             .open_dir_optional(crate::bootloader::EFI_DIR)
             .context("Opening /boot/efi")?
         {
-            remove_all_in_dir_no_xdev(&efidir)?;
+            remove_all_in_dir_no_xdev(&efidir, false)?;
         }
     }
     Ok(())
@@ -1730,14 +1741,8 @@ pub(crate) async fn install_to_filesystem(
         Some(ReplaceMode::Wipe) => {
             let rootfs_fd = rootfs_fd.try_clone()?;
             println!("Wiping contents of root");
-            tokio::task::spawn_blocking(move || {
-                for e in rootfs_fd.entries()? {
-                    let e = e?;
-                    rootfs_fd.remove_all_optional(e.file_name())?;
-                }
-                anyhow::Ok(())
-            })
-            .await??;
+            tokio::task::spawn_blocking(move || remove_all_in_dir_no_xdev(&rootfs_fd, true))
+                .await??;
         }
         Some(ReplaceMode::Alongside) => clean_boot_directories(&rootfs_fd)?,
         None => require_empty_rootdir(&rootfs_fd)?,

lib/src/lints.rs

@@ -11,8 +11,6 @@ use cap_std_ext::cap_std;
 use cap_std_ext::dirext::CapStdExtDirExt as _;
 use fn_error_context::context;
 
-use crate::utils::openat2_with_retry;
-
 /// Reference to embedded default baseimage content that should exist.
 const BASEIMAGE_REF: &str = "usr/share/doc/bootc/baseimage/base";
 
@@ -72,25 +70,6 @@ fn check_kernel(root: &Dir) -> Result<()> {
     Ok(())
 }
 
-/// Open the target directory, but return Ok(None) if this would cross a mount point.
-fn open_dir_noxdev(
-    parent: &Dir,
-    path: impl AsRef<std::path::Path>,
-) -> std::io::Result<Option<Dir>> {
-    use rustix::fs::{Mode, OFlags, ResolveFlags};
-    match openat2_with_retry(
-        parent,
-        path,
-        OFlags::CLOEXEC | OFlags::DIRECTORY | OFlags::NOFOLLOW,
-        Mode::empty(),
-        ResolveFlags::NO_XDEV | ResolveFlags::BENEATH,
-    ) {
-        Ok(r) => Ok(Some(Dir::reopen_dir(&r)?)),
-        Err(e) if e == rustix::io::Errno::XDEV => Ok(None),
-        Err(e) => return Err(e.into()),
-    }
-}
-
 fn check_utf8(dir: &Dir) -> Result<()> {
     for entry in dir.entries()? {
         let entry = entry?;
@@ -109,7 +88,7 @@ fn check_utf8(dir: &Dir) -> Result<()> {
                 "/{strname}: Found non-utf8 symlink target"
             );
         } else if ifmt.is_dir() {
-            let Some(subdir) = open_dir_noxdev(dir, entry.file_name())? else {
+            let Some(subdir) = crate::utils::open_dir_noxdev(dir, entry.file_name())? else {
                 continue;
             };
             if let Err(err) = check_utf8(&subdir) {
@@ -181,17 +160,6 @@ mod tests {
         Ok(tempdir)
     }
 
-    #[test]
-    fn test_open_noxdev() -> Result<()> {
-        let root = Dir::open_ambient_dir("/", cap_std::ambient_authority())?;
-        // This hard requires the host setup to have /usr/bin on the same filesystem as /
-        let usr = Dir::open_ambient_dir("/usr", cap_std::ambient_authority())?;
-        assert!(open_dir_noxdev(&usr, "bin").unwrap().is_some());
-        // Requires a mounted /proc, but that also seems ane.
-        assert!(open_dir_noxdev(&root, "proc").unwrap().is_none());
-        Ok(())
-    }
-
     #[test]
     fn test_var_run() -> Result<()> {
         let root = &fixture()?;

lib/src/utils.rs

@@ -110,6 +110,25 @@ pub(crate) fn open_dir_remount_rw(root: &Dir, target: &Utf8Path) -> Result<Dir>
     root.open_dir(target).map_err(anyhow::Error::new)
 }
 
+/// Open the target directory, but return Ok(None) if this would cross a mount point.
+pub fn open_dir_noxdev(
+    parent: &Dir,
+    path: impl AsRef<std::path::Path>,
+) -> std::io::Result<Option<Dir>> {
+    use rustix::fs::{Mode, OFlags, ResolveFlags};
+    match openat2_with_retry(
+        parent,
+        path,
+        OFlags::CLOEXEC | OFlags::DIRECTORY | OFlags::NOFOLLOW,
+        Mode::empty(),
+        ResolveFlags::NO_XDEV | ResolveFlags::BENEATH,
+    ) {
+        Ok(r) => Ok(Some(Dir::reopen_dir(&r)?)),
+        Err(e) if e == rustix::io::Errno::XDEV => Ok(None),
+        Err(e) => return Err(e.into()),
+    }
+}
+
 /// Given a target path, remove its immutability if present
 #[context("Removing immutable flag from {target}")]
 pub(crate) fn remove_immutability(root: &Dir, target: &Utf8Path) -> Result<()> {
@@ -223,6 +242,8 @@ pub(crate) fn digested_pullspec(image: &str, digest: &str) -> String {
 
 #[cfg(test)]
 mod tests {
+    use cap_std_ext::cap_std;
+
     use super::*;
 
     #[test]
@@ -269,4 +290,15 @@ mod tests {
             SignatureSource::ContainerPolicyAllowInsecure
         );
     }
+
+    #[test]
+    fn test_open_noxdev() -> Result<()> {
+        let root = Dir::open_ambient_dir("/", cap_std::ambient_authority())?;
+        // This hard requires the host setup to have /usr/bin on the same filesystem as /
+        let usr = Dir::open_ambient_dir("/usr", cap_std::ambient_authority())?;
+        assert!(open_dir_noxdev(&usr, "bin").unwrap().is_some());
+        // Requires a mounted /proc, but that also seems ane.
+        assert!(open_dir_noxdev(&root, "proc").unwrap().is_none());
+        Ok(())
+    }
 }