From ce51d882c0419b9bea107063524d87727fa469f6 Mon Sep 17 00:00:00 2001 From: Greg Greenway Date: Tue, 17 Oct 2017 15:46:15 -0700 Subject: [PATCH] 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 --- include/envoy/api/os_sys_calls.h | 6 +++++ source/common/api/os_sys_calls_impl.cc | 2 ++ source/common/api/os_sys_calls_impl.h | 1 + source/common/runtime/BUILD | 1 + source/common/runtime/runtime_impl.cc | 23 +++++++++++----- source/common/runtime/runtime_impl.h | 8 ++++-- source/server/server.cc | 5 +++- test/common/runtime/BUILD | 1 + test/common/runtime/runtime_impl_test.cc | 34 ++++++++++++++++++++---- test/mocks/api/mocks.h | 3 ++- test/test_common/utility.cc | 8 ++++-- 11 files changed, 74 insertions(+), 18 deletions(-) diff --git a/include/envoy/api/os_sys_calls.h b/include/envoy/api/os_sys_calls.h index e1ba4b0dee29..9843352ac569 100644 --- a/include/envoy/api/os_sys_calls.h +++ b/include/envoy/api/os_sys_calls.h @@ -2,6 +2,7 @@ #include // for mode_t #include // for sockaddr +#include #include #include @@ -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 OsSysCallsPtr; diff --git a/source/common/api/os_sys_calls_impl.cc b/source/common/api/os_sys_calls_impl.cc index 040d05c4828c..1c2de411d9a1 100644 --- a/source/common/api/os_sys_calls_impl.cc +++ b/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 diff --git a/source/common/api/os_sys_calls_impl.h b/source/common/api/os_sys_calls_impl.h index e14be7df3bf2..edbf658f5655 100644 --- a/source/common/api/os_sys_calls_impl.h +++ b/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 diff --git a/source/common/runtime/BUILD b/source/common/runtime/BUILD index bb2ce6cc6d1f..2e3199f94636 100644 --- a/source/common/runtime/BUILD +++ b/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", diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index a8a224905323..693e2787589e 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/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; diff --git a/source/common/runtime/runtime_impl.h b/source/common/runtime/runtime_impl.h index 12aaa426ee40..1b9a9414eb50 100644 --- a/source/common/runtime/runtime_impl.h +++ b/source/common/runtime/runtime_impl.h @@ -7,6 +7,7 @@ #include #include +#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 { 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 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 current_snapshot_; RuntimeStats stats_; + Api::OsSysCallsPtr os_sys_calls_; }; /** diff --git a/source/server/server.cc b/source/server/server.cc index a1dec267c0a9..a321f8c8449d 100644 --- a/source/server/server.cc +++ b/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())}; } diff --git a/test/common/runtime/BUILD b/test/common/runtime/BUILD index 43e33097f010..e9452ff24b93 100644 --- a/test/common/runtime/BUILD +++ b/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", diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc index 62ea45f30eec..abcf97750eec 100644 --- a/test/common/runtime/runtime_impl_test.cc +++ b/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,16 +71,25 @@ 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>()); + os_sys_calls_ = new NiceMock; + 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 tls; + NiceMock* os_sys_calls_{}; Stats::IsolatedStoreImpl store; MockRandomGenerator generator; @@ -85,7 +97,8 @@ class RuntimeImplTest : public testing::Test { }; 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")); } diff --git a/test/mocks/api/mocks.h b/test/mocks/api/mocks.h index a9090ce9b6e9..0d15602e8974 100644 --- a/test/mocks/api/mocks.h +++ b/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_; diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index 36014fa674e2..137384355020 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -117,12 +117,16 @@ std::vector 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 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; }