From cbd66d3c8a672ed1f762444e4bfd6cd9b44a924d Mon Sep 17 00:00:00 2001
From: Tom Cherry <tomcherry@google.com>
Date: Wed, 13 Sep 2017 14:39:45 -0700
Subject: [PATCH] init: fix crash when reboot is triggered by a builtin

Builtin commands may set the sys.powerctl property, which causes
reboot to be immediately processed.  Unfortunately, part of the reboot
processing involves clearing the action queue, so when this scenario
happens, ActionManager::ExecuteOneCommand() can abort due to its state
being unexpectedly changed.

Longer term, the real fix here is to split init and property service.
In this case, the property sets will be sent to property service and
the reboot will only be processed once property service responds back
to init that the property has been set.  Since that will not happen
within the action queue, there will be no risk of failure.

Short term, this change sets a flag in init to shutdown the device
before the next action is run, which defers the shutdown enough to fix
the crash, but continues to prevent any further commands from running.

Bug: 65374456
Test: force bullhead into the repro case and observe that it no longer
      repros

Merged-In: I89c73dad8d7912a845d694b095cab061b8dcc05e
Change-Id: I89c73dad8d7912a845d694b095cab061b8dcc05e
(cherry picked from commit 3633a4014a1a315000c3e6dee36b419473ab44b9)
---
 init/builtins.cpp |  5 +++--
 init/init.cpp     | 22 +++++++++++++++++++---
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/init/builtins.cpp b/init/builtins.cpp
index 56d0ae2c8..9e2efe282 100644
--- a/init/builtins.cpp
+++ b/init/builtins.cpp
@@ -262,7 +262,7 @@ static int do_mkdir(const std::vector<std::string>& args) {
                 "--prompt_and_wipe_data",
                 "--reason=set_policy_failed:"s + args[1]};
             reboot_into_recovery(options);
-            return -1;
+            return 0;
         }
     }
     return 0;
@@ -490,7 +490,8 @@ static int queue_fs_event(int code) {
         /* Setup a wipe via recovery, and reboot into recovery */
         PLOG(ERROR) << "fs_mgr_mount_all suggested recovery, so wiping data via recovery.";
         const std::vector<std::string> options = {"--wipe_data", "--reason=fs_mgr_mount_all" };
-        ret = reboot_into_recovery(options);
+        reboot_into_recovery(options);
+        return 0;
         /* If reboot worked, there is no return. */
     } else if (code == FS_MGR_MNTALL_DEV_FILE_ENCRYPTED) {
         if (e4crypt_install_keyring()) {
diff --git a/init/init.cpp b/init/init.cpp
index 58f5f2337..f65bfe08c 100644
--- a/init/init.cpp
+++ b/init/init.cpp
@@ -96,6 +96,8 @@ static std::unique_ptr<Timer> waiting_for_prop(nullptr);
 static std::string wait_prop_name;
 static std::string wait_prop_value;
 static bool shutting_down;
+static std::string shutdown_command;
+static bool do_shutdown = false;
 
 void DumpState() {
     ServiceManager::GetInstance().DumpState();
@@ -174,9 +176,16 @@ void property_changed(const std::string& name, const std::string& value) {
     // In non-thermal-shutdown case, 'shutdown' trigger will be fired to let device specific
     // commands to be executed.
     if (name == "sys.powerctl") {
-        if (HandlePowerctlMessage(value)) {
-            shutting_down = true;
-        }
+        // Despite the above comment, we can't call HandlePowerctlMessage() 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
+        // action queue.  Instead we set this flag and ensure that shutdown happens before the next
+        // command is run in the main init loop.
+        // TODO: once property service is removed from init, this will never happen from a builtin,
+        // but rather from a callback from the property service socket, in which case this hack can
+        // go away.
+        shutdown_command = value;
+        do_shutdown = true;
     }
 
     if (property_triggers_enabled) ActionManager::GetInstance().QueuePropertyChange(name, value);
@@ -1175,6 +1184,13 @@ int main(int argc, char** argv) {
         // By default, sleep until something happens.
         int epoll_timeout_ms = -1;
 
+        if (do_shutdown && !shutting_down) {
+            do_shutdown = false;
+            if (HandlePowerctlMessage(shutdown_command)) {
+                shutting_down = true;
+            }
+        }
+
         if (!(waiting_for_prop || sm.IsWaitingForExec())) {
             am.ExecuteOneCommand();
         }