From 1693f420d4d16954ecab83f5ab710f8f89d404a0 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Wed, 19 Oct 2022 16:30:15 -0700 Subject: [PATCH] init: Introduce class InterprocessFifo Prepare for introducing a second interprocess communication channel by introducing the class InterprocessFifo. Stop using std::unique_ptr<> for holding the pipe file descriptors. Handle EOF consistently. Bug: 213617178 Change-Id: Ic0cf18d3d8ea61b8ee17e64de8a9df2736e26728 Signed-off-by: Bart Van Assche --- init/Android.bp | 7 ++- init/interprocess_fifo.cpp | 96 +++++++++++++++++++++++++++++++++ init/interprocess_fifo.h | 52 ++++++++++++++++++ init/interprocess_fifo_test.cpp | 53 ++++++++++++++++++ init/service.cpp | 43 +++++++-------- init/service.h | 5 +- 6 files changed, 229 insertions(+), 27 deletions(-) create mode 100644 init/interprocess_fifo.cpp create mode 100644 init/interprocess_fifo.h create mode 100644 init/interprocess_fifo_test.cpp diff --git a/init/Android.bp b/init/Android.bp index f6f1e8c8c..06f696e4c 100644 --- a/init/Android.bp +++ b/init/Android.bp @@ -39,6 +39,7 @@ init_common_sources = [ "epoll.cpp", "import_parser.cpp", "interface_utils.cpp", + "interprocess_fifo.cpp", "keychords.cpp", "parser.cpp", "property_type.cpp", @@ -467,6 +468,7 @@ cc_test { "epoll_test.cpp", "firmware_handler_test.cpp", "init_test.cpp", + "interprocess_fifo_test.cpp", "keychords_test.cpp", "oneshot_on_test.cpp", "persistent_properties_test.cpp", @@ -481,7 +483,10 @@ cc_test { "ueventd_test.cpp", "util_test.cpp", ], - static_libs: ["libinit"], + static_libs: [ + "libgmock", + "libinit", + ], test_suites: [ "cts", diff --git a/init/interprocess_fifo.cpp b/init/interprocess_fifo.cpp new file mode 100644 index 000000000..6e0d03186 --- /dev/null +++ b/init/interprocess_fifo.cpp @@ -0,0 +1,96 @@ +/* + * Copyright (C) 2022 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "interprocess_fifo.h" + +#include + +#include + +using ::android::base::ErrnoError; +using ::android::base::Error; +using ::android::base::Result; + +namespace android { +namespace init { + +InterprocessFifo::InterprocessFifo() noexcept : fds_({-1, -1}) {} + +InterprocessFifo::InterprocessFifo(InterprocessFifo&& orig) noexcept : fds_({-1, -1}) { + std::swap(fds_, orig.fds_); +} + +InterprocessFifo::~InterprocessFifo() noexcept { + Close(); +} + +void InterprocessFifo::CloseFd(int& fd) noexcept { + if (fd >= 0) { + close(fd); + fd = -1; + } +} + +void InterprocessFifo::CloseReadFd() noexcept { + CloseFd(fds_[0]); +} + +void InterprocessFifo::CloseWriteFd() noexcept { + CloseFd(fds_[1]); +} + +void InterprocessFifo::Close() noexcept { + CloseReadFd(); + CloseWriteFd(); +} + +Result InterprocessFifo::Initialize() noexcept { + if (fds_[0] >= 0) { + return Error() << "already initialized"; + } + if (pipe(fds_.data()) < 0) { // NOLINT(android-cloexec-pipe) + return ErrnoError() << "pipe()"; + } + return {}; +} + +Result InterprocessFifo::Read() noexcept { + uint8_t byte; + ssize_t count = read(fds_[0], &byte, 1); + if (count < 0) { + return ErrnoError() << "read()"; + } + if (count == 0) { + return Error() << "read() EOF"; + } + DCHECK_EQ(count, 1); + return byte; +} + +Result InterprocessFifo::Write(uint8_t byte) noexcept { + ssize_t written = write(fds_[1], &byte, 1); + if (written < 0) { + return ErrnoError() << "write()"; + } + if (written == 0) { + return Error() << "write() EOF"; + } + DCHECK_EQ(written, 1); + return {}; +} + +} // namespace init +} // namespace android diff --git a/init/interprocess_fifo.h b/init/interprocess_fifo.h new file mode 100644 index 000000000..cdaac86b5 --- /dev/null +++ b/init/interprocess_fifo.h @@ -0,0 +1,52 @@ +/* + * Copyright (C) 2022 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include + +#include + +namespace android { +namespace init { + +// A FIFO for inter-process communication that uses a Unix pipe internally. +class InterprocessFifo { + public: + template + using Result = ::android::base::Result; + + InterprocessFifo() noexcept; + InterprocessFifo(const InterprocessFifo& orig) noexcept = delete; + InterprocessFifo(InterprocessFifo&& orig) noexcept; + InterprocessFifo& operator=(const InterprocessFifo& orig) noexcept = delete; + InterprocessFifo& operator=(InterprocessFifo&& orig) noexcept = delete; + ~InterprocessFifo() noexcept; + void CloseReadFd() noexcept; + void CloseWriteFd() noexcept; + void Close() noexcept; + Result Initialize() noexcept; + Result Write(uint8_t byte) noexcept; + Result Read() noexcept; + + private: + static void CloseFd(int& fd) noexcept; + + std::array fds_; +}; + +} // namespace init +} // namespace android diff --git a/init/interprocess_fifo_test.cpp b/init/interprocess_fifo_test.cpp new file mode 100644 index 000000000..81cfbacc8 --- /dev/null +++ b/init/interprocess_fifo_test.cpp @@ -0,0 +1,53 @@ +/* + * Copyright (C) 2022 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "interprocess_fifo.h" + +#include +#include + +#define ASSERT_OK(e) ASSERT_THAT(e, Ok()) +#define ASSERT_NOT_OK(e) ASSERT_THAT(e, Not(Ok())) + +using ::android::base::Result; +using ::android::base::testing::Ok; +using ::testing::Not; + +namespace android { +namespace init { + +TEST(FifoTest, WriteAndRead) { + InterprocessFifo fifo; + ASSERT_OK(fifo.Initialize()); + ASSERT_OK(fifo.Write('a')); + ASSERT_OK(fifo.Write('b')); + Result result = fifo.Read(); + ASSERT_OK(result); + EXPECT_EQ(*result, 'a'); + result = fifo.Read(); + ASSERT_OK(result); + EXPECT_EQ(*result, 'b'); + InterprocessFifo fifo2 = std::move(fifo); + ASSERT_NOT_OK(fifo.Write('c')); + ASSERT_NOT_OK(fifo.Read()); + ASSERT_OK(fifo2.Write('d')); + result = fifo2.Read(); + ASSERT_OK(result); + EXPECT_EQ(*result, 'd'); +} + +} // namespace init +} // namespace android diff --git a/init/service.cpp b/init/service.cpp index 6a9343db4..331cd880d 100644 --- a/init/service.cpp +++ b/init/service.cpp @@ -38,6 +38,7 @@ #include +#include "interprocess_fifo.h" #include "lmkd_service.h" #include "service_list.h" #include "util.h" @@ -442,14 +443,6 @@ Result Service::ExecStart() { return {}; } -static void ClosePipe(const std::array* pipe) { - for (const auto fd : *pipe) { - if (fd >= 0) { - close(fd); - } - } -} - Result Service::CheckConsole() { if (!(flags_ & SVC_CONSOLE)) { return {}; @@ -514,7 +507,7 @@ void Service::ConfigureMemcg() { // Enters namespaces, sets environment variables, writes PID files and runs the service executable. void Service::RunService(const std::vector& descriptors, - std::unique_ptr, decltype(&ClosePipe)> pipefd) { + InterprocessFifo cgroups_activated) { if (auto result = EnterNamespaces(namespaces_, name_, mount_namespace_); !result.ok()) { LOG(FATAL) << "Service '" << name_ << "' failed to set up namespaces: " << result.error(); } @@ -536,12 +529,12 @@ void Service::RunService(const std::vector& descriptors, // Wait until the cgroups have been created and until the cgroup controllers have been // activated. - char byte = 0; - if (read((*pipefd)[0], &byte, 1) < 0) { - PLOG(ERROR) << "failed to read from notification channel"; + Result byte = cgroups_activated.Read(); + if (!byte.ok()) { + LOG(ERROR) << name_ << ": failed to read from notification channel: " << byte.error(); } - pipefd.reset(); - if (!byte) { + cgroups_activated.Close(); + if (!*byte) { LOG(FATAL) << "Service '" << name_ << "' failed to start due to a fatal error"; _exit(EXIT_FAILURE); } @@ -605,10 +598,10 @@ Result Service::Start() { return {}; } - std::unique_ptr, decltype(&ClosePipe)> pipefd(new std::array{-1, -1}, - ClosePipe); - if (pipe(pipefd->data()) < 0) { - return ErrnoError() << "pipe()"; + InterprocessFifo cgroups_activated; + + if (Result result = cgroups_activated.Initialize(); !result.ok()) { + return result; } if (Result result = CheckConsole(); !result.ok()) { @@ -667,8 +660,11 @@ Result Service::Start() { if (pid == 0) { umask(077); - RunService(descriptors, std::move(pipefd)); + cgroups_activated.CloseWriteFd(); + RunService(descriptors, std::move(cgroups_activated)); _exit(127); + } else { + cgroups_activated.CloseReadFd(); } if (pid < 0) { @@ -697,8 +693,9 @@ Result Service::Start() { limit_percent_ != -1 || !limit_property_.empty(); errno = -createProcessGroup(proc_attr_.uid, pid_, use_memcg); if (errno != 0) { - if (char byte = 0; write((*pipefd)[1], &byte, 1) < 0) { - return ErrnoError() << "sending notification failed"; + Result result = cgroups_activated.Write(0); + if (!result.ok()) { + return Error() << "Sending notification failed: " << result.error(); } return Error() << "createProcessGroup(" << proc_attr_.uid << ", " << pid_ << ") failed for service '" << name_ << "'"; @@ -720,8 +717,8 @@ Result Service::Start() { LmkdRegister(name_, proc_attr_.uid, pid_, oom_score_adjust_); } - if (char byte = 1; write((*pipefd)[1], &byte, 1) < 0) { - return ErrnoError() << "sending notification failed"; + if (Result result = cgroups_activated.Write(1); !result.ok()) { + return Error() << "Sending cgroups activated notification failed: " << result.error(); } NotifyStateChange("running"); diff --git a/init/service.h b/init/service.h index ab198652a..b2c9909ed 100644 --- a/init/service.h +++ b/init/service.h @@ -31,6 +31,7 @@ #include "action.h" #include "capabilities.h" +#include "interprocess_fifo.h" #include "keyword_map.h" #include "mount_namespace.h" #include "parser.h" @@ -154,9 +155,7 @@ class Service { void ResetFlagsForStart(); Result CheckConsole(); void ConfigureMemcg(); - void RunService( - const std::vector& descriptors, - std::unique_ptr, void (*)(const std::array* pipe)> pipefd); + void RunService(const std::vector& descriptors, InterprocessFifo cgroups_activated); void SetMountNamespace(); static unsigned long next_start_order_; static bool is_exec_service_running_;