diff options
| author | android-build-prod (mdb) <android-build-team-robot@google.com> | 2020-10-01 20:30:01 +0000 |
|---|---|---|
| committer | android-build-prod (mdb) <android-build-team-robot@google.com> | 2020-10-01 20:30:01 +0000 |
| commit | 856365b4b8fdbcaee45bd9cac3380f69de84c481 (patch) | |
| tree | e60b2c1df95885dc9e0c67cf9558b8898b036188 | |
| parent | 38c21bba201db8c6ca0a52935adcfac6dda54636 (diff) | |
| parent | 49aee802dc56f64c5eb55504bbe88120fafb6f68 (diff) | |
| download | platform_external_libbrillo-sdk-release.tar.gz platform_external_libbrillo-sdk-release.tar.bz2 platform_external_libbrillo-sdk-release.zip | |
Snap for 6877830 from 49aee802dc56f64c5eb55504bbe88120fafb6f68 to sdk-releasesdk-release
Change-Id: I16701d8f27ec745ac49b6f018c5dc70507ac1781
71 files changed, 1911 insertions, 1176 deletions
@@ -8,17 +8,17 @@ import("//common-mk/proto_library.gni") group("all") { deps = [ - ":libbrillo-${libbase_ver}", - ":libbrillo-glib-${libbase_ver}", - ":libbrillo-test-${libbase_ver}", - ":libinstallattributes-${libbase_ver}", - ":libpolicy-${libbase_ver}", + ":libbrillo", + ":libbrillo-glib", + ":libbrillo-test", + ":libinstallattributes", + ":libpolicy", ] if (use.test) { deps += [ - ":libbrillo-${libbase_ver}_tests", - ":libinstallattributes-${libbase_ver}_tests", - ":libpolicy-${libbase_ver}_tests", + ":libbrillo_tests", + ":libinstallattributes_tests", + ":libpolicy_tests", ] } if (use.fuzzer) { @@ -44,7 +44,7 @@ config("target_defaults") { ] } -config("libbrillo-${libbase_ver}_configs") { +config("libbrillo_configs") { include_dirs = [ "../libbrillo" ] } @@ -56,7 +56,7 @@ libbrillo_sublibs = [ # |library_name| is library file name without "lib" prefix. This is needed # for composing -l*** flags in libbrillo-${libbasever}.so. # (Current version of GN deployed to ChromeOS doesn't have string_replace.) - library_name = "brillo-core-${libbase_ver}" + library_name = "brillo-core" if (use.dbus) { all_dependent_pkg_deps = [ "dbus-1" ] } @@ -81,11 +81,11 @@ libbrillo_sublibs = [ "brillo/process.cc", "brillo/process_information.cc", "brillo/process_reaper.cc", - "brillo/scoped_mount_namespace.cc", "brillo/scoped_umask.cc", "brillo/secure_blob.cc", "brillo/strings/string_utils.cc", "brillo/syslog_logging.cc", + "brillo/timezone/tzif_parser.cc", "brillo/type_name_undecorate.cc", "brillo/url_utils.cc", "brillo/userdb_utils.cc", @@ -112,13 +112,9 @@ libbrillo_sublibs = [ }, { - library_name = "brillo-blockdeviceutils-${libbase_ver}" - deps = [ - ":libbrillo-core-${libbase_ver}", - ] - sources = [ - "brillo/blkdev_utils/loop_device.cc", - ] + library_name = "brillo-blockdeviceutils" + deps = [ ":libbrillo-core" ] + sources = [ "brillo/blkdev_utils/loop_device.cc" ] if (use.device_mapper) { pkg_deps = [ "devmapper" ] sources += [ @@ -129,10 +125,10 @@ libbrillo_sublibs = [ }, { - library_name = "brillo-http-${libbase_ver}" + library_name = "brillo-http" deps = [ - ":libbrillo-core-${libbase_ver}", - ":libbrillo-streams-${libbase_ver}", + ":libbrillo-core", + ":libbrillo-streams", ] all_dependent_pkg_deps = [ "libcurl" ] sources = [ @@ -150,10 +146,8 @@ libbrillo_sublibs = [ }, { - library_name = "brillo-streams-${libbase_ver}" - deps = [ - ":libbrillo-core-${libbase_ver}", - ] + library_name = "brillo-streams" + deps = [ ":libbrillo-core" ] all_dependent_pkg_deps = [ "openssl" ] sources = [ "brillo/streams/file_stream.cc", @@ -169,34 +163,38 @@ libbrillo_sublibs = [ }, { - library_name = "brillo-cryptohome-${libbase_ver}" + library_name = "brillo-cryptohome" all_dependent_pkg_deps = [ "openssl" ] + sources = [ "brillo/cryptohome.cc" ] + }, + + { + library_name = "brillo-namespaces" + deps = [ ":libbrillo-core" ] sources = [ - "brillo/cryptohome.cc", + "brillo/namespaces/mount_namespace.cc", + "brillo/namespaces/platform.cc", + "brillo/scoped_mount_namespace.cc", ] }, { - library_name = "brillo-minijail-${libbase_ver}" + library_name = "brillo-minijail" all_dependent_pkg_deps = [ "libminijail" ] - sources = [ - "brillo/minijail/minijail.cc", - ] + sources = [ "brillo/minijail/minijail.cc" ] }, { - library_name = "brillo-protobuf-${libbase_ver}" + library_name = "brillo-protobuf" all_dependent_pkg_deps = [ "protobuf" ] - sources = [ - "brillo/proto_file_io.cc", - ] + sources = [ "brillo/proto_file_io.cc" ] }, ] if (use.udev) { libbrillo_sublibs += [ { - library_name = "brillo-udev-${libbase_ver}" + library_name = "brillo-udev" all_dependent_pkg_deps = [ "libudev" ] sources = [ "brillo/udev/udev.cc", @@ -238,7 +236,7 @@ foreach(attr, libbrillo_sublibs) { } } -generate_pkg_config("libbrillo_pc") { +generate_pkg_config("libbrillo-${libbase_ver}_pc") { name = "libbrillo" output_name = "libbrillo-${libbase_ver}" description = "brillo base library" @@ -256,20 +254,40 @@ generate_pkg_config("libbrillo_pc") { } } defines = [ "USE_RTTI_FOR_TYPE_TAGS" ] - libs = [ "-lbrillo-${libbase_ver}" ] + libs = [ "-lbrillo" ] +} + +generate_pkg_config("libbrillo_pc") { + name = "libbrillo" + output_name = "libbrillo" + description = "brillo base library" + version = libbase_ver + requires_private = default_pkg_deps + foreach(sublib, libbrillo_sublibs) { + if (defined(sublib.pkg_deps)) { + requires_private += sublib.pkg_deps + } + if (defined(sublib.public_pkg_deps)) { + requires_private += sublib.public_pkg_deps + } + if (defined(sublib.all_dependent_pkg_deps)) { + requires_private += sublib.all_dependent_pkg_deps + } + } + defines = [ "USE_RTTI_FOR_TYPE_TAGS" ] + libs = [ "-lbrillo" ] } -action("libbrillo-${libbase_ver}") { +action("libbrillo") { deps = [ + ":libbrillo-${libbase_ver}_pc", ":libbrillo_pc", ] foreach(sublib, libbrillo_sublibs) { deps += [ ":lib" + sublib.library_name ] } script = "//common-mk/write_args.py" - outputs = [ - "${root_out_dir}/lib/libbrillo-${libbase_ver}.so", - ] + outputs = [ "${root_out_dir}/lib/libbrillo.so" ] args = [ "--output" ] + outputs + [ "--" ] + [ "GROUP", "(", @@ -285,16 +303,42 @@ action("libbrillo-${libbase_ver}") { ] } -libbrillo_test_deps = [ "libbrillo-http-${libbase_ver}" ] +libbrillo_test_deps = [ "libbrillo-http" ] -generate_pkg_config("libbrillo-test_pc") { +generate_pkg_config("libbrillo-test-${libbase_ver}_pc") { name = "libbrillo-test" output_name = "libbrillo-test-${libbase_ver}" description = "brillo test library" version = libbase_ver # Because libbrillo-test is static, we have to depend directly on everything. - requires = [ "libbrillo-${libbase_ver}" ] + default_pkg_deps + requires = [ "libbrillo" ] + default_pkg_deps + foreach(name, libbrillo_test_deps) { + foreach(t, libbrillo_sublibs) { + if ("lib" + t.library_name == name) { + if (defined(t.pkg_deps)) { + requires += t.pkg_deps + } + if (defined(t.public_pkg_deps)) { + requires += t.public_pkg_deps + } + if (defined(t.all_dependent_pkg_deps)) { + requires += t.all_dependent_pkg_deps + } + } + } + } + libs = [ "-lbrillo-test" ] +} + +generate_pkg_config("libbrillo-test_pc") { + name = "libbrillo-test" + output_name = "libbrillo-test" + description = "brillo test library" + version = libbase_ver + + # Because libbrillo-test is static, we have to depend directly on everything. + requires = [ "libbrillo" ] + default_pkg_deps foreach(name, libbrillo_test_deps) { foreach(t, libbrillo_sublibs) { if ("lib" + t.library_name == name) { @@ -310,17 +354,18 @@ generate_pkg_config("libbrillo-test_pc") { } } } - libs = [ "-lbrillo-test-${libbase_ver}" ] + libs = [ "-lbrillo-test" ] } -static_library("libbrillo-test-${libbase_ver}") { +static_library("libbrillo-test") { configs -= [ "//common-mk:use_thin_archive" ] configs += [ "//common-mk:nouse_thin_archive", ":target_defaults", ] deps = [ - ":libbrillo-http-${libbase_ver}", + ":libbrillo-http", + ":libbrillo-test-${libbase_ver}_pc", ":libbrillo-test_pc", ] foreach(name, libbrillo_test_deps) { @@ -339,22 +384,20 @@ static_library("libbrillo-test-${libbase_ver}") { } } -shared_library("libinstallattributes-${libbase_ver}") { +shared_library("libinstallattributes") { configs += [ ":target_defaults" ] deps = [ ":libinstallattributes-includes", "../common-mk/external_dependencies:install_attributes-proto", ] all_dependent_pkg_deps = [ "protobuf-lite" ] - sources = [ - "install_attributes/libinstallattributes.cc", - ] + sources = [ "install_attributes/libinstallattributes.cc" ] } -shared_library("libpolicy-${libbase_ver}") { +shared_library("libpolicy") { configs += [ ":target_defaults" ] deps = [ - ":libinstallattributes-${libbase_ver}", + ":libinstallattributes", ":libpolicy-includes", "../common-mk/external_dependencies:policy-protos", ] @@ -383,19 +426,29 @@ if (use.dbus) { ] } -generate_pkg_config("libbrillo-glib_pc") { +generate_pkg_config("libbrillo-glib-${libbase_ver}_pc") { name = "libbrillo-glib" output_name = "libbrillo-glib-${libbase_ver}" description = "brillo glib wrapper library" version = libbase_ver requires_private = libbrillo_glib_pkg_deps - libs = [ "-lbrillo-glib-${libbase_ver}" ] + libs = [ "-lbrillo-glib" ] +} + +generate_pkg_config("libbrillo-glib_pc") { + name = "libbrillo-glib" + output_name = "libbrillo-glib" + description = "brillo glib wrapper library" + version = libbase_ver + requires_private = libbrillo_glib_pkg_deps + libs = [ "-lbrillo-glib" ] } -shared_library("libbrillo-glib-${libbase_ver}") { +shared_library("libbrillo-glib") { configs += [ ":target_defaults" ] deps = [ - ":libbrillo-${libbase_ver}", + ":libbrillo", + ":libbrillo-glib-${libbase_ver}_pc", ":libbrillo-glib_pc", ] all_dependent_pkg_deps = libbrillo_glib_pkg_deps @@ -412,35 +465,34 @@ shared_library("libbrillo-glib-${libbase_ver}") { } if (use.test) { - static_library("libbrillo-${libbase_ver}_static") { + static_library("libbrillo_static") { configs += [ ":target_defaults" ] deps = [ + ":libbrillo-${libbase_ver}_pc", ":libbrillo_pc", - ":libinstallattributes-${libbase_ver}", - ":libpolicy-${libbase_ver}", + ":libinstallattributes", + ":libpolicy", ] foreach(sublib, libbrillo_sublibs) { deps += [ ":lib" + sublib.library_name ] } - public_configs = [ ":libbrillo-${libbase_ver}_configs" ] + public_configs = [ ":libbrillo_configs" ] } - proto_library("libbrillo-${libbase_ver}_tests_proto") { + proto_library("libbrillo_tests_proto") { proto_in_dir = "brillo/dbus" proto_out_dir = "include/brillo/dbus" - sources = [ - "${proto_in_dir}/test.proto", - ] + sources = [ "${proto_in_dir}/test.proto" ] } - executable("libbrillo-${libbase_ver}_tests") { + executable("libbrillo_tests") { configs += [ "//common-mk:test", ":target_defaults", ] deps = [ - ":libbrillo-${libbase_ver}_static", - ":libbrillo-${libbase_ver}_tests_proto", - ":libbrillo-glib-${libbase_ver}", - ":libbrillo-test-${libbase_ver}", + ":libbrillo-glib", + ":libbrillo-test", + ":libbrillo_static", + ":libbrillo_tests_proto", ] pkg_deps = [ "libchrome-test-${libbase_ver}" ] cflags = [ "-Wno-format-zero-length" ] @@ -477,6 +529,7 @@ if (use.test) { "brillo/message_loops/fake_message_loop_test.cc", "brillo/message_loops/message_loop_test.cc", "brillo/mime_utils_test.cc", + "brillo/namespaces/mount_namespace_test.cc", "brillo/osrelease_reader_test.cc", "brillo/process_reaper_test.cc", "brillo/process_test.cc", @@ -491,6 +544,7 @@ if (use.test) { "brillo/streams/stream_test.cc", "brillo/streams/stream_utils_test.cc", "brillo/strings/string_utils_test.cc", + "brillo/timezone/tzif_parser_test.cc", "brillo/unittest_utils.cc", "brillo/url_utils_test.cc", "brillo/value_conversion_test.cc", @@ -519,29 +573,27 @@ if (use.test) { } } - executable("libinstallattributes-${libbase_ver}_tests") { + executable("libinstallattributes_tests") { configs += [ "//common-mk:test", ":target_defaults", ] deps = [ - ":libinstallattributes-${libbase_ver}", + ":libinstallattributes", "../common-mk/external_dependencies:install_attributes-proto", "../common-mk/testrunner:testrunner", ] - sources = [ - "install_attributes/tests/libinstallattributes_test.cc", - ] + sources = [ "install_attributes/tests/libinstallattributes_test.cc" ] } - executable("libpolicy-${libbase_ver}_tests") { + executable("libpolicy_tests") { configs += [ "//common-mk:test", ":target_defaults", ] deps = [ - ":libinstallattributes-${libbase_ver}", - ":libpolicy-${libbase_ver}", + ":libinstallattributes", + ":libpolicy", "../common-mk/external_dependencies:install_attributes-proto", "../common-mk/external_dependencies:policy-protos", "../common-mk/testrunner:testrunner", @@ -558,9 +610,7 @@ if (use.test) { if (use.fuzzer) { executable("libbrillo_data_encoding_fuzzer") { - sources = [ - "brillo/data_encoding_fuzzer.cc", - ] + sources = [ "brillo/data_encoding_fuzzer.cc" ] configs += [ "//common-mk/common_fuzzer:common_fuzzer" ] @@ -568,15 +618,11 @@ if (use.fuzzer) { include_dirs = [ "../libbrillo" ] - deps = [ - ":libbrillo-core-${libbase_ver}", - ] + deps = [ ":libbrillo-core" ] } executable("libbrillo_dbus_data_serialization_fuzzer") { - sources = [ - "brillo/dbus/data_serialization_fuzzer.cc", - ] + sources = [ "brillo/dbus/data_serialization_fuzzer.cc" ] configs += [ "//common-mk/common_fuzzer:common_fuzzer" ] @@ -584,15 +630,11 @@ if (use.fuzzer) { include_dirs = [ "../libbrillo" ] - deps = [ - ":libbrillo-core-${libbase_ver}", - ] + deps = [ ":libbrillo-core" ] } executable("libbrillo_http_form_data_fuzzer") { - sources = [ - "brillo/http/http_form_data_fuzzer.cc", - ] + sources = [ "brillo/http/http_form_data_fuzzer.cc" ] configs += [ "//common-mk/common_fuzzer:common_fuzzer" ] @@ -601,19 +643,16 @@ if (use.fuzzer) { include_dirs = [ "../libbrillo" ] deps = [ - ":libbrillo-http-${libbase_ver}", - ":libbrillo-streams-${libbase_ver}", + ":libbrillo-http", + ":libbrillo-streams", ] } } copy("libinstallattributes-includes") { - sources = [ - "install_attributes/libinstallattributes.h", - ] - outputs = [ - "${root_gen_dir}/include/install_attributes/{{source_file_part}}", - ] + sources = [ "install_attributes/libinstallattributes.h" ] + outputs = + [ "${root_gen_dir}/include/install_attributes/{{source_file_part}}" ] } copy("libpolicy-includes") { @@ -626,7 +665,5 @@ copy("libpolicy-includes") { "policy/policy_util.h", "policy/resilient_policy_util.h", ] - outputs = [ - "${root_gen_dir}/include/policy/{{source_file_part}}", - ] + outputs = [ "${root_gen_dir}/include/policy/{{source_file_part}}" ] } @@ -1,13 +1,9 @@ set noparent # Android owners -avakulenko@google.com -dpursell@google.com senj@google.com -stevefung@google.com # Chromium owners -benchan@google.com -derat@google.com vapier@google.com ejcaruso@google.com +hidehiko@google.com diff --git a/brillo/asynchronous_signal_handler.cc b/brillo/asynchronous_signal_handler.cc index b8ec529..38b1787 100644 --- a/brillo/asynchronous_signal_handler.cc +++ b/brillo/asynchronous_signal_handler.cc @@ -11,49 +11,41 @@ #include <base/bind.h> #include <base/files/file_util.h> #include <base/logging.h> -#include <base/message_loop/message_loop.h> -#include <base/posix/eintr_wrapper.h> - -namespace { -const int kInvalidDescriptor = -1; -} // namespace namespace brillo { -AsynchronousSignalHandler::AsynchronousSignalHandler() - : descriptor_(kInvalidDescriptor) { +AsynchronousSignalHandler::AsynchronousSignalHandler() { CHECK_EQ(sigemptyset(&signal_mask_), 0) << "Failed to initialize signal mask"; CHECK_EQ(sigemptyset(&saved_signal_mask_), 0) << "Failed to initialize signal mask"; } AsynchronousSignalHandler::~AsynchronousSignalHandler() { - if (descriptor_ != kInvalidDescriptor) { - MessageLoop::current()->CancelTask(fd_watcher_task_); + fd_watcher_ = nullptr; - if (IGNORE_EINTR(close(descriptor_)) != 0) - PLOG(WARNING) << "Failed to close file descriptor"; + if (!descriptor_.is_valid()) + return; - descriptor_ = kInvalidDescriptor; - CHECK_EQ(0, sigprocmask(SIG_SETMASK, &saved_signal_mask_, nullptr)); - } + // Close FD before restoring sigprocmask. + descriptor_.reset(); + CHECK_EQ(0, sigprocmask(SIG_SETMASK, &saved_signal_mask_, nullptr)); } void AsynchronousSignalHandler::Init() { - CHECK_EQ(kInvalidDescriptor, descriptor_); + // Making sure it is not yet initialized. + CHECK(!descriptor_.is_valid()); + + // Set sigprocmask before creating signalfd. CHECK_EQ(0, sigprocmask(SIG_BLOCK, &signal_mask_, &saved_signal_mask_)); - descriptor_ = - signalfd(descriptor_, &signal_mask_, SFD_CLOEXEC | SFD_NONBLOCK); - CHECK_NE(kInvalidDescriptor, descriptor_); - fd_watcher_task_ = MessageLoop::current()->WatchFileDescriptor( - FROM_HERE, - descriptor_, - MessageLoop::WatchMode::kWatchRead, - true, - base::Bind(&AsynchronousSignalHandler::OnFileCanReadWithoutBlocking, - base::Unretained(this))); - CHECK(fd_watcher_task_ != MessageLoop::kTaskIdNull) - << "Watching shutdown pipe failed."; + + // Creating signalfd, and start watching it. + descriptor_.reset(signalfd(-1, &signal_mask_, SFD_CLOEXEC | SFD_NONBLOCK)); + CHECK(descriptor_.is_valid()); + fd_watcher_ = base::FileDescriptorWatcher::WatchReadable( + descriptor_.get(), + base::BindRepeating(&AsynchronousSignalHandler::OnReadable, + base::Unretained(this))); + CHECK(fd_watcher_) << "Watching signalfd failed."; } void AsynchronousSignalHandler::RegisterHandler(int signal, @@ -65,15 +57,16 @@ void AsynchronousSignalHandler::RegisterHandler(int signal, void AsynchronousSignalHandler::UnregisterHandler(int signal) { Callbacks::iterator callback_it = registered_callbacks_.find(signal); - if (callback_it != registered_callbacks_.end()) { - registered_callbacks_.erase(callback_it); - ResetSignal(signal); - } + if (callback_it == registered_callbacks_.end()) + return; + registered_callbacks_.erase(callback_it); + CHECK_EQ(0, sigdelset(&signal_mask_, signal)); + UpdateSignals(); } -void AsynchronousSignalHandler::OnFileCanReadWithoutBlocking() { +void AsynchronousSignalHandler::OnReadable() { struct signalfd_siginfo info; - while (base::ReadFromFD(descriptor_, + while (base::ReadFromFD(descriptor_.get(), reinterpret_cast<char*>(&info), sizeof(info))) { int signal = info.ssi_signo; Callbacks::iterator callback_it = registered_callbacks_.find(signal); @@ -85,24 +78,29 @@ void AsynchronousSignalHandler::OnFileCanReadWithoutBlocking() { } const SignalHandler& callback = callback_it->second; bool must_unregister = callback.Run(info); - if (must_unregister) { + if (must_unregister) UnregisterHandler(signal); - } } } -void AsynchronousSignalHandler::ResetSignal(int signal) { - CHECK_EQ(0, sigdelset(&signal_mask_, signal)); - UpdateSignals(); -} - void AsynchronousSignalHandler::UpdateSignals() { - if (descriptor_ != kInvalidDescriptor) { - CHECK_EQ(0, sigprocmask(SIG_SETMASK, &saved_signal_mask_, nullptr)); - CHECK_EQ(0, sigprocmask(SIG_BLOCK, &signal_mask_, nullptr)); - CHECK_EQ(descriptor_, - signalfd(descriptor_, &signal_mask_, SFD_CLOEXEC | SFD_NONBLOCK)); + if (!descriptor_.is_valid()) + return; + sigset_t mask; +#ifdef __ANDROID__ + CHECK_EQ(0, sigemptyset(&mask)); + for (size_t i = 0; i < NSIG; ++i) { + if (sigismember(&signal_mask_, i) == 1 || sigismember(&saved_signal_mask_, i) == 1) { + CHECK_EQ(0, sigaddset(&mask, i)); + } } +#else + CHECK_EQ(0, sigorset(&mask, &signal_mask_, &saved_signal_mask_)); +#endif + CHECK_EQ(0, sigprocmask(SIG_SETMASK, &mask, nullptr)); + CHECK_EQ( + descriptor_.get(), + signalfd(descriptor_.get(), &signal_mask_, SFD_CLOEXEC | SFD_NONBLOCK)); } } // namespace brillo diff --git a/brillo/asynchronous_signal_handler.h b/brillo/asynchronous_signal_handler.h index ceae1ff..4b0edce 100644 --- a/brillo/asynchronous_signal_handler.h +++ b/brillo/asynchronous_signal_handler.h @@ -5,18 +5,16 @@ #ifndef LIBBRILLO_BRILLO_ASYNCHRONOUS_SIGNAL_HANDLER_H_ #define LIBBRILLO_BRILLO_ASYNCHRONOUS_SIGNAL_HANDLER_H_ -#include <signal.h> #include <sys/signalfd.h> #include <map> +#include <memory> #include <base/callback.h> -#include <base/compiler_specific.h> -#include <base/macros.h> -#include <base/message_loop/message_loop.h> +#include <base/files/file_descriptor_watcher_posix.h> +#include <base/files/scoped_file.h> #include <brillo/asynchronous_signal_handler_interface.h> #include <brillo/brillo_export.h> -#include <brillo/message_loops/message_loop.h> namespace brillo { // Sets up signal handlers for registered signals, and converts signal receipt @@ -25,10 +23,14 @@ namespace brillo { class BRILLO_EXPORT AsynchronousSignalHandler final : public AsynchronousSignalHandlerInterface { public: + using AsynchronousSignalHandlerInterface::SignalHandler; + AsynchronousSignalHandler(); ~AsynchronousSignalHandler() override; - using AsynchronousSignalHandlerInterface::SignalHandler; + AsynchronousSignalHandler(const AsynchronousSignalHandler&) = delete; + AsynchronousSignalHandler& + operator=(const AsynchronousSignalHandler&) = delete; // Initialize the handler. void Init(); @@ -40,17 +42,20 @@ class BRILLO_EXPORT AsynchronousSignalHandler final : private: // Called from the main loop when we can read from |descriptor_|, indicated // that a signal was processed. - void OnFileCanReadWithoutBlocking(); + void OnReadable(); - // Controller used to manage watching of signalling pipe. - MessageLoop::TaskId fd_watcher_task_{MessageLoop::kTaskIdNull}; + // Updates the set of signals that this handler listens to. + BRILLO_PRIVATE void UpdateSignals(); - // The registered callbacks. - typedef std::map<int, SignalHandler> Callbacks; + // Map from signal to its registered callback. + using Callbacks = std::map<int, SignalHandler>; Callbacks registered_callbacks_; // File descriptor for accepting signals indicated by |signal_mask_|. - int descriptor_; + base::ScopedFD descriptor_; + + // Controller used to manage watching of signalling pipe. + std::unique_ptr<base::FileDescriptorWatcher::Controller> fd_watcher_; // A set of signals to be handled after the dispatcher is running. sigset_t signal_mask_; @@ -58,15 +63,6 @@ class BRILLO_EXPORT AsynchronousSignalHandler final : // A copy of the signal mask before the dispatcher starts, which will be // used to restore to the original state when the dispatcher stops. sigset_t saved_signal_mask_; - - // Resets the given signal to its default behavior. Doesn't touch - // |registered_callbacks_|. - BRILLO_PRIVATE void ResetSignal(int signal); - - // Updates the set of signals that this handler listens to. - BRILLO_PRIVATE void UpdateSignals(); - - DISALLOW_COPY_AND_ASSIGN(AsynchronousSignalHandler); }; } // namespace brillo diff --git a/brillo/asynchronous_signal_handler_interface.h b/brillo/asynchronous_signal_handler_interface.h index ef0012d..7bae444 100644 --- a/brillo/asynchronous_signal_handler_interface.h +++ b/brillo/asynchronous_signal_handler_interface.h @@ -20,7 +20,8 @@ class BRILLO_EXPORT AsynchronousSignalHandlerInterface { virtual ~AsynchronousSignalHandlerInterface() = default; // The callback called when a signal is received. - using SignalHandler = base::Callback<bool(const struct signalfd_siginfo&)>; + using SignalHandler = + base::RepeatingCallback<bool(const struct signalfd_siginfo&)>; // Register a new handler for the given |signal|, replacing any previously // registered handler. |callback| will be called on the thread the diff --git a/brillo/binder_watcher.cc b/brillo/binder_watcher.cc index 9752204..51b0f59 100644 --- a/brillo/binder_watcher.cc +++ b/brillo/binder_watcher.cc @@ -33,24 +33,11 @@ void OnBinderReadReady() { namespace brillo { -BinderWatcher::BinderWatcher(MessageLoop* message_loop) - : message_loop_(message_loop) {} +BinderWatcher::BinderWatcher() = default; -BinderWatcher::BinderWatcher() : message_loop_(nullptr) {} - -BinderWatcher::~BinderWatcher() { - if (task_id_ != MessageLoop::kTaskIdNull) - message_loop_->CancelTask(task_id_); -} +BinderWatcher::~BinderWatcher() = default; bool BinderWatcher::Init() { - if (!message_loop_) - message_loop_ = MessageLoop::current(); - if (!message_loop_) { - LOG(ERROR) << "Must initialize a brillo::MessageLoop to use BinderWatcher"; - return false; - } - int binder_fd = -1; ProcessState::self()->setThreadPoolMaxThreadCount(0); IPCThreadState::self()->disableBackgroundScheduling(true); @@ -66,13 +53,10 @@ bool BinderWatcher::Init() { } VLOG(1) << "Got binder FD " << binder_fd; - task_id_ = message_loop_->WatchFileDescriptor( - FROM_HERE, + watcher_ = base::FileDescriptorWatcher::WatchReadable( binder_fd, - MessageLoop::kWatchRead, - true /* persistent */, - base::Bind(&OnBinderReadReady)); - if (task_id_ == MessageLoop::kTaskIdNull) { + base::BindRepeating(&OnBinderReadReady)); + if (!watcher_) { LOG(ERROR) << "Failed to watch binder FD"; return false; } diff --git a/brillo/binder_watcher.h b/brillo/binder_watcher.h index ece999d..d7af50e 100644 --- a/brillo/binder_watcher.h +++ b/brillo/binder_watcher.h @@ -17,8 +17,10 @@ #ifndef LIBBRILLO_BRILLO_BINDER_WATCHER_H_ #define LIBBRILLO_BRILLO_BINDER_WATCHER_H_ +#include <memory> + +#include <base/files/file_descriptor_watcher_posix.h> #include <base/macros.h> -#include <brillo/message_loops/message_loop.h> namespace brillo { @@ -26,9 +28,6 @@ namespace brillo { // make the message loop watch for binder events and pass them to libbinder. class BinderWatcher final { public: - // Construct the BinderWatcher using the passed |message_loop| if not null or - // the current MessageLoop otherwise. - explicit BinderWatcher(MessageLoop* message_loop); BinderWatcher(); ~BinderWatcher(); @@ -36,8 +35,7 @@ class BinderWatcher final { bool Init(); private: - MessageLoop::TaskId task_id_{MessageLoop::kTaskIdNull}; - MessageLoop* message_loop_; + std::unique_ptr<base::FileDescriptorWatcher::Controller> watcher_; DISALLOW_COPY_AND_ASSIGN(BinderWatcher); }; diff --git a/brillo/blkdev_utils/loop_device.cc b/brillo/blkdev_utils/loop_device.cc index bd1b67c..2b2219d 100644 --- a/brillo/blkdev_utils/loop_device.cc +++ b/brillo/blkdev_utils/loop_device.cc @@ -72,7 +72,8 @@ int GetDeviceNumber(const base::FilePath& sys_block_loopdev_path) { std::vector<std::string> device_ids = base::SplitString( device_string, ":", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY); - if (device_ids.size() != 2 || device_ids[0] != base::IntToString(LOOP_MAJOR)) + if (device_ids.size() != 2 || + device_ids[0] != base::NumberToString(LOOP_MAJOR)) return -1; base::StringToInt(device_ids[1], &device_number); diff --git a/brillo/cryptohome.cc b/brillo/cryptohome.cc index 88e4739..a82356e 100644 --- a/brillo/cryptohome.cc +++ b/brillo/cryptohome.cc @@ -41,7 +41,7 @@ static char g_system_salt_path[PATH_MAX] = "/home/.shadow/salt"; static std::string* salt = nullptr; -static bool EnsureSystemSaltIsLoaded() { +bool EnsureSystemSaltIsLoaded() { if (salt && !salt->empty()) return true; FilePath salt_path(g_system_salt_path); diff --git a/brillo/cryptohome.h b/brillo/cryptohome.h index 798d3a0..a9d5927 100644 --- a/brillo/cryptohome.h +++ b/brillo/cryptohome.h @@ -74,6 +74,9 @@ BRILLO_EXPORT void SetSystemSalt(std::string* salt); // Returns the system salt. BRILLO_EXPORT std::string* GetSystemSalt(); +// Ensures the system salt is loaded in the memory. +BRILLO_EXPORT bool EnsureSystemSaltIsLoaded(); + } // namespace home } // namespace cryptohome } // namespace brillo diff --git a/brillo/daemons/daemon.cc b/brillo/daemons/daemon.cc index b706017..82de826 100644 --- a/brillo/daemons/daemon.cc +++ b/brillo/daemons/daemon.cc @@ -4,6 +4,7 @@ #include <brillo/daemons/daemon.h> +#include <signal.h> #include <sysexits.h> #include <time.h> @@ -27,7 +28,7 @@ int Daemon::Run() { return exit_code; message_loop_.PostTask( - base::Bind(&Daemon::OnEventLoopStartedTask, base::Unretained(this))); + base::BindOnce(&Daemon::OnEventLoopStartedTask, base::Unretained(this))); message_loop_.Run(); OnShutdown(&exit_code_); diff --git a/brillo/dbus/data_serialization.cc b/brillo/dbus/data_serialization.cc index fa348a0..5f47d67 100644 --- a/brillo/dbus/data_serialization.cc +++ b/brillo/dbus/data_serialization.cc @@ -320,7 +320,6 @@ bool PopValueFromReader(dbus::MessageReader* reader, brillo::Any* value) { LOG(FATAL) << "Unknown D-Bus data type: " << variant_reader.GetDataType(); return false; } - return true; } } // namespace dbus_utils diff --git a/brillo/dbus/dbus_method_invoker_test.cc b/brillo/dbus/dbus_method_invoker_test.cc index 9e6600a..c0c681b 100644 --- a/brillo/dbus/dbus_method_invoker_test.cc +++ b/brillo/dbus/dbus_method_invoker_test.cc @@ -83,18 +83,16 @@ class DBusMethodInvokerTest : public testing::Test { GetObjectProxy(kTestServiceName, dbus::ObjectPath(kTestPath))) .WillRepeatedly(Return(mock_object_proxy_.get())); int def_timeout_ms = dbus::ObjectProxy::TIMEOUT_USE_DEFAULT; - EXPECT_CALL( - *mock_object_proxy_, - MIGRATE_MockCallMethodAndBlockWithErrorDetails(_, def_timeout_ms, _)) + EXPECT_CALL(*mock_object_proxy_, + CallMethodAndBlockWithErrorDetails(_, def_timeout_ms, _)) .WillRepeatedly(Invoke(this, &DBusMethodInvokerTest::CreateResponse)); } void TearDown() override { bus_ = nullptr; } - MIGRATE_WrapObjectProxyResponseType(Response) - CreateResponse(dbus::MethodCall* method_call, - int /* timeout_ms */, - dbus::ScopedDBusError* dbus_error) { + std::unique_ptr<Response> CreateResponse(dbus::MethodCall* method_call, + int /* timeout_ms */, + dbus::ScopedDBusError* dbus_error) { if (method_call->GetInterface() == kTestInterface) { if (method_call->GetMember() == kTestMethod1) { MessageReader reader(method_call); @@ -105,12 +103,12 @@ class DBusMethodInvokerTest : public testing::Test { auto response = Response::CreateEmpty(); MessageWriter writer(response.get()); writer.AppendString(std::to_string(v1 + v2)); - return MIGRATE_WrapObjectProxyResponseConversion(response); + return response; } } else if (method_call->GetMember() == kTestMethod2) { method_call->SetSerial(123); dbus_set_error(dbus_error->get(), "org.MyError", "My error message"); - return MIGRATE_WrapObjectProxyResponseEmpty; + return std::unique_ptr<dbus::Response>(); } else if (method_call->GetMember() == kTestMethod3) { MessageReader reader(method_call); dbus_utils_test::TestMessage msg; @@ -118,7 +116,7 @@ class DBusMethodInvokerTest : public testing::Test { auto response = Response::CreateEmpty(); MessageWriter writer(response.get()); AppendValueToWriter(&writer, msg); - return MIGRATE_WrapObjectProxyResponseConversion(response); + return response; } } else if (method_call->GetMember() == kTestMethod4) { method_call->SetSerial(123); @@ -128,13 +126,13 @@ class DBusMethodInvokerTest : public testing::Test { auto response = Response::CreateEmpty(); MessageWriter writer(response.get()); writer.AppendFileDescriptor(fd.get()); - return MIGRATE_WrapObjectProxyResponseConversion(response); + return response; } } } LOG(ERROR) << "Unexpected method call: " << method_call->ToString(); - return MIGRATE_WrapObjectProxyResponseEmpty; + return std::unique_ptr<dbus::Response>(); } std::string CallTestMethod(int v1, int v2) { @@ -245,7 +243,7 @@ class AsyncDBusMethodInvokerTest : public testing::Test { .WillRepeatedly(Return(mock_object_proxy_.get())); int def_timeout_ms = dbus::ObjectProxy::TIMEOUT_USE_DEFAULT; EXPECT_CALL(*mock_object_proxy_, - MIGRATE_CallMethodWithErrorCallback(_, def_timeout_ms, _, _)) + DoCallMethodWithErrorCallback(_, def_timeout_ms, _, _)) .WillRepeatedly(Invoke(this, &AsyncDBusMethodInvokerTest::HandleCall)); } @@ -253,10 +251,8 @@ class AsyncDBusMethodInvokerTest : public testing::Test { void HandleCall(dbus::MethodCall* method_call, int /* timeout_ms */, - dbus::ObjectProxy::ResponseCallback - MIGRATE_WrapObjectProxyCallback(success_callback), - dbus::ObjectProxy::ErrorCallback - MIGRATE_WrapObjectProxyCallback(error_callback)) { + dbus::ObjectProxy::ResponseCallback* success_callback, + dbus::ObjectProxy::ErrorCallback* error_callback) { if (method_call->GetInterface() == kTestInterface) { if (method_call->GetMember() == kTestMethod1) { MessageReader reader(method_call); @@ -267,16 +263,14 @@ class AsyncDBusMethodInvokerTest : public testing::Test { auto response = Response::CreateEmpty(); MessageWriter writer(response.get()); writer.AppendString(std::to_string(v1 + v2)); - std::move(MIGRATE_WrapObjectProxyCallback(success_callback)) - .Run(response.get()); + std::move(*success_callback).Run(response.get()); } return; } else if (method_call->GetMember() == kTestMethod2) { method_call->SetSerial(123); auto error_response = dbus::ErrorResponse::FromMethodCall( method_call, "org.MyError", "My error message"); - std::move(MIGRATE_WrapObjectProxyCallback(error_callback)) - .Run(error_response.get()); + std::move(*error_callback).Run(error_response.get()); return; } } diff --git a/brillo/dbus/dbus_object.cc b/brillo/dbus/dbus_object.cc index 12eb353..502af3e 100644 --- a/brillo/dbus/dbus_object.cc +++ b/brillo/dbus/dbus_object.cc @@ -9,6 +9,7 @@ #include <vector> #include <base/bind.h> +#include <base/bind_helpers.h> #include <base/logging.h> #include <brillo/dbus/async_event_sequencer.h> #include <brillo/dbus/exported_object_manager.h> @@ -41,8 +42,7 @@ DBusInterface::DBusInterface(DBusObject* dbus_object, const std::string& interface_name) : dbus_object_(dbus_object), interface_name_(interface_name), - // TODO(crbug.com/909719): Use base::DoNothing() - release_interface_cb_(base::Bind([]() {})) {} + release_interface_cb_(base::DoNothing()) {} void DBusInterface::AddProperty(const std::string& property_name, ExportedPropertyBase* prop_base) { diff --git a/brillo/dbus/dbus_property.h b/brillo/dbus/dbus_property.h index 77b7328..f82759e 100644 --- a/brillo/dbus/dbus_property.h +++ b/brillo/dbus/dbus_property.h @@ -5,6 +5,8 @@ #ifndef LIBBRILLO_BRILLO_DBUS_DBUS_PROPERTY_H_ #define LIBBRILLO_BRILLO_DBUS_DBUS_PROPERTY_H_ +#include <utility> + #include <brillo/dbus/data_serialization.h> #include <dbus/property.h> @@ -42,7 +44,7 @@ class Property : public ::dbus::PropertyBase { // remote object. void Set(const T& value, ::dbus::PropertySet::SetCallback callback) { set_value_ = value; - property_set()->Set(this, callback); + property_set()->Set(this, std::move(callback)); } // Synchronous version of Set(). diff --git a/brillo/dbus/dbus_signal_handler_test.cc b/brillo/dbus/dbus_signal_handler_test.cc index edd0eca..35bad65 100644 --- a/brillo/dbus/dbus_signal_handler_test.cc +++ b/brillo/dbus/dbus_signal_handler_test.cc @@ -50,7 +50,7 @@ class DBusSignalHandlerTest : public testing::Test { void CallSignal(SignalHandlerSink* sink, Args... args) { dbus::ObjectProxy::SignalCallback signal_callback; EXPECT_CALL(*mock_object_proxy_, - MIGRATE_ConnectToSignal(kInterface, kSignal, _, _)) + DoConnectToSignal(kInterface, kSignal, _, _)) .WillOnce(SaveArg<2>(&signal_callback)); brillo::dbus_utils::ConnectToSignal( @@ -71,8 +71,7 @@ class DBusSignalHandlerTest : public testing::Test { }; TEST_F(DBusSignalHandlerTest, ConnectToSignal) { - EXPECT_CALL(*mock_object_proxy_, - MIGRATE_ConnectToSignal(kInterface, kSignal, _, _)) + EXPECT_CALL(*mock_object_proxy_, DoConnectToSignal(kInterface, kSignal, _, _)) .Times(1); brillo::dbus_utils::ConnectToSignal( diff --git a/brillo/dbus/exported_object_manager.cc b/brillo/dbus/exported_object_manager.cc index 61dae68..a2ae1fe 100644 --- a/brillo/dbus/exported_object_manager.cc +++ b/brillo/dbus/exported_object_manager.cc @@ -89,11 +89,11 @@ ExportedObjectManager::HandleGetManagedObjects() { // DICT<STRING,VARIANT>>> ) bus_->AssertOnOriginThread(); ExportedObjectManager::ObjectMap objects; - for (const auto path_pair : registered_objects_) { + for (const auto& path_pair : registered_objects_) { std::map<std::string, VariantDictionary>& interfaces = objects[path_pair.first]; const InterfaceProperties& interface2properties = path_pair.second; - for (const auto interface : interface2properties) { + for (const auto& interface : interface2properties) { interface.second.Run(&interfaces[interface.first]); } } diff --git a/brillo/file_utils.cc b/brillo/file_utils.cc index b4370d1..3661551 100644 --- a/brillo/file_utils.cc +++ b/brillo/file_utils.cc @@ -11,11 +11,13 @@ #include <utility> #include <vector> +#include <base/files/file_enumerator.h> #include <base/files/file_path.h> #include <base/files/file_util.h> #include <base/logging.h> #include <base/posix/eintr_wrapper.h> #include <base/rand_util.h> +#include <base/stl_util.h> #include <base/strings/string_number_conversions.h> #include <base/strings/stringprintf.h> #include <base/time/time.h> @@ -140,7 +142,7 @@ bool TouchFileInternal(const base::FilePath& path, std::string GetRandomSuffix() { const int kBufferSize = 6; unsigned char buffer[kBufferSize]; - base::RandBytes(buffer, arraysize(buffer)); + base::RandBytes(buffer, base::size(buffer)); std::string suffix; for (int i = 0; i < kBufferSize; ++i) { int random_value = buffer[i] % (2 * 26 + 10); @@ -505,4 +507,20 @@ bool WriteToFileAtomic(const base::FilePath& path, return true; } +int64_t ComputeDirectoryDiskUsage(const base::FilePath& root_path) { + constexpr size_t S_BLKSIZE = 512; + int64_t running_blocks = 0; + base::FileEnumerator file_iter(root_path, true, + base::FileEnumerator::FILES | + base::FileEnumerator::DIRECTORIES | + base::FileEnumerator::SHOW_SYM_LINKS); + while (!file_iter.Next().empty()) { + // st_blocks in struct stat is the number of S_BLKSIZE (512) bytes sized + // blocks occupied by this file. + running_blocks += file_iter.GetInfo().stat().st_blocks; + } + // Each block is S_BLKSIZE (512) bytes so *S_BLKSIZE. + return running_blocks * S_BLKSIZE; +} + } // namespace brillo diff --git a/brillo/file_utils.h b/brillo/file_utils.h index 3862a43..f328165 100644 --- a/brillo/file_utils.h +++ b/brillo/file_utils.h @@ -144,6 +144,48 @@ BRILLO_EXPORT bool WriteBlobToFileAtomic(const base::FilePath& path, blob.size(), mode); } +// ComputeDirectoryDiskUsage() is similar to base::ComputeDirectorySize() in +// libbase, but it returns the actual disk usage instead of the apparent size. +// In another word, ComputeDirectoryDiskUsage() behaves like "du -s +// --apparent-size", and ComputeDirectorySize() behaves like "du -s". The +// primary difference is that sparse file and files on filesystem with +// transparent compression will report smaller file size than +// ComputeDirectorySize(). Returns the total used bytes. +// The following behaviours of this function is guaranteed and is verified by +// unit tests: +// - This function recursively processes directory down the tree, so disk space +// used by files in all the subdirectories are counted. +// - Symbolic links will not be followed (the size of link itself is counted, +// the target is not) +// - Hidden files are counted as well. +// The following behaviours are not guaranteed, and it is recommended to avoid +// them in the field. Their current behaviour is provided for reference only: +// - This function doesn't care about filesystem boundaries, so it'll cross +// filesystem boundary to count file size if there's one in the specified +// directory. +// - Hard links will be treated like normal files, so they could be +// over-reported. +// - Directories that the current user doesn't have permission to list/stat will +// be ignored, and an error will be logged but the returned result could be +// under-reported without error in the returned value. +// - Deduplication (should the filesystem support it) is ignored, and the result +// could be over-reported. +// - Doesn't check if |root_path| exists, a non-existent directory will results +// in 0 bytes without any error. +// - There are no limit on the depth of file system tree, the program will crash +// if it run out of memory to hold the entire depth of file system tree. +// - If the directory is modified during this function call, there's no +// guarantee on if the function will count the updated or original file system +// state. The function could choose to count the updated state for one file and +// original state for another file. +// - Non-POSIX system is not supported. +// - Disk space used by directory (and its subdirectories) itself is counted. +// +// Parameters +// root_path - The directory to compute the size for +BRILLO_EXPORT int64_t +ComputeDirectoryDiskUsage(const base::FilePath& root_path); + } // namespace brillo #endif // LIBBRILLO_BRILLO_FILE_UTILS_H_ diff --git a/brillo/file_utils_test.cc b/brillo/file_utils_test.cc index 9a1f646..3407cd1 100644 --- a/brillo/file_utils_test.cc +++ b/brillo/file_utils_test.cc @@ -13,6 +13,7 @@ #include <base/files/file_util.h> #include <base/files/scoped_temp_dir.h> #include <base/rand_util.h> +#include <base/stl_util.h> #include <base/strings/string_number_conversions.h> #include <gtest/gtest.h> @@ -29,8 +30,8 @@ constexpr int kPermissions755 = S_IRWXU | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH; std::string GetRandomSuffix() { const int kBufferSize = 6; unsigned char buffer[kBufferSize]; - base::RandBytes(buffer, arraysize(buffer)); - return base::HexEncode(buffer, arraysize(buffer)); + base::RandBytes(buffer, base::size(buffer)); + return base::HexEncode(buffer, base::size(buffer)); } bool IsNonBlockingFD(int fd) { @@ -343,4 +344,165 @@ TEST_F(FileUtilsTest, umask(old_mask); } +TEST_F(FileUtilsTest, ComputeDirectoryDiskUsageNormalRandomFile) { + // 2MB test file. + constexpr size_t kFileSize = 2 * 1024 * 1024; + + const base::FilePath dirname(GetTempName()); + EXPECT_TRUE(base::CreateDirectory(dirname)); + const base::FilePath filename = dirname.Append("test.temp"); + + std::string file_content = base::RandBytesAsString(kFileSize); + EXPECT_TRUE(WriteStringToFile(filename, file_content)); + + int64_t result_usage = ComputeDirectoryDiskUsage(dirname); + int64_t result_size = base::ComputeDirectorySize(dirname); + + // result_usage (what we are testing here) should be within +/-10% of ground + // truth. The variation is to account for filesystem overhead variations. + EXPECT_GT(result_usage, kFileSize / 10 * 9); + EXPECT_LT(result_usage, kFileSize / 10 * 11); + + // result_usage should be close to result_size, because the test file is + // random so it's disk usage should be similar to apparent size. + EXPECT_GT(result_usage, result_size / 10 * 9); + EXPECT_LT(result_usage, result_size / 10 * 11); +} + +TEST_F(FileUtilsTest, ComputeDirectoryDiskUsageDeepRandomFile) { + // 2MB test file. + constexpr size_t kFileSize = 2 * 1024 * 1024; + + const base::FilePath dirname(GetTempName()); + EXPECT_TRUE(base::CreateDirectory(dirname)); + base::FilePath currentlevel = dirname; + for (int i = 0; i < 10; i++) { + base::FilePath nextlevel = currentlevel.Append("test.dir"); + EXPECT_TRUE(base::CreateDirectory(nextlevel)); + currentlevel = nextlevel; + } + const base::FilePath filename = currentlevel.Append("test.temp"); + + std::string file_content = base::RandBytesAsString(kFileSize); + EXPECT_TRUE(WriteStringToFile(filename, file_content)); + + int64_t result_usage = ComputeDirectoryDiskUsage(dirname); + int64_t result_size = base::ComputeDirectorySize(dirname); + + // result_usage (what we are testing here) should be within +/-10% of ground + // truth. The variation is to account for filesystem overhead variations. + EXPECT_GT(result_usage, kFileSize / 10 * 9); + EXPECT_LT(result_usage, kFileSize / 10 * 11); + + // result_usage should be close to result_size, because the test file is + // random so it's disk usage should be similar to apparent size. + EXPECT_GT(result_usage, result_size / 10 * 9); + EXPECT_LT(result_usage, result_size / 10 * 11); +} + +TEST_F(FileUtilsTest, ComputeDirectoryDiskUsageHiddenRandomFile) { + // 2MB test file. + constexpr size_t kFileSize = 2 * 1024 * 1024; + + const base::FilePath dirname(GetTempName()); + EXPECT_TRUE(base::CreateDirectory(dirname)); + // File name starts with a dot, so it's a hidden file. + const base::FilePath filename = dirname.Append(".test.temp"); + + std::string file_content = base::RandBytesAsString(kFileSize); + EXPECT_TRUE(WriteStringToFile(filename, file_content)); + + int64_t result_usage = ComputeDirectoryDiskUsage(dirname); + int64_t result_size = base::ComputeDirectorySize(dirname); + + // result_usage (what we are testing here) should be within +/-10% of ground + // truth. The variation is to account for filesystem overhead variations. + EXPECT_GT(result_usage, kFileSize / 10 * 9); + EXPECT_LT(result_usage, kFileSize / 10 * 11); + + // result_usage should be close to result_size, because the test file is + // random so it's disk usage should be similar to apparent size. + EXPECT_GT(result_usage, result_size / 10 * 9); + EXPECT_LT(result_usage, result_size / 10 * 11); +} + +TEST_F(FileUtilsTest, ComputeDirectoryDiskUsageSparseFile) { + // 128MB sparse test file. + constexpr size_t kFileSize = 128 * 1024 * 1024; + constexpr size_t kFileSizeThreshold = 64 * 1024; + + const base::FilePath dirname(GetTempName()); + EXPECT_TRUE(base::CreateDirectory(dirname)); + const base::FilePath filename = dirname.Append("test.temp"); + + int fd = + open(filename.value().c_str(), O_CREAT | O_WRONLY, S_IRUSR | S_IWUSR); + EXPECT_NE(fd, -1); + // Calling ftruncate on an empty file will create a sparse file. + EXPECT_EQ(0, ftruncate(fd, kFileSize)); + + int64_t result_usage = ComputeDirectoryDiskUsage(dirname); + int64_t result_size = base::ComputeDirectorySize(dirname); + + // result_usage (what we are testing here) should be less than + // kFileSizeThreshold, the threshold is to account for filesystem overhead + // variations. + EXPECT_LT(result_usage, kFileSizeThreshold); + + // Since we are dealing with sparse files here, the apparent size should be + // much much larger than the actual disk usage. + EXPECT_LT(result_usage, result_size / 1000); +} + +TEST_F(FileUtilsTest, ComputeDirectoryDiskUsageSymlinkFile) { + // 2MB test file. + constexpr size_t kFileSize = 2 * 1024 * 1024; + + const base::FilePath dirname(GetTempName()); + EXPECT_TRUE(base::CreateDirectory(dirname)); + const base::FilePath filename = dirname.Append("test.temp"); + const base::FilePath linkname = dirname.Append("test.link"); + + std::string file_content = base::RandBytesAsString(kFileSize); + EXPECT_TRUE(WriteStringToFile(filename, file_content)); + + // Create a symlink. + EXPECT_TRUE(base::CreateSymbolicLink(filename, linkname)); + + int64_t result_usage = ComputeDirectoryDiskUsage(dirname); + + // result_usage (what we are testing here) should be within +/-10% of ground + // truth. The variation is to account for filesystem overhead variations. + // Note that it's not 2x kFileSize because symblink is not counted twice. + EXPECT_GT(result_usage, kFileSize / 10 * 9); + EXPECT_LT(result_usage, kFileSize / 10 * 11); +} + +TEST_F(FileUtilsTest, ComputeDirectoryDiskUsageSymlinkDir) { + // 2MB test file. + constexpr size_t kFileSize = 2 * 1024 * 1024; + + const base::FilePath parentname(GetTempName()); + EXPECT_TRUE(base::CreateDirectory(parentname)); + const base::FilePath dirname = parentname.Append("target.dir"); + EXPECT_TRUE(base::CreateDirectory(dirname)); + const base::FilePath linkname = parentname.Append("link.dir"); + + const base::FilePath filename = dirname.Append("test.temp"); + + std::string file_content = base::RandBytesAsString(kFileSize); + EXPECT_TRUE(WriteStringToFile(filename, file_content)); + + // Create a symlink. + EXPECT_TRUE(base::CreateSymbolicLink(dirname, linkname)); + + int64_t result_usage = ComputeDirectoryDiskUsage(dirname); + + // result_usage (what we are testing here) should be within +/-10% of ground + // truth. The variation is to account for filesystem overhead variations. + // Note that it's not 2x kFileSize because symblink is not counted twice. + EXPECT_GT(result_usage, kFileSize / 10 * 9); + EXPECT_LT(result_usage, kFileSize / 10 * 11); +} + } // namespace brillo diff --git a/brillo/files/file_util_test.cc b/brillo/files/file_util_test.cc index 98d28fc..f1ba527 100644 --- a/brillo/files/file_util_test.cc +++ b/brillo/files/file_util_test.cc @@ -6,6 +6,7 @@ #include <base/files/file_util.h> #include <base/rand_util.h> +#include <base/stl_util.h> #include <base/strings/string_number_conversions.h> #include <brillo/files/file_util.h> #include <brillo/files/safe_fd.h> @@ -42,8 +43,8 @@ std::ostream& operator<<(std::ostream& os, const brillo::SafeFD::Error err) { std::string GetRandomSuffix() { const int kBufferSize = 6; unsigned char buffer[kBufferSize]; - base::RandBytes(buffer, arraysize(buffer)); - return base::HexEncode(buffer, arraysize(buffer)); + base::RandBytes(buffer, base::size(buffer)); + return base::HexEncode(buffer, base::size(buffer)); } void FileTest::SetUpTestCase() { diff --git a/brillo/files/safe_fd.cc b/brillo/files/safe_fd.cc index 855207d..ac19dc3 100644 --- a/brillo/files/safe_fd.cc +++ b/brillo/files/safe_fd.cc @@ -446,7 +446,8 @@ SafeFD::Error SafeFD::Unlink(const std::string& name) { SafeFD::Error SafeFD::Rmdir(const std::string& name, bool recursive, - size_t max_depth) { + size_t max_depth, + bool keep_going) { if (!fd_.is_valid()) { return SafeFD::Error::kNotInitialized; } @@ -460,6 +461,8 @@ SafeFD::Error SafeFD::Rmdir(const std::string& name, return err; } + SafeFD::Error last_err = SafeFD::Error::kNoError; + if (recursive) { SafeFD dir_fd; std::tie(dir_fd, err) = @@ -490,32 +493,36 @@ SafeFD::Error SafeFD::Rmdir(const std::string& name, errno = 0; const dirent* entry = HANDLE_EINTR_IF_EQ(readdir(dir.get()), nullptr); while (entry != nullptr) { - if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0) { - goto continue_; - } + SafeFD::Error err = [&]() { + if (strcmp(entry->d_name, ".") == 0 || + strcmp(entry->d_name, "..") == 0) { + return SafeFD::Error::kNoError; + } - struct stat child_info; - if (fstatat(dir_fd.get(), entry->d_name, &child_info, - AT_NO_AUTOMOUNT | AT_SYMLINK_NOFOLLOW) != 0) { - return SafeFD::Error::kIOError; - } + struct stat child_info; + if (fstatat(dir_fd.get(), entry->d_name, &child_info, + AT_NO_AUTOMOUNT | AT_SYMLINK_NOFOLLOW) != 0) { + return SafeFD::Error::kIOError; + } - if (child_info.st_dev != dir_info.st_dev) { - return SafeFD::Error::kBoundaryDetected; - } + if (child_info.st_dev != dir_info.st_dev) { + return SafeFD::Error::kBoundaryDetected; + } - SafeFD::Error err; - if (entry->d_type == DT_DIR) { - err = dir_fd.Rmdir(entry->d_name, true, max_depth - 1); - } else { - err = dir_fd.Unlink(entry->d_name); - } + if (entry->d_type != DT_DIR) { + return dir_fd.Unlink(entry->d_name); + } + + return dir_fd.Rmdir(entry->d_name, true, max_depth - 1, keep_going); + }(); if (IsError(err)) { - return err; + if (!keep_going) { + return err; + } + last_err = err; } - continue_: errno = 0; entry = HANDLE_EINTR_IF_EQ(readdir(dir.get()), nullptr); } @@ -530,9 +537,16 @@ SafeFD::Error SafeFD::Rmdir(const std::string& name, if (errno == ENOTDIR) { return SafeFD::Error::kWrongType; } + // If there was an error during the recursive delete, we expect unlink + // to fail with ENOTEMPTY and we bubble the error from recursion + // instead. + if (IsError(last_err) && errno == ENOTEMPTY) { + return last_err; + } return SafeFD::Error::kIOError; } - return SafeFD::Error::kNoError; + + return last_err; } } // namespace brillo diff --git a/brillo/files/safe_fd.h b/brillo/files/safe_fd.h index 5e126b4..3c77362 100644 --- a/brillo/files/safe_fd.h +++ b/brillo/files/safe_fd.h @@ -183,10 +183,13 @@ class SafeFD { // recursive - if true also unlink child paths. // max_depth - limit on recursion depth to prevent fd exhaustion and stack // overflows. + // keep_going - in recursive case continue deleting even in the face of + // errors. If all entries cannot be deleted, the last error encountered + // during recursion is returned. BRILLO_EXPORT Error Rmdir(const std::string& name, bool recursive = false, - size_t max_depth = kDefaultMaxPathDepth) - WARN_UNUSED_RESULT; + size_t max_depth = kDefaultMaxPathDepth, + bool keep_going = true) WARN_UNUSED_RESULT; private: BRILLO_EXPORT static const char* RootPath; diff --git a/brillo/files/safe_fd_test.cc b/brillo/files/safe_fd_test.cc index fff3a6c..6401f3d 100644 --- a/brillo/files/safe_fd_test.cc +++ b/brillo/files/safe_fd_test.cc @@ -624,4 +624,75 @@ TEST_F(SafeFDTest, Rmdir_WrongType) { EXPECT_EQ(subdir.first.Rmdir(kFileName), SafeFD::Error::kWrongType); } +TEST_F(SafeFDTest, Rmdir_Recursive_KeepGoing) { + ASSERT_TRUE(SetupSubdir()); + + ASSERT_TRUE(base::CreateDirectory(sub_dir_path_.Append(kSubdirName))); + + // Give us something to iterate over. + constexpr int kNumSentinel = 25; + for (int i = 0; i < kNumSentinel; i++) { + SafeFD::SafeFDResult file = + root_.MakeFile(sub_dir_path_.Append(GetRandomSuffix())); + EXPECT_EQ(file.second, SafeFD::Error::kNoError); + ASSERT_TRUE(file.first.is_valid()); + } + + // Recursively delete with a max level that is too small. Capture errno. + SafeFD::Error result = root_.Rmdir(kSubdirName, true /*recursive*/, + 1 /*max_depth*/, true /*keep_going*/); + int rmdir_errno = errno; + + EXPECT_EQ(result, SafeFD::Error::kExceededMaximum); + + // If we keep going, the last operation will be the post-order unlink of + // the top-level directory. This has to fail with ENOTEMPTY since we did + // not delete the too-deep sub-directories. This particular behavior + // should not be part of the API contract and this can be relaxed if the + // implementation is changed. + EXPECT_EQ(rmdir_errno, ENOTEMPTY); + + // The deep directory must still exist. + ASSERT_TRUE( + base::DeleteFile(sub_dir_path_.Append(kSubdirName), false /*recursive*/)); + + // We cannot control the iteration order so even if we incorrectly + // stopped early the directory might still be empty if the deep + // directories were last in the iteration order. But a non-empty + // directory is always incorrect. + ASSERT_TRUE(base::IsDirectoryEmpty(sub_dir_path_)); +} + +TEST_F(SafeFDTest, Rmdir_Recursive_StopOnError) { + ASSERT_TRUE(SetupSubdir()); + + ASSERT_TRUE(base::CreateDirectory(sub_dir_path_.Append(kSubdirName))); + + // Give us something to iterate over. + constexpr int kNumSentinel = 25; + for (int i = 0; i < kNumSentinel; i++) { + SafeFD::SafeFDResult file = + root_.MakeFile(sub_dir_path_.Append(GetRandomSuffix())); + EXPECT_EQ(file.second, SafeFD::Error::kNoError); + ASSERT_TRUE(file.first.is_valid()); + } + + // Recursively delete with a max level that is too small. Capture errno. + SafeFD::Error result = root_.Rmdir(kSubdirName, true /*recursive*/, + 1 /*max_depth*/, false /*keep_going*/); + int rmdir_errno = errno; + + EXPECT_EQ(result, SafeFD::Error::kExceededMaximum); + + // If we stop on encountering a too-deep directory, we never actually + // make any libc calls that encounter errors. This particular behavior + // should not be part of the API contract and this can be relaxed if the + // implementation is changed. + EXPECT_EQ(rmdir_errno, 0); + + // The deep directory must still exist. + ASSERT_TRUE( + base::DeleteFile(sub_dir_path_.Append(kSubdirName), false /*recursive*/)); +} + } // namespace brillo diff --git a/brillo/flag_helper.cc b/brillo/flag_helper.cc index 065a1c7..1c332d3 100644 --- a/brillo/flag_helper.cc +++ b/brillo/flag_helper.cc @@ -86,6 +86,22 @@ const char* Int32Flag::GetType() const { return "int"; } +UInt32Flag::UInt32Flag(const char* name, + uint32_t* value, + const char* default_value, + const char* help, + bool visible) + : Flag(name, default_value, help, visible), value_(value) { +} + +bool UInt32Flag::SetValue(const std::string& value) { + return base::StringToUint(value, value_); +} + +const char* UInt32Flag::GetType() const { + return "uint32"; +} + Int64Flag::Int64Flag(const char* name, int64_t* value, const char* default_value, diff --git a/brillo/flag_helper.h b/brillo/flag_helper.h index 810a00c..c6d63cd 100644 --- a/brillo/flag_helper.h +++ b/brillo/flag_helper.h @@ -20,6 +20,7 @@ // // DEFINE_bool(name, default_value, help) // DEFINE_int32(name, default_value, help) +// DEFINE_uint32(name, default_value, help) // DEFINE_int64(name, default_value, help) // DEFINE_uint64(name, default_value, help) // DEFINE_double(name, default_value, help) @@ -118,6 +119,21 @@ class BRILLO_EXPORT Int32Flag final : public Flag { int* value_; }; +class BRILLO_EXPORT UInt32Flag final : public Flag { + public: + UInt32Flag(const char* name, + uint32_t* value, + const char* default_value, + const char* help, + bool visible); + bool SetValue(const std::string& value) override; + + const char* GetType() const override; + + private: + uint32_t* value_; +}; + class BRILLO_EXPORT Int64Flag final : public Flag { public: Int64Flag(const char* name, @@ -191,6 +207,8 @@ class BRILLO_EXPORT StringFlag final : public Flag { #define DEFINE_int32(name, value, help) \ DEFINE_type(int, Int32Flag, name, value, help) +#define DEFINE_uint32(name, value, help) \ + DEFINE_type(uint32_t, UInt32Flag, name, value, help) #define DEFINE_int64(name, value, help) \ DEFINE_type(int64_t, Int64Flag, name, value, help) #define DEFINE_uint64(name, value, help) \ diff --git a/brillo/flag_helper_test.cc b/brillo/flag_helper_test.cc index 29c6429..7c7164d 100644 --- a/brillo/flag_helper_test.cc +++ b/brillo/flag_helper_test.cc @@ -8,6 +8,7 @@ #include <base/command_line.h> #include <base/macros.h> +#include <base/stl_util.h> #include <brillo/flag_helper.h> #include <gtest/gtest.h> @@ -29,6 +30,8 @@ TEST_F(FlagHelperTest, Defaults) { DEFINE_int32(int32_1, INT32_MIN, "Test int32 flag"); DEFINE_int32(int32_2, 0, "Test int32 flag"); DEFINE_int32(int32_3, INT32_MAX, "Test int32 flag"); + DEFINE_uint32(uint32_1, 0, "Test uint32 flag"); + DEFINE_uint32(uint32_2, UINT32_MAX, "Test uint32 flag"); DEFINE_int64(int64_1, INT64_MIN, "Test int64 flag"); DEFINE_int64(int64_2, 0, "Test int64 flag"); DEFINE_int64(int64_3, INT64_MAX, "Test int64 flag"); @@ -41,17 +44,19 @@ TEST_F(FlagHelperTest, Defaults) { DEFINE_string(string_2, "value", "Test string flag"); const char* argv[] = {"test_program"}; - base::CommandLine command_line(arraysize(argv), argv); + base::CommandLine command_line(base::size(argv), argv); brillo::FlagHelper::GetInstance()->set_command_line_for_testing( &command_line); - brillo::FlagHelper::Init(arraysize(argv), argv, "TestDefaultTrue"); + brillo::FlagHelper::Init(base::size(argv), argv, "TestDefaultTrue"); EXPECT_TRUE(FLAGS_bool1); EXPECT_FALSE(FLAGS_bool2); EXPECT_EQ(FLAGS_int32_1, INT32_MIN); EXPECT_EQ(FLAGS_int32_2, 0); EXPECT_EQ(FLAGS_int32_3, INT32_MAX); + EXPECT_EQ(FLAGS_uint32_1, 0); + EXPECT_EQ(FLAGS_uint32_2, UINT32_MAX); EXPECT_EQ(FLAGS_int64_1, INT64_MIN); EXPECT_EQ(FLAGS_int64_2, 0); EXPECT_EQ(FLAGS_int64_3, INT64_MAX); @@ -74,6 +79,8 @@ TEST_F(FlagHelperTest, SetValueDoubleDash) { DEFINE_int32(int32_1, 1, "Test int32 flag"); DEFINE_int32(int32_2, 1, "Test int32 flag"); DEFINE_int32(int32_3, 1, "Test int32 flag"); + DEFINE_uint32(uint32_1, 1, "Test uint32 flag"); + DEFINE_uint32(uint32_2, 1, "Test uint32 flag"); DEFINE_int64(int64_1, 1, "Test int64 flag"); DEFINE_int64(int64_2, 1, "Test int64 flag"); DEFINE_int64(int64_3, 1, "Test int64 flag"); @@ -93,6 +100,8 @@ TEST_F(FlagHelperTest, SetValueDoubleDash) { "--int32_1=-2147483648", "--int32_2=0", "--int32_3=2147483647", + "--uint32_1=0", + "--uint32_2=4294967295", "--int64_1=-9223372036854775808", "--int64_2=0", "--int64_3=9223372036854775807", @@ -103,11 +112,11 @@ TEST_F(FlagHelperTest, SetValueDoubleDash) { "--double_3=100.5", "--string_1=", "--string_2=value"}; - base::CommandLine command_line(arraysize(argv), argv); + base::CommandLine command_line(base::size(argv), argv); brillo::FlagHelper::GetInstance()->set_command_line_for_testing( &command_line); - brillo::FlagHelper::Init(arraysize(argv), argv, "TestDefaultTrue"); + brillo::FlagHelper::Init(base::size(argv), argv, "TestDefaultTrue"); EXPECT_TRUE(FLAGS_bool1); EXPECT_FALSE(FLAGS_bool2); @@ -116,6 +125,8 @@ TEST_F(FlagHelperTest, SetValueDoubleDash) { EXPECT_EQ(FLAGS_int32_1, INT32_MIN); EXPECT_EQ(FLAGS_int32_2, 0); EXPECT_EQ(FLAGS_int32_3, INT32_MAX); + EXPECT_EQ(FLAGS_uint32_1, 0); + EXPECT_EQ(FLAGS_uint32_2, UINT32_MAX); EXPECT_EQ(FLAGS_int64_1, INT64_MIN); EXPECT_EQ(FLAGS_int64_2, 0); EXPECT_EQ(FLAGS_int64_3, INT64_MAX); @@ -136,6 +147,8 @@ TEST_F(FlagHelperTest, SetValueSingleDash) { DEFINE_int32(int32_1, 1, "Test int32 flag"); DEFINE_int32(int32_2, 1, "Test int32 flag"); DEFINE_int32(int32_3, 1, "Test int32 flag"); + DEFINE_uint64(uint32_1, 1, "Test uint32 flag"); + DEFINE_uint64(uint32_2, 1, "Test uint32 flag"); DEFINE_int64(int64_1, 1, "Test int64 flag"); DEFINE_int64(int64_2, 1, "Test int64 flag"); DEFINE_int64(int64_3, 1, "Test int64 flag"); @@ -153,6 +166,8 @@ TEST_F(FlagHelperTest, SetValueSingleDash) { "-int32_1=-2147483648", "-int32_2=0", "-int32_3=2147483647", + "-uint32_1=0", + "-uint32_2=4294967295", "-int64_1=-9223372036854775808", "-int64_2=0", "-int64_3=9223372036854775807", @@ -163,17 +178,19 @@ TEST_F(FlagHelperTest, SetValueSingleDash) { "-double_3=100.5", "-string_1=", "-string_2=value"}; - base::CommandLine command_line(arraysize(argv), argv); + base::CommandLine command_line(base::size(argv), argv); brillo::FlagHelper::GetInstance()->set_command_line_for_testing( &command_line); - brillo::FlagHelper::Init(arraysize(argv), argv, "TestDefaultTrue"); + brillo::FlagHelper::Init(base::size(argv), argv, "TestDefaultTrue"); EXPECT_TRUE(FLAGS_bool1); EXPECT_FALSE(FLAGS_bool2); EXPECT_EQ(FLAGS_int32_1, INT32_MIN); EXPECT_EQ(FLAGS_int32_2, 0); EXPECT_EQ(FLAGS_int32_3, INT32_MAX); + EXPECT_EQ(FLAGS_uint32_1, 0); + EXPECT_EQ(FLAGS_uint32_2, UINT32_MAX); EXPECT_EQ(FLAGS_int64_1, INT64_MIN); EXPECT_EQ(FLAGS_int64_2, 0); EXPECT_EQ(FLAGS_int64_3, INT64_MAX); @@ -192,11 +209,11 @@ TEST_F(FlagHelperTest, DuplicateSetValue) { DEFINE_int32(int32_1, 0, "Test in32 flag"); const char* argv[] = {"test_program", "--int32_1=5", "--int32_1=10"}; - base::CommandLine command_line(arraysize(argv), argv); + base::CommandLine command_line(base::size(argv), argv); brillo::FlagHelper::GetInstance()->set_command_line_for_testing( &command_line); - brillo::FlagHelper::Init(arraysize(argv), argv, "TestDuplicateSetvalue"); + brillo::FlagHelper::Init(base::size(argv), argv, "TestDuplicateSetvalue"); EXPECT_EQ(FLAGS_int32_1, 10); } @@ -206,11 +223,11 @@ TEST_F(FlagHelperTest, FlagTerminator) { DEFINE_int32(int32_1, 0, "Test int32 flag"); const char* argv[] = {"test_program", "--int32_1=5", "--", "--int32_1=10"}; - base::CommandLine command_line(arraysize(argv), argv); + base::CommandLine command_line(base::size(argv), argv); brillo::FlagHelper::GetInstance()->set_command_line_for_testing( &command_line); - brillo::FlagHelper::Init(arraysize(argv), argv, "TestFlagTerminator"); + brillo::FlagHelper::Init(base::size(argv), argv, "TestFlagTerminator"); EXPECT_EQ(FLAGS_int32_1, 5); } @@ -220,13 +237,14 @@ TEST_F(FlagHelperTest, FlagTerminator) { TEST_F(FlagHelperTest, HelpMessage) { DEFINE_bool(bool_1, true, "Test bool flag"); DEFINE_int32(int_1, 0, "Test int flag"); + DEFINE_uint32(uint32_1, 0, "Test uint32 flag"); DEFINE_int64(int64_1, 0, "Test int64 flag"); DEFINE_uint64(uint64_1, 0, "Test uint64 flag"); DEFINE_double(double_1, 0, "Test double flag"); DEFINE_string(string_1, "", "Test string flag"); const char* argv[] = {"test_program", "--int_1=value", "--help"}; - base::CommandLine command_line(arraysize(argv), argv); + base::CommandLine command_line(base::size(argv), argv); brillo::FlagHelper::GetInstance()->set_command_line_for_testing( &command_line); @@ -235,7 +253,7 @@ TEST_F(FlagHelperTest, HelpMessage) { stdout = stderr; ASSERT_EXIT( - brillo::FlagHelper::Init(arraysize(argv), argv, "TestHelpMessage"), + brillo::FlagHelper::Init(base::size(argv), argv, "TestHelpMessage"), ::testing::ExitedWithCode(EX_OK), "TestHelpMessage\n\n" " --bool_1 \\(Test bool flag\\) type: bool default: true\n" @@ -244,6 +262,7 @@ TEST_F(FlagHelperTest, HelpMessage) { " --int64_1 \\(Test int64 flag\\) type: int64 default: 0\n" " --int_1 \\(Test int flag\\) type: int default: 0\n" " --string_1 \\(Test string flag\\) type: string default: \"\"\n" + " --uint32_1 \\(Test uint32 flag\\) type: uint32 default: 0\n" " --uint64_1 \\(Test uint64 flag\\) type: uint64 default: 0\n"); stdout = orig; @@ -253,7 +272,7 @@ TEST_F(FlagHelperTest, HelpMessage) { // to exit with EX_USAGE error code and corresponding error message. TEST_F(FlagHelperTest, UnknownFlag) { const char* argv[] = {"test_program", "--flag=value"}; - base::CommandLine command_line(arraysize(argv), argv); + base::CommandLine command_line(base::size(argv), argv); brillo::FlagHelper::GetInstance()->set_command_line_for_testing( &command_line); @@ -261,7 +280,7 @@ TEST_F(FlagHelperTest, UnknownFlag) { FILE* orig = stdout; stdout = stderr; - ASSERT_EXIT(brillo::FlagHelper::Init(arraysize(argv), argv, "TestIntExit"), + ASSERT_EXIT(brillo::FlagHelper::Init(base::size(argv), argv, "TestIntExit"), ::testing::ExitedWithCode(EX_USAGE), "ERROR: unknown command line flag 'flag'"); @@ -274,7 +293,7 @@ TEST_F(FlagHelperTest, BoolParseError) { DEFINE_bool(bool_1, 0, "Test bool flag"); const char* argv[] = {"test_program", "--bool_1=value"}; - base::CommandLine command_line(arraysize(argv), argv); + base::CommandLine command_line(base::size(argv), argv); brillo::FlagHelper::GetInstance()->set_command_line_for_testing( &command_line); @@ -283,7 +302,7 @@ TEST_F(FlagHelperTest, BoolParseError) { stdout = stderr; ASSERT_EXIT( - brillo::FlagHelper::Init(arraysize(argv), argv, "TestBoolParseError"), + brillo::FlagHelper::Init(base::size(argv), argv, "TestBoolParseError"), ::testing::ExitedWithCode(EX_DATAERR), "ERROR: illegal value 'value' specified for bool flag 'bool_1'"); @@ -296,7 +315,7 @@ TEST_F(FlagHelperTest, Int32ParseError) { DEFINE_int32(int_1, 0, "Test int flag"); const char* argv[] = {"test_program", "--int_1=value"}; - base::CommandLine command_line(arraysize(argv), argv); + base::CommandLine command_line(base::size(argv), argv); brillo::FlagHelper::GetInstance()->set_command_line_for_testing( &command_line); @@ -304,11 +323,57 @@ TEST_F(FlagHelperTest, Int32ParseError) { FILE* orig = stdout; stdout = stderr; - ASSERT_EXIT(brillo::FlagHelper::Init(arraysize(argv), - argv, - "TestInt32ParseError"), - ::testing::ExitedWithCode(EX_DATAERR), - "ERROR: illegal value 'value' specified for int flag 'int_1'"); + ASSERT_EXIT( + brillo::FlagHelper::Init(base::size(argv), argv, "TestInt32ParseError"), + ::testing::ExitedWithCode(EX_DATAERR), + "ERROR: illegal value 'value' specified for int flag 'int_1'"); + + stdout = orig; +} + +// Test that when passing an incorrect/unparsable type to a command line flag, +// the program exits with code EX_DATAERR and outputs a corresponding message. +TEST_F(FlagHelperTest, Uint32ParseErrorUppperBound) { + DEFINE_uint32(uint32_1, 0, "Test uint32 flag"); + + // test with UINT32_MAX + 1 + const char* argv[] = {"test_program", "--uint32_1=4294967296"}; + base::CommandLine command_line(base::size(argv), argv); + + brillo::FlagHelper::GetInstance()->set_command_line_for_testing( + &command_line); + + FILE* orig = stdout; + stdout = stderr; + + ASSERT_EXIT( + brillo::FlagHelper::Init(base::size(argv), argv, "TestUint32ParseError"), + ::testing::ExitedWithCode(EX_DATAERR), + "ERROR: illegal value '4294967296' specified for uint32 flag " + "'uint32_1'"); + + stdout = orig; +} + +// Test that when passing an incorrect/unparsable type to a command line flag, +// the program exits with code EX_DATAERR and outputs a corresponding message. +TEST_F(FlagHelperTest, Uint32ParseErrorNegativeValue) { + DEFINE_uint32(uint32_1, 0, "Test uint32 flag"); + + const char* argv[] = {"test_program", "--uint32_1=-1"}; + base::CommandLine command_line(base::size(argv), argv); + + brillo::FlagHelper::GetInstance()->set_command_line_for_testing( + &command_line); + + FILE* orig = stdout; + stdout = stderr; + + ASSERT_EXIT( + brillo::FlagHelper::Init(base::size(argv), argv, "TestUint32ParseError"), + ::testing::ExitedWithCode(EX_DATAERR), + "ERROR: illegal value '-1' specified for uint32 flag " + "'uint32_1'"); stdout = orig; } @@ -319,7 +384,7 @@ TEST_F(FlagHelperTest, Int64ParseError) { DEFINE_int64(int64_1, 0, "Test int64 flag"); const char* argv[] = {"test_program", "--int64_1=value"}; - base::CommandLine command_line(arraysize(argv), argv); + base::CommandLine command_line(base::size(argv), argv); brillo::FlagHelper::GetInstance()->set_command_line_for_testing( &command_line); @@ -328,7 +393,7 @@ TEST_F(FlagHelperTest, Int64ParseError) { stdout = stderr; ASSERT_EXIT( - brillo::FlagHelper::Init(arraysize(argv), argv, "TestInt64ParseError"), + brillo::FlagHelper::Init(base::size(argv), argv, "TestInt64ParseError"), ::testing::ExitedWithCode(EX_DATAERR), "ERROR: illegal value 'value' specified for int64 flag " "'int64_1'"); @@ -342,7 +407,7 @@ TEST_F(FlagHelperTest, UInt64ParseError) { DEFINE_uint64(uint64_1, 0, "Test uint64 flag"); const char* argv[] = {"test_program", "--uint64_1=value"}; - base::CommandLine command_line(arraysize(argv), argv); + base::CommandLine command_line(base::size(argv), argv); brillo::FlagHelper::GetInstance()->set_command_line_for_testing( &command_line); @@ -351,7 +416,7 @@ TEST_F(FlagHelperTest, UInt64ParseError) { stdout = stderr; ASSERT_EXIT( - brillo::FlagHelper::Init(arraysize(argv), argv, "TestUInt64ParseError"), + brillo::FlagHelper::Init(base::size(argv), argv, "TestUInt64ParseError"), ::testing::ExitedWithCode(EX_DATAERR), "ERROR: illegal value 'value' specified for uint64 flag " "'uint64_1'"); diff --git a/brillo/http/http_proxy_test.cc b/brillo/http/http_proxy_test.cc index eb44263..a0d1bfa 100644 --- a/brillo/http/http_proxy_test.cc +++ b/brillo/http/http_proxy_test.cc @@ -32,23 +32,20 @@ class HttpProxyTest : public testing::Test { public: void ResolveProxyHandlerAsync(dbus::MethodCall* method_call, int timeout_msec, - dbus::ObjectProxy::ResponseCallback - MIGRATE_WrapObjectProxyCallback(callback)) { + dbus::ObjectProxy::ResponseCallback* callback) { if (null_dbus_response_) { - std::move(MIGRATE_WrapObjectProxyCallback(callback)).Run(nullptr); + std::move(*callback).Run(nullptr); return; } - std::move(MIGRATE_WrapObjectProxyCallback(callback)) - .Run(CreateDBusResponse(method_call).get()); + std::move(*callback).Run(CreateDBusResponse(method_call).get()); } - MIGRATE_WrapObjectProxyResponseType(dbus::Response) - ResolveProxyHandler(dbus::MethodCall* method_call, int timeout_msec) { + std::unique_ptr<dbus::Response> ResolveProxyHandler( + dbus::MethodCall* method_call, int timeout_msec) { if (null_dbus_response_) { - return MIGRATE_WrapObjectProxyResponseEmpty; + return std::unique_ptr<dbus::Response>(); } - return MIGRATE_WrapObjectProxyResponseConversion( - CreateDBusResponse(method_call)); + return CreateDBusResponse(method_call); } MOCK_METHOD(void, @@ -102,7 +99,7 @@ class HttpProxyTest : public testing::Test { TEST_F(HttpProxyTest, DBusNullResponseFails) { std::vector<std::string> proxies; null_dbus_response_ = true; - EXPECT_CALL(*object_proxy_, MIGRATE_MockCallMethodAndBlock(_, _)) + EXPECT_CALL(*object_proxy_, CallMethodAndBlock(_, _)) .WillOnce(Invoke(this, &HttpProxyTest::ResolveProxyHandler)); EXPECT_FALSE(GetChromeProxyServers(bus_, kTestUrl, &proxies)); } @@ -110,14 +107,14 @@ TEST_F(HttpProxyTest, DBusNullResponseFails) { TEST_F(HttpProxyTest, DBusInvalidResponseFails) { std::vector<std::string> proxies; invalid_dbus_response_ = true; - EXPECT_CALL(*object_proxy_, MIGRATE_MockCallMethodAndBlock(_, _)) + EXPECT_CALL(*object_proxy_, CallMethodAndBlock(_, _)) .WillOnce(Invoke(this, &HttpProxyTest::ResolveProxyHandler)); EXPECT_FALSE(GetChromeProxyServers(bus_, kTestUrl, &proxies)); } TEST_F(HttpProxyTest, NoProxies) { std::vector<std::string> proxies; - EXPECT_CALL(*object_proxy_, MIGRATE_MockCallMethodAndBlock(_, _)) + EXPECT_CALL(*object_proxy_, CallMethodAndBlock(_, _)) .WillOnce(Invoke(this, &HttpProxyTest::ResolveProxyHandler)); EXPECT_TRUE(GetChromeProxyServers(bus_, kTestUrl, &proxies)); EXPECT_THAT(proxies, ElementsAre(kDirectProxy)); @@ -126,7 +123,7 @@ TEST_F(HttpProxyTest, NoProxies) { TEST_F(HttpProxyTest, MultipleProxiesWithoutDirect) { proxy_info_ = "proxy example.com; socks5 foo.com;"; std::vector<std::string> proxies; - EXPECT_CALL(*object_proxy_, MIGRATE_MockCallMethodAndBlock(_, _)) + EXPECT_CALL(*object_proxy_, CallMethodAndBlock(_, _)) .WillOnce(Invoke(this, &HttpProxyTest::ResolveProxyHandler)); EXPECT_TRUE(GetChromeProxyServers(bus_, kTestUrl, &proxies)); EXPECT_THAT(proxies, ElementsAre("http://example.com", "socks5://foo.com", @@ -137,7 +134,7 @@ TEST_F(HttpProxyTest, MultipleProxiesWithDirect) { proxy_info_ = "socks foo.com; Https example.com ; badproxy example2.com ; " "socks5 test.com ; proxy foobar.com; DIRECT "; std::vector<std::string> proxies; - EXPECT_CALL(*object_proxy_, MIGRATE_MockCallMethodAndBlock(_, _)) + EXPECT_CALL(*object_proxy_, CallMethodAndBlock(_, _)) .WillOnce(Invoke(this, &HttpProxyTest::ResolveProxyHandler)); EXPECT_TRUE(GetChromeProxyServers(bus_, kTestUrl, &proxies)); EXPECT_THAT(proxies, ElementsAre("socks4://foo.com", "https://example.com", @@ -147,7 +144,7 @@ TEST_F(HttpProxyTest, MultipleProxiesWithDirect) { TEST_F(HttpProxyTest, DBusNullResponseFailsAsync) { null_dbus_response_ = true; - EXPECT_CALL(*object_proxy_, MIGRATE_CallMethod(_, _, _)) + EXPECT_CALL(*object_proxy_, DoCallMethod(_, _, _)) .WillOnce(Invoke(this, &HttpProxyTest::ResolveProxyHandlerAsync)); EXPECT_CALL(*this, GetProxiesCallback(false, _)); GetChromeProxyServersAsync( @@ -157,7 +154,7 @@ TEST_F(HttpProxyTest, DBusNullResponseFailsAsync) { TEST_F(HttpProxyTest, DBusInvalidResponseFailsAsync) { invalid_dbus_response_ = true; - EXPECT_CALL(*object_proxy_, MIGRATE_CallMethod(_, _, _)) + EXPECT_CALL(*object_proxy_, DoCallMethod(_, _, _)) .WillOnce(Invoke(this, &HttpProxyTest::ResolveProxyHandlerAsync)); EXPECT_CALL(*this, GetProxiesCallback(false, _)); GetChromeProxyServersAsync( @@ -173,7 +170,7 @@ TEST_F(HttpProxyTest, MultipleProxiesWithDirectAsync) { std::vector<std::string> expected = { "socks4://foo.com", "https://example.com", "socks5://test.com", "http://foobar.com", kDirectProxy}; - EXPECT_CALL(*object_proxy_, MIGRATE_CallMethod(_, _, _)) + EXPECT_CALL(*object_proxy_, DoCallMethod(_, _, _)) .WillOnce(Invoke(this, &HttpProxyTest::ResolveProxyHandlerAsync)); EXPECT_CALL(*this, GetProxiesCallback(true, expected)); GetChromeProxyServersAsync( diff --git a/brillo/http/http_transport_curl.cc b/brillo/http/http_transport_curl.cc index 45a28a3..de6899a 100644 --- a/brillo/http/http_transport_curl.cc +++ b/brillo/http/http_transport_curl.cc @@ -7,10 +7,11 @@ #include <limits> #include <base/bind.h> +#include <base/files/file_descriptor_watcher_posix.h> #include <base/files/file_util.h> #include <base/logging.h> -#include <base/message_loop/message_loop.h> #include <base/strings/stringprintf.h> +#include <base/threading/thread_task_runner_handle.h> #include <brillo/http/http_connection_curl.h> #include <brillo/http/http_request.h> #include <brillo/strings/string_utils.h> @@ -22,7 +23,7 @@ namespace curl { // This is a class that stores connection data on particular CURL socket // and provides file descriptor watcher to monitor read and/or write operations // on the socket's file descriptor. -class Transport::SocketPollData : public base::MessagePumpForIO::FdWatcher { +class Transport::SocketPollData { public: SocketPollData(const std::shared_ptr<CurlInterface>& curl_interface, CURLM* curl_multi_handle, @@ -31,27 +32,35 @@ class Transport::SocketPollData : public base::MessagePumpForIO::FdWatcher { : curl_interface_(curl_interface), curl_multi_handle_(curl_multi_handle), transport_(transport), - socket_fd_(socket_fd), - file_descriptor_watcher_(FROM_HERE) {} + socket_fd_(socket_fd) {} - // Returns the pointer for the socket-specific file descriptor watcher. - base::MessagePumpForIO::FdWatchController* GetWatcher() { - return &file_descriptor_watcher_; + void StopWatcher() { + read_watcher_ = nullptr; + write_watcher_ = nullptr; } - private: - // Overrides from base::MessagePumpForIO::Watcher. - void OnFileCanReadWithoutBlocking(int fd) override { - OnSocketReady(fd, CURL_CSELECT_IN); + bool WatchReadable() { + read_watcher_ = base::FileDescriptorWatcher::WatchReadable( + socket_fd_, + base::BindRepeating(&Transport::SocketPollData::OnSocketReady, + base::Unretained(this), + CURL_CSELECT_IN)); + return read_watcher_.get(); } - void OnFileCanWriteWithoutBlocking(int fd) override { - OnSocketReady(fd, CURL_CSELECT_OUT); + + bool WatchWritable() { + write_watcher_ = base::FileDescriptorWatcher::WatchWritable( + socket_fd_, + base::BindRepeating(&Transport::SocketPollData::OnSocketReady, + base::Unretained(this), + CURL_CSELECT_OUT)); + return write_watcher_.get(); } + private: // Data on the socket is available to be read from or written to. // Notify CURL of the action it needs to take on the socket file descriptor. - void OnSocketReady(int fd, int action) { - CHECK_EQ(socket_fd_, fd) << "Unexpected socket file descriptor"; + void OnSocketReady(int action) { int still_running_count = 0; CURLMcode code = curl_interface_->MultiSocketAction( curl_multi_handle_, socket_fd_, action, &still_running_count); @@ -70,8 +79,9 @@ class Transport::SocketPollData : public base::MessagePumpForIO::FdWatcher { Transport* transport_; // The socket file descriptor for the connection. curl_socket_t socket_fd_; - // File descriptor watcher to notify us of asynchronous I/O on the FD. - base::MessagePumpForIO::FdWatchController file_descriptor_watcher_; + + std::unique_ptr<base::FileDescriptorWatcher::Controller> read_watcher_; + std::unique_ptr<base::FileDescriptorWatcher::Controller> write_watcher_; DISALLOW_COPY_AND_ASSIGN(SocketPollData); }; @@ -212,8 +222,7 @@ std::shared_ptr<http::Connection> Transport::CreateConnection( void Transport::RunCallbackAsync(const base::Location& from_here, const base::Closure& callback) { - base::MessageLoopForIO::current()->task_runner()->PostTask( - from_here, callback); + base::ThreadTaskRunnerHandle::Get()->PostTask(from_here, callback); } RequestID Transport::StartAsyncTransfer(http::Connection* connection, @@ -386,42 +395,22 @@ int Transport::MultiSocketCallback(CURL* easy, // Make sure we stop watching the socket file descriptor now, before // we schedule the SocketPollData for deletion. - poll_data->GetWatcher()->StopWatchingFileDescriptor(); + poll_data->StopWatcher(); // This method can be called indirectly from SocketPollData::OnSocketReady, // so delay destruction of SocketPollData object till the next loop cycle. - base::MessageLoopForIO::current()->task_runner()->DeleteSoon(FROM_HERE, - poll_data); + base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE, poll_data); return 0; } - base::MessagePumpForIO::Mode watch_mode = base::MessagePumpForIO::WATCH_READ; - switch (what) { - case CURL_POLL_IN: - watch_mode = base::MessagePumpForIO::WATCH_READ; - break; - case CURL_POLL_OUT: - watch_mode = base::MessagePumpForIO::WATCH_WRITE; - break; - case CURL_POLL_INOUT: - watch_mode = base::MessagePumpForIO::WATCH_READ_WRITE; - break; - default: - LOG(FATAL) << "Unknown CURL socket action: " << what; - break; - } + poll_data->StopWatcher(); + + bool success = true; + if (what == CURL_POLL_IN || what == CURL_POLL_INOUT) + success = poll_data->WatchReadable() && success; + if (what == CURL_POLL_OUT || what == CURL_POLL_INOUT) + success = poll_data->WatchWritable() && success; - // WatchFileDescriptor() can be called with the same controller object - // (watcher) to amend the watch mode, however this has cumulative effect. - // For example, if we were watching a file descriptor for READ operations - // and now call it to watch for WRITE, it will end up watching for both - // READ and WRITE. This is not what we want here, so stop watching the - // file descriptor on previous controller before starting with a different - // mode. - if (!poll_data->GetWatcher()->StopWatchingFileDescriptor()) - LOG(WARNING) << "Failed to stop watching the previous socket descriptor"; - CHECK(base::MessageLoopForIO::current()->WatchFileDescriptor( - s, true, watch_mode, poll_data->GetWatcher(), poll_data)) - << "Failed to watch the CURL socket."; + CHECK(success) << "Failed to watch the CURL socket."; return 0; } @@ -433,11 +422,11 @@ int Transport::MultiTimerCallback(CURLM* /* multi */, // Cancel any previous timer callbacks. transport->weak_ptr_factory_for_timer_.InvalidateWeakPtrs(); if (timeout_ms >= 0) { - base::MessageLoopForIO::current()->task_runner()->PostDelayedTask( - FROM_HERE, - base::Bind(&Transport::OnTimer, - transport->weak_ptr_factory_for_timer_.GetWeakPtr()), - base::TimeDelta::FromMilliseconds(timeout_ms)); + base::ThreadTaskRunnerHandle::Get()->PostDelayedTask( + FROM_HERE, + base::Bind(&Transport::OnTimer, + transport->weak_ptr_factory_for_timer_.GetWeakPtr()), + base::TimeDelta::FromMilliseconds(timeout_ms)); } return 0; } diff --git a/brillo/http/http_transport_curl_test.cc b/brillo/http/http_transport_curl_test.cc index 40ef23e..6e94978 100644 --- a/brillo/http/http_transport_curl_test.cc +++ b/brillo/http/http_transport_curl_test.cc @@ -8,6 +8,7 @@ #include <base/bind.h> #include <base/message_loop/message_loop.h> #include <base/run_loop.h> +#include <base/threading/thread_task_runner_handle.h> #include <brillo/http/http_connection_curl.h> #include <brillo/http/http_request.h> #include <brillo/http/mock_curl_api.h> @@ -242,7 +243,7 @@ TEST_F(HttpCurlTransportAsyncTest, StartAsyncTransfer) { auto success_callback = base::Bind([]( int* success_call_count, const base::Closure& quit_closure, RequestID /* request_id */, std::unique_ptr<http::Response> /* resp */) { - base::MessageLoop::current()->task_runner()->PostTask( + base::ThreadTaskRunnerHandle::Get()->PostTask( FROM_HERE, quit_closure); (*success_call_count)++; }, &success_call_count, run_loop.QuitClosure()); diff --git a/brillo/message_loops/base_message_loop.cc b/brillo/message_loops/base_message_loop.cc index 9a9e43f..c3499ab 100644 --- a/brillo/message_loops/base_message_loop.cc +++ b/brillo/message_loops/base_message_loop.cc @@ -30,12 +30,11 @@ #include <base/run_loop.h> #include <base/strings/string_number_conversions.h> #include <base/strings/string_split.h> +#include <base/threading/thread_task_runner_handle.h> #include <brillo/location_logging.h> #include <brillo/strings/string_utils.h> -using base::Closure; - namespace { const char kMiscMinorPath[] = "/proc/misc"; @@ -49,7 +48,7 @@ const int BaseMessageLoop::kInvalidMinor = -1; const int BaseMessageLoop::kUninitializedMinor = -2; BaseMessageLoop::BaseMessageLoop() { - CHECK(!base::MessageLoop::current()) + CHECK(!base::ThreadTaskRunnerHandle::IsSet()) << "You can't create a base::MessageLoopForIO when another " "base::MessageLoop is already created for this thread."; owned_base_loop_.reset(new base::MessageLoopForIO()); @@ -62,13 +61,6 @@ BaseMessageLoop::BaseMessageLoop(base::MessageLoopForIO* base_loop) watcher_(std::make_unique<base::FileDescriptorWatcher>(base_loop_)) {} BaseMessageLoop::~BaseMessageLoop() { - for (auto& io_task : io_tasks_) { - DVLOG_LOC(io_task.second.location(), 1) - << "Removing file descriptor watcher task_id " << io_task.first - << " leaked on BaseMessageLoop, scheduled from this location."; - io_task.second.StopWatching(); - } - // Note all pending canceled delayed tasks when destroying the message loop. size_t lazily_deleted_tasks = 0; for (const auto& delayed_task : delayed_tasks_) { @@ -87,85 +79,21 @@ BaseMessageLoop::~BaseMessageLoop() { MessageLoop::TaskId BaseMessageLoop::PostDelayedTask( const base::Location& from_here, - const Closure &task, + base::OnceClosure task, base::TimeDelta delay) { TaskId task_id = NextTaskId(); bool base_scheduled = base_loop_->task_runner()->PostDelayedTask( from_here, - base::Bind(&BaseMessageLoop::OnRanPostedTask, - weak_ptr_factory_.GetWeakPtr(), - task_id), + base::BindOnce(&BaseMessageLoop::OnRanPostedTask, + weak_ptr_factory_.GetWeakPtr(), task_id), delay); DVLOG_LOC(from_here, 1) << "Scheduling delayed task_id " << task_id << " to run in " << delay << "."; if (!base_scheduled) return MessageLoop::kTaskIdNull; - delayed_tasks_.emplace(task_id, DelayedTask{from_here, task_id, task}); - return task_id; -} - -MessageLoop::TaskId BaseMessageLoop::WatchFileDescriptor( - const base::Location& from_here, - int fd, - WatchMode mode, - bool persistent, - const Closure &task) { - // base::MessageLoopForIO CHECKS that "fd >= 0", so we handle that case here. - if (fd < 0) - return MessageLoop::kTaskIdNull; - - base::MessagePumpForIO::Mode base_mode = base::MessagePumpForIO::WATCH_READ; - switch (mode) { - case MessageLoop::kWatchRead: - base_mode = base::MessagePumpForIO::WATCH_READ; - break; - case MessageLoop::kWatchWrite: - base_mode = base::MessagePumpForIO::WATCH_WRITE; - break; - default: - return MessageLoop::kTaskIdNull; - } - - TaskId task_id = NextTaskId(); - auto it_bool = io_tasks_.emplace( - std::piecewise_construct, - std::forward_as_tuple(task_id), - std::forward_as_tuple( - from_here, this, task_id, fd, base_mode, persistent, task)); - // This should always insert a new element. - DCHECK(it_bool.second); - bool scheduled = it_bool.first->second.StartWatching(); - DVLOG_LOC(from_here, 1) - << "Watching fd " << fd << " for " - << (mode == MessageLoop::kWatchRead ? "reading" : "writing") - << (persistent ? " persistently" : " just once") - << " as task_id " << task_id - << (scheduled ? " successfully" : " failed."); - - if (!scheduled) { - io_tasks_.erase(task_id); - return MessageLoop::kTaskIdNull; - } - -#ifndef __ANDROID_HOST__ - // Determine if the passed fd is the binder file descriptor. For that, we need - // to check that is a special char device and that the major and minor device - // numbers match. The binder file descriptor can't be removed and added back - // to an epoll group when there's work available to be done by the file - // descriptor due to bugs in the binder driver (b/26524111) when used with - // epoll. Therefore, we flag the binder fd and never attempt to remove it. - // This may cause the binder file descriptor to be attended with higher - // priority and cause starvation of other events. - struct stat buf; - if (fstat(fd, &buf) == 0 && - S_ISCHR(buf.st_mode) && - major(buf.st_rdev) == MISC_MAJOR && - minor(buf.st_rdev) == GetBinderMinor()) { - it_bool.first->second.RunImmediately(); - } -#endif - + delayed_tasks_.emplace(task_id, + DelayedTask{from_here, task_id, std::move(task)}); return task_id; } @@ -173,13 +101,9 @@ bool BaseMessageLoop::CancelTask(TaskId task_id) { if (task_id == kTaskIdNull) return false; auto delayed_task_it = delayed_tasks_.find(task_id); - if (delayed_task_it == delayed_tasks_.end()) { - // This might be an IOTask then. - auto io_task_it = io_tasks_.find(task_id); - if (io_task_it == io_tasks_.end()) - return false; - return io_task_it->second.CancelTask(); - } + if (delayed_task_it == delayed_tasks_.end()) + return false; + // A DelayedTask was found for this task_id at this point. // Check if the callback was already canceled but we have the entry in @@ -189,10 +113,10 @@ bool BaseMessageLoop::CancelTask(TaskId task_id) { DVLOG_LOC(delayed_task_it->second.location, 1) << "Removing task_id " << task_id << " scheduled from this location."; - // We reset to closure to a null Closure to release all the resources + // We reset to closure to a null OnceClosure to release all the resources // used by this closure at this point, but we don't remove the task_id from // delayed_tasks_ since we can't tell base::MessageLoopForIO to not run it. - delayed_task_it->second.closure = Closure(); + delayed_task_it->second.closure.Reset(); return true; } @@ -229,7 +153,7 @@ void BaseMessageLoop::BreakLoop() { base_run_loop_->Quit(); } -Closure BaseMessageLoop::QuitClosure() const { +base::RepeatingClosure BaseMessageLoop::QuitClosure() const { if (base_run_loop_ == nullptr) return base::DoNothing(); return base_run_loop_->QuitClosure(); @@ -241,8 +165,7 @@ MessageLoop::TaskId BaseMessageLoop::NextTaskId() { res = ++last_id_; // We would run out of memory before we run out of task ids. } while (!res || - delayed_tasks_.find(res) != delayed_tasks_.end() || - io_tasks_.find(res) != io_tasks_.end()); + delayed_tasks_.find(res) != delayed_tasks_.end()); return res; } @@ -255,9 +178,7 @@ void BaseMessageLoop::OnRanPostedTask(MessageLoop::TaskId task_id) { << " scheduled from this location."; // Mark the task as canceled while we are running it so CancelTask returns // false. - Closure closure = std::move(task_it->second.closure); - task_it->second.closure = Closure(); - closure.Run(); + std::move(task_it->second.closure).Run(); // If the |run_once_| flag is set, it is because we are instructed to run // only once callback. @@ -269,15 +190,6 @@ void BaseMessageLoop::OnRanPostedTask(MessageLoop::TaskId task_id) { delayed_tasks_.erase(task_it); } -void BaseMessageLoop::OnFileReadyPostedTask(MessageLoop::TaskId task_id) { - auto task_it = io_tasks_.find(task_id); - // Even if this task was canceled while we were waiting in the message loop - // for this method to run, the entry in io_tasks_ should still be present, but - // won't do anything. - DCHECK(task_it != io_tasks_.end()); - task_it->second.OnFileReadyPostedTask(); -} - int BaseMessageLoop::ParseBinderMinor( const std::string& file_contents) { int result = kInvalidMinor; @@ -311,141 +223,4 @@ unsigned int BaseMessageLoop::GetBinderMinor() { return binder_minor_; } -BaseMessageLoop::IOTask::IOTask(const base::Location& location, - BaseMessageLoop* loop, - MessageLoop::TaskId task_id, - int fd, - base::MessagePumpForIO::Mode base_mode, - bool persistent, - const Closure& task) - : location_(location), loop_(loop), task_id_(task_id), - fd_(fd), base_mode_(base_mode), persistent_(persistent), closure_(task), - fd_watcher_(FROM_HERE) {} - -bool BaseMessageLoop::IOTask::StartWatching() { - // Please see MessagePumpLibevent for definition. - static_assert(std::is_same<base::MessagePumpForIO, base::MessagePumpLibevent>::value, - "MessagePumpForIO::WatchFileDescriptor is not supported " - "when MessagePumpForIO is not a MessagePumpLibevent."); - - return static_cast<base::MessagePumpLibevent*>( - loop_->base_loop_->pump_.get())->WatchFileDescriptor( - fd_, persistent_, base_mode_, &fd_watcher_, this); -} - -void BaseMessageLoop::IOTask::StopWatching() { - // This is safe to call even if we are not watching for it. - fd_watcher_.StopWatchingFileDescriptor(); -} - -void BaseMessageLoop::IOTask::OnFileCanReadWithoutBlocking(int /* fd */) { - OnFileReady(); -} - -void BaseMessageLoop::IOTask::OnFileCanWriteWithoutBlocking(int /* fd */) { - OnFileReady(); -} - -void BaseMessageLoop::IOTask::OnFileReady() { - // For file descriptors marked with the immediate_run flag, we don't call - // StopWatching() and wait, instead we dispatch the callback immediately. - if (immediate_run_) { - posted_task_pending_ = true; - OnFileReadyPostedTask(); - return; - } - - // When the file descriptor becomes available we stop watching for it and - // schedule a task to run the callback from the main loop. The callback will - // run using the same scheduler used to run other delayed tasks, avoiding - // starvation of the available posted tasks if there are file descriptors - // always available. The new posted task will use the same TaskId as the - // current file descriptor watching task an could be canceled in either state, - // when waiting for the file descriptor or waiting in the main loop. - StopWatching(); - bool base_scheduled = loop_->base_loop_->task_runner()->PostTask( - location_, - base::Bind(&BaseMessageLoop::OnFileReadyPostedTask, - loop_->weak_ptr_factory_.GetWeakPtr(), - task_id_)); - posted_task_pending_ = true; - if (base_scheduled) { - DVLOG_LOC(location_, 1) - << "Dispatching task_id " << task_id_ << " for " - << (base_mode_ == base::MessagePumpForIO::WATCH_READ ? - "reading" : "writing") - << " file descriptor " << fd_ << ", scheduled from this location."; - } else { - // In the rare case that PostTask() fails, we fall back to run it directly. - // This would indicate a bigger problem with the message loop setup. - LOG(ERROR) << "Error on base::MessageLoopForIO::PostTask()."; - OnFileReadyPostedTask(); - } -} - -void BaseMessageLoop::IOTask::OnFileReadyPostedTask() { - // We can't access |this| after running the |closure_| since it could call - // CancelTask on its own task_id, so we copy the members we need now. - BaseMessageLoop* loop_ptr = loop_; - DCHECK(posted_task_pending_ = true); - posted_task_pending_ = false; - - // If this task was already canceled, the closure will be null and there is - // nothing else to do here. This execution doesn't count a step for RunOnce() - // unless we have a callback to run. - if (closure_.is_null()) { - loop_->io_tasks_.erase(task_id_); - return; - } - - DVLOG_LOC(location_, 1) - << "Running task_id " << task_id_ << " for " - << (base_mode_ == base::MessagePumpForIO::WATCH_READ ? - "reading" : "writing") - << " file descriptor " << fd_ << ", scheduled from this location."; - - if (persistent_) { - // In the persistent case we just run the callback. If this callback cancels - // the task id, we can't access |this| anymore, so we re-start watching the - // file descriptor before running the callback, unless this is a fd where - // we didn't stop watching the file descriptor when it became available. - if (!immediate_run_) - StartWatching(); - closure_.Run(); - } else { - // This will destroy |this|, the fd_watcher and therefore stop watching this - // file descriptor. - Closure closure_copy = std::move(closure_); - loop_->io_tasks_.erase(task_id_); - // Run the closure from the local copy we just made. - closure_copy.Run(); - } - - if (loop_ptr->run_once_) { - loop_ptr->run_once_ = false; - loop_ptr->BreakLoop(); - } -} - -bool BaseMessageLoop::IOTask::CancelTask() { - if (closure_.is_null()) - return false; - - DVLOG_LOC(location_, 1) - << "Removing task_id " << task_id_ << " scheduled from this location."; - - if (!posted_task_pending_) { - // Destroying the FileDescriptorWatcher implicitly stops watching the file - // descriptor. This will delete our instance. - loop_->io_tasks_.erase(task_id_); - return true; - } - // The IOTask is waiting for the message loop to run its delayed task, so - // it is not watching for the file descriptor. We release the closure - // resources now but keep the IOTask instance alive while we wait for the - // callback to run and delete the IOTask. - closure_ = Closure(); - return true; -} - } // namespace brillo diff --git a/brillo/message_loops/base_message_loop.h b/brillo/message_loops/base_message_loop.h index c038ac7..75e4361 100644 --- a/brillo/message_loops/base_message_loop.h +++ b/brillo/message_loops/base_message_loop.h @@ -42,15 +42,9 @@ class BRILLO_EXPORT BaseMessageLoop : public MessageLoop { // MessageLoop overrides. TaskId PostDelayedTask(const base::Location& from_here, - const base::Closure& task, + base::OnceClosure task, base::TimeDelta delay) override; using MessageLoop::PostDelayedTask; - TaskId WatchFileDescriptor(const base::Location& from_here, - int fd, - WatchMode mode, - bool persistent, - const base::Closure& task) override; - using MessageLoop::WatchFileDescriptor; bool CancelTask(TaskId task_id) override; bool RunOnce(bool may_block) override; void Run() override; @@ -58,7 +52,7 @@ class BRILLO_EXPORT BaseMessageLoop : public MessageLoop { // Returns a callback that will quit the current message loop. If the message // loop is not running, an empty (null) callback is returned. - base::Closure QuitClosure() const; + base::RepeatingClosure QuitClosure() const; private: FRIEND_TEST(BaseMessageLoopTest, ParseBinderMinor); @@ -75,12 +69,6 @@ class BRILLO_EXPORT BaseMessageLoop : public MessageLoop { // scheduled with Post*Task() of id |task_id|, even if it was canceled. void OnRanPostedTask(MessageLoop::TaskId task_id); - // Called from the message loop when the IOTask should run the scheduled - // callback. This is a simple wrapper of IOTask::OnFileReadyPostedTask() - // posted from the BaseMessageLoop so it is deleted when the BaseMessageLoop - // goes out of scope since we can't cancel the callback otherwise. - void OnFileReadyPostedTask(MessageLoop::TaskId task_id); - // Return a new unused task_id. TaskId NextTaskId(); @@ -91,69 +79,7 @@ class BRILLO_EXPORT BaseMessageLoop : public MessageLoop { base::Location location; MessageLoop::TaskId task_id; - base::Closure closure; - }; - - class IOTask : public base::MessagePumpForIO::FdWatcher { - public: - IOTask(const base::Location& location, - BaseMessageLoop* loop, - MessageLoop::TaskId task_id, - int fd, - base::MessagePumpForIO::Mode base_mode, - bool persistent, - const base::Closure& task); - - const base::Location& location() const { return location_; } - - // Used to start/stop watching the file descriptor while keeping the - // IOTask entry available. - bool StartWatching(); - void StopWatching(); - - // Called from the message loop as a PostTask() when the file descriptor is - // available, scheduled to run from OnFileReady(). - void OnFileReadyPostedTask(); - - // Cancel the IOTask and returns whether it was actually canceled, with the - // same semantics as MessageLoop::CancelTask(). - bool CancelTask(); - - // Sets the closure to be run immediately whenever the file descriptor - // becomes ready. - void RunImmediately() { immediate_run_ = true; } - - private: - base::Location location_; - BaseMessageLoop* loop_; - - // These are the arguments passed in the constructor, basically forwarding - // all the arguments passed to WatchFileDescriptor() plus the assigned - // TaskId for this task. - MessageLoop::TaskId task_id_; - int fd_; - base::MessagePumpForIO::Mode base_mode_; - bool persistent_; - base::Closure closure_; - - base::MessagePumpForIO::FdWatchController fd_watcher_; - - // Tells whether there is a pending call to OnFileReadPostedTask(). - bool posted_task_pending_{false}; - - // Whether the registered callback should be running immediately when the - // file descriptor is ready, as opposed to posting a task to the main loop - // to prevent starvation. - bool immediate_run_{false}; - - // base::MessageLoopForIO::Watcher overrides: - void OnFileCanReadWithoutBlocking(int fd) override; - void OnFileCanWriteWithoutBlocking(int fd) override; - - // Common implementation for both the read and write case. - void OnFileReady(); - - DISALLOW_COPY_AND_ASSIGN(IOTask); + base::OnceClosure closure; }; // The base::MessageLoopForIO instance owned by this class, if any. This @@ -163,9 +89,6 @@ class BRILLO_EXPORT BaseMessageLoop : public MessageLoop { // Tasks blocked on a timeout. std::map<MessageLoop::TaskId, DelayedTask> delayed_tasks_; - // Tasks blocked on I/O. - std::map<MessageLoop::TaskId, IOTask> io_tasks_; - // Flag to mark that we should run the message loop only one iteration. bool run_once_{false}; diff --git a/brillo/message_loops/fake_message_loop.cc b/brillo/message_loops/fake_message_loop.cc index 41f5b51..185b20c 100644 --- a/brillo/message_loops/fake_message_loop.cc +++ b/brillo/message_loops/fake_message_loop.cc @@ -15,7 +15,7 @@ FakeMessageLoop::FakeMessageLoop(base::SimpleTestClock* clock) MessageLoop::TaskId FakeMessageLoop::PostDelayedTask( const base::Location& from_here, - const base::Closure& task, + base::OnceClosure task, base::TimeDelta delay) { // If no SimpleTestClock was provided, we use the last time we fired a // callback. In this way, tasks scheduled from a Closure will have the right @@ -25,7 +25,7 @@ MessageLoop::TaskId FakeMessageLoop::PostDelayedTask( MessageLoop::TaskId current_id = ++last_id_; // FakeMessageLoop is limited to only 2^64 tasks. That should be enough. CHECK(current_id); - tasks_.emplace(current_id, ScheduledTask{from_here, false, task}); + tasks_.emplace(current_id, ScheduledTask{from_here, std::move(task)}); fire_order_.push(std::make_pair(current_time_ + delay, current_id)); VLOG_LOC(from_here, 1) << "Scheduling delayed task_id " << current_id << " to run at " << current_time_ + delay @@ -33,20 +33,6 @@ MessageLoop::TaskId FakeMessageLoop::PostDelayedTask( return current_id; } -MessageLoop::TaskId FakeMessageLoop::WatchFileDescriptor( - const base::Location& from_here, - int fd, - WatchMode mode, - bool persistent, - const base::Closure& task) { - MessageLoop::TaskId current_id = ++last_id_; - // FakeMessageLoop is limited to only 2^64 tasks. That should be enough. - CHECK(current_id); - tasks_.emplace(current_id, ScheduledTask{from_here, persistent, task}); - fds_watched_.emplace(std::make_pair(fd, mode), current_id); - return current_id; -} - bool FakeMessageLoop::CancelTask(TaskId task_id) { if (task_id == MessageLoop::kTaskIdNull) return false; @@ -58,36 +44,6 @@ bool FakeMessageLoop::CancelTask(TaskId task_id) { bool FakeMessageLoop::RunOnce(bool may_block) { if (test_clock_) current_time_ = test_clock_->Now(); - // Try to fire ready file descriptors first. - for (const auto& fd_mode : fds_ready_) { - const auto& fd_watched = fds_watched_.find(fd_mode); - if (fd_watched == fds_watched_.end()) - continue; - // The fd_watched->second task might have been canceled and we never removed - // it from the fds_watched_, so we fix that now. - const auto& scheduled_task_ref = tasks_.find(fd_watched->second); - if (scheduled_task_ref == tasks_.end()) { - fds_watched_.erase(fd_watched); - continue; - } - VLOG_LOC(scheduled_task_ref->second.location, 1) - << "Running task_id " << fd_watched->second - << " for watching file descriptor " << fd_mode.first << " for " - << (fd_mode.second == MessageLoop::kWatchRead ? "reading" : "writing") - << (scheduled_task_ref->second.persistent ? - " persistently" : " just once") - << " scheduled from this location."; - if (scheduled_task_ref->second.persistent) { - scheduled_task_ref->second.callback.Run(); - } else { - base::Closure callback = std::move(scheduled_task_ref->second.callback); - tasks_.erase(scheduled_task_ref); - fds_watched_.erase(fd_watched); - callback.Run(); - } - return true; - } - // Try to fire time-based callbacks afterwards. while (!fire_order_.empty() && (may_block || fire_order_.top().first <= current_time_)) { @@ -108,32 +64,22 @@ bool FakeMessageLoop::RunOnce(bool may_block) { // Move the Closure out of the map before delete it. We need to delete the // entry from the map before we call the callback, since calling CancelTask // for the task you are running now should fail and return false. - base::Closure callback = std::move(scheduled_task_ref->second.callback); + base::OnceClosure callback = std::move(scheduled_task_ref->second.callback); VLOG_LOC(scheduled_task_ref->second.location, 1) << "Running task_id " << task_ref.second << " at time " << current_time_ << " from this location."; tasks_.erase(scheduled_task_ref); - callback.Run(); + std::move(callback).Run(); return true; } return false; } -void FakeMessageLoop::SetFileDescriptorReadiness(int fd, - WatchMode mode, - bool ready) { - if (ready) - fds_ready_.emplace(fd, mode); - else - fds_ready_.erase(std::make_pair(fd, mode)); -} - bool FakeMessageLoop::PendingTasks() { for (const auto& task : tasks_) { VLOG_LOC(task.second.location, 1) - << "Pending " << (task.second.persistent ? "persistent " : "") - << "task_id " << task.first << " scheduled from here."; + << "Pending task_id " << task.first << " scheduled from here."; } return !tasks_.empty(); } diff --git a/brillo/message_loops/fake_message_loop.h b/brillo/message_loops/fake_message_loop.h index 4b6e8ac..783af1b 100644 --- a/brillo/message_loops/fake_message_loop.h +++ b/brillo/message_loops/fake_message_loop.h @@ -37,26 +37,14 @@ class BRILLO_EXPORT FakeMessageLoop : public MessageLoop { ~FakeMessageLoop() override = default; TaskId PostDelayedTask(const base::Location& from_here, - const base::Closure& task, + base::OnceClosure task, base::TimeDelta delay) override; using MessageLoop::PostDelayedTask; - TaskId WatchFileDescriptor(const base::Location& from_here, - int fd, - WatchMode mode, - bool persistent, - const base::Closure& task) override; - using MessageLoop::WatchFileDescriptor; bool CancelTask(TaskId task_id) override; bool RunOnce(bool may_block) override; // FakeMessageLoop methods: - // Pretend, for the purpose of the FakeMessageLoop watching for file - // descriptors, that the file descriptor |fd| readiness to perform the - // operation described by |mode| is |ready|. Initially, no file descriptor - // is ready for any operation. - void SetFileDescriptorReadiness(int fd, WatchMode mode, bool ready); - // Return whether there are peding tasks. Useful to check that no // callbacks were leaked. bool PendingTasks(); @@ -64,8 +52,7 @@ class BRILLO_EXPORT FakeMessageLoop : public MessageLoop { private: struct ScheduledTask { base::Location location; - bool persistent; - base::Closure callback; + base::OnceClosure callback; }; // The sparse list of scheduled pending callbacks. @@ -79,13 +66,6 @@ class BRILLO_EXPORT FakeMessageLoop : public MessageLoop { std::vector<std::pair<base::Time, MessageLoop::TaskId>>, std::greater<std::pair<base::Time, MessageLoop::TaskId>>> fire_order_; - // The bag of watched (fd, mode) pair associated with the TaskId that's - // watching them. - std::multimap<std::pair<int, WatchMode>, MessageLoop::TaskId> fds_watched_; - - // The set of (fd, mode) pairs that are faked as ready. - std::set<std::pair<int, WatchMode>> fds_ready_; - base::SimpleTestClock* test_clock_ = nullptr; base::Time current_time_ = base::Time::FromDoubleT(1246996800.); diff --git a/brillo/message_loops/fake_message_loop_test.cc b/brillo/message_loops/fake_message_loop_test.cc index b4b839c..a5d0607 100644 --- a/brillo/message_loops/fake_message_loop_test.cc +++ b/brillo/message_loops/fake_message_loop_test.cc @@ -15,7 +15,7 @@ #include <brillo/message_loops/message_loop.h> -using base::Bind; +using base::BindOnce; using base::Time; using base::TimeDelta; using std::vector; @@ -45,17 +45,18 @@ TEST_F(FakeMessageLoopTest, CancelTaskInvalidValuesTest) { TEST_F(FakeMessageLoopTest, PostDelayedTaskRunsInOrder) { vector<int> order; - auto callback = [](std::vector<int>* order, int value) { - order->push_back(value); - }; - loop_->PostDelayedTask(Bind(callback, base::Unretained(&order), 1), - TimeDelta::FromSeconds(1)); - loop_->PostDelayedTask(Bind(callback, base::Unretained(&order), 4), - TimeDelta::FromSeconds(4)); - loop_->PostDelayedTask(Bind(callback, base::Unretained(&order), 3), - TimeDelta::FromSeconds(3)); - loop_->PostDelayedTask(Bind(callback, base::Unretained(&order), 2), - TimeDelta::FromSeconds(2)); + loop_->PostDelayedTask( + BindOnce([](vector<int>* order) { order->push_back(1); }, &order), + TimeDelta::FromSeconds(1)); + loop_->PostDelayedTask( + BindOnce([](vector<int>* order) { order->push_back(4); }, &order), + TimeDelta::FromSeconds(4)); + loop_->PostDelayedTask( + BindOnce([](vector<int>* order) { order->push_back(3); }, &order), + TimeDelta::FromSeconds(3)); + loop_->PostDelayedTask( + BindOnce([](vector<int>* order) { order->push_back(2); }, &order), + TimeDelta::FromSeconds(2)); // Run until all the tasks are run. loop_->Run(); EXPECT_EQ((vector<int>{1, 2, 3, 4}), order); @@ -84,34 +85,6 @@ TEST_F(FakeMessageLoopTest, PostDelayedTaskAdvancesTheTime) { EXPECT_EQ(start + TimeDelta::FromSeconds(3), clock_.Now()); } -TEST_F(FakeMessageLoopTest, WatchFileDescriptorWaits) { - int fd = 1234; - // We will simulate this situation. At the beginning, we will watch for a - // file descriptor that won't trigger for 10s. Then we will pretend it is - // ready after 10s and expect its callback to run just once. - int called = 0; - TaskId task_id = loop_->WatchFileDescriptor( - FROM_HERE, fd, MessageLoop::kWatchRead, false, - Bind([](int* called) { (*called)++; }, base::Unretained(&called))); - EXPECT_NE(MessageLoop::kTaskIdNull, task_id); - - EXPECT_NE(MessageLoop::kTaskIdNull, - loop_->PostDelayedTask(Bind(&FakeMessageLoop::BreakLoop, - base::Unretained(loop_.get())), - TimeDelta::FromSeconds(10))); - EXPECT_NE(MessageLoop::kTaskIdNull, - loop_->PostDelayedTask(Bind(&FakeMessageLoop::BreakLoop, - base::Unretained(loop_.get())), - TimeDelta::FromSeconds(20))); - loop_->Run(); - EXPECT_EQ(0, called); - - loop_->SetFileDescriptorReadiness(fd, MessageLoop::kWatchRead, true); - loop_->Run(); - EXPECT_EQ(1, called); - EXPECT_FALSE(loop_->CancelTask(task_id)); -} - TEST_F(FakeMessageLoopTest, PendingTasksTest) { loop_->PostDelayedTask(base::DoNothing(), TimeDelta::FromSeconds(1)); EXPECT_TRUE(loop_->PendingTasks()); diff --git a/brillo/message_loops/message_loop.h b/brillo/message_loops/message_loop.h index e9f804e..13c4dc2 100644 --- a/brillo/message_loops/message_loop.h +++ b/brillo/message_loops/message_loop.h @@ -6,6 +6,7 @@ #define LIBBRILLO_BRILLO_MESSAGE_LOOPS_MESSAGE_LOOP_H_ #include <string> +#include <utility> #include <base/callback.h> #include <base/location.h> @@ -50,48 +51,20 @@ class BRILLO_EXPORT MessageLoop { // at a later point. // This methond can only be called from the same thread running the main loop. virtual TaskId PostDelayedTask(const base::Location& from_here, - const base::Closure& task, + base::OnceClosure task, base::TimeDelta delay) = 0; // Variant without the Location for easier usage. - TaskId PostDelayedTask(const base::Closure& task, base::TimeDelta delay) { - return PostDelayedTask(base::Location(), task, delay); + TaskId PostDelayedTask(base::OnceClosure task, base::TimeDelta delay) { + return PostDelayedTask(base::Location(), std::move(task), delay); } // A convenience method to schedule a call with no delay. // This methond can only be called from the same thread running the main loop. - TaskId PostTask(const base::Closure& task) { - return PostDelayedTask(task, base::TimeDelta()); + TaskId PostTask(base::OnceClosure task) { + return PostDelayedTask(std::move(task), base::TimeDelta()); } - TaskId PostTask(const base::Location& from_here, - const base::Closure& task) { - return PostDelayedTask(from_here, task, base::TimeDelta()); - } - - // Watch mode flag used to watch for file descriptors. - enum WatchMode { - kWatchRead, - kWatchWrite, - }; - - // Watch a file descriptor |fd| for it to be ready to perform the operation - // passed in |mode| without blocking. When that happens, the |task| closure - // will be executed. If |persistent| is true, the file descriptor will - // continue to be watched and |task| will continue to be called until the task - // is canceled with CancelTask(). - // Returns the TaskId describing this task. In case of error, returns - // kTaskIdNull. - virtual TaskId WatchFileDescriptor(const base::Location& from_here, - int fd, - WatchMode mode, - bool persistent, - const base::Closure& task) = 0; - - // Convenience function to call WatchFileDescriptor() without a location. - TaskId WatchFileDescriptor(int fd, - WatchMode mode, - bool persistent, - const base::Closure& task) { - return WatchFileDescriptor(base::Location(), fd, mode, persistent, task); + TaskId PostTask(const base::Location& from_here, base::OnceClosure task) { + return PostDelayedTask(from_here, std::move(task), base::TimeDelta()); } // Cancel a scheduled task. Returns whether the task was canceled. For diff --git a/brillo/message_loops/message_loop_test.cc b/brillo/message_loops/message_loop_test.cc index 7b57015..86c41a6 100644 --- a/brillo/message_loops/message_loop_test.cc +++ b/brillo/message_loops/message_loop_test.cc @@ -22,12 +22,13 @@ #include <brillo/message_loops/message_loop_utils.h> #include <brillo/unittest_utils.h> -using base::Bind; +using base::BindOnce; +using base::BindRepeating; using base::TimeDelta; namespace { -// Convenience functions for passing to base::Bind. +// Convenience functions for passing to base::Bind{Once,Repeating}. void SetToTrue(bool* b) { *b = true; } @@ -36,10 +37,6 @@ bool ReturnBool(bool *b) { return *b; } -void Increment(int* i) { - (*i)++; -} - } // namespace namespace brillo { @@ -84,7 +81,8 @@ TYPED_TEST(MessageLoopTest, CancelTaskInvalidValuesTest) { TYPED_TEST(MessageLoopTest, PostTaskTest) { bool called = false; - TaskId task_id = this->loop_->PostTask(FROM_HERE, Bind(&SetToTrue, &called)); + TaskId task_id = + this->loop_->PostTask(FROM_HERE, BindOnce(&SetToTrue, &called)); EXPECT_NE(MessageLoop::kTaskIdNull, task_id); MessageLoopRunMaxIterations(this->loop_.get(), 100); EXPECT_TRUE(called); @@ -93,7 +91,8 @@ TYPED_TEST(MessageLoopTest, PostTaskTest) { // Tests that we can cancel tasks right after we schedule them. TYPED_TEST(MessageLoopTest, PostTaskCancelledTest) { bool called = false; - TaskId task_id = this->loop_->PostTask(FROM_HERE, Bind(&SetToTrue, &called)); + TaskId task_id = + this->loop_->PostTask(FROM_HERE, BindOnce(&SetToTrue, &called)); EXPECT_TRUE(this->loop_->CancelTask(task_id)); MessageLoopRunMaxIterations(this->loop_.get(), 100); EXPECT_FALSE(called); @@ -103,12 +102,12 @@ TYPED_TEST(MessageLoopTest, PostTaskCancelledTest) { TYPED_TEST(MessageLoopTest, PostDelayedTaskRunsEventuallyTest) { bool called = false; - TaskId task_id = this->loop_->PostDelayedTask( - FROM_HERE, Bind(&SetToTrue, &called), TimeDelta::FromMilliseconds(50)); + TaskId task_id = + this->loop_->PostDelayedTask(FROM_HERE, BindOnce(&SetToTrue, &called), + TimeDelta::FromMilliseconds(50)); EXPECT_NE(MessageLoop::kTaskIdNull, task_id); - MessageLoopRunUntil(this->loop_.get(), - TimeDelta::FromSeconds(10), - Bind(&ReturnBool, &called)); + MessageLoopRunUntil(this->loop_.get(), TimeDelta::FromSeconds(10), + BindRepeating(&ReturnBool, &called)); // Check that the main loop finished before the 10 seconds timeout, so it // finished due to the callback being called and not due to the timeout. EXPECT_TRUE(called); @@ -122,251 +121,19 @@ TYPED_TEST(MessageLoopTest, PostDelayedTaskWithoutLocation) { EXPECT_EQ(1, MessageLoopRunMaxIterations(this->loop_.get(), 100)); } -TYPED_TEST(MessageLoopTest, WatchForInvalidFD) { - bool called = false; - EXPECT_EQ(MessageLoop::kTaskIdNull, this->loop_->WatchFileDescriptor( - FROM_HERE, -1, MessageLoop::kWatchRead, true, - Bind(&SetToTrue, &called))); - EXPECT_EQ(MessageLoop::kTaskIdNull, this->loop_->WatchFileDescriptor( - FROM_HERE, -1, MessageLoop::kWatchWrite, true, - Bind(&SetToTrue, &called))); - EXPECT_EQ(0, MessageLoopRunMaxIterations(this->loop_.get(), 100)); - EXPECT_FALSE(called); -} - -TYPED_TEST(MessageLoopTest, CancelWatchedFileDescriptor) { - ScopedPipe pipe; - bool called = false; - TaskId task_id = this->loop_->WatchFileDescriptor( - FROM_HERE, pipe.reader, MessageLoop::kWatchRead, true, - Bind(&SetToTrue, &called)); - EXPECT_NE(MessageLoop::kTaskIdNull, task_id); - // The reader end is blocked because we didn't write anything to the writer - // end. - EXPECT_EQ(0, MessageLoopRunMaxIterations(this->loop_.get(), 100)); - EXPECT_FALSE(called); - EXPECT_TRUE(this->loop_->CancelTask(task_id)); -} - -// When you watch a file descriptor for reading, the guaranties are that a -// blocking call to read() on that file descriptor will not block. This should -// include the case when the other end of a pipe is closed or the file is empty. -TYPED_TEST(MessageLoopTest, WatchFileDescriptorTriggersWhenPipeClosed) { - ScopedPipe pipe; - bool called = false; - EXPECT_EQ(0, HANDLE_EINTR(close(pipe.writer))); - pipe.writer = -1; - TaskId task_id = this->loop_->WatchFileDescriptor( - FROM_HERE, pipe.reader, MessageLoop::kWatchRead, true, - Bind(&SetToTrue, &called)); - EXPECT_NE(MessageLoop::kTaskIdNull, task_id); - // The reader end is not blocked because we closed the writer end so a read on - // the reader end would return 0 bytes read. - EXPECT_NE(0, MessageLoopRunMaxIterations(this->loop_.get(), 10)); - EXPECT_TRUE(called); - EXPECT_TRUE(this->loop_->CancelTask(task_id)); -} - -// When a WatchFileDescriptor task is scheduled with |persistent| = true, we -// should keep getting a call whenever the file descriptor is ready. -TYPED_TEST(MessageLoopTest, WatchFileDescriptorPersistently) { - ScopedPipe pipe; - EXPECT_EQ(1, HANDLE_EINTR(write(pipe.writer, "a", 1))); - - int called = 0; - TaskId task_id = this->loop_->WatchFileDescriptor( - FROM_HERE, pipe.reader, MessageLoop::kWatchRead, true, - Bind(&Increment, &called)); - EXPECT_NE(MessageLoop::kTaskIdNull, task_id); - // We let the main loop run for 20 iterations to give it enough iterations to - // verify that our callback was called more than one. We only check that our - // callback is called more than once. - EXPECT_EQ(20, MessageLoopRunMaxIterations(this->loop_.get(), 20)); - EXPECT_LT(1, called); - EXPECT_TRUE(this->loop_->CancelTask(task_id)); -} - -TYPED_TEST(MessageLoopTest, WatchFileDescriptorNonPersistent) { - ScopedPipe pipe; - EXPECT_EQ(1, HANDLE_EINTR(write(pipe.writer, "a", 1))); - - int called = 0; - TaskId task_id = this->loop_->WatchFileDescriptor( - FROM_HERE, pipe.reader, MessageLoop::kWatchRead, false, - Bind(&Increment, &called)); - EXPECT_NE(MessageLoop::kTaskIdNull, task_id); - // We let the main loop run for 20 iterations but we just expect it to run - // at least once. The callback should be called exactly once since we - // scheduled it non-persistently. After it ran, we shouldn't be able to cancel - // this task. - EXPECT_LT(0, MessageLoopRunMaxIterations(this->loop_.get(), 20)); - EXPECT_EQ(1, called); - EXPECT_FALSE(this->loop_->CancelTask(task_id)); -} - -TYPED_TEST(MessageLoopTest, WatchFileDescriptorForReadAndWriteSimultaneously) { - ScopedSocketPair socks; - EXPECT_EQ(1, HANDLE_EINTR(write(socks.right, "a", 1))); - // socks.left should be able to read this "a" and should also be able to write - // without blocking since the kernel has some buffering for it. - - TaskId read_task_id = this->loop_->WatchFileDescriptor( - FROM_HERE, socks.left, MessageLoop::kWatchRead, true, - Bind([] (MessageLoop* loop, TaskId* read_task_id) { - EXPECT_TRUE(loop->CancelTask(*read_task_id)) - << "task_id" << *read_task_id; - }, this->loop_.get(), &read_task_id)); - EXPECT_NE(MessageLoop::kTaskIdNull, read_task_id); - - TaskId write_task_id = this->loop_->WatchFileDescriptor( - FROM_HERE, socks.left, MessageLoop::kWatchWrite, true, - Bind([] (MessageLoop* loop, TaskId* write_task_id) { - EXPECT_TRUE(loop->CancelTask(*write_task_id)); - }, this->loop_.get(), &write_task_id)); - EXPECT_NE(MessageLoop::kTaskIdNull, write_task_id); - - EXPECT_LT(0, MessageLoopRunMaxIterations(this->loop_.get(), 20)); - - EXPECT_FALSE(this->loop_->CancelTask(read_task_id)); - EXPECT_FALSE(this->loop_->CancelTask(write_task_id)); -} - // Test that we can cancel the task we are running, and should just fail. TYPED_TEST(MessageLoopTest, DeleteTaskFromSelf) { bool cancel_result = true; // We would expect this to be false. TaskId task_id; task_id = this->loop_->PostTask( FROM_HERE, - Bind([](bool* cancel_result, MessageLoop* loop, TaskId* task_id) { - *cancel_result = loop->CancelTask(*task_id); - }, &cancel_result, this->loop_.get(), &task_id)); + BindOnce( + [](bool* cancel_result, MessageLoop* loop, TaskId* task_id) { + *cancel_result = loop->CancelTask(*task_id); + }, + &cancel_result, this->loop_.get(), &task_id)); EXPECT_EQ(1, MessageLoopRunMaxIterations(this->loop_.get(), 100)); EXPECT_FALSE(cancel_result); } -// Test that we can cancel a non-persistent file descriptor watching callback, -// which should fail. -TYPED_TEST(MessageLoopTest, DeleteNonPersistenIOTaskFromSelf) { - ScopedPipe pipe; - TaskId task_id = this->loop_->WatchFileDescriptor( - FROM_HERE, pipe.writer, MessageLoop::kWatchWrite, false /* persistent */, - Bind([](MessageLoop* loop, TaskId* task_id) { - EXPECT_FALSE(loop->CancelTask(*task_id)); - *task_id = MessageLoop::kTaskIdNull; - }, this->loop_.get(), &task_id)); - EXPECT_NE(MessageLoop::kTaskIdNull, task_id); - EXPECT_EQ(1, MessageLoopRunMaxIterations(this->loop_.get(), 100)); - EXPECT_EQ(MessageLoop::kTaskIdNull, task_id); -} - -// Test that we can cancel a persistent file descriptor watching callback from -// the same callback. -TYPED_TEST(MessageLoopTest, DeletePersistenIOTaskFromSelf) { - ScopedPipe pipe; - TaskId task_id = this->loop_->WatchFileDescriptor( - FROM_HERE, pipe.writer, MessageLoop::kWatchWrite, true /* persistent */, - Bind([](MessageLoop* loop, TaskId* task_id) { - EXPECT_TRUE(loop->CancelTask(*task_id)); - *task_id = MessageLoop::kTaskIdNull; - }, this->loop_.get(), &task_id)); - EXPECT_NE(MessageLoop::kTaskIdNull, task_id); - EXPECT_EQ(1, MessageLoopRunMaxIterations(this->loop_.get(), 100)); - EXPECT_EQ(MessageLoop::kTaskIdNull, task_id); -} - -// Test that we can cancel several persistent file descriptor watching callbacks -// from a scheduled callback. In the BaseMessageLoop implementation, this code -// will cause us to cancel an IOTask that has a pending delayed task, but -// otherwise is a valid test case on all implementations. -TYPED_TEST(MessageLoopTest, DeleteAllPersistenIOTaskFromSelf) { - const int kNumTasks = 5; - ScopedPipe pipes[kNumTasks]; - TaskId task_ids[kNumTasks]; - - for (int i = 0; i < kNumTasks; ++i) { - task_ids[i] = this->loop_->WatchFileDescriptor( - FROM_HERE, pipes[i].writer, MessageLoop::kWatchWrite, - true /* persistent */, - Bind([] (MessageLoop* loop, TaskId* task_ids) { - for (int j = 0; j < kNumTasks; ++j) { - // Once we cancel all the tasks, none should run, so this code runs - // only once from one callback. - EXPECT_TRUE(loop->CancelTask(task_ids[j])); - task_ids[j] = MessageLoop::kTaskIdNull; - } - }, this->loop_.get(), task_ids)); - } - MessageLoopRunMaxIterations(this->loop_.get(), 100); - for (int i = 0; i < kNumTasks; ++i) { - EXPECT_EQ(MessageLoop::kTaskIdNull, task_ids[i]); - } -} - -// Test that if there are several tasks watching for file descriptors to be -// available or simply waiting in the message loop are fairly scheduled to run. -// In other words, this test ensures that having a file descriptor always -// available doesn't prevent other file descriptors watching tasks or delayed -// tasks to be dispatched, causing starvation. -TYPED_TEST(MessageLoopTest, AllTasksAreEqual) { - int total_calls = 0; - - // First, schedule a repeating timeout callback to run from the main loop. - int timeout_called = 0; - base::Closure timeout_callback; - MessageLoop::TaskId timeout_task; - timeout_callback = base::Bind( - [](MessageLoop* loop, int* timeout_called, int* total_calls, - base::Closure* timeout_callback, MessageLoop::TaskId* timeout_task) { - (*timeout_called)++; - (*total_calls)++; - *timeout_task = loop->PostTask(FROM_HERE, *timeout_callback); - if (*total_calls > 100) - loop->BreakLoop(); - }, - this->loop_.get(), &timeout_called, &total_calls, &timeout_callback, - &timeout_task); - timeout_task = this->loop_->PostTask(FROM_HERE, timeout_callback); - - // Second, schedule several file descriptor watchers. - const int kNumTasks = 3; - ScopedPipe pipes[kNumTasks]; - MessageLoop::TaskId tasks[kNumTasks]; - - int reads[kNumTasks] = {}; - base::Callback<void(int)> fd_callback = base::Bind( - [](MessageLoop* loop, ScopedPipe* pipes, int* reads, - int* total_calls, int i) { - reads[i]++; - (*total_calls)++; - char c; - EXPECT_EQ(1, HANDLE_EINTR(read(pipes[i].reader, &c, 1))); - if (*total_calls > 100) - loop->BreakLoop(); - }, this->loop_.get(), pipes, reads, &total_calls); - - for (int i = 0; i < kNumTasks; ++i) { - tasks[i] = this->loop_->WatchFileDescriptor( - FROM_HERE, pipes[i].reader, MessageLoop::kWatchRead, - true /* persistent */, - Bind(fd_callback, i)); - // Make enough bytes available on each file descriptor. This should not - // block because we set the size of the file descriptor buffer when - // creating it. - std::vector<char> blob(1000, 'a'); - EXPECT_EQ(blob.size(), - HANDLE_EINTR(write(pipes[i].writer, blob.data(), blob.size()))); - } - this->loop_->Run(); - EXPECT_GT(total_calls, 100); - // We run the loop up 100 times and expect each callback to run at least 10 - // times. A good scheduler should balance these callbacks. - EXPECT_GE(timeout_called, 10); - EXPECT_TRUE(this->loop_->CancelTask(timeout_task)); - for (int i = 0; i < kNumTasks; ++i) { - EXPECT_GE(reads[i], 10) << "Reading from pipes[" << i << "], fd " - << pipes[i].reader; - EXPECT_TRUE(this->loop_->CancelTask(tasks[i])); - } -} - } // namespace brillo diff --git a/brillo/message_loops/message_loop_utils.cc b/brillo/message_loops/message_loop_utils.cc index c16f268..0f3214b 100644 --- a/brillo/message_loops/message_loop_utils.cc +++ b/brillo/message_loops/message_loop_utils.cc @@ -9,15 +9,14 @@ namespace brillo { -void MessageLoopRunUntil( - MessageLoop* loop, - base::TimeDelta timeout, - base::Callback<bool()> terminate) { +void MessageLoopRunUntil(MessageLoop* loop, + base::TimeDelta timeout, + base::RepeatingCallback<bool()> terminate) { bool timeout_called = false; MessageLoop::TaskId task_id = loop->PostDelayedTask( FROM_HERE, - base::Bind([](bool* timeout_called) { *timeout_called = true; }, - base::Unretained(&timeout_called)), + base::BindOnce([](bool* timeout_called) { *timeout_called = true; }, + &timeout_called), timeout); while (!timeout_called && (terminate.is_null() || !terminate.Run())) loop->RunOnce(true); diff --git a/brillo/message_loops/message_loop_utils.h b/brillo/message_loops/message_loop_utils.h index d49ebdf..7384ddb 100644 --- a/brillo/message_loops/message_loop_utils.h +++ b/brillo/message_loops/message_loop_utils.h @@ -18,7 +18,7 @@ namespace brillo { BRILLO_EXPORT void MessageLoopRunUntil( MessageLoop* loop, base::TimeDelta timeout, - base::Callback<bool()> terminate); + base::RepeatingCallback<bool()> terminate); // Run the MessageLoop |loop| for up to |iterations| times without blocking. // Return the number of tasks run. diff --git a/brillo/message_loops/mock_message_loop.h b/brillo/message_loops/mock_message_loop.h index c84e585..357ec24 100644 --- a/brillo/message_loops/mock_message_loop.h +++ b/brillo/message_loops/mock_message_loop.h @@ -37,17 +37,9 @@ class BRILLO_EXPORT MockMessageLoop : public MessageLoop { &fake_loop_, static_cast<TaskId(FakeMessageLoop::*)( const base::Location&, - const base::Closure&, + base::OnceClosure, base::TimeDelta)>( &FakeMessageLoop::PostDelayedTask))); - ON_CALL(*this, WatchFileDescriptor( - ::testing::_, ::testing::_, ::testing::_, ::testing::_, ::testing::_)) - .WillByDefault(::testing::Invoke( - &fake_loop_, - static_cast<TaskId(FakeMessageLoop::*)( - const base::Location&, int, WatchMode, bool, - const base::Closure&)>( - &FakeMessageLoop::WatchFileDescriptor))); ON_CALL(*this, CancelTask(::testing::_)) .WillByDefault(::testing::Invoke(&fake_loop_, &FakeMessageLoop::CancelTask)); @@ -59,15 +51,9 @@ class BRILLO_EXPORT MockMessageLoop : public MessageLoop { MOCK_METHOD(TaskId, PostDelayedTask, - (const base::Location&, const base::Closure&, base::TimeDelta), + (const base::Location&, base::OnceClosure, base::TimeDelta), (override)); using MessageLoop::PostDelayedTask; - MOCK_METHOD( - TaskId, - WatchFileDescriptor, - (const base::Location&, int, WatchMode, bool, const base::Closure&), - (override)); - using MessageLoop::WatchFileDescriptor; MOCK_METHOD(bool, CancelTask, (TaskId), (override)); MOCK_METHOD(bool, RunOnce, (bool), (override)); diff --git a/brillo/minijail/minijail.cc b/brillo/minijail/minijail.cc index a08233d..9f88585 100644 --- a/brillo/minijail/minijail.cc +++ b/brillo/minijail/minijail.cc @@ -121,6 +121,23 @@ bool Minijail::RunPipes(struct minijail* jail, #endif // __ANDROID__ } +bool Minijail::RunEnvPipes(struct minijail* jail, + vector<char*> args, + vector<char*> env, + pid_t* pid, + int* stdin, + int* stdout, + int* stderr) { +#if defined(__ANDROID__) + return minijail_run_env_pid_pipes_no_preload(jail, args[0], args.data(), + env.data(), pid, stdin, stdout, + stderr) == 0; +#else + return minijail_run_env_pid_pipes(jail, args[0], args.data(), env.data(), pid, + stdin, stdout, stderr) == 0; +#endif // __ANDROID__ +} + bool Minijail::RunAndDestroy(struct minijail* jail, vector<char*> args, pid_t* pid) { @@ -157,4 +174,16 @@ bool Minijail::RunPipesAndDestroy(struct minijail* jail, return res; } +bool Minijail::RunEnvPipesAndDestroy(struct minijail* jail, + vector<char*> args, + vector<char*> env, + pid_t* pid, + int* stdin, + int* stdout, + int* stderr) { + bool res = RunEnvPipes(jail, args, env, pid, stdin, stdout, stderr); + Destroy(jail); + return res; +} + } // namespace brillo diff --git a/brillo/minijail/minijail.h b/brillo/minijail/minijail.h index c71211d..6cdc7ad 100644 --- a/brillo/minijail/minijail.h +++ b/brillo/minijail/minijail.h @@ -89,6 +89,14 @@ class BRILLO_EXPORT Minijail { int* stdout, int* stderr); + // minijail_run_env_pid_pipes + virtual bool RunEnvPipes(struct minijail* jail, + std::vector<char*> args, + std::vector<char*> env, + pid_t* pid, + int* stdin, + int* stdout, + int* stderr); // Run() and Destroy() virtual bool RunAndDestroy(struct minijail* jail, std::vector<char*> args, @@ -113,6 +121,15 @@ class BRILLO_EXPORT Minijail { int* stdout, int* stderr); + // RunEnvPipes() and Destroy() + virtual bool RunEnvPipesAndDestroy(struct minijail* jail, + std::vector<char*> args, + std::vector<char*> env, + pid_t* pid, + int* stdin, + int* stdout, + int* stderr); + protected: Minijail(); diff --git a/brillo/minijail/mock_minijail.h b/brillo/minijail/mock_minijail.h index 6c95405..21e7ad7 100644 --- a/brillo/minijail/mock_minijail.h +++ b/brillo/minijail/mock_minijail.h @@ -44,6 +44,20 @@ class MockMinijail : public brillo::Minijail { (struct minijail*, std::vector<char*>, int*), (override)); MOCK_METHOD(bool, + RunPipes, + (struct minijail*, std::vector<char*>, pid_t*, int*, int*, int*), + (override)); + MOCK_METHOD(bool, + RunEnvPipes, + (struct minijail*, + std::vector<char*>, + std::vector<char*>, + pid_t*, + int*, + int*, + int*), + (override)); + MOCK_METHOD(bool, RunAndDestroy, (struct minijail*, std::vector<char*>, pid_t*), (override)); @@ -59,6 +73,16 @@ class MockMinijail : public brillo::Minijail { RunPipesAndDestroy, (struct minijail*, std::vector<char*>, pid_t*, int*, int*, int*), (override)); + MOCK_METHOD(bool, + RunEnvPipesAndDestroy, + (struct minijail*, + std::vector<char*>, + std::vector<char*>, + pid_t*, + int*, + int*, + int*), + (override)); private: DISALLOW_COPY_AND_ASSIGN(MockMinijail); diff --git a/brillo/namespaces/OWNERS b/brillo/namespaces/OWNERS new file mode 100644 index 0000000..e96f211 --- /dev/null +++ b/brillo/namespaces/OWNERS @@ -0,0 +1,2 @@ +betuls@chromium.org +jorgelo@chromium.org diff --git a/brillo/namespaces/mock_platform.h b/brillo/namespaces/mock_platform.h new file mode 100644 index 0000000..1b96b46 --- /dev/null +++ b/brillo/namespaces/mock_platform.h @@ -0,0 +1,37 @@ +// Copyright 2020 The Chromium OS Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef LIBBRILLO_BRILLO_NAMESPACES_MOCK_PLATFORM_H_ +#define LIBBRILLO_BRILLO_NAMESPACES_MOCK_PLATFORM_H_ + +#include "brillo/namespaces/platform.h" + +#include <string> + +#include <base/files/file_path.h> +#include <gmock/gmock.h> + +namespace brillo { + +class MockPlatform : public Platform { + public: + MockPlatform() {} + virtual ~MockPlatform() {} + + MOCK_METHOD(bool, Unmount, (const base::FilePath&, bool, bool*), (override)); + MOCK_METHOD(pid_t, Fork, (), (override)); + MOCK_METHOD(pid_t, Waitpid, (pid_t, int*), (override)); + MOCK_METHOD(int, + Mount, + (const std::string&, + const std::string&, + const std::string&, + uint64_t, + const void*), + (override)); +}; + +} // namespace brillo + +#endif // LIBBRILLO_BRILLO_NAMESPACES_MOCK_PLATFORM_H_ diff --git a/brillo/namespaces/mount_namespace.cc b/brillo/namespaces/mount_namespace.cc new file mode 100644 index 0000000..1944983 --- /dev/null +++ b/brillo/namespaces/mount_namespace.cc @@ -0,0 +1,114 @@ +// Copyright 2020 The Chromium OS Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Contains the implementation of class MountNamespace for libbrillo. + +#include "brillo/namespaces/mount_namespace.h" + +#include <sched.h> +#include <sys/mount.h> +#include <sys/types.h> + +#include <string> + +#include <base/files/file_path.h> +#include <base/files/file_util.h> +#include <base/logging.h> +#include <base/strings/stringprintf.h> +#include <brillo/namespaces/platform.h> + +namespace brillo { +MountNamespace::MountNamespace(const base::FilePath& ns_path, + Platform* platform) + : ns_path_(ns_path), platform_(platform), exists_(false) {} + +MountNamespace::~MountNamespace() { + if (exists_) + Destroy(); +} + +bool MountNamespace::Create() { + if (platform_->FileSystemIsNsfs(ns_path_)) { + LOG(ERROR) << "Mount namespace at " << ns_path_.value() + << " already exists."; + return false; + } + int fd_mounted[2]; + int fd_unshared[2]; + char byte = '\0'; + if (pipe(fd_mounted) != 0) { + PLOG(ERROR) << "Cannot create mount signalling pipe"; + return false; + } + if (pipe(fd_unshared) != 0) { + PLOG(ERROR) << "Cannot create unshare signalling pipe"; + return false; + } + pid_t pid = platform_->Fork(); + if (pid < 0) { + PLOG(ERROR) << "Fork failed"; + } else if (pid == 0) { + // Child. + close(fd_mounted[1]); + close(fd_unshared[0]); + if (unshare(CLONE_NEWNS) != 0) { + PLOG(ERROR) << "unshare(CLONE_NEWNS) failed"; + exit(1); + } + base::WriteFileDescriptor(fd_unshared[1], &byte, 1); + base::ReadFromFD(fd_mounted[0], &byte, 1); + exit(0); + } else { + // Parent. + close(fd_mounted[0]); + close(fd_unshared[1]); + std::string proc_ns_path = base::StringPrintf("/proc/%d/ns/mnt", pid); + bool mount_success = true; + base::ReadFromFD(fd_unshared[0], &byte, 1); + if (platform_->Mount(proc_ns_path, ns_path_.value(), "", MS_BIND) != 0) { + PLOG(ERROR) << "Mount(" << proc_ns_path << ", " << ns_path_.value() + << ", MS_BIND) failed"; + mount_success = false; + } + base::WriteFileDescriptor(fd_mounted[1], &byte, 1); + + int status; + if (platform_->Waitpid(pid, &status) < 0) { + PLOG(ERROR) << "waitpid(" << pid << ") failed"; + return false; + } + if (!WIFEXITED(status)) { + LOG(ERROR) << "Child process did not exit normally."; + } else if (WEXITSTATUS(status) != 0) { + LOG(ERROR) << "Child process failed."; + } else { + exists_ = mount_success; + } + } + return exists_; +} + +bool MountNamespace::Destroy() { + if (!exists_) { + LOG(ERROR) << "Mount namespace at " << ns_path_.value() + << "does not exist, cannot destroy"; + return false; + } + bool was_busy; + if (!platform_->Unmount(ns_path_, false /*lazy*/, &was_busy)) { + PLOG(ERROR) << "Failed to unmount " << ns_path_.value(); + if (was_busy) { + LOG(ERROR) << ns_path_.value().c_str() << " was busy"; + } + // If Unmount() fails, keep the object valid by keeping |exists_| + // set to true. + return false; + } else { + VLOG(1) << "Unmounted namespace at " << ns_path_.value(); + } + exists_ = false; + return true; +} + +} // namespace brillo diff --git a/brillo/namespaces/mount_namespace.h b/brillo/namespaces/mount_namespace.h new file mode 100644 index 0000000..bfadff0 --- /dev/null +++ b/brillo/namespaces/mount_namespace.h @@ -0,0 +1,70 @@ +// Copyright 2020 The Chromium OS Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef LIBBRILLO_BRILLO_NAMESPACES_MOUNT_NAMESPACE_H_ +#define LIBBRILLO_BRILLO_NAMESPACES_MOUNT_NAMESPACE_H_ + +#include "brillo/namespaces/platform.h" + +#include <base/files/file_path.h> +#include <base/macros.h> +#include <brillo/brillo_export.h> + +namespace brillo { + +class BRILLO_EXPORT MountNamespaceInterface { + // An interface declaring the basic functionality of a mount namespace bound + // to a specific path. This basic functionality consists of reporting the + // namespace path. + public: + virtual ~MountNamespaceInterface() = default; + + virtual const base::FilePath& path() const = 0; +}; + +class BRILLO_EXPORT UnownedMountNamespace : public MountNamespaceInterface { + // A class to store and retrieve the path of a persistent namespace. This + // class doesn't create nor destroy the namespace. + public: + explicit UnownedMountNamespace(const base::FilePath& ns_path) + : ns_path_(ns_path) {} + + ~UnownedMountNamespace() override; + + const base::FilePath& path() const override { return ns_path_; } + + private: + base::FilePath ns_path_; + + DISALLOW_COPY_AND_ASSIGN(UnownedMountNamespace); +}; + +class BRILLO_EXPORT MountNamespace : public MountNamespaceInterface { + // A class to create a persistent mount namespace bound to a specific path. + // A new mount namespace is unshared from the mount namespace of the calling + // process when Create() is called; the namespace of the calling process + // remains unchanged. Recurring creation on a path is not allowed. + // + // Given that we cannot ensure that creation always succeeds this class is not + // fully RAII, but once the namespace is created (with Create()), it will be + // destroyed when the object goes out of scope. + public: + MountNamespace(const base::FilePath& ns_path, Platform* platform); + ~MountNamespace() override; + + bool Create(); + bool Destroy(); + const base::FilePath& path() const override { return ns_path_; } + + private: + base::FilePath ns_path_; + Platform* platform_; + bool exists_; + + DISALLOW_COPY_AND_ASSIGN(MountNamespace); +}; + +} // namespace brillo + +#endif // LIBBRILLO_BRILLO_NAMESPACES_MOUNT_NAMESPACE_H_ diff --git a/brillo/namespaces/mount_namespace_test.cc b/brillo/namespaces/mount_namespace_test.cc new file mode 100644 index 0000000..1bfa038 --- /dev/null +++ b/brillo/namespaces/mount_namespace_test.cc @@ -0,0 +1,92 @@ +// Copyright 2020 The Chromium OS Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "brillo/namespaces/mock_platform.h" +#include "brillo/namespaces/mount_namespace.h" +#include "brillo/namespaces/platform.h" + +#include <unistd.h> + +#include <memory> + +#include <base/files/file_path.h> +#include <gmock/gmock.h> +#include <gtest/gtest.h> + +using ::testing::_; +using ::testing::DoAll; +using ::testing::NiceMock; +using ::testing::Return; +using ::testing::SetArgPointee; + +namespace brillo { + +class MountNamespaceTest : public ::testing::Test { + public: + MountNamespaceTest() {} + ~MountNamespaceTest() {} + void SetUp() {} + + void TearDown() {} + + protected: + NiceMock<MockPlatform> platform_; + + private: + DISALLOW_COPY_AND_ASSIGN(MountNamespaceTest); +}; + +TEST_F(MountNamespaceTest, CreateNamespace) { + std::unique_ptr<MountNamespace> ns = + std::make_unique<MountNamespace>(base::FilePath(), &platform_); + EXPECT_CALL(platform_, Fork()).WillOnce(Return(1)); + EXPECT_CALL(platform_, Mount(_, _, _, _, _)).WillOnce(Return(0)); + EXPECT_CALL(platform_, Waitpid(_, _)) + .WillOnce(DoAll(SetArgPointee<1>(0x00000000), Return(0))); + EXPECT_TRUE(ns->Create()); + EXPECT_CALL(platform_, Unmount(ns->path(), _, _)).WillOnce(Return(true)); +} + +TEST_F(MountNamespaceTest, CreateNamespaceFailedOnWaitpid) { + std::unique_ptr<MountNamespace> ns = + std::make_unique<MountNamespace>(base::FilePath(), &platform_); + EXPECT_CALL(platform_, Fork()).WillOnce(Return(1)); + EXPECT_CALL(platform_, Mount(_, _, _, _, _)).WillOnce(Return(0)); + EXPECT_CALL(platform_, Waitpid(_, _)).WillOnce(Return(-1)); + EXPECT_FALSE(ns->Create()); +} + +TEST_F(MountNamespaceTest, CreateNamespaceFailedOnMount) { + std::unique_ptr<MountNamespace> ns = + std::make_unique<MountNamespace>(base::FilePath(), &platform_); + EXPECT_CALL(platform_, Fork()).WillOnce(Return(1)); + EXPECT_CALL(platform_, Mount(_, _, _, _, _)).WillOnce(Return(-1)); + EXPECT_FALSE(ns->Create()); +} + +TEST_F(MountNamespaceTest, CreateNamespaceFailedOnStatus) { + std::unique_ptr<MountNamespace> ns = + std::make_unique<MountNamespace>(base::FilePath(), &platform_); + EXPECT_CALL(platform_, Fork()).WillOnce(Return(1)); + EXPECT_CALL(platform_, Mount(_, _, _, _, _)).WillOnce(Return(0)); + EXPECT_CALL(platform_, Waitpid(_, _)) + .WillOnce(DoAll(SetArgPointee<1>(0xFFFFFFFF), Return(0))); + EXPECT_FALSE(ns->Create()); +} + +TEST_F(MountNamespaceTest, DestroyAfterUnmountFailsAndUnmountSucceeds) { + std::unique_ptr<MountNamespace> ns = + std::make_unique<MountNamespace>(base::FilePath(), &platform_); + EXPECT_CALL(platform_, Fork()).WillOnce(Return(1)); + EXPECT_CALL(platform_, Mount(_, _, _, _, _)).WillOnce(Return(0)); + EXPECT_CALL(platform_, Waitpid(_, _)) + .WillOnce(DoAll(SetArgPointee<1>(0x00000000), Return(0))); + EXPECT_TRUE(ns->Create()); + EXPECT_CALL(platform_, Unmount(ns->path(), _, _)).WillOnce(Return(false)); + EXPECT_FALSE(ns->Destroy()); + EXPECT_CALL(platform_, Unmount(ns->path(), _, _)).WillOnce(Return(true)); + EXPECT_TRUE(ns->Destroy()); +} + +} // namespace brillo diff --git a/brillo/namespaces/platform.cc b/brillo/namespaces/platform.cc new file mode 100644 index 0000000..5fe9140 --- /dev/null +++ b/brillo/namespaces/platform.cc @@ -0,0 +1,75 @@ +// Copyright 2020 The Chromium OS Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Contains the implementation of class Platform for libbrillo. + +#include "brillo/namespaces/platform.h" + +#include <errno.h> +#include <linux/magic.h> +#include <stdio.h> +#include <sys/mount.h> +#include <sys/stat.h> +#include <sys/vfs.h> +#include <sys/wait.h> + +#include <memory> + +#include <base/files/file_path.h> + +using base::FilePath; + +namespace brillo { + +Platform::Platform() {} + +Platform::~Platform() {} + +bool Platform::FileSystemIsNsfs(const FilePath& ns_path) { + struct statfs buff; + if (statfs(ns_path.value().c_str(), &buff) < 0) { + PLOG(ERROR) << "Statfs() error for " << ns_path.value(); + return false; + } + if ((uint64_t)buff.f_type == NSFS_MAGIC) { + return true; + } + return false; +} + +bool Platform::Unmount(const FilePath& path, bool lazy, bool* was_busy) { + int flags = 0; + if (lazy) { + flags = MNT_DETACH; + } + if (umount2(path.value().c_str(), flags) != 0) { + if (was_busy) { + *was_busy = (errno == EBUSY); + } + return false; + } + if (was_busy) { + *was_busy = false; + } + return true; +} + +int Platform::Mount(const std::string& source, + const std::string& target, + const std::string& fs_type, + uint64_t mount_flags, + const void* data) { + return mount(source.c_str(), target.c_str(), fs_type.c_str(), mount_flags, + data); +} + +pid_t Platform::Fork() { + return fork(); +} + +pid_t Platform::Waitpid(pid_t pid, int* status) { + return waitpid(pid, status, 0); +} + +} // namespace brillo diff --git a/brillo/namespaces/platform.h b/brillo/namespaces/platform.h new file mode 100644 index 0000000..6ef6a73 --- /dev/null +++ b/brillo/namespaces/platform.h @@ -0,0 +1,71 @@ +// Copyright 2020 The Chromium OS Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef LIBBRILLO_BRILLO_NAMESPACES_PLATFORM_H_ +#define LIBBRILLO_BRILLO_NAMESPACES_PLATFORM_H_ + +#include <sys/types.h> + +#include <memory> +#include <string> + +#include <base/files/file_path.h> +#include <base/macros.h> +#include <brillo/brillo_export.h> + +namespace brillo { +// Platform specific routines abstraction layer. +// Also helps us to be able to mock them in tests. +class BRILLO_EXPORT Platform { + public: + Platform(); + + virtual ~Platform(); + // Calls the platform fork() function and returns the pid returned + // by fork(). + virtual pid_t Fork(); + + // Calls the platform unmount. + // + // Parameters + // path - The path to unmount + // lazy - Whether to call a lazy unmount + // was_busy (OUT) - Set to true on return if the mount point was busy + virtual bool Unmount(const base::FilePath& path, bool lazy, bool* was_busy); + + // Calls the platform mount. + // + // Parameters + // source - The path to mount from + // target - The path to mount to + // fs_type - File system type of the mount + // mount_flags - Flags spesifying the type of the mount operation + // data - Mount options + virtual int Mount(const std::string& source, + const std::string& target, + const std::string& fs_type, + uint64_t mount_flags, + const void* = nullptr); + + // Checks the file system type of the |path| and returns true if the + // filesystem type is nsfs. + // + // Parameters + // path - The path to check the file system type + virtual bool FileSystemIsNsfs(const base::FilePath& path); + + // Calls the platform waitpid() function and returns the value returned by + // waitpid(). + // + // Parameters + // pid - The child pid to be waited on + // status (OUT)- Termination status of a child process. + virtual pid_t Waitpid(pid_t pid, int* status); + + DISALLOW_COPY_AND_ASSIGN(Platform); +}; + +} // namespace brillo + +#endif // LIBBRILLO_BRILLO_NAMESPACES_PLATFORM_H_ diff --git a/brillo/scoped_mount_namespace.cc b/brillo/scoped_mount_namespace.cc index 0f35e82..09f3c75 100644 --- a/brillo/scoped_mount_namespace.cc +++ b/brillo/scoped_mount_namespace.cc @@ -38,7 +38,7 @@ std::unique_ptr<ScopedMountNamespace> ScopedMountNamespace::CreateForPid( // static std::unique_ptr<ScopedMountNamespace> ScopedMountNamespace::CreateFromPath( - base::FilePath ns_path) { + const base::FilePath& ns_path) { base::ScopedFD original_mount_namespace_fd( HANDLE_EINTR(open(kCurrentMountNamespacePath, O_RDONLY))); if (!original_mount_namespace_fd.is_valid()) { diff --git a/brillo/scoped_mount_namespace.h b/brillo/scoped_mount_namespace.h index f360221..e029d78 100644 --- a/brillo/scoped_mount_namespace.h +++ b/brillo/scoped_mount_namespace.h @@ -28,7 +28,7 @@ class BRILLO_EXPORT ScopedMountNamespace { // Enters the mount namespace identified by |path| and returns a unique_ptr // that restores the original mount namespace when it goes out of scope. static std::unique_ptr<ScopedMountNamespace> CreateFromPath( - base::FilePath ns_path); + const base::FilePath& ns_path); explicit ScopedMountNamespace(base::ScopedFD mount_namespace_fd); ~ScopedMountNamespace(); diff --git a/brillo/secure_allocator.h b/brillo/secure_allocator.h index 0cbb8d9..de0b348 100644 --- a/brillo/secure_allocator.h +++ b/brillo/secure_allocator.h @@ -5,13 +5,51 @@ #ifndef LIBBRILLO_BRILLO_SECURE_ALLOCATOR_H_ #define LIBBRILLO_BRILLO_SECURE_ALLOCATOR_H_ +#include <errno.h> +#include <sys/mman.h> +#include <unistd.h> + +#include <limits> #include <memory> +#include <base/callback_helpers.h> +#include <base/logging.h> #include <brillo/brillo_export.h> namespace brillo { // SecureAllocator is a stateless derivation of std::allocator that clears -// the contents of the object on deallocation. +// the contents of the object on deallocation. Additionally, to prevent the +// memory from being leaked, we use the following defensive mechanisms: +// +// 1. Use page-aligned memory so that it can be locked (therefore, use mmap() +// instead of malloc()). Note that mlock()s are not inherited over fork(), +// +// 2. Always allocate memory in multiples of pages: this adds a memory overhead +// of ~1 page for each object. Moreover, the extra memory is not available +// for the allocated object to expand into: the container expects that the +// memory allocated to it matches the size set in reserve(). +// TODO(sarthakkukreti): Figure out if it is possible to propagate the real +// capacity to the container without an intrusive change to the STL. +// [Example: allow __recommend() override in allocators for containers.] +// +// 3. Mark the memory segments as undumpable, unmergeable. +// +// 4. Use MADV_WIPEONFORK: +// this results in a new anonymous vma instead of copying over the contents +// of the secure object after a fork(). By default [MADV_DOFORK], the vma is +// marked as copy-on-write, and the first process which writes to the secure +// object after fork get a new copy. This may break the security guarantees +// setup above. Another alternative is to use MADV_DONTFORK which results in +// the memory mapping not getting copied over to child process at all: this +// may result in cases where if the child process gets segmentation faults +// on attempts to access virtual addresses in the secure object's address +// range, +// +// With MADV_WIPEONFORK, the child processes can access the secure object +// memory safely, but the contents of the secure object appear as zero to +// the child process. Note that threads share the virtual address space and +// secure objects would be transparent across threads. +// TODO(sarthakkukreti): Figure out patterns to pass secure data over fork(). template <typename T> class BRILLO_PRIVATE SecureAllocator : public std::allocator<T> { public: @@ -19,21 +57,118 @@ class BRILLO_PRIVATE SecureAllocator : public std::allocator<T> { using typename std::allocator<T>::size_type; using typename std::allocator<T>::value_type; - // Implicit std::allocator constructors. + // Constructors that wrap over std::allocator. + // Make sure that the allocator's static members are only allocated once. + SecureAllocator() noexcept : std::allocator<T>() {} + SecureAllocator(const SecureAllocator& other) noexcept + : std::allocator<T>(other) {} + + template <class U> + SecureAllocator(const SecureAllocator<U>& other) noexcept + : std::allocator<T>(other) {} template <typename U> struct rebind { typedef SecureAllocator<U> other; }; - // Allocation/deallocation: use the std::allocation functions but make sure - // that on deallocation, the contents of the element are cleared out. - pointer allocate(size_type n, pointer = {}) { - return std::allocator<T>::allocate(n); + // Return cached max_size. Deprecated in C++17, removed in C++20. + size_type max_size() const { return max_size_; } + + // Allocation: allocate ceil(size/pagesize) for holding the data. + pointer allocate(size_type n, pointer hint = nullptr) { + pointer buffer = nullptr; + // Check if n can be theoretically allocated. + CHECK_LT(n, max_size()); + + // std::allocator is expected to throw a std::bad_alloc on failing to + // allocate the memory correctly. Instead of returning a nullptr, which + // confuses the standard template library, use CHECK(false) to crash on + // the failure path. + base::ScopedClosureRunner fail_on_allocation_error(base::Bind([]() { + PLOG(ERROR) << "Failed to allocate secure memory"; + CHECK(false); + })); + + // Check if n = 0: there's nothing to allocate; + if (n == 0) + return nullptr; + + // Calculate the page-aligned buffer size. + size_type buffer_size = CalculatePageAlignedBufferSize(n); + + // Memory locking granularity is per-page: mmap ceil(size/page size) pages. + buffer = reinterpret_cast<pointer>( + mmap(nullptr, buffer_size, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS, -1, 0)); + if (buffer == MAP_FAILED) + return nullptr; + + // Lock buffer into physical memory. + if (mlock(buffer, buffer_size)) { + CHECK_NE(errno, ENOMEM) << "It is likely that SecureAllocator have " + "exceeded the RLIMIT_MEMLOCK limit"; + return nullptr; + } + + // Mark memory as non dumpable in a core dump. + if (madvise(buffer, buffer_size, MADV_DONTDUMP)) + return nullptr; + + // Mark memory as non mergeable with another page, even if the contents + // are the same. + if (madvise(buffer, buffer_size, MADV_UNMERGEABLE)) { + // MADV_UNMERGEABLE is only available if the kernel has been configured + // with CONFIG_KSM set. If the CONFIG_KSM flag has not been set, then + // pages are not mergeable so this madvise option is not necessary. + // + // In the case where CONFIG_KSM is not set, EINVAL is the error set. + // Since this error value is expected in some cases, we don't return a + // nullptr. + if (errno != EINVAL) + return nullptr; + } + + // Make this mapping available to child processes but don't copy data from + // the secure object's pages during fork. With MADV_DONTFORK, the + // vma is not mapped in the child process which leads to segmentation + // faults if the child process tries to access this address. For example, + // if the parent process creates a SecureObject, forks() and the child + // process tries to call the destructor at the virtual address. + if (madvise(buffer, buffer_size, MADV_WIPEONFORK)) + return nullptr; + + ignore_result(fail_on_allocation_error.Release()); + + // Allocation was successful. + return buffer; + } + + // Destroy object before deallocation. Deprecated in C++17, removed in C++20. + // After destroying the object, clear the contents of where the object was + // stored. + template <class U> + void destroy(U* p) { + // Return if the pointer is invalid. + if (!p) + return; + std::allocator<U>::destroy(p); + clear_contents(p, sizeof(U)); } virtual void deallocate(pointer p, size_type n) { - clear_contents(p, n * sizeof(value_type)); - std::allocator<T>::deallocate(p, n); + // Check if n can be theoretically deallocated. + CHECK_LT(n, max_size()); + + // Check if n = 0 or p is a nullptr: there's nothing to deallocate; + if (n == 0 || !p) + return; + + // Calculate the page-aligned buffer size. + size_type buffer_size = CalculatePageAlignedBufferSize(n); + + clear_contents(p, buffer_size); + munlock(p, buffer_size); + munmap(p, buffer_size); } protected: @@ -54,8 +189,53 @@ class BRILLO_PRIVATE SecureAllocator : public std::allocator<T> { } #undef __attribute_no_opt + + private: + // Calculate the page-aligned buffer size. + size_t CalculatePageAlignedBufferSize(size_type n) { + size_type real_size = n * sizeof(value_type); + size_type page_aligned_remainder = real_size % page_size_; + size_type padding = + page_aligned_remainder != 0 ? page_size_ - page_aligned_remainder : 0; + return real_size + padding; + } + + static size_t CalculatePageSize() { + long ret = sysconf(_SC_PAGESIZE); // NOLINT [runtime/int] + + // Initialize page size. + CHECK_GT(ret, 0L); + return ret; + } + + // Since the allocator reuses page size and max size consistently, + // cache these values initially and reuse. + static size_t GetMaxSizeForType(size_t page_size) { + // Initialize max size that can be theoretically allocated. + // Calculate the max size that is page-aligned. + size_t max_theoretical_size = std::numeric_limits<size_type>::max(); + size_t max_page_aligned_size = + max_theoretical_size - (max_theoretical_size % page_size); + + return max_page_aligned_size / sizeof(value_type); + } + + // Page size on system. + static const size_type page_size_; + // Max theoretical count for type on system. + static const size_type max_size_; }; +// Inline definitions are only allowed for static const members with integral +// constexpr initializers, define static members of SecureAllocator types here. +template <typename T> +const typename SecureAllocator<T>::size_type SecureAllocator<T>::page_size_ = + SecureAllocator<T>::CalculatePageSize(); + +template <typename T> +const typename SecureAllocator<T>::size_type SecureAllocator<T>::max_size_ = + SecureAllocator<T>::GetMaxSizeForType(SecureAllocator<T>::page_size_); + } // namespace brillo #endif // LIBBRILLO_BRILLO_SECURE_ALLOCATOR_H_ diff --git a/brillo/secure_blob_test.cc b/brillo/secure_blob_test.cc index 75f3cfb..7242e86 100644 --- a/brillo/secure_blob_test.cc +++ b/brillo/secure_blob_test.cc @@ -234,16 +234,18 @@ class TestSecureAllocator : public SecureAllocator<T> { public: using typename SecureAllocator<T>::pointer; using typename SecureAllocator<T>::size_type; + using typename SecureAllocator<T>::value_type; int GetErasedCount() { return erased_count; } protected: void clear_contents(pointer p, size_type n) override { SecureAllocator<T>::clear_contents(p, n); + unsigned char *v = reinterpret_cast<unsigned char*>(p); for (int i = 0; i < n; i++) { - EXPECT_EQ(p[i], 0); + EXPECT_EQ(v[i], 0); + erased_count++; } - erased_count++; } private: @@ -259,7 +261,54 @@ TEST(SecureAllocator, ErasureOnDeallocation) { // Deallocate memory; the mock class should check for cleared data. e.deallocate(test_string_addr, 15); - EXPECT_EQ(e.GetErasedCount(), 1); + // The deallocation should have traversed the complete page. + EXPECT_EQ(e.GetErasedCount(), 4096); } +TEST(SecureAllocator, MultiPageCorrectness) { + // Make sure that the contents are cleared on deallocation. + TestSecureAllocator<uint64_t> e; + + // Allocate 4100*8 bytes. + uint64_t *test_array = e.allocate(4100); + + // Check if the space was correctly allocated for long long. + for (int i = 0; i < 4100; i++) + test_array[i] = 0xF0F0F0F0F0F0F0F0; + + // Deallocate memory; the mock class should check for cleared data. + e.deallocate(test_array, 4100); + // 36864 bytes is the next highest size that is a multiple of the page size. + EXPECT_EQ(e.GetErasedCount(), 36864); +} + +// DeathTests fork a new process and check how it proceeds. Take advantage +// of this and check if the value of SecureString is passed on to +// forked children. +#if GTEST_IS_THREADSAFE +// Check if the contents of the container are zeroed out. +void CheckPropagationOnFork(const brillo::SecureBlob& forked_blob, + const Blob& reference) { + LOG(INFO) << forked_blob.to_string(); + for (int i = 0; i < forked_blob.size(); i++) { + CHECK_NE(reference[i], forked_blob[i]); + CHECK_EQ(forked_blob[i], 0); + } + exit(0); +} + +TEST(SecureAllocatorDeathTest, ErasureOnFork) { + Blob reference = BlobFromString("Test String"); + SecureBlob erasable_blob(reference.begin(), reference.end()); + + EXPECT_EXIT(CheckPropagationOnFork(erasable_blob, reference), + ::testing::ExitedWithCode(0), ""); + + // In the original process, check the SecureBlob to see if it has not + // changed. + for (int i = 0; i < erasable_blob.size(); i++) + EXPECT_EQ(erasable_blob[i], reference[i]); +} +#endif // GTEST_IS_THREADSAFE + } // namespace brillo diff --git a/brillo/streams/file_stream.cc b/brillo/streams/file_stream.cc index db22192..70b25dd 100644 --- a/brillo/streams/file_stream.cc +++ b/brillo/streams/file_stream.cc @@ -12,6 +12,7 @@ #include <utility> #include <base/bind.h> +#include <base/files/file_descriptor_watcher_posix.h> #include <base/files/file_util.h> #include <base/posix/eintr_wrapper.h> #include <brillo/errors/error_codes.h> @@ -86,15 +87,11 @@ class FileDescriptor : public FileStream::FileDescriptorInterface { ErrorPtr* error) override { if (stream_utils::IsReadAccessMode(mode)) { CHECK(read_data_callback_.is_null()); - MessageLoop::current()->CancelTask(read_watcher_); - read_watcher_ = MessageLoop::current()->WatchFileDescriptor( - FROM_HERE, + read_watcher_ = base::FileDescriptorWatcher::WatchReadable( fd_, - MessageLoop::WatchMode::kWatchRead, - false, // persistent - base::Bind(&FileDescriptor::OnFileCanReadWithoutBlocking, - base::Unretained(this))); - if (read_watcher_ == MessageLoop::kTaskIdNull) { + base::BindRepeating(&FileDescriptor::OnReadable, + base::Unretained(this))); + if (!read_watcher_) { Error::AddTo(error, FROM_HERE, errors::stream::kDomain, errors::stream::kInvalidParameter, "File descriptor doesn't support watching for reading."); @@ -104,15 +101,11 @@ class FileDescriptor : public FileStream::FileDescriptorInterface { } if (stream_utils::IsWriteAccessMode(mode)) { CHECK(write_data_callback_.is_null()); - MessageLoop::current()->CancelTask(write_watcher_); - write_watcher_ = MessageLoop::current()->WatchFileDescriptor( - FROM_HERE, + write_watcher_ = base::FileDescriptorWatcher::WatchWritable( fd_, - MessageLoop::WatchMode::kWatchWrite, - false, // persistent - base::Bind(&FileDescriptor::OnFileCanWriteWithoutBlocking, - base::Unretained(this))); - if (write_watcher_ == MessageLoop::kTaskIdNull) { + base::BindRepeating(&FileDescriptor::OnWritable, + base::Unretained(this))); + if (!write_watcher_) { Error::AddTo(error, FROM_HERE, errors::stream::kDomain, errors::stream::kInvalidParameter, "File descriptor doesn't support watching for writing."); @@ -157,31 +150,26 @@ class FileDescriptor : public FileStream::FileDescriptorInterface { void CancelPendingAsyncOperations() override { read_data_callback_.Reset(); - if (read_watcher_ != MessageLoop::kTaskIdNull) { - MessageLoop::current()->CancelTask(read_watcher_); - read_watcher_ = MessageLoop::kTaskIdNull; - } - + read_watcher_ = nullptr; write_data_callback_.Reset(); - if (write_watcher_ != MessageLoop::kTaskIdNull) { - MessageLoop::current()->CancelTask(write_watcher_); - write_watcher_ = MessageLoop::kTaskIdNull; - } + write_watcher_ = nullptr; } // Called from the brillo::MessageLoop when the file descriptor is available // for reading. - void OnFileCanReadWithoutBlocking() { + void OnReadable() { CHECK(!read_data_callback_.is_null()); - DataCallback cb = read_data_callback_; - read_data_callback_.Reset(); + + read_watcher_ = nullptr; + DataCallback cb = std::move(read_data_callback_); cb.Run(Stream::AccessMode::READ); } - void OnFileCanWriteWithoutBlocking() { + void OnWritable() { CHECK(!write_data_callback_.is_null()); - DataCallback cb = write_data_callback_; - write_data_callback_.Reset(); + + write_watcher_ = nullptr; + DataCallback cb = std::move(write_data_callback_); cb.Run(Stream::AccessMode::WRITE); } @@ -200,9 +188,9 @@ class FileDescriptor : public FileStream::FileDescriptorInterface { DataCallback read_data_callback_; DataCallback write_data_callback_; - // MessageLoop tasks monitoring read/write operations on the file descriptor. - MessageLoop::TaskId read_watcher_{MessageLoop::kTaskIdNull}; - MessageLoop::TaskId write_watcher_{MessageLoop::kTaskIdNull}; + // Monitoring read/write operations on the file descriptor. + std::unique_ptr<base::FileDescriptorWatcher::Controller> read_watcher_; + std::unique_ptr<base::FileDescriptorWatcher::Controller> write_watcher_; DISALLOW_COPY_AND_ASSIGN(FileDescriptor); }; diff --git a/brillo/streams/file_stream_test.cc b/brillo/streams/file_stream_test.cc index 23ef64c..36bad07 100644 --- a/brillo/streams/file_stream_test.cc +++ b/brillo/streams/file_stream_test.cc @@ -25,6 +25,7 @@ #include <gmock/gmock.h> #include <gtest/gtest.h> +using testing::DoAll; using testing::InSequence; using testing::Return; using testing::ReturnArg; diff --git a/brillo/streams/input_stream_set.cc b/brillo/streams/input_stream_set.cc index 0ceb5b4..847bf05 100644 --- a/brillo/streams/input_stream_set.cc +++ b/brillo/streams/input_stream_set.cc @@ -172,7 +172,7 @@ bool InputStreamSet::WaitForData( return stream->WaitForData(mode, callback, error); } - MessageLoop::current()->PostTask(FROM_HERE, base::Bind(callback, mode)); + MessageLoop::current()->PostTask(FROM_HERE, base::BindOnce(callback, mode)); return true; } diff --git a/brillo/streams/memory_stream.cc b/brillo/streams/memory_stream.cc index 54f127a..f4f9cca 100644 --- a/brillo/streams/memory_stream.cc +++ b/brillo/streams/memory_stream.cc @@ -185,7 +185,7 @@ bool MemoryStream::CheckContainer(ErrorPtr* error) const { bool MemoryStream::WaitForData(AccessMode mode, const base::Callback<void(AccessMode)>& callback, ErrorPtr* /* error */) { - MessageLoop::current()->PostTask(FROM_HERE, base::Bind(callback, mode)); + MessageLoop::current()->PostTask(FROM_HERE, base::BindOnce(callback, mode)); return true; } diff --git a/brillo/streams/stream.cc b/brillo/streams/stream.cc index 6a40c00..80f2df4 100644 --- a/brillo/streams/stream.cc +++ b/brillo/streams/stream.cc @@ -213,8 +213,9 @@ bool Stream::ReadAsyncImpl( if (force_async_callback) { MessageLoop::current()->PostTask( FROM_HERE, - base::Bind(&Stream::OnReadAsyncDone, weak_ptr_factory_.GetWeakPtr(), - success_callback, read, eos)); + base::BindOnce(&Stream::OnReadAsyncDone, + weak_ptr_factory_.GetWeakPtr(), + success_callback, read, eos)); } else { is_async_read_pending_ = false; success_callback.Run(read, eos); @@ -277,8 +278,9 @@ bool Stream::WriteAsyncImpl( if (force_async_callback) { MessageLoop::current()->PostTask( FROM_HERE, - base::Bind(&Stream::OnWriteAsyncDone, weak_ptr_factory_.GetWeakPtr(), - success_callback, written)); + base::BindOnce(&Stream::OnWriteAsyncDone, + weak_ptr_factory_.GetWeakPtr(), + success_callback, written)); } else { is_async_write_pending_ = false; success_callback.Run(written); diff --git a/brillo/streams/stream_utils.cc b/brillo/streams/stream_utils.cc index 6f8a1d0..5029e3a 100644 --- a/brillo/streams/stream_utils.cc +++ b/brillo/streams/stream_utils.cc @@ -213,7 +213,7 @@ void CopyData(StreamPtr in_stream, state->success_callback = success_callback; state->error_callback = error_callback; brillo::MessageLoop::current()->PostTask(FROM_HERE, - base::Bind(&PerformRead, state)); + base::BindOnce(&PerformRead, state)); } } // namespace stream_utils diff --git a/brillo/streams/stream_utils_test.cc b/brillo/streams/stream_utils_test.cc index e0b327d..50fb67e 100644 --- a/brillo/streams/stream_utils_test.cc +++ b/brillo/streams/stream_utils_test.cc @@ -25,7 +25,7 @@ ACTION_TEMPLATE(InvokeAsyncCallback, HAS_1_TEMPLATE_PARAMS(int, k), AND_1_VALUE_PARAMS(size)) { brillo::MessageLoop::current()->PostTask( - FROM_HERE, base::Bind(std::get<k>(args), size)); + FROM_HERE, base::BindOnce(std::get<k>(args), size)); return true; } @@ -42,7 +42,8 @@ ACTION_TEMPLATE(InvokeAsyncErrorCallback, brillo::ErrorPtr error; brillo::Error::AddTo(&error, FROM_HERE, "test", code, "message"); brillo::MessageLoop::current()->PostTask( - FROM_HERE, base::Bind(std::get<k>(args), base::Owned(error.release()))); + FROM_HERE, base::BindOnce(std::get<k>(args), + base::Owned(error.release()))); return true; } diff --git a/brillo/streams/tls_stream.cc b/brillo/streams/tls_stream.cc index cc63258..4b8a227 100644 --- a/brillo/streams/tls_stream.cc +++ b/brillo/streams/tls_stream.cc @@ -393,10 +393,10 @@ bool TlsStream::TlsStreamImpl::Init(StreamPtr socket, if (MessageLoop::ThreadHasCurrent()) { MessageLoop::current()->PostTask( FROM_HERE, - base::Bind(&TlsStreamImpl::DoHandshake, - weak_ptr_factory_.GetWeakPtr(), - success_callback, - error_callback)); + base::BindOnce(&TlsStreamImpl::DoHandshake, + weak_ptr_factory_.GetWeakPtr(), + success_callback, + error_callback)); } else { DoHandshake(success_callback, error_callback); } diff --git a/brillo/timezone/EST_test.tzif b/brillo/timezone/EST_test.tzif Binary files differnew file mode 100644 index 0000000..ae34663 --- /dev/null +++ b/brillo/timezone/EST_test.tzif diff --git a/brillo/timezone/Indian_Christmas_test.tzif b/brillo/timezone/Indian_Christmas_test.tzif Binary files differnew file mode 100644 index 0000000..066c1e9 --- /dev/null +++ b/brillo/timezone/Indian_Christmas_test.tzif diff --git a/brillo/timezone/Pacific_Fiji_test.tzif b/brillo/timezone/Pacific_Fiji_test.tzif Binary files differnew file mode 100644 index 0000000..76ae63e --- /dev/null +++ b/brillo/timezone/Pacific_Fiji_test.tzif diff --git a/brillo/timezone/tzif_parser.cc b/brillo/timezone/tzif_parser.cc new file mode 100644 index 0000000..5756196 --- /dev/null +++ b/brillo/timezone/tzif_parser.cc @@ -0,0 +1,164 @@ +// Copyright 2018 The Chromium OS Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "brillo/timezone/tzif_parser.h" + +#include <arpa/inet.h> +#include <stdint.h> +#include <string.h> +#include <utility> +#include <vector> + +#include <base/files/file.h> +#include <base/files/file_path.h> +#include <base/logging.h> +#include <base/stl_util.h> +#include <base/strings/string_util.h> + +namespace { + +struct tzif_header { + char magic[4]; + char version; + char reserved[15]; + int32_t ttisgmtcnt; + int32_t ttisstdcnt; + int32_t leapcnt; + int32_t timecnt; + int32_t typecnt; + int32_t charcnt; +}; + +bool ReadInt(base::File* file, int32_t* out_int) { + DCHECK(out_int); + int32_t buf; + int read = file->ReadAtCurrentPos(reinterpret_cast<char*>(&buf), sizeof(buf)); + if (read != sizeof(buf)) { + return false; + } + // Values are stored in network byte order (highest-order byte first). + // We probably need to convert them to match the endianness of our system. + *out_int = ntohl(buf); + return true; +} + +bool ParseTzifHeader(base::File* tzfile, struct tzif_header* header) { + DCHECK(header); + int read = tzfile->ReadAtCurrentPos(header->magic, sizeof(header->magic)); + if (read != sizeof(header->magic)) { + return false; + } + if (memcmp(header->magic, "TZif", 4) != 0) { + return false; + } + + read = tzfile->ReadAtCurrentPos(&header->version, sizeof(header->version)); + if (read != sizeof(header->version)) { + return false; + } + if (header->version != '\0' && header->version != '2' && + header->version != '3') { + return false; + } + + read = tzfile->ReadAtCurrentPos(header->reserved, sizeof(header->reserved)); + if (read != sizeof(header->reserved)) { + return false; + } + for (size_t i = 0; i < sizeof(header->reserved); i++) { + if (header->reserved[i] != 0) { + return false; + } + } + + if (!ReadInt(tzfile, &header->ttisgmtcnt) || header->ttisgmtcnt < 0) { + return false; + } + if (!ReadInt(tzfile, &header->ttisstdcnt) || header->ttisstdcnt < 0) { + return false; + } + if (!ReadInt(tzfile, &header->leapcnt) || header->leapcnt < 0) { + return false; + } + if (!ReadInt(tzfile, &header->timecnt) || header->timecnt < 0) { + return false; + } + if (!ReadInt(tzfile, &header->typecnt) || header->typecnt < 0) { + return false; + } + if (!ReadInt(tzfile, &header->charcnt) || header->charcnt < 0) { + return false; + } + return true; +} + +} // namespace + +namespace brillo { + +namespace timezone { + +base::Optional<std::string> GetPosixTimezone(const base::FilePath& tzif_path) { + base::FilePath to_parse; + if (tzif_path.IsAbsolute()) { + to_parse = tzif_path; + } else { + to_parse = base::FilePath("/usr/share/zoneinfo").Append(tzif_path); + } + base::File tzfile(to_parse, base::File::FLAG_OPEN | base::File::FLAG_READ); + struct tzif_header first_header; + if (!tzfile.IsValid() || !ParseTzifHeader(&tzfile, &first_header)) { + return base::nullopt; + } + + if (first_header.version == '\0') { + // Generating a POSIX-style TZ string from a TZif version 1 file is hard; + // TZ strings need relative dates (1st Sunday in March, 1st Sunday in Nov, + // etc.), but the version 1 files only give absolute dates for each + // year up to 2037. Fortunately version 1 files are no longer created by + // the newer versions of the timezone-data package. + // + // Because of this, we're not going to try and handle this, and instead just + // return an error. + return base::nullopt; + } + + // TZif versions 2 and 3 embed a POSIX-style TZ string after their + // contents. We read that out and return it. + + // Skip past the first body section and all of the second section. See + // 'man tzfile' for an explanation of these offset values. + int64_t first_body_size = + 4 * first_header.timecnt + 1 * first_header.timecnt + + (4 + 1 + 1) * first_header.typecnt + 1 * first_header.charcnt + + (4 + 4) * first_header.leapcnt + 1 * first_header.ttisstdcnt + + 1 * first_header.ttisgmtcnt; + tzfile.Seek(base::File::FROM_CURRENT, first_body_size); + + struct tzif_header second_header; + if (!ParseTzifHeader(&tzfile, &second_header)) { + return base::nullopt; + } + + int64_t second_body_size = + 8 * second_header.timecnt + 1 * second_header.timecnt + + (4 + 1 + 1) * second_header.typecnt + 1 * second_header.charcnt + + (8 + 4) * second_header.leapcnt + 1 * second_header.ttisstdcnt + + 1 * second_header.ttisgmtcnt; + int64_t offset = tzfile.Seek(base::File::FROM_CURRENT, second_body_size); + + std::string time_string(tzfile.GetLength() - offset, '\0'); + if (tzfile.ReadAtCurrentPos(base::data(time_string), time_string.size()) != + time_string.size()) { + return base::nullopt; + } + + // According to the spec, the embedded string is enclosed by '\n' characters. + base::TrimWhitespaceASCII(time_string, base::TRIM_ALL, &time_string); + return std::move(time_string); +} + +} // namespace timezone + +} // namespace brillo diff --git a/brillo/timezone/tzif_parser.h b/brillo/timezone/tzif_parser.h new file mode 100644 index 0000000..058079c --- /dev/null +++ b/brillo/timezone/tzif_parser.h @@ -0,0 +1,29 @@ +// Copyright 2020 The Chromium OS Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef LIBBRILLO_BRILLO_TIMEZONE_TZIF_PARSER_H_ +#define LIBBRILLO_BRILLO_TIMEZONE_TZIF_PARSER_H_ + +#include <string> + +#include <base/files/file_path.h> +#include <base/optional.h> +#include <brillo/brillo_export.h> + +namespace brillo { + +namespace timezone { + +// GetPosixTimezone takes a path to a tzfile, and returns the POSIX timezone in +// a string. See 'man tzfile' for more info on the format. If |tzif_path| is a +// relative path, it will be appended to /usr/share/zoneinfo/, otherwise +// |tzif_path| as an absolute path will be used directly. +base::Optional<std::string> BRILLO_EXPORT GetPosixTimezone( + const base::FilePath& tzif_path); + +} // namespace timezone + +} // namespace brillo + +#endif // LIBBRILLO_BRILLO_TIMEZONE_TZIF_PARSER_H_ diff --git a/brillo/timezone/tzif_parser_test.cc b/brillo/timezone/tzif_parser_test.cc new file mode 100644 index 0000000..305da4d --- /dev/null +++ b/brillo/timezone/tzif_parser_test.cc @@ -0,0 +1,46 @@ +// Copyright 2018 The Chromium OS Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include <stdlib.h> + +#include <base/files/file_path.h> +#include <gtest/gtest.h> + +#include "brillo/timezone/tzif_parser.h" + +namespace brillo { + +namespace timezone { + +class TzifParserTest : public ::testing::Test { + public: + TzifParserTest() { + source_dir_ = + base::FilePath(getenv("SRC")).Append("brillo").Append("timezone"); + } + + protected: + base::FilePath source_dir_; +}; + +TEST_F(TzifParserTest, EST) { + auto posix_result = GetPosixTimezone(source_dir_.Append("EST_test.tzif")); + EXPECT_EQ(posix_result, "EST5"); +} + +TEST_F(TzifParserTest, TzifVersionTwo) { + auto posix_result = + GetPosixTimezone(source_dir_.Append("Indian_Christmas_test.tzif")); + EXPECT_EQ(posix_result, "<+07>-7"); +} + +TEST_F(TzifParserTest, TzifVersionThree) { + auto posix_result = + GetPosixTimezone(source_dir_.Append("Pacific_Fiji_test.tzif")); + EXPECT_EQ(posix_result, "<+12>-12<+13>,M11.1.0,M1.2.2/123"); +} + +} // namespace timezone + +} // namespace brillo diff --git a/brillo/type_list.h b/brillo/type_list.h index c5ccc5e..b14ef1e 100644 --- a/brillo/type_list.h +++ b/brillo/type_list.h @@ -48,6 +48,22 @@ template <typename T, typename Types> using EnableIfIsOneOf = std::enable_if_t<type_list::is_one_of<T, Types>::value>; +// Enables a template if the type T is in the typelist Types and T is an +// arithmetic type (some sort of int or floating-point number). +template <typename T, typename Types> +using EnableIfIsOneOfArithmetic = + std::enable_if_t<std::is_arithmetic<T>::value && + type_list::is_one_of<T, Types>::value, + int>; + +// Enables a template if the type T is in the typelist Types and T is not an +// arithmetic type (is void, nullptr_t, or a non-fundamental type). +template <typename T, typename Types> +using EnableIfIsOneOfNonArithmetic = + std::enable_if_t<!std::is_arithmetic<T>::value && + type_list::is_one_of<T, Types>::value, + int>; + } // namespace brillo #endif // LIBBRILLO_BRILLO_TYPE_LIST_H_ diff --git a/policy/OWNERS b/policy/OWNERS index 0208469..74cea9e 100644 --- a/policy/OWNERS +++ b/policy/OWNERS @@ -1,5 +1,4 @@ emaxx@chromium.org -ljusten@chromium.org pmarko@chromium.org poromov@chromium.org rsorokin@chromium.org diff --git a/policy/device_policy_impl.cc b/policy/device_policy_impl.cc index 72d30b1..54ea1f9 100644 --- a/policy/device_policy_impl.cc +++ b/policy/device_policy_impl.cc @@ -16,6 +16,7 @@ #include <base/logging.h> #include <base/macros.h> #include <base/memory/ptr_util.h> +#include <base/stl_util.h> #include <base/time/time.h> #include <base/values.h> #include <openssl/evp.h> @@ -100,7 +101,7 @@ std::string DecodeConnectionType(int type) { "ethernet", "wifi", "wimax", "bluetooth", "cellular", }; - if (type < 0 || type >= static_cast<int>(arraysize(kConnectionTypes))) + if (type < 0 || type >= static_cast<int>(base::size(kConnectionTypes))) return std::string(); return kConnectionTypes[type]; |