Remove folders with bad permissions

Author: palfrey

Cargo.lock

@@ -22,6 +22,7 @@ rust_library(
         "@crates//:serde_json5",
         "@crates//:tokio",
         "@crates//:tonic",
+        "@crates//:walkdir",
     ],
 )
 

nativelink-error/BUILD.bazel

@@ -30,3 +30,4 @@ tonic = { version = "0.13.0", features = [
   "tls-ring",
   "transport",
 ], default-features = false }
+walkdir = { version = "2.5.0", default-features = false }

nativelink-error/Cargo.toml

@@ -277,6 +277,12 @@ impl From<Error> for tonic::Status {
     }
 }
 
+impl From<walkdir::Error> for Error {
+    fn from(value: walkdir::Error) -> Self {
+        Self::new(Code::Internal, value.to_string())
+    }
+}
+
 pub trait ResultExt<T> {
     /// # Errors
     ///

nativelink-error/src/lib.rs

@@ -81,6 +81,7 @@ rust_library(
         "@crates//:tracing-opentelemetry",
         "@crates//:tracing-subscriber",
         "@crates//:uuid",
+        "@crates//:walkdir",
     ],
 )
 
@@ -94,6 +95,7 @@ rust_test_suite(
         "tests/common_test.rs",
         "tests/evicting_map_test.rs",
         "tests/fastcdc_test.rs",
+        "tests/fs_test.rs",
         "tests/health_utils_test.rs",
         "tests/operation_id_tests.rs",
         "tests/origin_event_test.rs",

nativelink-util/BUILD.bazel

@@ -84,6 +84,7 @@ uuid = { version = "1.16.0", default-features = false, features = [
   "v4",
   "v6",
 ] }
+walkdir = { version = "2.5.0", default-features = false }
 
 [dev-dependencies]
 nativelink-macro = { path = "../nativelink-macro" }

nativelink-util/Cargo.toml

@@ -15,7 +15,7 @@
 use core::pin::Pin;
 use core::sync::atomic::{AtomicUsize, Ordering};
 use core::task::{Context, Poll};
-use std::fs::Metadata;
+use std::fs::{Metadata, Permissions};
 use std::io::{IoSlice, Seek};
 use std::path::{Path, PathBuf};
 
@@ -255,10 +255,7 @@ pub async fn hard_link(src: impl AsRef<Path>, dst: impl AsRef<Path>) -> Result<(
     call_with_permit(move |_| std::fs::hard_link(src, dst).map_err(Into::<Error>::into)).await
 }
 
-pub async fn set_permissions(
-    src: impl AsRef<Path>,
-    perm: std::fs::Permissions,
-) -> Result<(), Error> {
+pub async fn set_permissions(src: impl AsRef<Path>, perm: Permissions) -> Result<(), Error> {
     let src = src.as_ref().to_owned();
     call_with_permit(move |_| std::fs::set_permissions(src, perm).map_err(Into::<Error>::into))
         .await
@@ -361,7 +358,63 @@ pub async fn symlink_metadata(path: impl AsRef<Path>) -> Result<Metadata, Error>
     call_with_permit(move |_| std::fs::symlink_metadata(path).map_err(Into::<Error>::into)).await
 }
 
+// We can't just use the stock remove_dir_all as it falls over if someone's set readonly
+// permissions. This version walks the directories and fixes the permissions where needed
+// before deleting everything.
+#[cfg(not(target_family = "windows"))]
+fn internal_remove_dir_all(path: impl AsRef<Path>) -> Result<(), Error> {
+    // Because otherwise Windows builds complain about these things not being used
+    use std::io::ErrorKind;
+    use std::os::unix::fs::PermissionsExt;
+
+    use tracing::debug;
+    use walkdir::WalkDir;
+
+    for entry in WalkDir::new(&path) {
+        let Ok(entry) = &entry else {
+            debug!("Can't get into {entry:?}, assuming already deleted");
+            continue;
+        };
+        let metadata = entry.metadata()?;
+        if metadata.is_dir() {
+            match std::fs::remove_dir_all(entry.path()) {
+                Ok(()) => {}
+                Err(e) if e.kind() == ErrorKind::PermissionDenied => {
+                    std::fs::set_permissions(entry.path(), Permissions::from_mode(0o700)).err_tip(
+                        || format!("Setting permissions for {}", entry.path().display()),
+                    )?;
+                }
+                e @ Err(_) => e.err_tip(|| format!("Removing {}", entry.path().display()))?,
+            }
+        } else if metadata.is_file() {
+            std::fs::set_permissions(entry.path(), Permissions::from_mode(0o600))
+                .err_tip(|| format!("Setting permissions for {}", entry.path().display()))?;
+        }
+    }
+
+    // should now be safe to delete after we fixed all the permissions in the walk loop
+    match std::fs::remove_dir_all(&path) {
+        Ok(()) => {}
+        Err(e) if e.kind() == ErrorKind::NotFound => {}
+        e @ Err(_) => e.err_tip(|| {
+            format!(
+                "Removing {} after permissions fixes",
+                path.as_ref().display()
+            )
+        })?,
+    }
+    Ok(())
+}
+
+// We can't set the permissions easily in Windows, so just fallback to
+// the stock Rust remove_dir_all
+#[cfg(target_family = "windows")]
+fn internal_remove_dir_all(path: impl AsRef<Path>) -> Result<(), Error> {
+    std::fs::remove_dir_all(&path)?;
+    Ok(())
+}
+
 pub async fn remove_dir_all(path: impl AsRef<Path>) -> Result<(), Error> {
     let path = path.as_ref().to_owned();
-    call_with_permit(move |_| std::fs::remove_dir_all(path).map_err(Into::<Error>::into)).await
+    call_with_permit(move |_| internal_remove_dir_all(path)).await
 }

nativelink-util/src/fs.rs

@@ -0,0 +1,39 @@
+#![cfg(not(target_family = "windows"))]
+// Because windows does permissions differently
+
+use std::env;
+use std::fs::{self, Permissions};
+use std::os::unix::fs::PermissionsExt;
+
+use nativelink_error::ResultExt;
+use nativelink_macro::nativelink_test;
+use nativelink_util::fs::remove_dir_all;
+
+#[nativelink_test]
+async fn remove_files_with_bad_permissions() -> Result<(), Box<dyn core::error::Error>> {
+    let temp_dir = env::temp_dir();
+    let bad_perms_directory = temp_dir.join("bad_perms_directory");
+    if fs::exists(&bad_perms_directory)? {
+        remove_dir_all(&bad_perms_directory)
+            .await
+            .err_tip(|| format!("first remove_dir_all for {bad_perms_directory:?}"))?;
+    }
+    fs::create_dir(&bad_perms_directory)?;
+    let bad_perms_file = bad_perms_directory.join("bad_perms_file");
+    if !fs::exists(&bad_perms_file)? {
+        fs::write(&bad_perms_file, "").err_tip(|| "Can't create file")?;
+    }
+
+    fs::set_permissions(&bad_perms_directory, Permissions::from_mode(0o100)) // execute owner only
+        .err_tip(|| "Can't set perms on directory")?;
+
+    fs::set_permissions(&bad_perms_file, Permissions::from_mode(0o400)) // read owner only
+        .err_tip(|| "Can't set perms on file")?;
+
+    remove_dir_all(&bad_perms_directory)
+        .await
+        .err_tip(|| format!("second remove_dir_all for {bad_perms_directory:?}"))?;
+
+    assert!(!fs::exists(&bad_perms_directory)?);
+    Ok(())
+}

nativelink-util/tests/fs_test.rs

@@ -465,9 +465,12 @@ pub async fn new_local_worker(
     );
 
     if let Ok(path) = fs::canonicalize(&config.work_directory).await {
-        fs::remove_dir_all(path)
-            .await
-            .err_tip(|| "Could not remove work_directory in LocalWorker")?;
+        fs::remove_dir_all(&path).await.err_tip(|| {
+            format!(
+                "Could not remove work_directory '{}' in LocalWorker",
+                &path.as_path().to_str().unwrap_or("bad path")
+            )
+        })?;
     }
 
     fs::create_dir_all(&config.work_directory)