Replace use of readdir()/dirent.d_type with stat()/st_mode. (#1874)

d_type is documented as only working with some filesystems, and that
all programs must handle a value of DT_UNKNOWN and make a stat() call
to retrieve the full information.  To keep the code simpler, always make
the stat() call.

Signed-off-by: Greg Greenway <ggreenway@apple.com>

include/envoy/api/os_sys_calls.h

@@ -2,6 +2,7 @@
 
 #include <sys/mman.h>   // for mode_t
 #include <sys/socket.h> // for sockaddr
+#include <sys/stat.h>
 
 #include <memory>
 #include <string>
@@ -57,6 +58,11 @@ class OsSysCalls {
    * @see man 2 mmap
    */
   virtual void* mmap(void* addr, size_t length, int prot, int flags, int fd, off_t offset) PURE;
+
+  /**
+   * @see man 2 stat
+   */
+  virtual int stat(const char* pathname, struct stat* buf) PURE;
 };
 
 typedef std::unique_ptr<OsSysCalls> OsSysCallsPtr;

source/common/api/os_sys_calls_impl.cc

@@ -33,5 +33,7 @@ void* OsSysCallsImpl::mmap(void* addr, size_t length, int prot, int flags, int f
   return ::mmap(addr, length, prot, flags, fd, offset);
 }
 
+int OsSysCallsImpl::stat(const char* pathname, struct stat* buf) { return ::stat(pathname, buf); }
+
 } // namespace Api
 } // namespace Envoy

source/common/api/os_sys_calls_impl.h

@@ -16,6 +16,7 @@ class OsSysCallsImpl : public OsSysCalls {
   int shmUnlink(const char* name) override;
   int ftruncate(int fd, off_t length) override;
   void* mmap(void* addr, size_t length, int prot, int flags, int fd, off_t offset) override;
+  int stat(const char* pathname, struct stat* buf) override;
 };
 
 } // namespace Api

source/common/runtime/BUILD

