From 18278d2e9ccb9e7fdb8391055c56d1b35b3e39a6 Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Tue, 12 Nov 2019 16:21:20 -0800 Subject: [PATCH] init: make triggering shutdown from vendor_init better Previously, we assumed that TriggerShutdown() should never be called from vendor_init and used property service as a back up in case it ever did. We have since then found out that vendor_init may indeed call TriggerShutdown() and we want to make it just as strict as it is in init, wherein it will immediately start the shutdown sequence without executing any further commands. Test: init unit tests, trigger shuttdown from init and vendor_init Change-Id: I1f44dae801a28269eb8127879a8b7d6adff6f353 --- init/builtins.cpp | 15 ++++----------- init/host_init_stubs.h | 5 ----- init/init.cpp | 4 +++- init/init.h | 2 -- init/reboot.cpp | 2 +- init/service.cpp | 7 +++---- init/subcontext.cpp | 14 ++++++++++++++ init/subcontext.proto | 2 ++ init/subcontext_test.cpp | 19 +++++++++++++++++++ init/util.cpp | 2 ++ init/util.h | 2 ++ 11 files changed, 50 insertions(+), 24 deletions(-) diff --git a/init/builtins.cpp b/init/builtins.cpp index 5ee928e1e..a55514b82 100644 --- a/init/builtins.cpp +++ b/init/builtins.cpp @@ -140,14 +140,7 @@ static Result reboot_into_recovery(const std::vector& options if (!write_bootloader_message(options, &err)) { return Error() << "Failed to set bootloader message: " << err; } - // This function should only be reached from init and not from vendor_init, and we want to - // immediately trigger reboot instead of relaying through property_service. Older devices may - // still have paths that reach here from vendor_init, so we keep the property_set as a fallback. - if (getpid() == 1) { - TriggerShutdown("reboot,recovery"); - } else { - property_set("sys.powerctl", "reboot,recovery"); - } + trigger_shutdown("reboot,recovery"); return {}; } @@ -554,7 +547,7 @@ static Result queue_fs_event(int code, bool userdata_remount) { // support userdata remount on FDE devices, this should never been triggered. Time to // panic! LOG(ERROR) << "Userdata remount is not supported on FDE devices. How did you get here?"; - TriggerShutdown("reboot,requested-userdata-remount-on-fde-device"); + trigger_shutdown("reboot,requested-userdata-remount-on-fde-device"); } ActionManager::GetInstance().QueueEventTrigger("encrypt"); return {}; @@ -564,7 +557,7 @@ static Result queue_fs_event(int code, bool userdata_remount) { // don't support userdata remount on FDE devices, this should never been triggered. // Time to panic! LOG(ERROR) << "Userdata remount is not supported on FDE devices. How did you get here?"; - TriggerShutdown("reboot,requested-userdata-remount-on-fde-device"); + trigger_shutdown("reboot,requested-userdata-remount-on-fde-device"); } property_set("ro.crypto.state", "encrypted"); property_set("ro.crypto.type", "block"); @@ -1148,7 +1141,7 @@ static Result do_remount_userdata(const BuiltinArguments& args) { } // TODO(b/135984674): check that fstab contains /data. if (auto rc = fs_mgr_remount_userdata_into_checkpointing(&fstab); rc < 0) { - TriggerShutdown("reboot,mount-userdata-failed"); + trigger_shutdown("reboot,mount-userdata-failed"); } if (auto result = queue_fs_event(initial_mount_fstab_return_code, true); !result) { return Error() << "queue_fs_event() failed: " << result.error(); diff --git a/init/host_init_stubs.h b/init/host_init_stubs.h index 9b33a1c60..30d312963 100644 --- a/init/host_init_stubs.h +++ b/init/host_init_stubs.h @@ -35,11 +35,6 @@ namespace android { namespace init { -// init.h -inline void TriggerShutdown(const std::string&) { - abort(); -} - // property_service.h inline bool CanReadProperty(const std::string&, const std::string&) { return true; diff --git a/init/init.cpp b/init/init.cpp index ff86f8d68..8e2da59d5 100644 --- a/init/init.cpp +++ b/init/init.cpp @@ -180,7 +180,7 @@ void ResetWaitForProp() { waiting_for_prop.reset(); } -void TriggerShutdown(const std::string& command) { +static void TriggerShutdown(const std::string& command) { // We can't call HandlePowerctlMessage() directly in this function, // because it modifies the contents of the action queue, which can cause the action queue // to get into a bad state if this function is called from a command being executed by the @@ -681,6 +681,8 @@ int SecondStageMain(int argc, char** argv) { boot_clock::time_point start_time = boot_clock::now(); + trigger_shutdown = TriggerShutdown; + SetStdioToDevNull(argv); InitKernelLogging(argv); LOG(INFO) << "init second stage started!"; diff --git a/init/init.h b/init/init.h index d884a9447..0805940b2 100644 --- a/init/init.h +++ b/init/init.h @@ -31,8 +31,6 @@ namespace init { Parser CreateParser(ActionManager& action_manager, ServiceList& service_list); Parser CreateServiceOnlyParser(ServiceList& service_list); -void TriggerShutdown(const std::string& command); - bool start_waiting_for_property(const char *name, const char *value); void DumpState(); diff --git a/init/reboot.cpp b/init/reboot.cpp index 4a169692b..64ec1fb6d 100644 --- a/init/reboot.cpp +++ b/init/reboot.cpp @@ -731,7 +731,7 @@ static Result DoUserspaceReboot() { auto guard = android::base::make_scope_guard([] { // Leave shutdown so that we can handle a full reboot. LeaveShutdown(); - TriggerShutdown("reboot,abort-userspace-reboot"); + trigger_shutdown("reboot,abort-userspace-reboot"); }); // Triggering userspace-reboot-requested will result in a bunch of set_prop // actions. We should make sure, that all of them are propagated before diff --git a/init/service.cpp b/init/service.cpp index f8e98a2e9..574ff52e3 100644 --- a/init/service.cpp +++ b/init/service.cpp @@ -43,7 +43,6 @@ #if defined(__ANDROID__) #include -#include "init.h" #include "mount_namespace.h" #include "property_service.h" #else @@ -260,7 +259,7 @@ void Service::Reap(const siginfo_t& siginfo) { if ((siginfo.si_code != CLD_EXITED || siginfo.si_status != 0) && on_failure_reboot_target_) { LOG(ERROR) << "Service with 'reboot_on_failure' option failed, shutting down system."; - TriggerShutdown(*on_failure_reboot_target_); + trigger_shutdown(*on_failure_reboot_target_); } if (flags_ & SVC_EXEC) UnSetExec(); @@ -340,7 +339,7 @@ void Service::DumpState() const { Result Service::ExecStart() { auto reboot_on_failure = make_scope_guard([this] { if (on_failure_reboot_target_) { - TriggerShutdown(*on_failure_reboot_target_); + trigger_shutdown(*on_failure_reboot_target_); } }); @@ -371,7 +370,7 @@ Result Service::ExecStart() { Result Service::Start() { auto reboot_on_failure = make_scope_guard([this] { if (on_failure_reboot_target_) { - TriggerShutdown(*on_failure_reboot_target_); + trigger_shutdown(*on_failure_reboot_target_); } }); diff --git a/init/subcontext.cpp b/init/subcontext.cpp index 79fc372b6..e55265b0a 100644 --- a/init/subcontext.cpp +++ b/init/subcontext.cpp @@ -51,6 +51,8 @@ namespace android { namespace init { namespace { +std::string shutdown_command; + class SubcontextProcess { public: SubcontextProcess(const BuiltinFunctionMap* function_map, std::string context, int init_fd) @@ -153,6 +155,11 @@ void SubcontextProcess::MainLoop() { << subcontext_command.command_case(); } + if (!shutdown_command.empty()) { + reply.set_trigger_shutdown(shutdown_command); + shutdown_command.clear(); + } + if (auto result = SendMessage(init_fd_, reply); !result) { LOG(FATAL) << "Failed to send message to init: " << result.error(); } @@ -174,6 +181,8 @@ int SubcontextMain(int argc, char** argv, const BuiltinFunctionMap* function_map return 0; }; + trigger_shutdown = [](const std::string& command) { shutdown_command = command; }; + auto subcontext_process = SubcontextProcess(function_map, context, init_fd); subcontext_process.MainLoop(); return 0; @@ -254,6 +263,11 @@ Result Subcontext::TransmitMessage(const SubcontextCommand& sub Restart(); return Error() << "Unable to parse message from subcontext"; } + + if (subcontext_reply.has_trigger_shutdown()) { + trigger_shutdown(subcontext_reply.trigger_shutdown()); + } + return subcontext_reply; } diff --git a/init/subcontext.proto b/init/subcontext.proto index e68115e0e..068c7ce47 100644 --- a/init/subcontext.proto +++ b/init/subcontext.proto @@ -38,4 +38,6 @@ message SubcontextReply { Failure failure = 2; ExpandArgsReply expand_args_reply = 3; } + + optional string trigger_shutdown = 4; } \ No newline at end of file diff --git a/init/subcontext_test.cpp b/init/subcontext_test.cpp index 9cac35e7e..9c1a78818 100644 --- a/init/subcontext_test.cpp +++ b/init/subcontext_test.cpp @@ -26,6 +26,7 @@ #include #include "builtin_arguments.h" +#include "util.h" using namespace std::literals; @@ -142,6 +143,18 @@ TEST(subcontext, ContextString) { }); } +TEST(subcontext, TriggerShutdown) { + static constexpr const char kTestShutdownCommand[] = "reboot,test-shutdown-command"; + static std::string trigger_shutdown_command; + trigger_shutdown = [](const std::string& command) { trigger_shutdown_command = command; }; + RunTest([](auto& subcontext, auto& context_string) { + auto result = subcontext.Execute( + std::vector{"trigger_shutdown", kTestShutdownCommand}); + ASSERT_TRUE(result); + }); + EXPECT_EQ(kTestShutdownCommand, trigger_shutdown_command); +} + TEST(subcontext, ExpandArgs) { RunTest([](auto& subcontext, auto& context_string) { auto args = std::vector{ @@ -207,6 +220,11 @@ BuiltinFunctionMap BuildTestFunctionMap() { return Error() << args.context; }; + auto do_trigger_shutdown = [](const BuiltinArguments& args) -> Result { + trigger_shutdown(args[1]); + return {}; + }; + // clang-format off BuiltinFunctionMap test_function_map = { {"return_pids_as_error", {0, 0, {true, do_return_pids_as_error}}}, @@ -216,6 +234,7 @@ BuiltinFunctionMap BuildTestFunctionMap() { {"cause_log_fatal", {0, 0, {true, do_cause_log_fatal}}}, {"generate_sane_error", {0, 0, {true, do_generate_sane_error}}}, {"return_context_as_error", {0, 0, {true, do_return_context_as_error}}}, + {"trigger_shutdown", {1, 1, {true, do_trigger_shutdown}}}, }; // clang-format on return test_function_map; diff --git a/init/util.cpp b/init/util.cpp index ada9e7860..e5254dddf 100644 --- a/init/util.cpp +++ b/init/util.cpp @@ -61,6 +61,8 @@ namespace init { const std::string kDefaultAndroidDtDir("/proc/device-tree/firmware/android/"); +void (*trigger_shutdown)(const std::string& command) = nullptr; + // DecodeUid() - decodes and returns the given string, which can be either the // numeric or name representation, into the integer uid or gid. Result DecodeUid(const std::string& name) { diff --git a/init/util.h b/init/util.h index 3d81d7278..9d89ed7b2 100644 --- a/init/util.h +++ b/init/util.h @@ -35,6 +35,8 @@ namespace init { static const char kColdBootDoneProp[] = "ro.cold_boot_done"; +extern void (*trigger_shutdown)(const std::string& command); + Result CreateSocket(const std::string& name, int type, bool passcred, mode_t perm, uid_t uid, gid_t gid, const std::string& socketcon);