From 1dc6e43b23e7de27064fa4e30c39a637697287a9 Mon Sep 17 00:00:00 2001 From: "William A. Rowe Jr" Date: Sun, 26 Jan 2020 13:11:49 -0600 Subject: [PATCH] Coalesce directory management to Envoy::TestEnvironment (#9721) - Drop std::[*]filesystem logic - Implement/tests for new Envoy::Filesystem::splitFileName - Remove unneeded TestEnvironment::createParentPath - Move/rename Envoy::TestUtility::createDirectory to Envoy::TestEnvironment::createPath - Move/rename Envoy::TestUtility::removeDirectory to Envoy::TestEnvironment::removePath - Move Envoy::TestUtility::renameFile to Envoy::TestEnvironment - Move Envoy::TestUtility::createSymlink to Envoy::TestEnvironment - Move AtomicFileUpdater from test_common utility to environment - Simplify Envoy::Filesystem::Watcher implementations using Filesystem - Clean up watcher_impl flavors for splitFileName API - Implement/new tests for Envoy::Filesystem::illegalPath for win32 - Implement watcher_impl for win32 [Initial development by Sam Smith] - Ensure top level test tempdir exists when requested Co-authored-by: Sunjay Bhatia Co-authored-by: William A Rowe Jr Co-authored-by: Sam Smith Signed-off-by: William A Rowe Jr --- STYLE.md | 3 +- include/envoy/filesystem/filesystem.h | 20 +- include/envoy/filesystem/watcher.h | 4 +- source/common/event/dispatcher_impl.cc | 2 +- source/common/filesystem/BUILD | 16 +- source/common/filesystem/directory.h | 2 + .../common/filesystem/inotify/watcher_impl.cc | 22 +- .../common/filesystem/inotify/watcher_impl.h | 6 +- .../common/filesystem/kqueue/watcher_impl.cc | 36 ++- .../common/filesystem/kqueue/watcher_impl.h | 8 +- .../posix/directory_iterator_impl.cc | 11 +- .../filesystem/posix/filesystem_impl.cc | 14 +- .../common/filesystem/posix/filesystem_impl.h | 1 + .../filesystem/win32/filesystem_impl.cc | 132 ++++++++- .../common/filesystem/win32/filesystem_impl.h | 1 + .../common/filesystem/win32/watcher_impl.cc | 266 ++++++++++++++++++ source/common/filesystem/win32/watcher_impl.h | 74 +++++ .../filesystem_subscription_test_harness.h | 8 +- test/common/filesystem/directory_test.cc | 26 +- .../common/filesystem/filesystem_impl_test.cc | 85 +++++- test/common/filesystem/watcher_impl_test.cc | 41 +-- test/common/runtime/runtime_impl_test.cc | 4 +- test/config/utility.cc | 6 +- .../json_transcoder_filter_test.cc | 2 +- .../http/tap/tap_filter_integration_test.cc | 5 +- .../quiche/platform/quic_platform_test.cc | 4 +- test/mocks/filesystem/mocks.h | 3 +- test/server/server_test.cc | 4 +- .../server/unparseable_bootstrap.yaml | 1 + test/test_common/BUILD | 1 + test/test_common/environment.cc | 181 ++++++++---- test/test_common/environment.h | 44 ++- test/test_common/utility.cc | 58 ---- test/test_common/utility.h | 21 -- tools/check_format.py | 45 ++- tools/check_format_test_helper.py | 6 + tools/spelling_dictionary.txt | 4 + .../check_format/clang_format_double_off.cc | 7 + .../check_format/clang_format_double_on.cc | 6 + .../testdata/check_format/clang_format_off.cc | 13 + .../testdata/check_format/clang_format_on.cc | 9 + .../check_format/clang_format_on.cc.gold | 9 + .../check_format/clang_format_trailing_off.cc | 6 + 43 files changed, 959 insertions(+), 258 deletions(-) create mode 100644 source/common/filesystem/win32/watcher_impl.cc create mode 100644 source/common/filesystem/win32/watcher_impl.h create mode 100644 test/server/test_data/server/unparseable_bootstrap.yaml create mode 100644 tools/testdata/check_format/clang_format_double_off.cc create mode 100644 tools/testdata/check_format/clang_format_double_on.cc create mode 100644 tools/testdata/check_format/clang_format_off.cc create mode 100644 tools/testdata/check_format/clang_format_on.cc create mode 100644 tools/testdata/check_format/clang_format_on.cc.gold create mode 100644 tools/testdata/check_format/clang_format_trailing_off.cc diff --git a/STYLE.md b/STYLE.md index f25aa1b7977c..65b7acab1ec3 100644 --- a/STYLE.md +++ b/STYLE.md @@ -4,7 +4,8 @@ issues are taken care of automatically. The CircleCI tests will automatically check the code format and fail. There are make targets that can both check the format (check_format) as well as fix the code format for you (fix_format). Errors in - .clang-tidy are enforced while other warnings are suggestions. + .clang-tidy are enforced while other warnings are suggestions. Note that code and + comment blocks designated `clang-format off` must be closed with `clang-format on`. To run these checks locally, see [Support Tools](support/README.md). * Beyond code formatting, for the most part Envoy uses the [Google C++ style guidelines](https://google.github.io/styleguide/cppguide.html). diff --git a/include/envoy/filesystem/filesystem.h b/include/envoy/filesystem/filesystem.h index ae6cd015584e..503eb4d87d9c 100644 --- a/include/envoy/filesystem/filesystem.h +++ b/include/envoy/filesystem/filesystem.h @@ -65,6 +65,15 @@ class File { using FilePtr = std::unique_ptr; +/** + * Contains the result of splitting the file name and its parent directory from + * a given file path. + */ +struct PathSplitResult { + absl::string_view directory_; + absl::string_view file_; +}; + /** * Abstraction for some basic filesystem operations */ @@ -102,6 +111,13 @@ class Instance { */ virtual std::string fileReadToEnd(const std::string& path) PURE; + /** + * @path file path to split + * @return PathSplitResult containing the parent directory of the input path and the file name + * @note will throw an exception if path does not contain any path separator character + */ + virtual PathSplitResult splitPathFromFilename(absl::string_view path) PURE; + /** * Determine if the path is on a list of paths Envoy will refuse to access. This * is a basic sanity check for users, blacklisting some clearly bad paths. Paths @@ -130,10 +146,6 @@ struct DirectoryEntry { } }; -/** - * Abstraction for listing a directory. - * TODO(sesmith177): replace with std::filesystem::directory_iterator once we move to C++17 - */ class DirectoryIteratorImpl; class DirectoryIterator { public: diff --git a/include/envoy/filesystem/watcher.h b/include/envoy/filesystem/watcher.h index 2d8e729292aa..411923fe31ad 100644 --- a/include/envoy/filesystem/watcher.h +++ b/include/envoy/filesystem/watcher.h @@ -8,6 +8,8 @@ #include "envoy/common/platform.h" #include "envoy/common/pure.h" +#include "absl/strings/string_view.h" + namespace Envoy { namespace Filesystem { @@ -31,7 +33,7 @@ class Watcher { * @param events supplies the events to watch. * @param cb supplies the callback to invoke when a change occurs. */ - virtual void addWatch(const std::string& path, uint32_t events, OnChangedCb cb) PURE; + virtual void addWatch(absl::string_view path, uint32_t events, OnChangedCb cb) PURE; }; using WatcherPtr = std::unique_ptr; diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 85aad9f29a28..2ba07cf7a1ce 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -133,7 +133,7 @@ FileEventPtr DispatcherImpl::createFileEvent(int fd, FileReadyCb cb, FileTrigger Filesystem::WatcherPtr DispatcherImpl::createFilesystemWatcher() { ASSERT(isThreadSafe()); - return Filesystem::WatcherPtr{new Filesystem::WatcherImpl(*this)}; + return Filesystem::WatcherPtr{new Filesystem::WatcherImpl(*this, api_)}; } Network::ListenerPtr DispatcherImpl::createListener(Network::SocketSharedPtr&& socket, diff --git a/source/common/filesystem/BUILD b/source/common/filesystem/BUILD index f88f9d6fa2e3..7aa299d43d74 100644 --- a/source/common/filesystem/BUILD +++ b/source/common/filesystem/BUILD @@ -79,6 +79,9 @@ envoy_cc_library( "//bazel:apple": [ "kqueue/watcher_impl.cc", ], + "//bazel:windows_x86_64": [ + "win32/watcher_impl.cc", + ], "//conditions:default": [ "inotify/watcher_impl.cc", ], @@ -87,6 +90,9 @@ envoy_cc_library( "//bazel:apple": [ "kqueue/watcher_impl.h", ], + "//bazel:windows_x86_64": [ + "win32/watcher_impl.h", + ], "//conditions:default": [ "inotify/watcher_impl.h", ], @@ -96,13 +102,21 @@ envoy_cc_library( ], strip_include_prefix = select({ "//bazel:apple": "kqueue", + "//bazel:windows_x86_64": "win32", "//conditions:default": "inotify", }), deps = [ + "//include/envoy/api:api_interface", "//include/envoy/event:dispatcher_interface", "//source/common/common:assert_lib", "//source/common/common:linked_object", "//source/common/common:minimal_logger_lib", "//source/common/common:utility_lib", - ], + ] + select({ + "//bazel:windows_x86_64": [ + "//source/common/api:os_sys_calls_lib", + "//source/common/common:thread_lib", + ], + "//conditions:default": [], + }), ) diff --git a/source/common/filesystem/directory.h b/source/common/filesystem/directory.h index 61d29332e05d..a949ce724a72 100644 --- a/source/common/filesystem/directory.h +++ b/source/common/filesystem/directory.h @@ -2,6 +2,8 @@ #include +#include "envoy/filesystem/filesystem.h" + #include "common/filesystem/directory_iterator_impl.h" namespace Envoy { diff --git a/source/common/filesystem/inotify/watcher_impl.cc b/source/common/filesystem/inotify/watcher_impl.cc index c28c22514ac1..2db1bb0e56d6 100644 --- a/source/common/filesystem/inotify/watcher_impl.cc +++ b/source/common/filesystem/inotify/watcher_impl.cc @@ -3,6 +3,7 @@ #include #include +#include "envoy/api/api.h" #include "envoy/common/exception.h" #include "envoy/event/dispatcher.h" #include "envoy/event/file_event.h" @@ -15,8 +16,8 @@ namespace Envoy { namespace Filesystem { -WatcherImpl::WatcherImpl(Event::Dispatcher& dispatcher) - : inotify_fd_(inotify_init1(IN_NONBLOCK)), +WatcherImpl::WatcherImpl(Event::Dispatcher& dispatcher, Api::Api& api) + : api_(api), inotify_fd_(inotify_init1(IN_NONBLOCK)), inotify_event_(dispatcher.createFileEvent( inotify_fd_, [this](uint32_t events) -> void { @@ -30,26 +31,21 @@ WatcherImpl::WatcherImpl(Event::Dispatcher& dispatcher) WatcherImpl::~WatcherImpl() { close(inotify_fd_); } -void WatcherImpl::addWatch(const std::string& path, uint32_t events, OnChangedCb callback) { +void WatcherImpl::addWatch(absl::string_view path, uint32_t events, OnChangedCb callback) { // Because of general inotify pain, we always watch the directory that the file lives in, // and then synthetically raise per file events. - size_t last_slash = path.rfind('/'); - if (last_slash == std::string::npos) { - throw EnvoyException(absl::StrCat("invalid watch path ", path)); - } - - std::string directory = last_slash != 0 ? path.substr(0, last_slash) : "/"; - std::string file = StringUtil::subspan(path, last_slash + 1, path.size()); + const PathSplitResult result = api_.fileSystem().splitPathFromFilename(path); const uint32_t watch_mask = IN_MODIFY | IN_MOVED_TO; - int watch_fd = inotify_add_watch(inotify_fd_, directory.c_str(), watch_mask); + int watch_fd = inotify_add_watch(inotify_fd_, std::string(result.directory_).c_str(), watch_mask); if (watch_fd == -1) { throw EnvoyException( fmt::format("unable to add filesystem watch for file {}: {}", path, strerror(errno))); } - ENVOY_LOG(debug, "added watch for directory: '{}' file: '{}' fd: {}", directory, file, watch_fd); - callback_map_[watch_fd].watches_.push_back({file, events, callback}); + ENVOY_LOG(debug, "added watch for directory: '{}' file: '{}' fd: {}", result.directory_, + result.file_, watch_fd); + callback_map_[watch_fd].watches_.push_back({std::string(result.file_), events, callback}); } void WatcherImpl::onInotifyEvent() { diff --git a/source/common/filesystem/inotify/watcher_impl.h b/source/common/filesystem/inotify/watcher_impl.h index d83b5a428fda..40f903f43e4a 100644 --- a/source/common/filesystem/inotify/watcher_impl.h +++ b/source/common/filesystem/inotify/watcher_impl.h @@ -5,6 +5,7 @@ #include #include +#include "envoy/api/api.h" #include "envoy/event/dispatcher.h" #include "envoy/filesystem/watcher.h" @@ -20,11 +21,11 @@ namespace Filesystem { */ class WatcherImpl : public Watcher, Logger::Loggable { public: - WatcherImpl(Event::Dispatcher& dispatcher); + WatcherImpl(Event::Dispatcher& dispatcher, Api::Api& api); ~WatcherImpl() override; // Filesystem::Watcher - void addWatch(const std::string& path, uint32_t events, OnChangedCb cb) override; + void addWatch(absl::string_view path, uint32_t events, OnChangedCb cb) override; private: struct FileWatch { @@ -39,6 +40,7 @@ class WatcherImpl : public Watcher, Logger::Loggable { void onInotifyEvent(); + Api::Api& api_; int inotify_fd_; Event::FileEventPtr inotify_event_; std::unordered_map callback_map_; diff --git a/source/common/filesystem/kqueue/watcher_impl.cc b/source/common/filesystem/kqueue/watcher_impl.cc index 41795ebb99fa..c9d3eecc14a7 100644 --- a/source/common/filesystem/kqueue/watcher_impl.cc +++ b/source/common/filesystem/kqueue/watcher_impl.cc @@ -16,44 +16,40 @@ namespace Envoy { namespace Filesystem { -WatcherImpl::WatcherImpl(Event::Dispatcher& dispatcher) - : queue_(kqueue()), kqueue_event_(dispatcher.createFileEvent( - queue_, - [this](uint32_t events) -> void { - if (events & Event::FileReadyType::Read) { - onKqueueEvent(); - } - }, - Event::FileTriggerType::Edge, Event::FileReadyType::Read)) {} +WatcherImpl::WatcherImpl(Event::Dispatcher& dispatcher, Api::Api& api) + : api_(api), queue_(kqueue()), kqueue_event_(dispatcher.createFileEvent( + queue_, + [this](uint32_t events) -> void { + if (events & Event::FileReadyType::Read) { + onKqueueEvent(); + } + }, + Event::FileTriggerType::Edge, Event::FileReadyType::Read)) {} WatcherImpl::~WatcherImpl() { close(queue_); watches_.clear(); } -void WatcherImpl::addWatch(const std::string& path, uint32_t events, Watcher::OnChangedCb cb) { +void WatcherImpl::addWatch(absl::string_view path, uint32_t events, Watcher::OnChangedCb cb) { FileWatchPtr watch = addWatch(path, events, cb, false); if (watch == nullptr) { throw EnvoyException(absl::StrCat("invalid watch path ", path)); } } -WatcherImpl::FileWatchPtr WatcherImpl::addWatch(const std::string& path, uint32_t events, +WatcherImpl::FileWatchPtr WatcherImpl::addWatch(absl::string_view path, uint32_t events, Watcher::OnChangedCb cb, bool path_must_exist) { bool watching_dir = false; - int watch_fd = open(path.c_str(), O_SYMLINK); + std::string pathname(path); + int watch_fd = open(pathname.c_str(), O_SYMLINK); if (watch_fd == -1) { if (path_must_exist) { return nullptr; } - size_t last_slash = path.rfind('/'); - if (last_slash == std::string::npos) { - return nullptr; - } - - std::string directory = path.substr(0, last_slash); - watch_fd = open(directory.c_str(), 0); + watch_fd = + open(std::string(api_.fileSystem().splitPathFromFilename(path).directory_).c_str(), 0); if (watch_fd == -1) { return nullptr; } @@ -63,7 +59,7 @@ WatcherImpl::FileWatchPtr WatcherImpl::addWatch(const std::string& path, uint32_ FileWatchPtr watch(new FileWatch()); watch->fd_ = watch_fd; - watch->file_ = path; + watch->file_ = pathname; watch->events_ = events; watch->callback_ = cb; watch->watching_dir_ = watching_dir; diff --git a/source/common/filesystem/kqueue/watcher_impl.h b/source/common/filesystem/kqueue/watcher_impl.h index 7922d62ca391..b61ba721b531 100644 --- a/source/common/filesystem/kqueue/watcher_impl.h +++ b/source/common/filesystem/kqueue/watcher_impl.h @@ -4,6 +4,7 @@ #include #include +#include "envoy/api/api.h" #include "envoy/event/dispatcher.h" #include "envoy/filesystem/watcher.h" @@ -20,11 +21,11 @@ namespace Filesystem { */ class WatcherImpl : public Watcher, Logger::Loggable { public: - WatcherImpl(Event::Dispatcher& dispatcher); + WatcherImpl(Event::Dispatcher& dispatcher, Api::Api& api); ~WatcherImpl(); // Filesystem::Watcher - void addWatch(const std::string& path, uint32_t events, OnChangedCb cb) override; + void addWatch(absl::string_view path, uint32_t events, OnChangedCb cb) override; private: struct FileWatch : LinkedObject { @@ -40,10 +41,11 @@ class WatcherImpl : public Watcher, Logger::Loggable { using FileWatchPtr = std::shared_ptr; void onKqueueEvent(); - FileWatchPtr addWatch(const std::string& path, uint32_t events, Watcher::OnChangedCb cb, + FileWatchPtr addWatch(absl::string_view path, uint32_t events, Watcher::OnChangedCb cb, bool pathMustExist); void removeWatch(FileWatchPtr& watch); + Api::Api& api_; int queue_; std::unordered_map watches_; Event::FileEventPtr kqueue_event_; diff --git a/source/common/filesystem/posix/directory_iterator_impl.cc b/source/common/filesystem/posix/directory_iterator_impl.cc index f4d7b97419e4..06b4a910e12a 100644 --- a/source/common/filesystem/posix/directory_iterator_impl.cc +++ b/source/common/filesystem/posix/directory_iterator_impl.cc @@ -55,7 +55,16 @@ FileType DirectoryIteratorImpl::fileType(const std::string& full_path) const { const Api::SysCallIntResult result = os_sys_calls_.stat(full_path.c_str(), &stat_buf); if (result.rc_ != 0) { - throw EnvoyException(fmt::format("unable to stat file: '{}'", full_path)); + if (errno == ENOENT) { + // Special case. This directory entity is likely to be a symlink, + // but the reference is broken as the target could not be stat()'ed. + // If we confirm this with an lstat, treat this file entity as + // a regular file, which may be unlink()'ed. + if (::lstat(full_path.c_str(), &stat_buf) == 0 && S_ISLNK(stat_buf.st_mode)) { + return FileType::Regular; + } + } + throw EnvoyException(fmt::format("unable to stat file: '{}' ({})", full_path, errno)); } if (S_ISDIR(stat_buf.st_mode)) { diff --git a/source/common/filesystem/posix/filesystem_impl.cc b/source/common/filesystem/posix/filesystem_impl.cc index fbf993f8562c..70ddf0ecf98b 100644 --- a/source/common/filesystem/posix/filesystem_impl.cc +++ b/source/common/filesystem/posix/filesystem_impl.cc @@ -108,6 +108,19 @@ std::string InstanceImplPosix::fileReadToEnd(const std::string& path) { return file_string.str(); } +PathSplitResult InstanceImplPosix::splitPathFromFilename(absl::string_view path) { + size_t last_slash = path.rfind('/'); + if (last_slash == std::string::npos) { + throw EnvoyException(fmt::format("invalid file path {}", path)); + } + absl::string_view name = path.substr(last_slash + 1); + // truncate all trailing slashes, except root slash + if (last_slash == 0) { + ++last_slash; + } + return {path.substr(0, last_slash), name}; +} + bool InstanceImplPosix::illegalPath(const std::string& path) { // Special case, allow /dev/fd/* access here so that config can be passed in a // file descriptor from a bootstrap script via exec. The reason we do this @@ -141,7 +154,6 @@ bool InstanceImplPosix::illegalPath(const std::string& path) { } Api::SysCallStringResult InstanceImplPosix::canonicalPath(const std::string& path) { - // TODO(htuch): When we are using C++17, switch to std::filesystem::canonical. char* resolved_path = ::realpath(path.c_str(), nullptr); if (resolved_path == nullptr) { return {std::string(), errno}; diff --git a/source/common/filesystem/posix/filesystem_impl.h b/source/common/filesystem/posix/filesystem_impl.h index 5777c85ef4be..173be8918d33 100644 --- a/source/common/filesystem/posix/filesystem_impl.h +++ b/source/common/filesystem/posix/filesystem_impl.h @@ -39,6 +39,7 @@ class InstanceImplPosix : public Instance { bool directoryExists(const std::string& path) override; ssize_t fileSize(const std::string& path) override; std::string fileReadToEnd(const std::string& path) override; + PathSplitResult splitPathFromFilename(absl::string_view path) override; bool illegalPath(const std::string& path) override; private: diff --git a/source/common/filesystem/win32/filesystem_impl.cc b/source/common/filesystem/win32/filesystem_impl.cc index 7bb45378c9f1..ca9e246b13b7 100644 --- a/source/common/filesystem/win32/filesystem_impl.cc +++ b/source/common/filesystem/win32/filesystem_impl.cc @@ -1,6 +1,4 @@ #include -#include -#include #include #include @@ -8,11 +6,15 @@ #include #include "envoy/common/exception.h" +#include "envoy/common/platform.h" #include "common/common/assert.h" #include "common/common/fmt.h" #include "common/filesystem/filesystem_impl.h" +#include "absl/strings/str_cat.h" +#include "absl/strings/str_split.h" + namespace Envoy { namespace Filesystem { @@ -102,8 +104,132 @@ std::string InstanceImplWin32::fileReadToEnd(const std::string& path) { return file_string.str(); } +PathSplitResult InstanceImplWin32::splitPathFromFilename(absl::string_view path) { + size_t last_slash = path.find_last_of(":/\\"); + if (last_slash == std::string::npos) { + throw EnvoyException(fmt::format("invalid file path {}", path)); + } + absl::string_view name = path.substr(last_slash + 1); + // Truncate all trailing slashes, but retain the entire + // single '/', 'd:' drive, and 'd:\' drive root paths + if (last_slash == 0 || path[last_slash] == ':' || path[last_slash - 1] == ':') { + ++last_slash; + } + return {path.substr(0, last_slash), name}; +} + +// clang-format off +// +// Filename warnings and caveats are documented at; +// https://docs.microsoft.com/en-us/windows/win32/fileio/naming-a-file +// Originally prepared by wrowe@rowe-clan.net for the Apache APR project, see; +// http://svn.apache.org/viewvc/apr/apr/trunk/file_io/win32/filesys.c?view=log&pathrev=62242 +// +// Note special delimiter cases for path prefixes; +// "D:\" for local drive volumes +// "\server\share\" for network volumes +// "\\?\" to pass path directly to the underlying driver +// (invalidates the '/' separator and bypasses ".", ".." handling) +// "\\?\D:\" for local drive volumes +// "\\?\UNC\server\share\" for network volumes (literal "UNC") +// "\\.\" for device namespace (e.g. volume names, character devices) +// File path components must not end in whitespace or '.' (except literal "." and "..") +// Allow ':' for drive letter only (attempt to name alternate file stream) +// Allow '/', '\\' as path delimiters only +// Valid file name character excluding delimiters; + +static const char filename_char_table[] = { + 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + // ! " # $ % & ' ( ) * + , - . / 0 1 2 3 4 5 6 7 8 9 : ; < = > ? + 1, 1, 0, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 1, 1, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 0, 1, 0, 0, + // @ A B C D E F G H I J K L M N O P Q R S T U V W X Y Z [ \ ] ^ _ + 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 1, + // ` a b c d e f g h i j k l m n o p q r s t u v w x y z { | } ~ + 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 1, 1, 0, + // High bit codes are accepted (subject to code page translation rules) + 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, + 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, + 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, + 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1 +}; + +// The "COM#" and "LPT#" names below have boolean flag requiring a [1-9] suffix. +// This list can be avoided by observing dwFileAttributes & FILE_ATTRIBUTE_DEVICE +// within WIN32_FILE_ATTRIBUTE_DATA or WIN32_FIND_DATA results. +std::unordered_map pathelt_table = { + {"CON", false}, {"NUL", false}, {"AUX", false}, {"PRN", false}, {"COM", true}, {"LPT", true} +}; + +// clang-format on + bool InstanceImplWin32::illegalPath(const std::string& path) { - // Currently, we don't know of any obviously illegal paths on Windows + std::string pathbuffer = path; + absl::string_view pathname = pathbuffer; + + // Examine and skip common leading path patterns of \\?\ and + // reject paths with any other leading \\.\ device or an + // unrecognized \\*\ prefix + if ((pathname.size() >= 4) && (pathname[0] == '/' || pathname[0] == '\\') && + (pathname[1] == '/' || pathname[1] == '\\') && (pathname[3] == '/' || pathname[3] == '\\')) { + if (pathname[2] == '?') { + pathname = pathname.substr(4); + } else { + return true; + } + } + // Examine and accept D: drive prefix (last opportunity to + // accept a colon in the file path) and skip the D: component + // This may result in a relative-to working directory or absolute path on D: + if (pathname.size() >= 2 && std::isalpha(pathname[0]) && pathname[1] == ':') { + pathname = pathname.substr(2); + } + std::string ucase_prefix(" "); + std::vector pathelts = absl::StrSplit(pathname, absl::ByAnyChar("/\\")); + for (const std::string& elt : pathelts) { + // Accept element of empty, ".", ".." as special cases, + if (elt.size() == 0 || + (elt[0] == '.' && (elt.size() == 1 || (elt[1] == '.' && (elt.size() == 2))))) { + continue; + } + // Upper-case path segment prefix to compare to character device names + if (elt.size() >= 3) { + int i; + for (i = 0; i < 3; ++i) { + ucase_prefix[i] = ::toupper(elt[i]); + } + auto found_elt = pathelt_table.find(ucase_prefix); + + if (found_elt != pathelt_table.end()) { + // If a non-zero digit is significant, but not present, treat as not-found + if (!found_elt->second || (elt.size() >= 4 && ::isdigit(elt[i]) && elt[i++] != '0')) { + if (elt.size() == i) { + return true; + } + // The literal device name is invalid for both an exact match, + // and also when followed by (whitespace plus) any .ext suffix + for (auto ch = elt.begin() + i; ch != elt.end(); ++ch) { + if (*ch == '.') { + return true; + } + if (*ch != ' ') { + break; + } + } + } + } + } + + for (const char& ch : elt) { + if (!(filename_char_table[ch] & 1)) { + return true; + } + } + const char& lastch = elt[elt.size() - 1]; + if (lastch == ' ' || lastch == '.') { + return true; + } + } + return false; } diff --git a/source/common/filesystem/win32/filesystem_impl.h b/source/common/filesystem/win32/filesystem_impl.h index 8d0a2d8ba13d..f39b40378d64 100644 --- a/source/common/filesystem/win32/filesystem_impl.h +++ b/source/common/filesystem/win32/filesystem_impl.h @@ -37,6 +37,7 @@ class InstanceImplWin32 : public Instance { bool directoryExists(const std::string& path) override; ssize_t fileSize(const std::string& path) override; std::string fileReadToEnd(const std::string& path) override; + PathSplitResult splitPathFromFilename(absl::string_view path) override; bool illegalPath(const std::string& path) override; }; diff --git a/source/common/filesystem/win32/watcher_impl.cc b/source/common/filesystem/win32/watcher_impl.cc new file mode 100644 index 000000000000..eba6809ace85 --- /dev/null +++ b/source/common/filesystem/win32/watcher_impl.cc @@ -0,0 +1,266 @@ +#include "common/api/os_sys_calls_impl.h" +#include "common/common/assert.h" +#include "common/common/fmt.h" +#include "common/common/thread_impl.h" +#include "common/filesystem/watcher_impl.h" + +namespace Envoy { +namespace Filesystem { + +WatcherImpl::WatcherImpl(Event::Dispatcher& dispatcher, Api::Api& api) + : api_(api), os_sys_calls_(Api::OsSysCallsSingleton::get()) { + SOCKET_FD socks[2]; + Api::SysCallIntResult result = os_sys_calls_.socketpair(AF_INET, SOCK_STREAM, IPPROTO_TCP, socks); + ASSERT(result.rc_ == 0); + + event_read_ = socks[0]; + event_write_ = socks[1]; + result = os_sys_calls_.setsocketblocking(event_read_, false); + ASSERT(result.rc_ == 0); + result = os_sys_calls_.setsocketblocking(event_write_, false); + ASSERT(result.rc_ == 0); + + directory_event_ = dispatcher.createFileEvent( + event_read_, + [this](uint32_t events) -> void { + ASSERT(events == Event::FileReadyType::Read); + onDirectoryEvent(); + }, + Event::FileTriggerType::Level, Event::FileReadyType::Read); + + thread_exit_event_ = ::CreateEvent(nullptr, false, false, nullptr); + ASSERT(thread_exit_event_ != NULL); + keep_watching_ = true; + watch_thread_ = thread_factory_.createThread([this]() -> void { watchLoop(); }); +} + +WatcherImpl::~WatcherImpl() { + const BOOL rc = ::SetEvent(thread_exit_event_); + ASSERT(rc); + + watch_thread_->join(); + + for (auto& entry : callback_map_) { + ::CloseHandle(entry.second->dir_handle_); + ::CloseHandle(entry.second->overlapped_.hEvent); + } + ::CloseHandle(thread_exit_event_); + ::closesocket(event_read_); + ::closesocket(event_write_); +} + +void WatcherImpl::addWatch(absl::string_view path, uint32_t events, OnChangedCb cb) { + const PathSplitResult result = api_.fileSystem().splitPathFromFilename(path); + // ReadDirectoryChangesW only has a Unicode version, so we need + // to use wide strings here + const std::wstring directory = wstring_converter_.from_bytes(std::string(result.directory_)); + const std::wstring file = wstring_converter_.from_bytes(std::string(result.file_)); + + const HANDLE dir_handle = CreateFileW( + directory.c_str(), GENERIC_READ, FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + nullptr, OPEN_EXISTING, FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OVERLAPPED, NULL); + if (dir_handle == INVALID_HANDLE_VALUE) { + throw EnvoyException( + fmt::format("unable to open directory {}: {}", result.directory_, GetLastError())); + } + std::string fii_key(sizeof(FILE_ID_INFO), '\0'); + RELEASE_ASSERT( + GetFileInformationByHandleEx(dir_handle, FileIdInfo, &fii_key[0], sizeof(FILE_ID_INFO)), + fmt::format("unable to identify directory {}: {}", result.directory_, GetLastError())); + if (callback_map_.find(fii_key) != callback_map_.end()) { + CloseHandle(dir_handle); + } else { + callback_map_[fii_key] = std::make_unique(); + callback_map_[fii_key]->dir_handle_ = dir_handle; + callback_map_[fii_key]->buffer_.resize(16384); + callback_map_[fii_key]->watcher_ = this; + + // According to Microsoft docs, "the hEvent member of the OVERLAPPED structure is not used by + // the system, so you can use it yourself". We will use it for synchronization of the completion + // routines + HANDLE event_handle = ::CreateEvent(nullptr, false, false, nullptr); + RELEASE_ASSERT(event_handle, fmt::format("CreateEvent failed: {}", GetLastError())); + + callback_map_[fii_key]->overlapped_.hEvent = event_handle; + dir_watch_complete_events_.push_back(event_handle); + + // send the first ReadDirectoryChangesW request to our watch thread. This ensures that all of + // the io completion routines will run in that thread + DWORD rc = ::QueueUserAPC(&issueFirstRead, + static_cast(watch_thread_.get())->handle(), + reinterpret_cast(callback_map_[fii_key].get())); + RELEASE_ASSERT(rc, fmt::format("QueueUserAPC failed: {}", GetLastError())); + + // wait for issueFirstRead to confirm that it has issued a call to ReadDirectoryChangesW + rc = ::WaitForSingleObject(event_handle, INFINITE); + RELEASE_ASSERT(rc == WAIT_OBJECT_0, + fmt::format("WaitForSingleObject failed: {}", GetLastError())); + + ENVOY_LOG(debug, "created watch for directory: '{}' handle: {}", result.directory_, dir_handle); + } + + callback_map_[fii_key]->watches_.push_back({file, events, cb}); + ENVOY_LOG(debug, "added watch for file '{}' in directory '{}'", result.file_, result.directory_); +} + +void WatcherImpl::onDirectoryEvent() { + while (true) { + char data = 0; + const int rc = ::recv(event_read_, &data, sizeof(data), 0); + const int err = ::WSAGetLastError(); + if (rc == SOCKET_ERROR && err == WSAEWOULDBLOCK) { + return; + } + RELEASE_ASSERT(rc != SOCKET_ERROR, fmt::format("recv errored: {}", err)); + + if (data == 0) { + // no callbacks to run; this is just a notification that a DirectoryWatch exited + return; + } + + CbClosure callback; + bool exists = active_callbacks_.try_pop(callback); + RELEASE_ASSERT(exists, "expected callback, found none"); + ENVOY_LOG(debug, "executing callback"); + callback(); + } +} + +void WatcherImpl::issueFirstRead(ULONG_PTR param) { + DirectoryWatch* dir_watch = reinterpret_cast(param); + // Since the first member in each DirectoryWatch is an OVERLAPPED, we can pass + // a pointer to DirectoryWatch as the OVERLAPPED for ReadDirectoryChangesW. Then, the + // completion routine can use its OVERLAPPED* parameter to access the DirectoryWatch see: + // https://docs.microsoft.com/en-us/windows/desktop/ipc/named-pipe-server-using-completion-routines + ReadDirectoryChangesW(dir_watch->dir_handle_, &(dir_watch->buffer_[0]), + dir_watch->buffer_.capacity(), false, + FILE_NOTIFY_CHANGE_FILE_NAME | FILE_NOTIFY_CHANGE_LAST_WRITE, nullptr, + reinterpret_cast(param), &directoryChangeCompletion); + + const BOOL rc = ::SetEvent(dir_watch->overlapped_.hEvent); + ASSERT(rc); +} + +void WatcherImpl::endDirectoryWatch(SOCKET_FD sock, HANDLE event_handle) { + const BOOL rc = ::SetEvent(event_handle); + ASSERT(rc); + // let libevent know that a ReadDirectoryChangesW call returned + const char data = 0; + const int bytes_written = ::send(sock, &data, sizeof(data), 0); + RELEASE_ASSERT(bytes_written == sizeof(data), + fmt::format("failed to write 1 byte: {}", ::WSAGetLastError())); +} + +void WatcherImpl::directoryChangeCompletion(DWORD err, DWORD num_bytes, LPOVERLAPPED overlapped) { + DirectoryWatch* dir_watch = reinterpret_cast(overlapped); + WatcherImpl* watcher = dir_watch->watcher_; + PFILE_NOTIFY_INFORMATION fni = reinterpret_cast(&dir_watch->buffer_[0]); + + if (err == ERROR_OPERATION_ABORTED) { + ENVOY_LOG(debug, "ReadDirectoryChangesW aborted, exiting"); + endDirectoryWatch(watcher->event_write_, dir_watch->overlapped_.hEvent); + return; + } else if (err != 0) { + ENVOY_LOG(error, "ReadDirectoryChangesW errored: {}, exiting", err); + endDirectoryWatch(watcher->event_write_, dir_watch->overlapped_.hEvent); + return; + } else if (num_bytes < sizeof(_FILE_NOTIFY_INFORMATION)) { + ENVOY_LOG(error, "ReadDirectoryChangesW returned {} bytes, expected {}, exiting", num_bytes, + sizeof(_FILE_NOTIFY_INFORMATION)); + endDirectoryWatch(watcher->event_write_, dir_watch->overlapped_.hEvent); + return; + } + + DWORD next_entry = 0; + do { + fni = reinterpret_cast(reinterpret_cast(fni) + next_entry); + // the length of the file name is given in bytes, not wide characters + std::wstring file(fni->FileName, fni->FileNameLength / 2); + ENVOY_LOG(debug, "notification: handle: {} action: {:x} file: {}", dir_watch->dir_handle_, + fni->Action, watcher->wstring_converter_.to_bytes(file)); + + uint32_t events = 0; + if (fni->Action == FILE_ACTION_RENAMED_NEW_NAME) { + events |= Events::MovedTo; + } + if (fni->Action == FILE_ACTION_MODIFIED) { + events |= Events::Modified; + } + + for (FileWatch& watch : dir_watch->watches_) { + if (watch.file_ == file && (watch.events_ & events)) { + ENVOY_LOG(debug, "matched callback: file: {}", watcher->wstring_converter_.to_bytes(file)); + const auto cb = watch.cb_; + const auto cb_closure = [cb, events]() -> void { cb(events); }; + watcher->active_callbacks_.push(cb_closure); + // write a byte to the other end of the socket that libevent is watching + // this tells the libevent callback to pull this callback off the active_callbacks_ + // queue. We do this so that the callbacks are executed in the main libevent loop, + // not in this completion routine + const char data = 1; + const int bytes_written = ::send(watcher->event_write_, &data, sizeof(data), 0); + RELEASE_ASSERT(bytes_written == sizeof(data), + fmt::format("failed to write 1 byte: {}", ::WSAGetLastError())); + } + } + + next_entry = fni->NextEntryOffset; + } while (next_entry != 0); + + if (!watcher->keep_watching_.load()) { + ENVOY_LOG(debug, "ending watch on directory: handle: {}", dir_watch->dir_handle_); + endDirectoryWatch(watcher->event_write_, dir_watch->overlapped_.hEvent); + return; + } + + ReadDirectoryChangesW(dir_watch->dir_handle_, &(dir_watch->buffer_[0]), + dir_watch->buffer_.capacity(), false, + FILE_NOTIFY_CHANGE_FILE_NAME | FILE_NOTIFY_CHANGE_LAST_WRITE, nullptr, + overlapped, directoryChangeCompletion); +} + +void WatcherImpl::watchLoop() { + while (keep_watching_.load()) { + DWORD wait = WaitForSingleObjectEx(thread_exit_event_, INFINITE, true); + switch (wait) { + case WAIT_OBJECT_0: + // object is getting destroyed, exit the loop + keep_watching_.store(false); + break; + case WAIT_IO_COMPLETION: + // an IO completion routine finished, nothing to do + break; + default: + ENVOY_LOG(error, "WaitForSingleObjectEx: {}, GetLastError: {}, exiting", wait, + GetLastError()); + keep_watching_.store(false); + } + } + + for (auto& entry : callback_map_) { + ::CancelIoEx(entry.second->dir_handle_, nullptr); + } + + const int num_directories = dir_watch_complete_events_.size(); + if (num_directories > 0) { + while (true) { + DWORD wait = ::WaitForMultipleObjectsEx(num_directories, &dir_watch_complete_events_[0], true, + INFINITE, true); + + if (WAIT_OBJECT_0 <= wait && wait < (WAIT_OBJECT_0 + num_directories)) { + // we have no pending IO remaining + return; + } else if (wait == WAIT_IO_COMPLETION) { + // an io completion routine finished, keep waiting + continue; + } else { + ENVOY_LOG(error, "WaitForMultipleObjectsEx: {}, GetLastError: {}, exiting", wait, + GetLastError()); + return; + } + } + } +} + +} // namespace Filesystem +} // namespace Envoy diff --git a/source/common/filesystem/win32/watcher_impl.h b/source/common/filesystem/win32/watcher_impl.h new file mode 100644 index 000000000000..5d96e50b4535 --- /dev/null +++ b/source/common/filesystem/win32/watcher_impl.h @@ -0,0 +1,74 @@ +#pragma once + +#include + +#include +#include +#include +#include +#include +#include + +#include "envoy/api/api.h" +#include "envoy/event/dispatcher.h" +#include "envoy/filesystem/watcher.h" + +#include "common/api/os_sys_calls_impl.h" +#include "common/common/fmt.h" +#include "common/common/logger.h" +#include "common/common/thread_impl.h" + +namespace Envoy { +namespace Filesystem { + +class WatcherImpl : public Watcher, Logger::Loggable { +public: + WatcherImpl(Event::Dispatcher& dispatcher, Api::Api& api); + ~WatcherImpl(); + + // Filesystem::Watcher + void addWatch(absl::string_view path, uint32_t events, OnChangedCb cb) override; + +private: + static void issueFirstRead(ULONG_PTR param); + static void directoryChangeCompletion(DWORD err, DWORD num_bytes, LPOVERLAPPED overlapped); + static void endDirectoryWatch(SOCKET_FD sock, HANDLE hEvent); + void watchLoop(); + void onDirectoryEvent(); + + struct FileWatch { + // store the wide character string for ReadDirectoryChangesW + std::wstring file_; + uint32_t events_; + OnChangedCb cb_; + }; + + typedef std::function CbClosure; + + struct DirectoryWatch { + OVERLAPPED overlapped_; + std::list watches_; + HANDLE dir_handle_; + std::vector buffer_; + WatcherImpl* watcher_; + }; + + typedef std::unique_ptr DirectoryWatchPtr; + + Api::Api& api_; + std::unordered_map callback_map_; + Event::FileEventPtr directory_event_; + SOCKET_FD event_write_; + SOCKET_FD event_read_; + Thread::ThreadPtr watch_thread_; + Thread::ThreadFactoryImplWin32 thread_factory_; + HANDLE thread_exit_event_; + std::vector dir_watch_complete_events_; + std::atomic keep_watching_; + concurrency::concurrent_queue active_callbacks_; + Api::OsSysCallsImpl& os_sys_calls_; + std::wstring_convert> wstring_converter_; +}; + +} // namespace Filesystem +} // namespace Envoy diff --git a/test/common/config/filesystem_subscription_test_harness.h b/test/common/config/filesystem_subscription_test_harness.h index 6caf415e46d4..184f53fe9dd1 100644 --- a/test/common/config/filesystem_subscription_test_harness.h +++ b/test/common/config/filesystem_subscription_test_harness.h @@ -32,11 +32,7 @@ class FilesystemSubscriptionTestHarness : public SubscriptionTestHarness { api_(Api::createApiForTest(stats_store_)), dispatcher_(api_->allocateDispatcher()), subscription_(*dispatcher_, path_, callbacks_, stats_, validation_visitor_, *api_) {} - ~FilesystemSubscriptionTestHarness() override { - if (::access(path_.c_str(), F_OK) != -1) { - EXPECT_EQ(0, ::unlink(path_.c_str())); - } - } + ~FilesystemSubscriptionTestHarness() override { TestEnvironment::removePath(path_); } void startSubscription(const std::set& cluster_names) override { std::ifstream config_file(path_); @@ -52,7 +48,7 @@ class FilesystemSubscriptionTestHarness : public SubscriptionTestHarness { // Write JSON contents to file, rename to path_ and run dispatcher to catch // inotify. const std::string temp_path = TestEnvironment::writeStringToFileForTest("eds.json.tmp", json); - TestUtility::renameFile(temp_path, path_); + TestEnvironment::renameFile(temp_path, path_); if (run_dispatcher) { dispatcher_->run(Event::Dispatcher::RunType::NonBlock); } diff --git a/test/common/filesystem/directory_test.cc b/test/common/filesystem/directory_test.cc index cdc52b9121c7..7a48bdcefa99 100644 --- a/test/common/filesystem/directory_test.cc +++ b/test/common/filesystem/directory_test.cc @@ -14,10 +14,6 @@ namespace Envoy { namespace Filesystem { -// we are using this class to clean up all the files we create, -// as it looks like some versions of libstdc++ have a bug in -// std::experimental::filesystem::remove_all where it fails with nested directories: -// https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71313 class DirectoryTest : public testing::Test { public: DirectoryTest() : dir_path_(TestEnvironment::temporaryPath("envoy_test")) { @@ -25,7 +21,7 @@ class DirectoryTest : public testing::Test { } protected: - void SetUp() override { TestUtility::createDirectory(dir_path_); } + void SetUp() override { TestEnvironment::createPath(dir_path_); } void TearDown() override { while (!files_to_remove_.empty()) { @@ -38,7 +34,7 @@ class DirectoryTest : public testing::Test { void addSubDirs(std::list sub_dirs) { for (const std::string& dir_name : sub_dirs) { const std::string full_path = dir_path_ + "/" + dir_name; - TestUtility::createDirectory(full_path); + TestEnvironment::createPath(full_path); files_to_remove_.push(full_path); } } @@ -55,7 +51,7 @@ class DirectoryTest : public testing::Test { for (const auto& link : symlinks) { const std::string target_path = dir_path_ + "/" + link.first; const std::string link_path = dir_path_ + "/" + link.second; - TestUtility::createSymlink(target_path, link_path); + TestEnvironment::createSymlink(target_path, link_path); files_to_remove_.push(link_path); } } @@ -185,6 +181,20 @@ TEST_F(DirectoryTest, DirectoryWithSymlinkToDirectory) { EXPECT_EQ(expected, getDirectoryContents(dir_path_, false)); } +// Test that a broken symlink can be listed +TEST_F(DirectoryTest, DirectoryWithBrokenSymlink) { + addSubDirs({"sub_dir"}); + addSymlinks({{"sub_dir", "link_dir"}}); + TestEnvironment::removePath(dir_path_ + "/sub_dir"); + + const EntrySet expected = { + {".", FileType::Directory}, + {"..", FileType::Directory}, + {"link_dir", FileType::Regular}, + }; + EXPECT_EQ(expected, getDirectoryContents(dir_path_, false)); +} + // Test that we can list an empty directory TEST_F(DirectoryTest, DirectoryWithEmptyDirectory) { const EntrySet expected = { @@ -216,7 +226,7 @@ TEST(Directory, DirectoryHasTrailingPathSeparator) { #else const std::string dir_path(TestEnvironment::temporaryPath("envoy_test") + "/"); #endif - TestUtility::createDirectory(dir_path); + TestEnvironment::createPath(dir_path); const EntrySet expected = { {".", FileType::Directory}, diff --git a/test/common/filesystem/filesystem_impl_test.cc b/test/common/filesystem/filesystem_impl_test.cc index c8cfdc56ae3a..01ba587155d5 100644 --- a/test/common/filesystem/filesystem_impl_test.cc +++ b/test/common/filesystem/filesystem_impl_test.cc @@ -37,7 +37,7 @@ class FileSystemImplTest : public testing::Test { #endif }; -TEST_F(FileSystemImplTest, fileExists) { +TEST_F(FileSystemImplTest, FileExists) { EXPECT_FALSE(file_system_.fileExists("/dev/blahblahblah")); #ifdef WIN32 const std::string file_path = TestEnvironment::writeStringToFileForTest("test_envoy", "x"); @@ -49,7 +49,7 @@ TEST_F(FileSystemImplTest, fileExists) { #endif } -TEST_F(FileSystemImplTest, directoryExists) { +TEST_F(FileSystemImplTest, DirectoryExists) { EXPECT_FALSE(file_system_.directoryExists("/dev/blahblah")); #ifdef WIN32 const std::string file_path = TestEnvironment::writeStringToFileForTest("test_envoy", "x"); @@ -61,7 +61,7 @@ TEST_F(FileSystemImplTest, directoryExists) { #endif } -TEST_F(FileSystemImplTest, fileSize) { +TEST_F(FileSystemImplTest, FileSize) { #ifdef WIN32 EXPECT_EQ(0, file_system_.fileSize("NUL")); #else @@ -73,7 +73,7 @@ TEST_F(FileSystemImplTest, fileSize) { EXPECT_EQ(data.length(), file_system_.fileSize(file_path)); } -TEST_F(FileSystemImplTest, fileReadToEndSuccess) { +TEST_F(FileSystemImplTest, FileReadToEndSuccess) { const std::string data = "test string\ntest"; const std::string file_path = TestEnvironment::writeStringToFileForTest("test_envoy", data); @@ -82,7 +82,7 @@ TEST_F(FileSystemImplTest, fileReadToEndSuccess) { // Files are read into std::string; verify that all bytes (eg non-ascii characters) come back // unmodified -TEST_F(FileSystemImplTest, fileReadToEndSuccessBinary) { +TEST_F(FileSystemImplTest, FileReadToEndSuccessBinary) { std::string data; for (unsigned i = 0; i < 256; i++) { data.push_back(i); @@ -97,13 +97,13 @@ TEST_F(FileSystemImplTest, fileReadToEndSuccessBinary) { } } -TEST_F(FileSystemImplTest, fileReadToEndDoesNotExist) { +TEST_F(FileSystemImplTest, FileReadToEndDoesNotExist) { unlink(TestEnvironment::temporaryPath("envoy_this_not_exist").c_str()); EXPECT_THROW(file_system_.fileReadToEnd(TestEnvironment::temporaryPath("envoy_this_not_exist")), EnvoyException); } -TEST_F(FileSystemImplTest, fileReadToEndBlacklisted) { +TEST_F(FileSystemImplTest, FileReadToEndBlacklisted) { EXPECT_THROW(file_system_.fileReadToEnd("/dev/urandom"), EnvoyException); EXPECT_THROW(file_system_.fileReadToEnd("/proc/cpuinfo"), EnvoyException); EXPECT_THROW(file_system_.fileReadToEnd("/sys/block/sda/dev"), EnvoyException); @@ -121,6 +121,43 @@ TEST_F(FileSystemImplTest, CanonicalPathFail) { } #endif +TEST_F(FileSystemImplTest, SplitPathFromFilename) { + PathSplitResult result; + result = file_system_.splitPathFromFilename("/foo/bar/baz"); + EXPECT_EQ(result.directory_, "/foo/bar"); + EXPECT_EQ(result.file_, "baz"); + result = file_system_.splitPathFromFilename("/foo/bar"); + EXPECT_EQ(result.directory_, "/foo"); + EXPECT_EQ(result.file_, "bar"); + result = file_system_.splitPathFromFilename("/foo"); + EXPECT_EQ(result.directory_, "/"); + EXPECT_EQ(result.file_, "foo"); + result = file_system_.splitPathFromFilename("/"); + EXPECT_EQ(result.directory_, "/"); + EXPECT_EQ(result.file_, ""); + EXPECT_THROW(file_system_.splitPathFromFilename("nopathdelimeter"), EnvoyException); +#ifdef WIN32 + result = file_system_.splitPathFromFilename("c:\\foo/bar"); + EXPECT_EQ(result.directory_, "c:\\foo"); + EXPECT_EQ(result.file_, "bar"); + result = file_system_.splitPathFromFilename("c:/foo\\bar"); + EXPECT_EQ(result.directory_, "c:/foo"); + EXPECT_EQ(result.file_, "bar"); + result = file_system_.splitPathFromFilename("c:\\foo"); + EXPECT_EQ(result.directory_, "c:\\"); + EXPECT_EQ(result.file_, "foo"); + result = file_system_.splitPathFromFilename("c:foo"); + EXPECT_EQ(result.directory_, "c:"); + EXPECT_EQ(result.file_, "foo"); + result = file_system_.splitPathFromFilename("c:"); + EXPECT_EQ(result.directory_, "c:"); + EXPECT_EQ(result.file_, ""); + result = file_system_.splitPathFromFilename("\\\\?\\C:\\"); + EXPECT_EQ(result.directory_, "\\\\?\\C:\\"); + EXPECT_EQ(result.file_, ""); +#endif +} + TEST_F(FileSystemImplTest, IllegalPath) { EXPECT_FALSE(file_system_.illegalPath("/")); EXPECT_FALSE(file_system_.illegalPath("//")); @@ -132,6 +169,40 @@ TEST_F(FileSystemImplTest, IllegalPath) { EXPECT_FALSE(file_system_.illegalPath("/sys")); EXPECT_FALSE(file_system_.illegalPath("/sys/")); EXPECT_FALSE(file_system_.illegalPath("/_some_non_existent_file")); + EXPECT_TRUE(file_system_.illegalPath(R"EOF(\\.\foo)EOF")); + EXPECT_TRUE(file_system_.illegalPath(R"EOF(\\z\foo)EOF")); + EXPECT_TRUE(file_system_.illegalPath(R"EOF(\\?\nul)EOF")); + EXPECT_FALSE(file_system_.illegalPath(R"EOF(\\?\C:\foo)EOF")); + EXPECT_FALSE(file_system_.illegalPath(R"EOF(C:\foo)EOF")); + EXPECT_FALSE(file_system_.illegalPath(R"EOF(C:\foo/bar\baz)EOF")); + EXPECT_FALSE(file_system_.illegalPath("C:/foo")); + EXPECT_FALSE(file_system_.illegalPath("C:zfoo")); + EXPECT_FALSE(file_system_.illegalPath("C:/foo/bar/baz")); + EXPECT_TRUE(file_system_.illegalPath("C:/foo/b*ar/baz")); + EXPECT_TRUE(file_system_.illegalPath("C:/foo/b?ar/baz")); + EXPECT_TRUE(file_system_.illegalPath(R"EOF(C:/i/x"x)EOF")); + EXPECT_TRUE(file_system_.illegalPath(std::string("C:/i\0j", 6))); + EXPECT_TRUE(file_system_.illegalPath("C:/i/\177")); + EXPECT_TRUE(file_system_.illegalPath("C:/i/\alarm")); + EXPECT_FALSE(file_system_.illegalPath("C:/i/../j")); + EXPECT_FALSE(file_system_.illegalPath("C:/i/./j")); + EXPECT_FALSE(file_system_.illegalPath("C:/i/.j")); + EXPECT_TRUE(file_system_.illegalPath("C:/i/j.")); + EXPECT_TRUE(file_system_.illegalPath("C:/i/j ")); + EXPECT_FALSE(file_system_.illegalPath("C:/i///")); + EXPECT_TRUE(file_system_.illegalPath("C:/i/NUL")); + EXPECT_TRUE(file_system_.illegalPath("C:/i/nul")); + EXPECT_TRUE(file_system_.illegalPath("C:/i/nul.ext")); + EXPECT_TRUE(file_system_.illegalPath("C:/i/nul.ext.ext2")); + EXPECT_TRUE(file_system_.illegalPath("C:/i/nul .ext")); + EXPECT_TRUE(file_system_.illegalPath("C:/i/COM1")); + EXPECT_TRUE(file_system_.illegalPath("C:/i/COM1/whoops")); + EXPECT_TRUE(file_system_.illegalPath("C:/i/COM1.ext")); + EXPECT_TRUE(file_system_.illegalPath("C:/i/COM1 .ext")); + EXPECT_FALSE(file_system_.illegalPath("C:/i/COM1 ext")); + EXPECT_FALSE(file_system_.illegalPath("C:/i/COM1foo")); + EXPECT_FALSE(file_system_.illegalPath("C:/i/COM0")); + EXPECT_FALSE(file_system_.illegalPath("C:/i/COM")); #else EXPECT_TRUE(file_system_.illegalPath("/dev")); EXPECT_TRUE(file_system_.illegalPath("/dev/")); diff --git a/test/common/filesystem/watcher_impl_test.cc b/test/common/filesystem/watcher_impl_test.cc index cab0d694b61d..8251ad49f1ae 100644 --- a/test/common/filesystem/watcher_impl_test.cc +++ b/test/common/filesystem/watcher_impl_test.cc @@ -35,14 +35,14 @@ TEST_F(WatcherImplTest, All) { unlink(TestEnvironment::temporaryPath("envoy_test/watcher_new_target").c_str()); unlink(TestEnvironment::temporaryPath("envoy_test/watcher_new_link").c_str()); - TestUtility::createDirectory(TestEnvironment::temporaryPath("envoy_test")); + TestEnvironment::createPath(TestEnvironment::temporaryPath("envoy_test")); { std::ofstream file(TestEnvironment::temporaryPath("envoy_test/watcher_target")); } - TestUtility::createSymlink(TestEnvironment::temporaryPath("envoy_test/watcher_target"), - TestEnvironment::temporaryPath("envoy_test/watcher_link")); + TestEnvironment::createSymlink(TestEnvironment::temporaryPath("envoy_test/watcher_target"), + TestEnvironment::temporaryPath("envoy_test/watcher_link")); { std::ofstream file(TestEnvironment::temporaryPath("envoy_test/watcher_new_target")); } - TestUtility::createSymlink(TestEnvironment::temporaryPath("envoy_test/watcher_new_target"), - TestEnvironment::temporaryPath("envoy_test/watcher_new_link")); + TestEnvironment::createSymlink(TestEnvironment::temporaryPath("envoy_test/watcher_new_target"), + TestEnvironment::temporaryPath("envoy_test/watcher_new_link")); WatchCallback callback; EXPECT_CALL(callback, called(Watcher::Events::MovedTo)).Times(2); @@ -51,14 +51,14 @@ TEST_F(WatcherImplTest, All) { callback.called(events); dispatcher_->exit(); }); - TestUtility::renameFile(TestEnvironment::temporaryPath("envoy_test/watcher_new_link"), - TestEnvironment::temporaryPath("envoy_test/watcher_link")); + TestEnvironment::renameFile(TestEnvironment::temporaryPath("envoy_test/watcher_new_link"), + TestEnvironment::temporaryPath("envoy_test/watcher_link")); dispatcher_->run(Event::Dispatcher::RunType::Block); - TestUtility::createSymlink(TestEnvironment::temporaryPath("envoy_test/watcher_new_target"), - TestEnvironment::temporaryPath("envoy_test/watcher_new_link")); - TestUtility::renameFile(TestEnvironment::temporaryPath("envoy_test/watcher_new_link"), - TestEnvironment::temporaryPath("envoy_test/watcher_link")); + TestEnvironment::createSymlink(TestEnvironment::temporaryPath("envoy_test/watcher_new_target"), + TestEnvironment::temporaryPath("envoy_test/watcher_new_link")); + TestEnvironment::renameFile(TestEnvironment::temporaryPath("envoy_test/watcher_new_link"), + TestEnvironment::temporaryPath("envoy_test/watcher_link")); dispatcher_->run(Event::Dispatcher::RunType::Block); } @@ -70,7 +70,7 @@ TEST_F(WatcherImplTest, Create) { unlink(TestEnvironment::temporaryPath("envoy_test/watcher_new_link").c_str()); unlink(TestEnvironment::temporaryPath("envoy_test/other_file").c_str()); - TestUtility::createDirectory(TestEnvironment::temporaryPath("envoy_test")); + TestEnvironment::createPath(TestEnvironment::temporaryPath("envoy_test")); { std::ofstream file(TestEnvironment::temporaryPath("envoy_test/watcher_target")); } WatchCallback callback; @@ -84,17 +84,17 @@ TEST_F(WatcherImplTest, Create) { { std::ofstream file(TestEnvironment::temporaryPath("envoy_test/other_file")); } dispatcher_->run(Event::Dispatcher::RunType::NonBlock); - TestUtility::createSymlink(TestEnvironment::temporaryPath("envoy_test/watcher_target"), - TestEnvironment::temporaryPath("envoy_test/watcher_new_link")); - TestUtility::renameFile(TestEnvironment::temporaryPath("envoy_test/watcher_new_link"), - TestEnvironment::temporaryPath("envoy_test/watcher_link")); + TestEnvironment::createSymlink(TestEnvironment::temporaryPath("envoy_test/watcher_target"), + TestEnvironment::temporaryPath("envoy_test/watcher_new_link")); + TestEnvironment::renameFile(TestEnvironment::temporaryPath("envoy_test/watcher_new_link"), + TestEnvironment::temporaryPath("envoy_test/watcher_link")); dispatcher_->run(Event::Dispatcher::RunType::Block); } TEST_F(WatcherImplTest, Modify) { Filesystem::WatcherPtr watcher = dispatcher_->createFilesystemWatcher(); - TestUtility::createDirectory(TestEnvironment::temporaryPath("envoy_test")); + TestEnvironment::createPath(TestEnvironment::temporaryPath("envoy_test")); std::ofstream file(TestEnvironment::temporaryPath("envoy_test/watcher_target")); WatchCallback callback; @@ -106,6 +106,7 @@ TEST_F(WatcherImplTest, Modify) { dispatcher_->run(Event::Dispatcher::RunType::NonBlock); file << "text" << std::flush; + file.close(); EXPECT_CALL(callback, called(Watcher::Events::Modified)); dispatcher_->run(Event::Dispatcher::RunType::NonBlock); } @@ -125,7 +126,7 @@ TEST_F(WatcherImplTest, BadPath) { TEST_F(WatcherImplTest, ParentDirectoryRemoved) { Filesystem::WatcherPtr watcher = dispatcher_->createFilesystemWatcher(); - TestUtility::createDirectory(TestEnvironment::temporaryPath("envoy_test_empty")); + TestEnvironment::createPath(TestEnvironment::temporaryPath("envoy_test_empty")); WatchCallback callback; EXPECT_CALL(callback, called(testing::_)).Times(0); @@ -143,7 +144,11 @@ TEST_F(WatcherImplTest, ParentDirectoryRemoved) { TEST_F(WatcherImplTest, RootDirectoryPath) { Filesystem::WatcherPtr watcher = dispatcher_->createFilesystemWatcher(); +#ifndef WIN32 EXPECT_NO_THROW(watcher->addWatch("/", Watcher::Events::MovedTo, [&](uint32_t) -> void {})); +#else + EXPECT_NO_THROW(watcher->addWatch("c:\\", Watcher::Events::MovedTo, [&](uint32_t) -> void {})); +#endif } } // namespace Filesystem diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc index f3a1ad42de20..95f92827b007 100644 --- a/test/common/runtime/runtime_impl_test.cc +++ b/test/common/runtime/runtime_impl_test.cc @@ -100,8 +100,8 @@ class LoaderImplTest : public testing::Test { EXPECT_CALL(dispatcher_, createFilesystemWatcher_()).WillRepeatedly(InvokeWithoutArgs([this] { Filesystem::MockWatcher* mock_watcher = new NiceMock(); EXPECT_CALL(*mock_watcher, addWatch(_, Filesystem::Watcher::Events::MovedTo, _)) - .WillRepeatedly(Invoke( - [this](const std::string& path, uint32_t, Filesystem::Watcher::OnChangedCb cb) { + .WillRepeatedly( + Invoke([this](absl::string_view path, uint32_t, Filesystem::Watcher::OnChangedCb cb) { EXPECT_EQ(path, expected_watch_root_); on_changed_cbs_.emplace_back(cb); })); diff --git a/test/config/utility.cc b/test/config/utility.cc index de5c55049e3b..e4514f59e1e5 100644 --- a/test/config/utility.cc +++ b/test/config/utility.cc @@ -824,7 +824,7 @@ void ConfigHelper::setLds(absl::string_view version_info) { const std::string lds_filename = bootstrap().dynamic_resources().lds_config().path(); std::string file = TestEnvironment::writeStringToFileForTest( "new_lds_file", MessageUtil::getJsonStringFromMessage(lds)); - TestUtility::renameFile(file, lds_filename); + TestEnvironment::renameFile(file, lds_filename); } void ConfigHelper::setOutboundFramesLimits(uint32_t max_all_frames, uint32_t max_control_frames) { @@ -857,7 +857,7 @@ void CdsHelper::setCds(const std::vector& c // FilesystemSubscriptionImpl is subscribed to. std::string path = TestEnvironment::writeStringToFileForTest("cds.update.pb_text", cds_response.DebugString()); - TestUtility::renameFile(path, cds_path_); + TestEnvironment::renameFile(path, cds_path_); } EdsHelper::EdsHelper() : eds_path_(TestEnvironment::writeStringToFileForTest("eds.pb_text", "")) { @@ -879,7 +879,7 @@ void EdsHelper::setEds(const std::vectorcurrent_test_info()->name() + - "/"; + testing::UnitTest::GetInstance()->current_test_info()->name(); TestEnvironment::createPath(path_prefix); - return path_prefix; + return path_prefix + "/"; } std::vector diff --git a/test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc b/test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc index 4f6719337da8..09f610ede4c5 100644 --- a/test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc +++ b/test/extensions/quic_listeners/quiche/platform/quic_platform_test.cc @@ -614,7 +614,7 @@ class FileUtilsTest : public testing::Test { } protected: - void SetUp() override { Envoy::TestUtility::createDirectory(dir_path_); } + void SetUp() override { Envoy::TestEnvironment::createPath(dir_path_); } void TearDown() override { while (!files_to_remove_.empty()) { @@ -627,7 +627,7 @@ class FileUtilsTest : public testing::Test { void addSubDirs(std::list sub_dirs) { for (const std::string& dir_name : sub_dirs) { const std::string full_path = dir_path_ + "/" + dir_name; - Envoy::TestUtility::createDirectory(full_path); + Envoy::TestEnvironment::createPath(full_path); files_to_remove_.push(full_path); } } diff --git a/test/mocks/filesystem/mocks.h b/test/mocks/filesystem/mocks.h index 50889049a2f1..b6fca8ed4af1 100644 --- a/test/mocks/filesystem/mocks.h +++ b/test/mocks/filesystem/mocks.h @@ -52,6 +52,7 @@ class MockInstance : public Instance { MOCK_METHOD(bool, directoryExists, (const std::string&)); MOCK_METHOD(ssize_t, fileSize, (const std::string&)); MOCK_METHOD(std::string, fileReadToEnd, (const std::string&)); + MOCK_METHOD(PathSplitResult, splitPathFromFilename, (absl::string_view)); MOCK_METHOD(bool, illegalPath, (const std::string&)); }; @@ -60,7 +61,7 @@ class MockWatcher : public Watcher { MockWatcher(); ~MockWatcher() override; - MOCK_METHOD(void, addWatch, (const std::string&, uint32_t, OnChangedCb)); + MOCK_METHOD(void, addWatch, (absl::string_view, uint32_t, OnChangedCb)); }; } // namespace Filesystem diff --git a/test/server/server_test.cc b/test/server/server_test.cc index dcadc5ab3bba..e976a7a60c6d 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -440,7 +440,7 @@ TEST_P(ServerInstanceImplTest, V2ConfigOnly) { options_.service_cluster_name_ = "some_cluster_name"; options_.service_node_name_ = "some_node_name"; try { - initialize(std::string()); + initialize("test/server/test_data/server/unparseable_bootstrap.yaml"); FAIL(); } catch (const EnvoyException& e) { EXPECT_THAT(e.what(), HasSubstr("Unable to parse JSON as proto")); @@ -847,7 +847,7 @@ TEST_P(ServerInstanceImplTest, LogToFileError) { options_.service_cluster_name_ = "some_cluster_name"; options_.service_node_name_ = "some_node_name"; try { - initialize(std::string()); + initialize("test/server/test_data/server/empty_bootstrap.yaml"); FAIL(); } catch (const EnvoyException& e) { EXPECT_THAT(e.what(), HasSubstr("Failed to open log-file")); diff --git a/test/server/test_data/server/unparseable_bootstrap.yaml b/test/server/test_data/server/unparseable_bootstrap.yaml new file mode 100644 index 000000000000..3867010ce7fe --- /dev/null +++ b/test/server/test_data/server/unparseable_bootstrap.yaml @@ -0,0 +1 @@ +- foo: bar diff --git a/test/test_common/BUILD b/test/test_common/BUILD index c01d59968da8..f4ee907aea7f 100644 --- a/test/test_common/BUILD +++ b/test/test_common/BUILD @@ -34,6 +34,7 @@ envoy_cc_test_library( "//source/common/common:compiler_requirements_lib", "//source/common/common:macros", "//source/common/common:utility_lib", + "//source/common/filesystem:filesystem_lib", "//source/common/json:json_loader_lib", "//source/common/network:utility_lib", "//source/server:options_lib", diff --git a/test/test_common/environment.cc b/test/test_common/environment.cc index 8e52498ac099..9c5b0c61e987 100644 --- a/test/test_common/environment.cc +++ b/test/test_common/environment.cc @@ -1,14 +1,5 @@ #include "test/test_common/environment.h" -// TODO(asraa): Remove and rely only on when Envoy requires -// Clang >= 9. -#if defined(_LIBCPP_VERSION) && !defined(__APPLE__) -#include -#elif defined __has_include -#if __has_include() && !defined(__APPLE__) -#include -#endif -#endif #include #include #include @@ -17,15 +8,18 @@ #include #include +#include "envoy/common/platform.h" + #include "common/common/assert.h" #include "common/common/compiler_requirements.h" #include "common/common/logger.h" #include "common/common/macros.h" #include "common/common/utility.h" -#include "envoy/common/platform.h" +#include "common/filesystem/directory.h" #include "server/options_impl.h" +#include "test/test_common/file_system_for_test.h" #include "test/test_common/network_utility.h" #include "absl/strings/match.h" @@ -33,6 +27,8 @@ #include "spdlog/spdlog.h" using bazel::tools::cpp::runfiles::Runfiles; +using Envoy::Filesystem::Directory; +using Envoy::Filesystem::DirectoryEntry; namespace Envoy { namespace { @@ -43,11 +39,7 @@ std::string makeTempDir(std::string basename_template) { char* dirname = ::_mktemp(&name_template[0]); RELEASE_ASSERT(dirname != nullptr, fmt::format("failed to create tempdir from template: {} {}", name_template, strerror(errno))); -#if defined(_LIBCPP_VERSION) && _LIBCPP_VERSION >= 9000 && !defined(__APPLE__) - std::__fs::filesystem::create_directories(dirname); -#elif defined __cpp_lib_experimental_filesystem && !defined(__APPLE__) - std::experimental::filesystem::create_directories(dirname); -#endif + TestEnvironment::createPath(dirname); #else std::string name_template = "/tmp/" + basename_template; char* dirname = ::mkdtemp(&name_template[0]); @@ -69,13 +61,16 @@ std::string getOrCreateUnixDomainSocketDirectory() { } std::string getTemporaryDirectory() { + std::string temp_dir; if (std::getenv("TEST_TMPDIR")) { - return TestEnvironment::getCheckedEnvVar("TEST_TMPDIR"); - } - if (std::getenv("TMPDIR")) { - return TestEnvironment::getCheckedEnvVar("TMPDIR"); + temp_dir = TestEnvironment::getCheckedEnvVar("TEST_TMPDIR"); + } else if (std::getenv("TMPDIR")) { + temp_dir = TestEnvironment::getCheckedEnvVar("TMPDIR"); + } else { + return makeTempDir("envoy_test_tmp.XXXXXX"); } - return makeTempDir("envoy_test_tmp.XXXXXX"); + TestEnvironment::createPath(temp_dir); + return temp_dir; } // Allow initializeOptions() to remember CLI args for getOptions(). @@ -85,49 +80,97 @@ char** argv_; } // namespace void TestEnvironment::createPath(const std::string& path) { -#if defined(_LIBCPP_VERSION) && _LIBCPP_VERSION >= 9000 && !defined(__APPLE__) - // We don't want to rely on mkdir etc. if we can avoid it, since it might not - // exist in some environments such as ClusterFuzz. - std::__fs::filesystem::create_directories(std::__fs::filesystem::path(path)); -#elif defined __cpp_lib_experimental_filesystem - std::experimental::filesystem::create_directories(std::experimental::filesystem::path(path)); + if (Filesystem::fileSystemForTest().directoryExists(path)) { + return; + } + const Filesystem::PathSplitResult parent = + Filesystem::fileSystemForTest().splitPathFromFilename(path); + if (parent.file_.length() > 0) { + TestEnvironment::createPath(std::string(parent.directory_)); + } +#ifndef WIN32 + RELEASE_ASSERT(::mkdir(path.c_str(), S_IRWXU | S_IRWXG | S_IRWXO) == 0, + absl::StrCat("failed to create path: ", path)); #else - // No support on this system for std::filesystem or std::experimental::filesystem. - RELEASE_ASSERT(::system(("mkdir -p " + path).c_str()) == 0, ""); + RELEASE_ASSERT(::CreateDirectory(path.c_str(), NULL), + absl::StrCat("failed to create path: ", path)); #endif } -void TestEnvironment::createParentPath(const std::string& path) { -#if defined(_LIBCPP_VERSION) && _LIBCPP_VERSION >= 9000 && !defined(__APPLE__) - // We don't want to rely on mkdir etc. if we can avoid it, since it might not - // exist in some environments such as ClusterFuzz. - std::__fs::filesystem::create_directories(std::__fs::filesystem::path(path).parent_path()); -#elif defined __cpp_lib_experimental_filesystem && !defined(__APPLE__) - std::experimental::filesystem::create_directories( - std::experimental::filesystem::path(path).parent_path()); +// On linux, attempt to unlink any file that exists at path, +// ignoring the result code, to avoid traversing a symlink, +// On windows, also attempt to remove the directory in case +// it is actually a symlink/junction, ignoring the result code. +// Proceed to iteratively recurse the directory if it still remains +void TestEnvironment::removePath(const std::string& path) { + RELEASE_ASSERT(absl::StartsWith(path, TestEnvironment::temporaryDirectory()), + "cowardly refusing to remove test directory not in temp path"); +#ifndef WIN32 + (void)::unlink(path.c_str()); #else - // No support on this system for std::filesystem or std::experimental::filesystem. - RELEASE_ASSERT(::system(("mkdir -p $(dirname " + path + ")").c_str()) == 0, ""); + (void)::DeleteFile(path.c_str()); + (void)::RemoveDirectory(path.c_str()); #endif -} - -void TestEnvironment::removePath(const std::string& path) { - RELEASE_ASSERT(absl::StartsWith(path, TestEnvironment::temporaryDirectory()), ""); -#if defined(_LIBCPP_VERSION) && _LIBCPP_VERSION >= 9000 && !defined(__APPLE__) - // We don't want to rely on mkdir etc. if we can avoid it, since it might not - // exist in some environments such as ClusterFuzz. - if (!std::__fs::filesystem::exists(path)) { + if (!Filesystem::fileSystemForTest().directoryExists(path)) { return; } - std::__fs::filesystem::remove_all(std::__fs::filesystem::path(path)); -#elif defined __cpp_lib_experimental_filesystem && !defined(__APPLE__) - if (!std::experimental::filesystem::exists(path)) { - return; + Directory directory(path); + std::string entry_name; + entry_name.reserve(path.size() + 256); + entry_name.append(path); + entry_name.append("/"); + size_t fileidx = entry_name.size(); + for (const DirectoryEntry& entry : directory) { + entry_name.resize(fileidx); + entry_name.append(entry.name_); + if (entry.type_ == Envoy::Filesystem::FileType::Regular) { +#ifndef WIN32 + RELEASE_ASSERT(::unlink(entry_name.c_str()) == 0, + absl::StrCat("failed to remove file: ", entry_name)); +#else + RELEASE_ASSERT(::DeleteFile(entry_name.c_str()), + absl::StrCat("failed to remove file: ", entry_name)); +#endif + } else if (entry.type_ == Envoy::Filesystem::FileType::Directory) { + if (entry.name_ != "." && entry.name_ != "..") { + removePath(entry_name); + } + } } - std::experimental::filesystem::remove_all(std::experimental::filesystem::path(path)); +#ifndef WIN32 + RELEASE_ASSERT(::rmdir(path.c_str()) == 0, + absl::StrCat("failed to remove path: ", path, " (rmdir failed)")); #else - // No support on this system for std::filesystem or std::experimental::filesystem. - RELEASE_ASSERT(::system(("rm -rf " + path).c_str()) == 0, ""); + RELEASE_ASSERT(::RemoveDirectory(path.c_str()), absl::StrCat("failed to remove path: ", path)); +#endif +} + +void TestEnvironment::renameFile(const std::string& old_name, const std::string& new_name) { +#ifdef WIN32 + // use MoveFileEx, since ::rename will not overwrite an existing file. See + // https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/rename-wrename?view=vs-2017 + const BOOL rc = ::MoveFileEx(old_name.c_str(), new_name.c_str(), MOVEFILE_REPLACE_EXISTING); + ASSERT_NE(0, rc); +#else + const int rc = ::rename(old_name.c_str(), new_name.c_str()); + ASSERT_EQ(0, rc); +#endif +}; + +void TestEnvironment::createSymlink(const std::string& target, const std::string& link) { +#ifdef WIN32 + const DWORD attributes = ::GetFileAttributes(target.c_str()); + ASSERT_NE(attributes, INVALID_FILE_ATTRIBUTES); + int flags = SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE; + if (attributes & FILE_ATTRIBUTE_DIRECTORY) { + flags |= SYMBOLIC_LINK_FLAG_DIRECTORY; + } + + const BOOLEAN rc = ::CreateSymbolicLink(link.c_str(), target.c_str(), flags); + ASSERT_NE(rc, 0); +#else + const int rc = ::symlink(target.c_str(), link.c_str()); + ASSERT_EQ(rc, 0); #endif } @@ -276,6 +319,7 @@ std::string TestEnvironment::temporaryFileSubstitute(const std::string& path, const ParamMap& param_map, const PortMap& port_map, Network::Address::IpVersion version) { + RELEASE_ASSERT(!path.empty(), "requested path to substitute in is empty"); // Load the entire file as a string, regex replace one at a time and write it back out. Proper // templating might be better one day, but this works for now. const std::string json_path = TestEnvironment::runfilesPath(path); @@ -296,12 +340,12 @@ std::string TestEnvironment::temporaryFileSubstitute(const std::string& path, // Substitute paths and other common things. out_json_string = substitute(out_json_string, version); - const std::string extension = absl::EndsWith(path, ".yaml") + auto name = Filesystem::fileSystemForTest().splitPathFromFilename(path).file_; + const std::string extension = absl::EndsWith(name, ".yaml") ? ".yaml" - : absl::EndsWith(path, ".pb_text") ? ".pb_text" : ".json"; + : absl::EndsWith(name, ".pb_text") ? ".pb_text" : ".json"; const std::string out_json_path = - TestEnvironment::temporaryPath(path + ".with.ports" + extension); - createParentPath(out_json_path); + TestEnvironment::temporaryPath(name) + ".with.ports" + extension; { std::ofstream out_json_file(out_json_path); out_json_file << out_json_string; @@ -334,7 +378,6 @@ std::string TestEnvironment::writeStringToFileForTest(const std::string& filenam bool fully_qualified_path) { const std::string out_path = fully_qualified_path ? filename : TestEnvironment::temporaryPath(filename); - createParentPath(out_path); unlink(out_path.c_str()); { std::ofstream out_file(out_path, std::ios_base::binary); @@ -361,6 +404,7 @@ void TestEnvironment::setEnvVar(const std::string& name, const std::string& valu ASSERT_EQ(0, rc); #endif } + void TestEnvironment::unsetEnvVar(const std::string& name) { #ifdef WIN32 const int rc = ::_putenv_s(name.c_str(), ""); @@ -375,4 +419,25 @@ void TestEnvironment::setRunfiles(Runfiles* runfiles) { runfiles_ = runfiles; } Runfiles* TestEnvironment::runfiles_{}; +AtomicFileUpdater::AtomicFileUpdater(const std::string& filename) + : link_(filename), new_link_(absl::StrCat(filename, ".new")), + target1_(absl::StrCat(filename, ".target1")), target2_(absl::StrCat(filename, ".target2")), + use_target1_(true) { + unlink(link_.c_str()); + unlink(new_link_.c_str()); + unlink(target1_.c_str()); + unlink(target2_.c_str()); +} + +void AtomicFileUpdater::update(const std::string& contents) { + const std::string target = use_target1_ ? target1_ : target2_; + use_target1_ = !use_target1_; + { + std::ofstream file(target, std::ios_base::binary); + file << contents; + } + TestEnvironment::createSymlink(target, new_link_); + TestEnvironment::renameFile(new_link_, link_); +} + } // namespace Envoy diff --git a/test/test_common/environment.h b/test/test_common/environment.h index f5c82d5d5e51..ed9886ef63af 100644 --- a/test/test_common/environment.h +++ b/test/test_common/environment.h @@ -10,6 +10,8 @@ #include "common/json/json_loader.h" +#include "absl/strings/str_cat.h" +#include "absl/strings/string_view.h" #include "absl/types/optional.h" #include "tools/cpp/runfiles/runfiles.h" @@ -74,8 +76,8 @@ class TestEnvironment { * @param path path suffix. * @return std::string path qualified with temporary directory. */ - static std::string temporaryPath(const std::string& path) { - return temporaryDirectory() + "/" + path; + static std::string temporaryPath(absl::string_view path) { + return absl::StrCat(temporaryDirectory(), "/", path); } /** @@ -188,16 +190,24 @@ class TestEnvironment { static void createPath(const std::string& path); /** - * Create a parent path on the filesystem (mkdir -p $(dirname ...) equivalent). + * Remove a path on the filesystem (rm -rf ... equivalent). * @param path. */ - static void createParentPath(const std::string& path); + static void removePath(const std::string& path); /** - * Remove a path on the filesystem (rm -rf ... equivalent). - * @param path. + * Rename a file + * @param old_name + * @param new_name */ - static void removePath(const std::string& path); + static void renameFile(const std::string& old_name, const std::string& new_name); + + /** + * Create a symlink + * @param target + * @param link + */ + static void createSymlink(const std::string& target, const std::string& link); /** * Set environment variable. Same args as setenv(2). @@ -218,4 +228,24 @@ class TestEnvironment { static bazel::tools::cpp::runfiles::Runfiles* runfiles_; }; +/** + * A utility class for atomically updating a file using symbolic link swap. + * Note the file lifetime is limited to the instance of the AtomicFileUpdater + * which erases any existing files upon creation, used for specific test + * scenarios. See discussion at https://github.com/envoyproxy/envoy/pull/4298 + */ +class AtomicFileUpdater { +public: + AtomicFileUpdater(const std::string& filename); + + void update(const std::string& contents); + +private: + const std::string link_; + const std::string new_link_; + const std::string target1_; + const std::string target2_; + bool use_target1_; +}; + } // namespace Envoy diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index 37dc8a74277b..0281a62049df 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -244,43 +244,6 @@ std::vector TestUtility::split(const std::string& source, const std return ret; } -void TestUtility::renameFile(const std::string& old_name, const std::string& new_name) { -#ifdef WIN32 - // use MoveFileEx, since ::rename will not overwrite an existing file. See - // https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/rename-wrename?view=vs-2017 - const BOOL rc = ::MoveFileEx(old_name.c_str(), new_name.c_str(), MOVEFILE_REPLACE_EXISTING); - ASSERT_NE(0, rc); -#else - const int rc = ::rename(old_name.c_str(), new_name.c_str()); - ASSERT_EQ(0, rc); -#endif -}; - -void TestUtility::createDirectory(const std::string& name) { -#ifdef WIN32 - ::_mkdir(name.c_str()); -#else - ::mkdir(name.c_str(), S_IRWXU); -#endif -} - -void TestUtility::createSymlink(const std::string& target, const std::string& link) { -#ifdef WIN32 - const DWORD attributes = ::GetFileAttributes(target.c_str()); - ASSERT_NE(attributes, INVALID_FILE_ATTRIBUTES); - int flags = SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE; - if (attributes & FILE_ATTRIBUTE_DIRECTORY) { - flags |= SYMBOLIC_LINK_FLAG_DIRECTORY; - } - - const BOOLEAN rc = ::CreateSymbolicLink(link.c_str(), target.c_str(), flags); - ASSERT_NE(rc, 0); -#else - const int rc = ::symlink(target.c_str(), link.c_str()); - ASSERT_EQ(rc, 0); -#endif -} - // static absl::Time TestUtility::parseTime(const std::string& input, const std::string& input_format) { absl::Time time; @@ -361,27 +324,6 @@ void ConditionalInitializer::wait() { } } -AtomicFileUpdater::AtomicFileUpdater(const std::string& filename) - : link_(filename), new_link_(absl::StrCat(filename, ".new")), - target1_(absl::StrCat(filename, ".target1")), target2_(absl::StrCat(filename, ".target2")), - use_target1_(true) { - unlink(link_.c_str()); - unlink(new_link_.c_str()); - unlink(target1_.c_str()); - unlink(target2_.c_str()); -} - -void AtomicFileUpdater::update(const std::string& contents) { - const std::string target = use_target1_ ? target1_ : target2_; - use_target1_ = !use_target1_; - { - std::ofstream file(target, std::ios_base::binary); - file << contents; - } - TestUtility::createSymlink(target, new_link_); - TestUtility::renameFile(new_link_, link_); -} - constexpr std::chrono::milliseconds TestUtility::DefaultTimeout; namespace Http { diff --git a/test/test_common/utility.h b/test/test_common/utility.h index 433a7b363f8b..be94c0749d35 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -461,10 +461,6 @@ class TestUtility { static constexpr std::chrono::milliseconds DefaultTimeout = std::chrono::milliseconds(10000); - static void renameFile(const std::string& old_name, const std::string& new_name); - static void createDirectory(const std::string& name); - static void createSymlink(const std::string& target, const std::string& link); - /** * Return a prefix string matcher. * @param string prefix. @@ -616,23 +612,6 @@ class ConditionalInitializer { bool ready_{false}; }; -/** - * A utility class for atomically updating a file using symbolic link swap. - */ -class AtomicFileUpdater { -public: - AtomicFileUpdater(const std::string& filename); - - void update(const std::string& contents); - -private: - const std::string link_; - const std::string new_link_; - const std::string target1_; - const std::string target2_; - bool use_target1_; -}; - namespace Http { /** diff --git a/tools/check_format.py b/tools/check_format.py index 06735863f23a..e0de9b0a7f2c 100755 --- a/tools/check_format.py +++ b/tools/check_format.py @@ -164,13 +164,33 @@ # yapf: enable -# Map a line transformation function across each line of a file. -# .bak temporaries. -def replaceLines(path, line_xform): +# Map a line transformation function across each line of a file, +# writing the result lines as requested. +# If there is a clang format nesting or mismatch error, return the first occurrence +def evaluateLines(path, line_xform, write=True): + error_message = None + format_flag = True + output_lines = [] + for line_number, line in enumerate(readLines(path)): + if line.find("// clang-format off") != -1: + if not format_flag and error_message is None: + error_message = "%s:%d: %s" % (path, line_number + 1, "clang-format nested off") + format_flag = False + if line.find("// clang-format on") != -1: + if format_flag and error_message is None: + error_message = "%s:%d: %s" % (path, line_number + 1, "clang-format nested on") + format_flag = True + if format_flag: + output_lines.append(line_xform(line, line_number)) + else: + output_lines.append(line) # We used to use fileinput in the older Python 2.7 script, but this doesn't do # inplace mode and UTF-8 in Python 3, so doing it the manual way. - output_lines = [line_xform(line) for line in readLines(path)] - pathlib.Path(path).write_text('\n'.join(output_lines), encoding='utf-8') + if write: + pathlib.Path(path).write_text('\n'.join(output_lines), encoding='utf-8') + if not format_flag and error_message is None: + error_message = "%s:%d: %s" % (path, line_number + 1, "clang-format remains off") + return error_message # Obtain all the lines in a given file. @@ -409,19 +429,24 @@ def checkFileContents(file_path, checker): # notes have a different format. checkCurrentReleaseNotes(file_path, error_messages) - for line_number, line in enumerate(readLines(file_path)): + def checkFormatErrors(line, line_number): def reportError(message): error_messages.append("%s:%d: %s" % (file_path, line_number + 1, message)) checker(line, file_path, reportError) + + evaluate_failure = evaluateLines(file_path, checkFormatErrors, False) + if evaluate_failure is not None: + error_messages.append(evaluate_failure) + return error_messages DOT_MULTI_SPACE_REGEX = re.compile("\\. +") -def fixSourceLine(line): +def fixSourceLine(line, line_number): # Strip double space after '.' This may prove overenthusiastic and need to # be restricted to comments and metadata files but works for now. line = re.sub(DOT_MULTI_SPACE_REGEX, ". ", line) @@ -606,7 +631,7 @@ def checkBuildLine(line, file_path, reportError): reportError("Superfluous '@envoy//' prefix") -def fixBuildLine(file_path, line): +def fixBuildLine(file_path, line, line_number): if (envoy_build_rule_check and not isSkylarkFile(file_path) and not isWorkspaceFile(file_path) and not isExternalBuildFile(file_path)): line = line.replace("@envoy//", "//") @@ -614,7 +639,7 @@ def fixBuildLine(file_path, line): def fixBuildPath(file_path): - replaceLines(file_path, functools.partial(fixBuildLine, file_path)) + evaluateLines(file_path, functools.partial(fixBuildLine, file_path)) error_messages = [] @@ -654,7 +679,7 @@ def checkBuildPath(file_path): def fixSourcePath(file_path): - replaceLines(file_path, fixSourceLine) + evaluateLines(file_path, fixSourceLine) error_messages = [] diff --git a/tools/check_format_test_helper.py b/tools/check_format_test_helper.py index 7474b429b6d1..ac1cd083eeaa 100755 --- a/tools/check_format_test_helper.py +++ b/tools/check_format_test_helper.py @@ -209,6 +209,9 @@ def runChecks(): "grpc_shutdown.cc", "Don't call grpc_init() or grpc_shutdown() directly, instantiate Grpc::GoogleGrpcContext. " + "See #8282") + errors += checkUnfixableError("clang_format_double_off.cc", "clang-format nested off") + errors += checkUnfixableError("clang_format_trailing_off.cc", "clang-format remains off") + errors += checkUnfixableError("clang_format_double_on.cc", "clang-format nested on") errors += fixFileExpectingFailure( "api/missing_package.proto", "Unable to find package name for proto file: ./api/missing_package.proto") @@ -225,6 +228,8 @@ def runChecks(): errors += checkAndFixError("proto_style.cc", "incorrect protobuf type reference") errors += checkAndFixError("long_line.cc", "clang-format check failed") errors += checkAndFixError("header_order.cc", "header_order.py check failed") + errors += checkAndFixError("clang_format_on.cc", + "./clang_format_on.cc:7: over-enthusiastic spaces") # Validate that a missing license is added. errors += checkAndFixError("license.BUILD", "envoy_build_fixer check failed") # Validate that an incorrect license is replaced and reordered. @@ -252,6 +257,7 @@ def runChecks(): errors += checkFileExpectingOK("real_time_source_override.cc") errors += checkFileExpectingOK("time_system_wait_for.cc") + errors += checkFileExpectingOK("clang_format_off.cc") return errors diff --git a/tools/spelling_dictionary.txt b/tools/spelling_dictionary.txt index 746f36155a11..f7f36268c350 100644 --- a/tools/spelling_dictionary.txt +++ b/tools/spelling_dictionary.txt @@ -160,6 +160,7 @@ LEV LF LHS LLVM +LPT LRS MB MD @@ -296,6 +297,7 @@ UA UBSAN UDP UDS +UNC URI URL USEVC @@ -692,6 +694,7 @@ lookups loopback lossy lowp +lstat ltrim lua lyft @@ -922,6 +925,7 @@ ruleset runfiles runtime runtimes +rver sandboxed sanitization sanitizer diff --git a/tools/testdata/check_format/clang_format_double_off.cc b/tools/testdata/check_format/clang_format_double_off.cc new file mode 100644 index 000000000000..a279204d8c0a --- /dev/null +++ b/tools/testdata/check_format/clang_format_double_off.cc @@ -0,0 +1,7 @@ +namespace Envoy { + +// clang-format off +// Turning clang format off should not be nested +// clang-format off + +} // namespace Envoy diff --git a/tools/testdata/check_format/clang_format_double_on.cc b/tools/testdata/check_format/clang_format_double_on.cc new file mode 100644 index 000000000000..80a636133857 --- /dev/null +++ b/tools/testdata/check_format/clang_format_double_on.cc @@ -0,0 +1,6 @@ +namespace Envoy { + +// Turning clang format on when it already is enabled is not allowed +// clang-format on + +} // namespace Envoy diff --git a/tools/testdata/check_format/clang_format_off.cc b/tools/testdata/check_format/clang_format_off.cc new file mode 100644 index 000000000000..3e78b6a76d9e --- /dev/null +++ b/tools/testdata/check_format/clang_format_off.cc @@ -0,0 +1,13 @@ +namespace Envoy { + +// clang-format off +// Deliberate trailing spaces after periods in formatted content/comments +// need to be retained, follow clang-format handling for Envoy-specific +// rule sets. +// ~ ~ +// . . +// ) +// ---- +// clang-format on + +} // namespace Envoy diff --git a/tools/testdata/check_format/clang_format_on.cc b/tools/testdata/check_format/clang_format_on.cc new file mode 100644 index 000000000000..7dc7ac947be3 --- /dev/null +++ b/tools/testdata/check_format/clang_format_on.cc @@ -0,0 +1,9 @@ +namespace Envoy { + +// clang-format off +// Some ignored content. +// clang-format on +// Over enthusiastic spaces should be fixed after clang-format is turned on +// Too many spaces. Here. + +} // namespace Envoy diff --git a/tools/testdata/check_format/clang_format_on.cc.gold b/tools/testdata/check_format/clang_format_on.cc.gold new file mode 100644 index 000000000000..f65ba0e299b2 --- /dev/null +++ b/tools/testdata/check_format/clang_format_on.cc.gold @@ -0,0 +1,9 @@ +namespace Envoy { + +// clang-format off +// Some ignored content. +// clang-format on +// Over enthusiastic spaces should be fixed after clang-format is turned on +// Too many spaces. Here. + +} // namespace Envoy diff --git a/tools/testdata/check_format/clang_format_trailing_off.cc b/tools/testdata/check_format/clang_format_trailing_off.cc new file mode 100644 index 000000000000..cbd6ffdbe97e --- /dev/null +++ b/tools/testdata/check_format/clang_format_trailing_off.cc @@ -0,0 +1,6 @@ +namespace Envoy { + +// clang format should be turned back on before the end of the file +// clang-format off + +} // namespace Envoy