libbrillo: Allow SafeFD::Rmdir() to continue recursing after error

Add an option to allow SafeFD::Rmdir() to continue deleting files and
directories even if some of the elements cannot be deleted.

BUG=chromium:1072467
TEST=cros_workon_make --board=${BOARD} chromeos-base/libbrillo --test

Change-Id: Ic247892489afe4c5b52ce92533906188d425fb59
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2168294
Tested-by: Ereth McKnight-MacNeil <ereth@chromium.org>
Reviewed-by: Allen Webb <allenwebb@google.com>
Reviewed-by: Mattias Nissler <mnissler@chromium.org>
Commit-Queue: Ereth McKnight-MacNeil <ereth@chromium.org>
Cr-Mirrored-From: https://chromium.googlesource.com/chromiumos/platform2
Cr-Mirrored-Commit: d74d8aef7dc17e3b5894e1ccc3eaa9428ec58463

3 files changed, 98 insertions, 23 deletions

diff --git a/brillo/files/safe_fd.cc b/brillo/files/safe_fd.cc
index 855207d..ac19dc3 100644
--- a/brillo/files/safe_fd.cc
+++ b/brillo/files/safe_fd.cc
@@ -446,7 +446,8 @@ SafeFD::Error SafeFD::Unlink(const std::string& name) {
 
 SafeFD::Error SafeFD::Rmdir(const std::string& name,
                             bool recursive,
-                            size_t max_depth) {
+                            size_t max_depth,
+                            bool keep_going) {
   if (!fd_.is_valid()) {
     return SafeFD::Error::kNotInitialized;
   }
@@ -460,6 +461,8 @@ SafeFD::Error SafeFD::Rmdir(const std::string& name,
     return err;
   }
 
+  SafeFD::Error last_err = SafeFD::Error::kNoError;
+
   if (recursive) {
     SafeFD dir_fd;
     std::tie(dir_fd, err) =
@@ -490,32 +493,36 @@ SafeFD::Error SafeFD::Rmdir(const std::string& name,
     errno = 0;
     const dirent* entry = HANDLE_EINTR_IF_EQ(readdir(dir.get()), nullptr);
     while (entry != nullptr) {
-      if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0) {
-        goto continue_;
-      }
+      SafeFD::Error err = [&]() {
+        if (strcmp(entry->d_name, ".") == 0 ||
+            strcmp(entry->d_name, "..") == 0) {
+          return SafeFD::Error::kNoError;
+        }
 
-      struct stat child_info;
-      if (fstatat(dir_fd.get(), entry->d_name, &child_info,
-                  AT_NO_AUTOMOUNT | AT_SYMLINK_NOFOLLOW) != 0) {
-        return SafeFD::Error::kIOError;
-      }
+        struct stat child_info;
+        if (fstatat(dir_fd.get(), entry->d_name, &child_info,
+                    AT_NO_AUTOMOUNT | AT_SYMLINK_NOFOLLOW) != 0) {
+          return SafeFD::Error::kIOError;
+        }
 
-      if (child_info.st_dev != dir_info.st_dev) {
-        return SafeFD::Error::kBoundaryDetected;
-      }
+        if (child_info.st_dev != dir_info.st_dev) {
+          return SafeFD::Error::kBoundaryDetected;
+        }
 
-      SafeFD::Error err;
-      if (entry->d_type == DT_DIR) {
-        err = dir_fd.Rmdir(entry->d_name, true, max_depth - 1);
-      } else {
-        err = dir_fd.Unlink(entry->d_name);
-      }
+        if (entry->d_type != DT_DIR) {
+          return dir_fd.Unlink(entry->d_name);
+        }
+
+        return dir_fd.Rmdir(entry->d_name, true, max_depth - 1, keep_going);
+      }();
 
       if (IsError(err)) {
-        return err;
+        if (!keep_going) {
+          return err;
+        }
+        last_err = err;
       }
 
-    continue_:
       errno = 0;
       entry = HANDLE_EINTR_IF_EQ(readdir(dir.get()), nullptr);
     }
@@ -530,9 +537,16 @@ SafeFD::Error SafeFD::Rmdir(const std::string& name,
     if (errno == ENOTDIR) {
       return SafeFD::Error::kWrongType;
     }
+    // If there was an error during the recursive delete, we expect unlink
+    // to fail with ENOTEMPTY and we bubble the error from recursion
+    // instead.
+    if (IsError(last_err) && errno == ENOTEMPTY) {
+      return last_err;
+    }
     return SafeFD::Error::kIOError;
   }
-  return SafeFD::Error::kNoError;
+
+  return last_err;
 }
 
 }  // namespace brillo
diff --git a/brillo/files/safe_fd.h b/brillo/files/safe_fd.h
index 5e126b4..3c77362 100644
--- a/brillo/files/safe_fd.h
+++ b/brillo/files/safe_fd.h
@@ -183,10 +183,13 @@ class SafeFD {
   //  recursive - if true also unlink child paths.
   //  max_depth - limit on recursion depth to prevent fd exhaustion and stack
   //    overflows.
+  //  keep_going - in recursive case continue deleting even in the face of
+  //    errors. If all entries cannot be deleted, the last error encountered
+  //    during recursion is returned.
   BRILLO_EXPORT Error Rmdir(const std::string& name,
                             bool recursive = false,
-                            size_t max_depth = kDefaultMaxPathDepth)
-      WARN_UNUSED_RESULT;
+                            size_t max_depth = kDefaultMaxPathDepth,
+                            bool keep_going = true) WARN_UNUSED_RESULT;
 
  private:
   BRILLO_EXPORT static const char* RootPath;
diff --git a/brillo/files/safe_fd_test.cc b/brillo/files/safe_fd_test.cc
index fff3a6c..6e08fdd 100644
--- a/brillo/files/safe_fd_test.cc
+++ b/brillo/files/safe_fd_test.cc
@@ -624,4 +624,62 @@ TEST_F(SafeFDTest, Rmdir_WrongType) {
   EXPECT_EQ(subdir.first.Rmdir(kFileName), SafeFD::Error::kWrongType);
 }
 
+TEST_F(SafeFDTest, Rmdir_Recursive_KeepGoing) {
+  ASSERT_TRUE(SetupSubdir());
+
+  ASSERT_TRUE(base::CreateDirectory(sub_dir_path_.Append(kSubdirName)));
+
+  // We cannot control the directory listing order. But we can ensure
+  // there are more than 1e29 orderings, of which one would result in a
+  // false pass.
+  constexpr int kNumSentinel = 25;
+  for (int i = 0; i < kNumSentinel; i++) {
+    SafeFD::SafeFDResult file =
+        root_.MakeFile(sub_dir_path_.Append(GetRandomSuffix()));
+    EXPECT_EQ(file.second, SafeFD::Error::kNoError);
+    ASSERT_TRUE(file.first.is_valid());
+  }
+
+  // Recursively delete with a max level that is too small.
+  EXPECT_EQ(root_.Rmdir(kSubdirName, true /*recursive*/, 1 /*max_depth*/,
+                        true /*keep_going*/),
+            SafeFD::Error::kExceededMaximum);
+
+  // The deep directory must still exist.
+  ASSERT_TRUE(
+      base::DeleteFile(sub_dir_path_.Append(kSubdirName), false /*recursive*/));
+
+  // But we must have kept going and deleted all other entries.
+  ASSERT_TRUE(base::IsDirectoryEmpty(sub_dir_path_));
+}
+
+TEST_F(SafeFDTest, Rmdir_Recursive_StopOnError) {
+  ASSERT_TRUE(SetupSubdir());
+
+  ASSERT_TRUE(base::CreateDirectory(sub_dir_path_.Append(kSubdirName)));
+
+  // We cannot control the directory listing order. But we can ensure
+  // there are more than 1e29 orderings, of which one would result in a
+  // false failure.
+  constexpr int kNumSentinel = 25;
+  for (int i = 0; i < kNumSentinel; i++) {
+    SafeFD::SafeFDResult file =
+        root_.MakeFile(sub_dir_path_.Append(GetRandomSuffix()));
+    EXPECT_EQ(file.second, SafeFD::Error::kNoError);
+    ASSERT_TRUE(file.first.is_valid());
+  }
+
+  // Recursively delete with a max level that is too small.
+  EXPECT_EQ(root_.Rmdir(kSubdirName, true /*recursive*/, 1 /*max_depth*/,
+                        false /*keep_going*/),
+            SafeFD::Error::kExceededMaximum);
+
+  // The deep directory must still exist.
+  ASSERT_TRUE(
+      base::DeleteFile(sub_dir_path_.Append(kSubdirName), false /*recursive*/));
+
+  // But there has to be at least a few files left since we gave up.
+  ASSERT_FALSE(base::IsDirectoryEmpty(sub_dir_path_));
+}
+
 }  // namespace brillo