From 247ffbf3140a454fb2facf0e66ca1547e0833797 Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Mon, 8 Jul 2019 15:09:36 -0700 Subject: [PATCH] Fix a few clang-tidy issues and add NOLINT for others android-base: * Add NOLINT for expanding namespace std for std::string* ostream overload libdm: * Fix missing parentesis around macro parameters init: * Fix missing CLOEXEC usage and add NOLINT for the intended usages. * Fix missing parentesis around macro parameters * Fix erase() / remove_if() idiom * Correctly specific unsigned char when intended * 'namespace flags' should be signed, since 'flags' it signed for clone() * Add clear to property restore vector to empty after move * Explicit comparison against 0 for strcmp Test: build Change-Id: I8c31dafda2c43ebc5aa50124cbbd6e23ed2c4101 --- base/include/android-base/logging.h | 2 +- fs_mgr/libdm/include/libdm/dm.h | 2 +- init/action_manager.cpp | 3 ++- init/builtins.cpp | 4 ++-- init/first_stage_init.cpp | 4 ++-- init/persistent_properties.cpp | 2 +- init/property_service.cpp | 4 ++-- init/service.cpp | 2 +- init/service.h | 7 +++---- init/service_test.cpp | 6 +++--- init/service_utils.cpp | 9 +++------ init/service_utils.h | 2 +- init/subcontext.cpp | 2 +- init/tokenizer_test.cpp | 1 + init/uevent_listener.cpp | 4 ++-- init/util.cpp | 2 +- 16 files changed, 27 insertions(+), 29 deletions(-) diff --git a/base/include/android-base/logging.h b/base/include/android-base/logging.h index f94cc258e..ab6476c59 100644 --- a/base/include/android-base/logging.h +++ b/base/include/android-base/logging.h @@ -469,7 +469,7 @@ class ScopedLogSeverity { } // namespace base } // namespace android -namespace std { +namespace std { // NOLINT(cert-dcl58-cpp) // Emit a warning of ostream<< with std::string*. The intention was most likely to print *string. // diff --git a/fs_mgr/libdm/include/libdm/dm.h b/fs_mgr/libdm/include/libdm/dm.h index afcb09015..08376c02f 100644 --- a/fs_mgr/libdm/include/libdm/dm.h +++ b/fs_mgr/libdm/include/libdm/dm.h @@ -38,7 +38,7 @@ #define DM_VERSION2 (0) #define DM_ALIGN_MASK (7) -#define DM_ALIGN(x) ((x + DM_ALIGN_MASK) & ~DM_ALIGN_MASK) +#define DM_ALIGN(x) (((x) + DM_ALIGN_MASK) & ~DM_ALIGN_MASK) namespace android { namespace dm { diff --git a/init/action_manager.cpp b/init/action_manager.cpp index 9de40859f..985b8adf5 100644 --- a/init/action_manager.cpp +++ b/init/action_manager.cpp @@ -88,7 +88,8 @@ void ActionManager::ExecuteOneCommand() { current_command_ = 0; if (action->oneshot()) { auto eraser = [&action](std::unique_ptr& a) { return a.get() == action; }; - actions_.erase(std::remove_if(actions_.begin(), actions_.end(), eraser)); + actions_.erase(std::remove_if(actions_.begin(), actions_.end(), eraser), + actions_.end()); } } } diff --git a/init/builtins.cpp b/init/builtins.cpp index 55b024891..ba2c7ac2e 100644 --- a/init/builtins.cpp +++ b/init/builtins.cpp @@ -243,7 +243,7 @@ static Result do_ifup(const BuiltinArguments& args) { strlcpy(ifr.ifr_name, args[1].c_str(), IFNAMSIZ); - unique_fd s(TEMP_FAILURE_RETRY(socket(AF_INET, SOCK_DGRAM, 0))); + unique_fd s(TEMP_FAILURE_RETRY(socket(AF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0))); if (s < 0) return ErrnoError() << "opening socket failed"; if (ioctl(s, SIOCGIFFLAGS, &ifr) < 0) { @@ -784,7 +784,7 @@ static Result do_write(const BuiltinArguments& args) { } static Result readahead_file(const std::string& filename, bool fully) { - android::base::unique_fd fd(TEMP_FAILURE_RETRY(open(filename.c_str(), O_RDONLY))); + android::base::unique_fd fd(TEMP_FAILURE_RETRY(open(filename.c_str(), O_RDONLY | O_CLOEXEC))); if (fd == -1) { return ErrnoError() << "Error opening file"; } diff --git a/init/first_stage_init.cpp b/init/first_stage_init.cpp index 36eece8d2..b60c450b0 100644 --- a/init/first_stage_init.cpp +++ b/init/first_stage_init.cpp @@ -78,7 +78,7 @@ void FreeRamdisk(DIR* dir, dev_t dev) { if (S_ISDIR(info.st_mode)) { is_dir = true; - auto fd = openat(dfd, de->d_name, O_RDONLY | O_DIRECTORY); + auto fd = openat(dfd, de->d_name, O_RDONLY | O_DIRECTORY | O_CLOEXEC); if (fd >= 0) { auto subdir = std::unique_ptr{fdopendir(fd), closedir}; @@ -152,7 +152,7 @@ int FirstStageMain(int argc, char** argv) { std::vector> errors; #define CHECKCALL(x) \ - if (x != 0) errors.emplace_back(#x " failed", errno); + if ((x) != 0) errors.emplace_back(#x " failed", errno); // Clear the umask. umask(0); diff --git a/init/persistent_properties.cpp b/init/persistent_properties.cpp index 73787b913..baa9ad474 100644 --- a/init/persistent_properties.cpp +++ b/init/persistent_properties.cpp @@ -69,7 +69,7 @@ Result LoadLegacyPersistentProperties() { continue; } - unique_fd fd(openat(dirfd(dir.get()), entry->d_name, O_RDONLY | O_NOFOLLOW)); + unique_fd fd(openat(dirfd(dir.get()), entry->d_name, O_RDONLY | O_NOFOLLOW | O_CLOEXEC)); if (fd == -1) { PLOG(ERROR) << "Unable to open persistent property file \"" << entry->d_name << "\""; continue; diff --git a/init/property_service.cpp b/init/property_service.cpp index 1520c9f33..b89914f12 100644 --- a/init/property_service.cpp +++ b/init/property_service.cpp @@ -667,9 +667,9 @@ static void LoadProperties(char* data, const char* filter, const char* filename, if (flen > 0) { if (filter[flen - 1] == '*') { - if (strncmp(key, filter, flen - 1)) continue; + if (strncmp(key, filter, flen - 1) != 0) continue; } else { - if (strcmp(key, filter)) continue; + if (strcmp(key, filter) != 0) continue; } } diff --git a/init/service.cpp b/init/service.cpp index cd08f3d1d..f95b67582 100644 --- a/init/service.cpp +++ b/init/service.cpp @@ -126,7 +126,7 @@ Service::Service(const std::string& name, Subcontext* subcontext_for_restart_com : Service(name, 0, 0, 0, {}, 0, "", subcontext_for_restart_commands, args) {} Service::Service(const std::string& name, unsigned flags, uid_t uid, gid_t gid, - const std::vector& supp_gids, unsigned namespace_flags, + const std::vector& supp_gids, int namespace_flags, const std::string& seclabel, Subcontext* subcontext_for_restart_commands, const std::vector& args) : name_(name), diff --git a/init/service.h b/init/service.h index 78f94ce9e..cc35a8d6d 100644 --- a/init/service.h +++ b/init/service.h @@ -69,9 +69,8 @@ class Service { const std::vector& args); Service(const std::string& name, unsigned flags, uid_t uid, gid_t gid, - const std::vector& supp_gids, unsigned namespace_flags, - const std::string& seclabel, Subcontext* subcontext_for_restart_commands, - const std::vector& args); + const std::vector& supp_gids, int namespace_flags, const std::string& seclabel, + Subcontext* subcontext_for_restart_commands, const std::vector& args); static std::unique_ptr MakeTemporaryOneshotService(const std::vector& args); @@ -109,7 +108,7 @@ class Service { int crash_count() const { return crash_count_; } uid_t uid() const { return proc_attr_.uid; } gid_t gid() const { return proc_attr_.gid; } - unsigned namespace_flags() const { return namespaces_.flags; } + int namespace_flags() const { return namespaces_.flags; } const std::vector& supp_gids() const { return proc_attr_.supp_gids; } const std::string& seclabel() const { return seclabel_; } const std::vector& keycodes() const { return keycodes_; } diff --git a/init/service_test.cpp b/init/service_test.cpp index 4bfaa6b78..6a34acc55 100644 --- a/init/service_test.cpp +++ b/init/service_test.cpp @@ -30,7 +30,7 @@ namespace init { TEST(service, pod_initialized) { constexpr auto memory_size = sizeof(Service); - alignas(alignof(Service)) char old_memory[memory_size]; + alignas(alignof(Service)) unsigned char old_memory[memory_size]; for (std::size_t i = 0; i < memory_size; ++i) { old_memory[i] = 0xFF; @@ -45,7 +45,7 @@ TEST(service, pod_initialized) { EXPECT_EQ(0, service_in_old_memory->crash_count()); EXPECT_EQ(0U, service_in_old_memory->uid()); EXPECT_EQ(0U, service_in_old_memory->gid()); - EXPECT_EQ(0U, service_in_old_memory->namespace_flags()); + EXPECT_EQ(0, service_in_old_memory->namespace_flags()); EXPECT_EQ(IoSchedClass_NONE, service_in_old_memory->ioprio_class()); EXPECT_EQ(0, service_in_old_memory->ioprio_pri()); EXPECT_EQ(0, service_in_old_memory->priority()); @@ -64,7 +64,7 @@ TEST(service, pod_initialized) { EXPECT_EQ(0, service_in_old_memory2->crash_count()); EXPECT_EQ(0U, service_in_old_memory2->uid()); EXPECT_EQ(0U, service_in_old_memory2->gid()); - EXPECT_EQ(0U, service_in_old_memory2->namespace_flags()); + EXPECT_EQ(0, service_in_old_memory2->namespace_flags()); EXPECT_EQ(IoSchedClass_NONE, service_in_old_memory2->ioprio_class()); EXPECT_EQ(0, service_in_old_memory2->ioprio_pri()); EXPECT_EQ(0, service_in_old_memory2->priority()); diff --git a/init/service_utils.cpp b/init/service_utils.cpp index f88ea9758..34aa8370f 100644 --- a/init/service_utils.cpp +++ b/init/service_utils.cpp @@ -120,22 +120,19 @@ Result SetUpPidNamespace(const char* name) { } void ZapStdio() { - int fd; - fd = open("/dev/null", O_RDWR); + auto fd = unique_fd{open("/dev/null", O_RDWR | O_CLOEXEC)}; dup2(fd, 0); dup2(fd, 1); dup2(fd, 2); - close(fd); } void OpenConsole(const std::string& console) { - int fd = open(console.c_str(), O_RDWR); - if (fd == -1) fd = open("/dev/null", O_RDWR); + auto fd = unique_fd{open(console.c_str(), O_RDWR | O_CLOEXEC)}; + if (fd == -1) fd.reset(open("/dev/null", O_RDWR | O_CLOEXEC)); ioctl(fd, TIOCSCTTY, 0); dup2(fd, 0); dup2(fd, 1); dup2(fd, 2); - close(fd); } } // namespace diff --git a/init/service_utils.h b/init/service_utils.h index c26b12343..365cb298b 100644 --- a/init/service_utils.h +++ b/init/service_utils.h @@ -30,7 +30,7 @@ namespace android { namespace init { struct NamespaceInfo { - unsigned flags; + int flags; // Pair of namespace type, path to name. std::vector> namespaces_to_enter; }; diff --git a/init/subcontext.cpp b/init/subcontext.cpp index 02ed507a7..2f9541ba7 100644 --- a/init/subcontext.cpp +++ b/init/subcontext.cpp @@ -246,7 +246,7 @@ void Subcontext::Fork() { // We explicitly do not use O_CLOEXEC here, such that we can reference this FD by number // in the subcontext process after we exec. - int child_fd = dup(subcontext_socket); + int child_fd = dup(subcontext_socket); // NOLINT(android-cloexec-dup) if (child_fd < 0) { PLOG(FATAL) << "Could not dup child_fd"; } diff --git a/init/tokenizer_test.cpp b/init/tokenizer_test.cpp index acfc7c7d7..6b31683e0 100644 --- a/init/tokenizer_test.cpp +++ b/init/tokenizer_test.cpp @@ -46,6 +46,7 @@ void RunTest(const std::string& data, const std::vector return; case T_NEWLINE: tokens.emplace_back(std::move(current_line)); + current_line.clear(); break; case T_TEXT: current_line.emplace_back(state.text); diff --git a/init/uevent_listener.cpp b/init/uevent_listener.cpp index 62cd2be3a..ac633776b 100644 --- a/init/uevent_listener.cpp +++ b/init/uevent_listener.cpp @@ -131,7 +131,7 @@ ListenerAction UeventListener::RegenerateUeventsForDir(DIR* d, const ListenerCallback& callback) const { int dfd = dirfd(d); - int fd = openat(dfd, "uevent", O_WRONLY); + int fd = openat(dfd, "uevent", O_WRONLY | O_CLOEXEC); if (fd >= 0) { write(fd, "add\n", 4); close(fd); @@ -146,7 +146,7 @@ ListenerAction UeventListener::RegenerateUeventsForDir(DIR* d, while ((de = readdir(d)) != nullptr) { if (de->d_type != DT_DIR || de->d_name[0] == '.') continue; - fd = openat(dfd, de->d_name, O_RDONLY | O_DIRECTORY); + fd = openat(dfd, de->d_name, O_RDONLY | O_DIRECTORY | O_CLOEXEC); if (fd < 0) continue; std::unique_ptr d2(fdopendir(fd), closedir); diff --git a/init/util.cpp b/init/util.cpp index 2b3424229..058a1119f 100644 --- a/init/util.cpp +++ b/init/util.cpp @@ -450,7 +450,7 @@ static void InitAborter(const char* abort_message) { // SetStdioToDevNull() must be called again in second stage init. void SetStdioToDevNull(char** argv) { // Make stdin/stdout/stderr all point to /dev/null. - int fd = open("/dev/null", O_RDWR); + int fd = open("/dev/null", O_RDWR); // NOLINT(android-cloexec-open) if (fd == -1) { int saved_errno = errno; android::base::InitLogging(argv, &android::base::KernelLogger, InitAborter);