Make directory iterators not throw exceptions (envoyproxy#31062)

* Make directory iterators not throw exceptions With the file system cache using directory iterators in the eviction thread, exceptions can be a problem (e.g. if a file is deleted while the directory iterator iterates, a `stat()` call can be provoked to fail and throw an exception). This change removes those exceptions, and also makes the directory iterator behavior less ambiguous with respect to trailing slashes (one error message showed `path//file` which should be remedied by this). Incidentally partially cleaned up `THROW_IF_NOT_OK` set of macros. --------- Signed-off-by: Raven Black <[email protected]>
Winbobob · Dec 1, 2023 · 4d154dd · 4d154dd
1 parent 7b911de
commit 4d154dd
Show file tree

Hide file tree

Showing 10 changed files with 124 additions and 54 deletions.
diff --git a/envoy/common/exception.h b/envoy/common/exception.h
@@ -29,30 +29,27 @@ class EnvoyException : public std::runtime_error {
   EnvoyException(const std::string& message) : std::runtime_error(message) {}
 };
 
-#define THROW_IF_NOT_OK(status_fn)                                                                 \
-  {                                                                                                \
-    const absl::Status status = status_fn;                                                         \
-    if (!status.ok()) {                                                                            \
-      throwEnvoyExceptionOrPanic(std::string(status.message()));                                   \
+#define THROW_IF_NOT_OK_REF(status)                                                                \
+  do {                                                                                             \
+    if (!(status).ok()) {                                                                          \
+      throwEnvoyExceptionOrPanic(std::string((status).message()));                                 \
     }                                                                                              \
-  }
+  } while (0)
+
+#define THROW_IF_NOT_OK(status_fn)                                                                 \
+  do {                                                                                             \
+    const absl::Status status = (status_fn);                                                       \
+    THROW_IF_NOT_OK_REF(status);                                                                   \
+  } while (0)
 
 // Simple macro to handle bridging functions which return absl::StatusOr, and
 // functions which throw errors.
 //
-// The completely unnecessary throw action argument is just so 'throw' appears
-// at the call site, so format checks about use of exceptions are triggered.
-#ifdef ENVOY_DISABLE_EXCEPTIONS
-#define THROW_IF_STATUS_NOT_OK(variable, throw_action)                                             \
-  if (!variable.status().ok()) {                                                                   \
-    PANIC(std::string(variable.status().message()));                                               \
-  }
-#else
-#define THROW_IF_STATUS_NOT_OK(variable, throw_action)                                             \
-  if (!variable.status().ok()) {                                                                   \
-    throw_action EnvoyException(std::string(variable.status().message()));                         \
-  }
-#endif
+// The completely unnecessary throw_action argument was just so 'throw' appears
+// at the call site, so format checks about use of exceptions would be triggered.
+// This didn't work, so the variable is no longer used and is not duplicated in
+// the macros above.
+#define THROW_IF_STATUS_NOT_OK(variable, throw_action) THROW_IF_NOT_OK_REF(variable.status());
 
 #define RETURN_IF_STATUS_NOT_OK(variable)                                                          \
   if (!variable.status().ok()) {                                                                   \

diff --git a/envoy/filesystem/filesystem.h b/envoy/filesystem/filesystem.h
@@ -241,6 +241,9 @@ struct DirectoryEntry {
 };
 
 class DirectoryIteratorImpl;
+
+// Failures during this iteration will be silent; check status() after initialization
+// and after each increment, if error-handling is desired.
 class DirectoryIterator {
 public:
   DirectoryIterator() : entry_({"", FileType::Other, absl::nullopt}) {}
@@ -252,8 +255,11 @@ class DirectoryIterator {
 
   virtual DirectoryIteratorImpl& operator++() PURE;
 
+  const absl::Status& status() const { return status_; }
+
 protected:
   DirectoryEntry entry_;
+  absl::Status status_;
 };
 
 } // namespace Filesystem

diff --git a/source/common/filesystem/directory.h b/source/common/filesystem/directory.h
@@ -10,6 +10,11 @@ namespace Envoy {
 namespace Filesystem {
 
 // This class does not do any validation of input data. Do not use with untrusted inputs.
+//
+// DirectoryIteratorImpl will fail silently and act like an empty iterator in case of
+// error opening the directory, and will silently skip files that can't be stat'ed. Check
+// DirectoryIteratorImpl::status() after initialization and after each increment if
+// silent failure is not the desired behavior.
 class Directory {
 public:
   Directory(const std::string& directory_path) : directory_path_(directory_path) {}

diff --git a/source/common/filesystem/posix/directory_iterator_impl.cc b/source/common/filesystem/posix/directory_iterator_impl.cc
@@ -4,13 +4,20 @@
 #include "source/common/common/utility.h"
 #include "source/common/filesystem/directory_iterator_impl.h"
 
+#include "absl/strings/strip.h"
+
 namespace Envoy {
 namespace Filesystem {
 
 DirectoryIteratorImpl::DirectoryIteratorImpl(const std::string& directory_path)
-    : directory_path_(directory_path), os_sys_calls_(Api::OsSysCallsSingleton::get()) {
+    : directory_path_(absl::StripSuffix(directory_path, "/")),
+      os_sys_calls_(Api::OsSysCallsSingleton::get()) {
   openDirectory();
-  nextEntry();
+  if (status_.ok()) {
+    nextEntry();
+  } else {
+    entry_ = {"", FileType::Other, absl::nullopt};
+  }
 }
 
 DirectoryIteratorImpl::~DirectoryIteratorImpl() {
@@ -28,27 +35,35 @@ void DirectoryIteratorImpl::openDirectory() {
   DIR* temp_dir = ::opendir(directory_path_.c_str());
   dir_ = temp_dir;
   if (!dir_) {
-    throwEnvoyExceptionOrPanic(
-        fmt::format("unable to open directory {}: {}", directory_path_, errorDetails(errno)));
+    status_ = absl::ErrnoToStatus(errno, fmt::format("unable to open directory {}: {}",
+                                                     directory_path_, errorDetails(errno)));
   }
 }
 
 void DirectoryIteratorImpl::nextEntry() {
   errno = 0;
   dirent* entry = ::readdir(dir_);
-  if (entry == nullptr && errno != 0) {
-    throwEnvoyExceptionOrPanic(
-        fmt::format("unable to iterate directory {}: {}", directory_path_, errorDetails(errno)));
-  }
 
   if (entry == nullptr) {
     entry_ = {"", FileType::Other, absl::nullopt};
+    if (errno != 0) {
+      status_ = absl::ErrnoToStatus(errno, fmt::format("unable to iterate directory {}: {}",
+                                                       directory_path_, errorDetails(errno)));
+    }
   } else {
-    entry_ = makeEntry(entry->d_name);
+    auto result = makeEntry(entry->d_name);
+    status_ = result.status();
+    if (!status_.ok()) {
+      // If we failed to stat one file it might have just been deleted,
+      // keep iterating over the rest of the dir.
+      return nextEntry();
+    } else {
+      entry_ = result.value();
+    }
   }
 }
 
-DirectoryEntry DirectoryIteratorImpl::makeEntry(absl::string_view filename) const {
+absl::StatusOr<DirectoryEntry> DirectoryIteratorImpl::makeEntry(absl::string_view filename) const {
   const std::string full_path = absl::StrCat(directory_path_, "/", filename);
   struct stat stat_buf;
   const Api::SysCallIntResult result = os_sys_calls_.stat(full_path.c_str(), &stat_buf);
@@ -62,10 +77,8 @@ DirectoryEntry DirectoryIteratorImpl::makeEntry(absl::string_view filename) cons
         return DirectoryEntry{std::string{filename}, FileType::Regular, absl::nullopt};
       }
     }
-    // TODO: throwing an exception here makes this dangerous to use in worker threads,
-    // and in general since it's not clear to the user of Directory that an exception
-    // may be thrown. Perhaps make this return StatusOr and handle failures gracefully.
-    throwEnvoyExceptionOrPanic(fmt::format("unable to stat file: '{}' ({})", full_path, errno));
+    return absl::ErrnoToStatus(
+        result.errno_, fmt::format("unable to stat file: '{}' ({})", full_path, result.errno_));
   } else if (S_ISDIR(stat_buf.st_mode)) {
     return DirectoryEntry{std::string{filename}, FileType::Directory, absl::nullopt};
   } else if (S_ISREG(stat_buf.st_mode)) {

diff --git a/source/common/filesystem/posix/directory_iterator_impl.h b/source/common/filesystem/posix/directory_iterator_impl.h
@@ -29,7 +29,7 @@ class DirectoryIteratorImpl : public DirectoryIterator {
   void nextEntry();
   void openDirectory();
 
-  DirectoryEntry makeEntry(absl::string_view filename) const;
+  absl::StatusOr<DirectoryEntry> makeEntry(absl::string_view filename) const;
 
   std::string directory_path_;
   DIR* dir_{nullptr};

diff --git a/source/common/filesystem/win32/directory_iterator_impl.cc b/source/common/filesystem/win32/directory_iterator_impl.cc
@@ -3,20 +3,27 @@
 #include "source/common/common/fmt.h"
 #include "source/common/filesystem/directory_iterator_impl.h"
 
+#include "absl/strings/strip.h"
+
 namespace Envoy {
 namespace Filesystem {
 
 DirectoryIteratorImpl::DirectoryIteratorImpl(const std::string& directory_path)
     : DirectoryIterator(), find_handle_(INVALID_HANDLE_VALUE) {
+  absl::string_view path = absl::StripSuffix(directory_path, "/");
+  path = absl::StripSuffix(path, "\\");
   WIN32_FIND_DATA find_data;
-  const std::string glob = directory_path + "\\*";
+  const std::string glob = absl::StrCat(path, "\\*");
   find_handle_ = ::FindFirstFile(glob.c_str(), &find_data);
   if (find_handle_ == INVALID_HANDLE_VALUE) {
-    throw EnvoyException(
-        fmt::format("unable to open directory {}: {}", directory_path, ::GetLastError()));
+    status_ =
+        absl::UnknownError(fmt::format("unable to open directory {}: {}", path, ::GetLastError()));
+  }
+  if (status_.ok()) {
+    entry_ = makeEntry(find_data);
+  } else {
+    entry_ = {"", FileType::Other, absl::nullopt};
   }
-
-  entry_ = makeEntry(find_data);
 }
 
 DirectoryIteratorImpl::~DirectoryIteratorImpl() {
@@ -29,12 +36,12 @@ DirectoryIteratorImpl& DirectoryIteratorImpl::operator++() {
   WIN32_FIND_DATA find_data;
   const BOOL ret = ::FindNextFile(find_handle_, &find_data);
   const DWORD err = ::GetLastError();
-  if (ret == 0 && err != ERROR_NO_MORE_FILES) {
-    throw EnvoyException(fmt::format("unable to iterate directory: {}", err));
-  }
 
   if (ret == 0) {
     entry_ = {"", FileType::Other, absl::nullopt};
+    if (err != ERROR_NO_MORE_FILES) {
+      status_ = absl::UnknownError(fmt::format("unable to iterate directory: {}", err));
+    }
   } else {
     entry_ = makeEntry(find_data);
   }

diff --git a/source/common/filter/config_discovery_impl.cc b/source/common/filter/config_discovery_impl.cc
@@ -225,7 +225,7 @@ void FilterConfigProviderManagerImplBase::applyLastOrDefaultConfig(
   bool last_config_valid = false;
   if (subscription->lastConfig()) {
     TRY_ASSERT_MAIN_THREAD {
-      THROW_IF_NOT_OK(provider.validateTypeUrl(subscription->lastTypeUrl()))
+      THROW_IF_NOT_OK(provider.validateTypeUrl(subscription->lastTypeUrl()));
       provider.validateMessage(filter_config_name, *subscription->lastConfig(),
                                subscription->lastFactoryName());
       last_config_valid = true;

diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc
@@ -425,7 +425,11 @@ void DiskLayer::walkDirectory(const std::string& path, const std::string& prefix
   }
 
   Filesystem::Directory directory(path);
-  for (const Filesystem::DirectoryEntry& entry : directory) {
+  Filesystem::DirectoryIteratorImpl it = directory.begin();
+  THROW_IF_NOT_OK_REF(it.status());
+  for (; it != directory.end(); ++it) {
+    THROW_IF_NOT_OK_REF(it.status());
+    Filesystem::DirectoryEntry entry = *it;
     std::string full_path = path + "/" + entry.name_;
     std::string full_prefix;
     if (prefix.empty()) {
@@ -474,6 +478,7 @@ void DiskLayer::walkDirectory(const std::string& path, const std::string& prefix
 #endif
     }
   }
+  THROW_IF_NOT_OK_REF(it.status());
 }
 
 ProtoLayer::ProtoLayer(absl::string_view name, const ProtobufWkt::Struct& proto)

diff --git a/test/common/filesystem/BUILD b/test/common/filesystem/BUILD
@@ -23,6 +23,7 @@ envoy_cc_test(
     deps = [
         "//source/common/filesystem:directory_lib",
         "//test/test_common:environment_lib",
+        "//test/test_common:status_utility_lib",
     ],
 )
 

diff --git a/test/common/filesystem/directory_test.cc b/test/common/filesystem/directory_test.cc
@@ -6,6 +6,7 @@
 #include "source/common/filesystem/directory.h"
 
 #include "test/test_common/environment.h"
+#include "test/test_common/status_utility.h"
 #include "test/test_common/utility.h"
 
 #include "absl/container/node_hash_set.h"
@@ -15,6 +16,9 @@
 namespace Envoy {
 namespace Filesystem {
 
+using StatusHelpers::HasStatus;
+using testing::HasSubstr;
+
 // NOLINTNEXTLINE(readability-identifier-naming)
 void PrintTo(const DirectoryEntry& entry, std::ostream* os) {
   *os << "{name=" << entry.name_ << ", type=" << static_cast<int>(entry.type_) << ", size=";
@@ -240,6 +244,40 @@ TEST_F(DirectoryTest, DirectoryWithBrokenSymlink) {
   EXPECT_EQ(expected, getDirectoryContents(dir_path_, false));
 }
 
+#ifndef WIN32
+// Test that removing a file while iterating continues to the next file.
+TEST_F(DirectoryTest, FileDeletedWhileIterating) {
+  addFiles({"file", "file2"});
+  DirectoryIteratorImpl dir_iterator(dir_path_);
+  EntrySet found;
+  TestEnvironment::removePath(dir_path_ + "/file");
+  TestEnvironment::removePath(dir_path_ + "/file2");
+  while (!(*dir_iterator).name_.empty()) {
+    found.insert(*dir_iterator);
+    ++dir_iterator;
+  }
+  // Test environment can potentially list files in any order, so if the
+  // first file was one of the temporary files it will still be in the
+  // set. The important thing is that at least one of them is *not* in
+  // the set, and we didn't crash.
+  EXPECT_THAT(found, testing::AnyOf(
+                         EntrySet{
+                             {".", FileType::Directory, absl::nullopt},
+                             {"..", FileType::Directory, absl::nullopt},
+                         },
+                         EntrySet{
+                             {".", FileType::Directory, absl::nullopt},
+                             {"..", FileType::Directory, absl::nullopt},
+                             {"file", FileType::Regular, 0},
+                         },
+                         EntrySet{
+                             {".", FileType::Directory, absl::nullopt},
+                             {"..", FileType::Directory, absl::nullopt},
+                             {"file1", FileType::Regular, 0},
+                         }));
+}
+#endif
+
 // Test that we can list an empty directory
 TEST_F(DirectoryTest, DirectoryWithEmptyDirectory) {
   const EntrySet expected = {
@@ -249,18 +287,16 @@ TEST_F(DirectoryTest, DirectoryWithEmptyDirectory) {
   EXPECT_EQ(expected, getDirectoryContents(dir_path_, false));
 }
 
-// Test that the constructor throws an exception when given a non-existing path
+// Test that the status is an error when given a non-existing path
 TEST(DirectoryIteratorImpl, NonExistingDir) {
   const std::string dir_path("some/non/existing/dir");
-
+  DirectoryIteratorImpl dir_iterator(dir_path);
 #ifdef WIN32
-  EXPECT_THROW_WITH_MESSAGE(
-      DirectoryIteratorImpl dir_iterator(dir_path), EnvoyException,
-      fmt::format("unable to open directory {}: {}", dir_path, ERROR_PATH_NOT_FOUND));
+  EXPECT_THAT(dir_iterator.status(),
+              HasStatus(absl::StatusCode::kUnknown, HasSubstr("unable to open directory")));
 #else
-  EXPECT_THROW_WITH_MESSAGE(
-      DirectoryIteratorImpl dir_iterator(dir_path), EnvoyException,
-      fmt::format("unable to open directory {}: No such file or directory", dir_path));
+  EXPECT_THAT(dir_iterator.status(),
+              HasStatus(absl::StatusCode::kNotFound, HasSubstr("unable to open directory")));
 #endif
 }
 
@@ -284,8 +320,8 @@ TEST_F(DirectoryTest, Fifo) {
 // instead by directly calling the private function.
 TEST_F(DirectoryTest, MakeEntryThrowsOnStatFailure) {
   Directory directory(dir_path_);
-  EXPECT_THROW_WITH_REGEX(directory.begin().makeEntry("foo"), EnvoyException,
-                          "unable to stat file: '.*foo' .*");
+  EXPECT_THAT(directory.begin().makeEntry("foo"),
+              HasStatus(absl::StatusCode::kNotFound, HasSubstr("unable to stat file")));
 }
 #endif