@@ -14,6 +14,7 @@ envoy_cc_library(
     hdrs = ["runtime_impl.h"],
     external_deps = ["ssl"],
     deps = [
+        "//include/envoy/api:os_sys_calls_interface",
         "//include/envoy/common:optional",
         "//include/envoy/event:dispatcher_interface",
         "//include/envoy/runtime:runtime_interface",

source/common/runtime/runtime_impl.cc

@@ -148,8 +148,9 @@ std::string RandomGeneratorImpl::uuid() {
 }
 
 SnapshotImpl::SnapshotImpl(const std::string& root_path, const std::string& override_path,
-                           RuntimeStats& stats, RandomGenerator& generator)
-    : generator_(generator) {
+                           RuntimeStats& stats, RandomGenerator& generator,
+                           Api::OsSysCalls& os_sys_calls)
+    : generator_(generator), os_sys_calls_(os_sys_calls) {
   try {
     walkDirectory(root_path, "");
     if (Filesystem::directoryExists(override_path)) {
@@ -208,10 +209,16 @@ void SnapshotImpl::walkDirectory(const std::string& path, const std::string& pre
       full_prefix = prefix + "." + entry->d_name;
     }
 
-    if (entry->d_type == DT_DIR && std::string(entry->d_name) != "." &&
+    struct stat stat_result;
+    int rc = os_sys_calls_.stat(full_path.c_str(), &stat_result);
+    if (rc != 0) {
+      throw EnvoyException(fmt::format("unable to stat file: '{}'", full_path));
+    }
+
+    if (S_ISDIR(stat_result.st_mode) && std::string(entry->d_name) != "." &&
         std::string(entry->d_name) != "..") {
       walkDirectory(full_path, full_prefix);
-    } else if (entry->d_type == DT_REG) {
+    } else if (S_ISREG(stat_result.st_mode)) {
       // Suck the file into a string. This is not very efficient but it should be good enough
       // for small files. Also, as noted elsewhere, none of this is non-blocking which could
       // theoretically lead to issues.
@@ -235,10 +242,11 @@ void SnapshotImpl::walkDirectory(const std::string& path, const std::string& pre
 LoaderImpl::LoaderImpl(Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator& tls,
                        const std::string& root_symlink_path, const std::string& subdir,
                        const std::string& override_dir, Stats::Store& store,
-                       RandomGenerator& generator)
+                       RandomGenerator& generator, Api::OsSysCallsPtr os_sys_calls)
     : watcher_(dispatcher.createFilesystemWatcher()), tls_(tls.allocateSlot()),
       generator_(generator), root_path_(root_symlink_path + "/" + subdir),
-      override_path_(root_symlink_path + "/" + override_dir), stats_(generateStats(store)) {
+      override_path_(root_symlink_path + "/" + override_dir), stats_(generateStats(store)),
+      os_sys_calls_(std::move(os_sys_calls)) {
   watcher_->addWatch(root_symlink_path, Filesystem::Watcher::Events::MovedTo,
                      [this](uint32_t) -> void { onSymlinkSwap(); });
 
@@ -253,7 +261,8 @@ RuntimeStats LoaderImpl::generateStats(Stats::Store& store) {
 }
 
 void LoaderImpl::onSymlinkSwap() {
-  current_snapshot_.reset(new SnapshotImpl(root_path_, override_path_, stats_, generator_));
+  current_snapshot_.reset(
+      new SnapshotImpl(root_path_, override_path_, stats_, generator_, *os_sys_calls_));
   ThreadLocal::ThreadLocalObjectSharedPtr ptr_copy = current_snapshot_;
   tls_->set([ptr_copy](Event::Dispatcher&) -> ThreadLocal::ThreadLocalObjectSharedPtr {
     return ptr_copy;

source/common/runtime/runtime_impl.h

@@ -7,6 +7,7 @@
 #include <string>
 #include <unordered_map>
 
+#include "envoy/api/os_sys_calls.h"
 #include "envoy/common/exception.h"
 #include "envoy/common/optional.h"
 #include "envoy/runtime/runtime.h"
@@ -62,7 +63,7 @@ class SnapshotImpl : public Snapshot,
                      Logger::Loggable<Logger::Id::runtime> {
 public:
   SnapshotImpl(const std::string& root_path, const std::string& override_path, RuntimeStats& stats,
-               RandomGenerator& generator);
+               RandomGenerator& generator, Api::OsSysCalls& os_sys_calls);
 
   // Runtime::Snapshot
   bool featureEnabled(const std::string& key, uint64_t default_value, uint64_t random_value,
@@ -114,6 +115,7 @@ class SnapshotImpl : public Snapshot,
 
   std::unordered_map<std::string, Entry> values_;
   RandomGenerator& generator_;
+  Api::OsSysCalls& os_sys_calls_;
 };
 
 /**
@@ -126,7 +128,8 @@ class LoaderImpl : public Loader {
 public:
   LoaderImpl(Event::Dispatcher& dispatcher, ThreadLocal::SlotAllocator& tls,
              const std::string& root_symlink_path, const std::string& subdir,
-             const std::string& override_dir, Stats::Store& store, RandomGenerator& generator);
+             const std::string& override_dir, Stats::Store& store, RandomGenerator& generator,
+             Api::OsSysCallsPtr os_sys_calls);
 
   // Runtime::Loader
   Snapshot& snapshot() override;
@@ -142,6 +145,7 @@ class LoaderImpl : public Loader {
   std::string override_path_;
   std::shared_ptr<SnapshotImpl> current_snapshot_;
   RuntimeStats stats_;
+  Api::OsSysCallsPtr os_sys_calls_;
 };
 
 /**

source/server/server.cc

@@ -14,6 +14,7 @@
 #include "envoy/upstream/cluster_manager.h"
 
 #include "common/api/api_impl.h"
+#include "common/api/os_sys_calls_impl.h"
 #include "common/common/utility.h"
 #include "common/common/version.h"
 #include "common/config/bootstrap_json.h"
@@ -260,9 +261,11 @@ Runtime::LoaderPtr InstanceUtil::createRuntime(Instance& server,
         config.runtime()->overrideSubdirectory() + "/" + server.localInfo().clusterName();
     ENVOY_LOG(info, "runtime override subdirectory: {}", override_subdirectory);
 
+    Api::OsSysCallsPtr os_sys_calls(new Api::OsSysCallsImpl);
     return Runtime::LoaderPtr{new Runtime::LoaderImpl(
         server.dispatcher(), server.threadLocal(), config.runtime()->symlinkRoot(),
-        config.runtime()->subdirectory(), override_subdirectory, server.stats(), server.random())};
+        config.runtime()->subdirectory(), override_subdirectory, server.stats(), server.random(),
+        std::move(os_sys_calls))};
   } else {
     return Runtime::LoaderPtr{new Runtime::NullLoaderImpl(server.random())};
   }

test/common/runtime/BUILD

@@ -22,6 +22,7 @@ envoy_cc_test(
     deps = [
         "//source/common/runtime:runtime_lib",
         "//source/common/stats:stats_lib",
+        "//test/mocks/api:api_mocks",
         "//test/mocks/event:event_mocks",
         "//test/mocks/filesystem:filesystem_mocks",
         "//test/mocks/runtime:runtime_mocks",

test/common/runtime/runtime_impl_test.cc

@@ -4,6 +4,7 @@
 #include "common/runtime/runtime_impl.h"
 #include "common/stats/stats_impl.h"
 
+#include "test/mocks/api/mocks.h"
 #include "test/mocks/event/mocks.h"
 #include "test/mocks/filesystem/mocks.h"
 #include "test/mocks/runtime/mocks.h"
@@ -13,9 +14,11 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
+using testing::Invoke;
 using testing::NiceMock;
 using testing::Return;
 using testing::ReturnNew;
+using testing::_;
 
 namespace Envoy {
 namespace Runtime {
@@ -68,24 +71,34 @@ class RuntimeImplTest : public testing::Test {
         {TestEnvironment::runfilesPath("test/common/runtime/filesystem_setup.sh")});
   }
 
-  void setup(const std::string& primary_dir, const std::string& override_dir) {
+  void setup() {
     EXPECT_CALL(dispatcher, createFilesystemWatcher_())
         .WillOnce(ReturnNew<NiceMock<Filesystem::MockWatcher>>());
 
+    os_sys_calls_ = new NiceMock<Api::MockOsSysCalls>;
+    ON_CALL(*os_sys_calls_, stat(_, _))
+        .WillByDefault(
+            Invoke([](const char* filename, struct stat* stat) { return ::stat(filename, stat); }));
+  }
+
+  void run(const std::string& primary_dir, const std::string& override_dir) {
+    Api::OsSysCallsPtr os_sys_calls(os_sys_calls_);
     loader.reset(new LoaderImpl(dispatcher, tls, TestEnvironment::temporaryPath(primary_dir),
-                                "envoy", override_dir, store, generator));
+                                "envoy", override_dir, store, generator, std::move(os_sys_calls)));
   }
 
   Event::MockDispatcher dispatcher;
   NiceMock<ThreadLocal::MockInstance> tls;
+  NiceMock<Api::MockOsSysCalls>* os_sys_calls_{};
 
   Stats::IsolatedStoreImpl store;
   MockRandomGenerator generator;
   std::unique_ptr<LoaderImpl> loader;
 };
 
 TEST_F(RuntimeImplTest, All) {
-  setup("test/common/runtime/test_data/current", "envoy_override");
+  setup();
+  run("test/common/runtime/test_data/current", "envoy_override");
 
   // Basic string getting.
   EXPECT_EQ("world", loader->snapshot().get("file2"));
@@ -116,10 +129,21 @@ TEST_F(RuntimeImplTest, All) {
   EXPECT_EQ("hello override", loader->snapshot().get("file1"));
 }
 
-TEST_F(RuntimeImplTest, BadDirectory) { setup("/baddir", "/baddir"); }
+TEST_F(RuntimeImplTest, BadDirectory) {
+  setup();
+  run("/baddir", "/baddir");
+}
+
+TEST_F(RuntimeImplTest, BadStat) {
+  setup();
+  EXPECT_CALL(*os_sys_calls_, stat(_, _)).WillOnce(Return(-1));
+  run("test/common/runtime/test_data/current", "envoy_override");
+  EXPECT_EQ(store.counter("runtime.load_error").value(), 1);
+}
 
 TEST_F(RuntimeImplTest, OverrideFolderDoesNotExist) {
-  setup("test/common/runtime/test_data/current", "envoy_override_does_not_exist");
+  setup();
+  run("test/common/runtime/test_data/current", "envoy_override_does_not_exist");
 
   EXPECT_EQ("hello", loader->snapshot().get("file1"));
 }

test/mocks/api/mocks.h

@@ -39,7 +39,7 @@ class MockOsSysCalls : public OsSysCalls {
   MockOsSysCalls();
   ~MockOsSysCalls();
 
-  // Filesystem::OsSysCalls
+  // Api::OsSysCalls
   ssize_t write(int fd, const void* buffer, size_t num_bytes) override;
   int open(const std::string& full_path, int flags, int mode) override;
   MOCK_METHOD3(bind, int(int sockfd, const sockaddr* addr, socklen_t addrlen));
@@ -50,6 +50,7 @@ class MockOsSysCalls : public OsSysCalls {
   MOCK_METHOD1(shmUnlink, int(const char*));
   MOCK_METHOD2(ftruncate, int(int fd, off_t length));
   MOCK_METHOD6(mmap, void*(void* addr, size_t length, int prot, int flags, int fd, off_t offset));
+  MOCK_METHOD2(stat, int(const char* name, struct stat* stat));
 
   size_t num_writes_;
   size_t num_open_;

test/test_common/utility.cc

@@ -117,12 +117,16 @@ std::vector<std::string> TestUtility::listFiles(const std::string& path, bool re
   dirent* entry;
   while ((entry = readdir(dir)) != nullptr) {
     std::string file_name = fmt::format("{}/{}", path, std::string(entry->d_name));
-    if (recursive && entry->d_type == DT_DIR && std::string(entry->d_name) != "." &&
+    struct stat stat_result;
+    int rc = ::stat(file_name.c_str(), &stat_result);
+    EXPECT_EQ(rc, 0);
+
+    if (recursive && S_ISDIR(stat_result.st_mode) && std::string(entry->d_name) != "." &&
         std::string(entry->d_name) != "..") {
       std::vector<std::string> more_file_names = listFiles(file_name, recursive);
       file_names.insert(file_names.end(), more_file_names.begin(), more_file_names.end());
       continue;
-    } else if (entry->d_type == DT_DIR) {
+    } else if (S_ISDIR(stat_result.st_mode)) {
       continue;
     }