Implement susped, resume and cancel for the Postinstall action. This patch sends SIGSTOP/SIGCONT to the running postinstall program to suspend/resume the child process, and SIGKILL when cancelling it. Bug: 27272144 TEST=Added unittest to check the signal being sent. Change-Id: Iebe9bd34448ad1d0a5340c82e1fd839ff8c69dd2
diff --git a/Android.mk b/Android.mk index 20b9b5e..c8bf904 100644 --- a/Android.mk +++ b/Android.mk
@@ -711,7 +711,7 @@ $(call ue-unittest-sample-image,disk_ext2_1k.img) $(call ue-unittest-sample-image,disk_ext2_4k.img) $(call ue-unittest-sample-image,disk_ext2_4k_empty.img) -$(call ue-unittest-sample-image,disk_ext2_ue_settings.img) +$(call ue-unittest-sample-image,disk_ext2_unittest.img) # test_http_server (type: executable) # ======================================================== @@ -747,7 +747,7 @@ ue_unittest_disk_ext2_1k.img \ ue_unittest_disk_ext2_4k.img \ ue_unittest_disk_ext2_4k_empty.img \ - ue_unittest_disk_ext2_ue_settings.img + ue_unittest_disk_ext2_unittest.img LOCAL_MODULE_CLASS := EXECUTABLES LOCAL_CPP_EXTENSION := .cc LOCAL_CLANG := true
diff --git a/common/subprocess.cc b/common/subprocess.cc index f43aaac..61f2d54 100644 --- a/common/subprocess.cc +++ b/common/subprocess.cc
@@ -207,7 +207,12 @@ if (pid_record == subprocess_records_.end()) return; pid_record->second->callback.Reset(); - kill(pid, SIGTERM); + if (kill(pid, SIGTERM) != 0) { + PLOG(WARNING) << "Error sending SIGTERM to " << pid; + } + // Release the pid now so we don't try to kill it if Subprocess is destroyed + // before the corresponding ChildExitedCallback() is called. + pid_record->second->proc.Release(); } bool Subprocess::SynchronousExec(const vector<string>& cmd,
diff --git a/common/test_utils.cc b/common/test_utils.cc index 0e7a8ef..4ab1663 100644 --- a/common/test_utils.cc +++ b/common/test_utils.cc
@@ -103,6 +103,15 @@ 0xbe, 0x9f, 0xa3, 0x5d, }; +string Readlink(const string& path) { + vector<char> buf(PATH_MAX + 1); + ssize_t r = readlink(path.c_str(), buf.data(), buf.size()); + if (r < 0) + return ""; + CHECK_LT(r, static_cast<ssize_t>(buf.size())); + return string(buf.begin(), buf.begin() + r); +} + bool IsXAttrSupported(const base::FilePath& dir_path) { char *path = strdup(dir_path.Append("xattr_test_XXXXXX").value().c_str());
diff --git a/common/test_utils.h b/common/test_utils.h index 2c8a6de..60ec90e 100644 --- a/common/test_utils.h +++ b/common/test_utils.h
@@ -90,6 +90,9 @@ return chdir(path.c_str()); } +// Reads a symlink from disk. Returns empty string on failure. +std::string Readlink(const std::string& path); + // Checks if xattr is supported in the directory specified by // |dir_path| which must be writable. Returns true if the feature is // supported, false if not or if an error occurred.
diff --git a/payload_consumer/postinstall_runner_action.cc b/payload_consumer/postinstall_runner_action.cc index 9ebef53..7fa7009 100644 --- a/payload_consumer/postinstall_runner_action.cc +++ b/payload_consumer/postinstall_runner_action.cc
@@ -16,13 +16,15 @@ #include "update_engine/payload_consumer/postinstall_runner_action.h" +#include <signal.h> #include <stdlib.h> #include <sys/mount.h> +#include <sys/types.h> #include <vector> -#include <base/bind.h> #include <base/files/file_path.h> #include <base/files/file_util.h> +#include <base/logging.h> #include <base/strings/string_util.h> #include "update_engine/common/action_processor.h" @@ -126,25 +128,31 @@ // Runs the postinstall script asynchronously to free up the main loop while // it's running. vector<string> command = {abs_path, partition.target_path}; - if (!Subprocess::Get().Exec( - command, - base::Bind( - &PostinstallRunnerAction::CompletePartitionPostinstall, - base::Unretained(this)))) { + current_command_ = Subprocess::Get().Exec( + command, + base::Bind(&PostinstallRunnerAction::CompletePartitionPostinstall, + base::Unretained(this))); + // Subprocess::Exec should never return a negative process id. + CHECK_GE(current_command_, 0); + + if (!current_command_) CompletePartitionPostinstall(1, "Postinstall didn't launch"); - } } -void PostinstallRunnerAction::CompletePartitionPostinstall( - int return_code, - const string& output) { +void PostinstallRunnerAction::CleanupMount() { utils::UnmountFilesystem(fs_mount_dir_); -#ifndef ANDROID +#ifndef __ANDROID__ if (!base::DeleteFile(base::FilePath(fs_mount_dir_), false)) { PLOG(WARNING) << "Not removing temporary mountpoint " << fs_mount_dir_; } -#endif // !ANDROID +#endif // !__ANDROID__ fs_mount_dir_.clear(); +} + +void PostinstallRunnerAction::CompletePartitionPostinstall( + int return_code, const string& output) { + current_command_ = 0; + CleanupMount(); if (return_code != 0) { LOG(ERROR) << "Postinst command failed with code: " << return_code; @@ -196,4 +204,30 @@ } } +void PostinstallRunnerAction::SuspendAction() { + if (!current_command_) + return; + if (kill(current_command_, SIGSTOP) != 0) { + PLOG(ERROR) << "Couldn't pause child process " << current_command_; + } +} + +void PostinstallRunnerAction::ResumeAction() { + if (!current_command_) + return; + if (kill(current_command_, SIGCONT) != 0) { + PLOG(ERROR) << "Couldn't resume child process " << current_command_; + } +} + +void PostinstallRunnerAction::TerminateProcessing() { + if (!current_command_) + return; + // Calling KillExec() will discard the callback we registered and therefore + // the unretained reference to this object. + Subprocess::Get().KillExec(current_command_); + current_command_ = 0; + CleanupMount(); +} + } // namespace chromeos_update_engine
diff --git a/payload_consumer/postinstall_runner_action.h b/payload_consumer/postinstall_runner_action.h index 08dedd2..a626ce6 100644 --- a/payload_consumer/postinstall_runner_action.h +++ b/payload_consumer/postinstall_runner_action.h
@@ -34,10 +34,11 @@ explicit PostinstallRunnerAction(BootControlInterface* boot_control) : PostinstallRunnerAction(boot_control, nullptr) {} + // InstallPlanAction overrides. void PerformAction() override; - - // Note that there's no support for terminating this action currently. - void TerminateProcessing() override { CHECK(false); } + void SuspendAction() override; + void ResumeAction() override; + void TerminateProcessing() override; // Debugging/logging static std::string StaticType() { return "PostinstallRunnerAction"; } @@ -54,6 +55,9 @@ void PerformPartitionPostinstall(); + // Unmount and remove the mountpoint directory if needed. + void CleanupMount(); + // Subprocess::Exec callback. void CompletePartitionPostinstall(int return_code, const std::string& output); @@ -82,6 +86,9 @@ // file name; used for testing. const char* powerwash_marker_file_; + // Postinstall command currently running, or 0 if no program running. + pid_t current_command_{0}; + DISALLOW_COPY_AND_ASSIGN(PostinstallRunnerAction); };
diff --git a/payload_consumer/postinstall_runner_action_unittest.cc b/payload_consumer/postinstall_runner_action_unittest.cc index 01d8d8f..c6c5ecf 100644 --- a/payload_consumer/postinstall_runner_action_unittest.cc +++ b/payload_consumer/postinstall_runner_action_unittest.cc
@@ -24,6 +24,7 @@ #include <string> #include <vector> +#include <base/bind.h> #include <base/files/file_util.h> #include <base/message_loop/message_loop.h> #include <base/strings/string_util.h> @@ -47,24 +48,30 @@ class PostinstActionProcessorDelegate : public ActionProcessorDelegate { public: - PostinstActionProcessorDelegate() - : code_(ErrorCode::kError), - code_set_(false) {} + PostinstActionProcessorDelegate() = default; void ProcessingDone(const ActionProcessor* processor, - ErrorCode code) { + ErrorCode code) override { MessageLoop::current()->BreakLoop(); + processing_done_called_ = true; } + void ProcessingStopped(const ActionProcessor* processor) override { + MessageLoop::current()->BreakLoop(); + processing_stopped_called_ = true; + } + void ActionCompleted(ActionProcessor* processor, AbstractAction* action, - ErrorCode code) { + ErrorCode code) override { if (action->Type() == PostinstallRunnerAction::StaticType()) { code_ = code; code_set_ = true; } } - ErrorCode code_; - bool code_set_; + ErrorCode code_{ErrorCode::kError}; + bool code_set_{false}; + bool processing_done_called_{false}; + bool processing_stopped_called_{false}; }; class PostinstallRunnerActionTest : public ::testing::Test { @@ -78,9 +85,9 @@ // We use a test-specific powerwash marker file, to avoid race conditions. powerwash_marker_file_ = working_dir_ + "/factory_install_reset"; // These tests use the postinstall files generated by "generate_images.sh" - // stored in the "disk_ext2_ue_settings.img" image. + // stored in the "disk_ext2_unittest.img" image. postinstall_image_ = test_utils::GetBuildArtifactsPath() - .Append("gen/disk_ext2_ue_settings.img") + .Append("gen/disk_ext2_unittest.img") .value(); ASSERT_EQ(0U, getuid()) << "Run these tests as root."; @@ -97,6 +104,49 @@ const string& postinstall_program, bool powerwash_required); + public: + void ResumeRunningAction() { + ASSERT_NE(nullptr, postinstall_action_); + postinstall_action_->ResumeAction(); + } + + void SuspendRunningAction() { + if (!postinstall_action_ || !postinstall_action_->current_command_ || + test_utils::Readlink(base::StringPrintf( + "/proc/%d/fd/0", postinstall_action_->current_command_)) != + "/dev/zero") { + // We need to wait for the postinstall command to start and flag that it + // is ready by redirecting its input to /dev/zero. + loop_.PostDelayedTask( + FROM_HERE, + base::Bind(&PostinstallRunnerActionTest::SuspendRunningAction, + base::Unretained(this)), + base::TimeDelta::FromMilliseconds(100)); + } else { + postinstall_action_->SuspendAction(); + // Schedule to be resumed in a little bit. + loop_.PostDelayedTask( + FROM_HERE, + base::Bind(&PostinstallRunnerActionTest::ResumeRunningAction, + base::Unretained(this)), + base::TimeDelta::FromMilliseconds(100)); + } + } + + void CancelWhenStarted() { + if (!postinstall_action_ || !postinstall_action_->current_command_) { + // Wait for the postinstall command to run. + loop_.PostDelayedTask( + FROM_HERE, + base::Bind(&PostinstallRunnerActionTest::CancelWhenStarted, + base::Unretained(this)), + base::TimeDelta::FromMilliseconds(10)); + } else { + CHECK(processor_); + processor_->StopProcessing(); + } + } + protected: base::MessageLoopForIO base_loop_; brillo::BaseMessageLoop loop_{&base_loop_}; @@ -112,6 +162,10 @@ FakeBootControl fake_boot_control_; PostinstActionProcessorDelegate delegate_; + + // A pointer to the posinstall_runner action and the processor. + PostinstallRunnerAction* postinstall_action_{nullptr}; + ActionProcessor* processor_{nullptr}; }; void PostinstallRunnerActionTest::RunPosinstallAction( @@ -119,6 +173,7 @@ const string& postinstall_program, bool powerwash_required) { ActionProcessor processor; + processor_ = &processor; ObjectFeederAction<InstallPlan> feeder_action; InstallPlan::Partition part; part.name = "part"; @@ -132,6 +187,7 @@ feeder_action.set_obj(install_plan); PostinstallRunnerAction runner_action(&fake_boot_control_, powerwash_marker_file_.c_str()); + postinstall_action_ = &runner_action; BondActions(&feeder_action, &runner_action); ObjectCollectorAction<InstallPlan> collector_action; BondActions(&runner_action, &collector_action); @@ -144,15 +200,14 @@ base::Bind([&processor] { processor.StartProcessing(); })); loop_.Run(); ASSERT_FALSE(processor.IsRunning()); - EXPECT_TRUE(delegate_.code_set_); -} - -// Death tests don't seem to be working on Hardy -TEST_F(PostinstallRunnerActionTest, DISABLED_RunAsRootDeathTest) { - ASSERT_EQ(0U, getuid()); - PostinstallRunnerAction runner_action(&fake_boot_control_); - ASSERT_DEATH({ runner_action.TerminateProcessing(); }, - "postinstall_runner_action.h:.*] Check failed"); + postinstall_action_ = nullptr; + processor_ = nullptr; + EXPECT_TRUE(delegate_.processing_stopped_called_ || + delegate_.processing_done_called_); + if (delegate_.processing_done_called_) { + // Sanity check that the code was set when the processor finishes. + EXPECT_TRUE(delegate_.code_set_); + } } // Test that postinstall succeeds in the simple case of running the default @@ -161,6 +216,7 @@ ScopedLoopbackDeviceBinder loop(postinstall_image_, false, nullptr); RunPosinstallAction(loop.dev(), kPostinstallDefaultScript, false); EXPECT_EQ(ErrorCode::kSuccess, delegate_.code_); + EXPECT_TRUE(delegate_.processing_done_called_); // Since powerwash_required was false, this should not trigger a powerwash. EXPECT_FALSE(utils::FileExists(powerwash_marker_file_.c_str())); @@ -229,4 +285,31 @@ } #endif // __ANDROID__ +// Check that you can suspend/resume postinstall actions. +TEST_F(PostinstallRunnerActionTest, RunAsRootSuspendResumeActionTest) { + ScopedLoopbackDeviceBinder loop(postinstall_image_, false, nullptr); + + // We need to wait for the child to run and setup its signal handler. + loop_.PostTask(FROM_HERE, + base::Bind(&PostinstallRunnerActionTest::SuspendRunningAction, + base::Unretained(this))); + RunPosinstallAction(loop.dev(), "bin/postinst_suspend", false); + // postinst_suspend returns 0 only if it was suspended at some point. + EXPECT_EQ(ErrorCode::kSuccess, delegate_.code_); + EXPECT_TRUE(delegate_.processing_done_called_); +} + +// Test that we can cancel a postinstall action while it is running. +TEST_F(PostinstallRunnerActionTest, RunAsRootCancelPostinstallActionTest) { + ScopedLoopbackDeviceBinder loop(postinstall_image_, false, nullptr); + + // Wait for the action to start and then cancel it. + CancelWhenStarted(); + RunPosinstallAction(loop.dev(), "bin/postinst_suspend", false); + // When canceling the action, the action never finished and therefore we had + // a ProcessingStopped call instead. + EXPECT_FALSE(delegate_.code_set_); + EXPECT_TRUE(delegate_.processing_stopped_called_); +} + } // namespace chromeos_update_engine
diff --git a/payload_generator/ext2_filesystem_unittest.cc b/payload_generator/ext2_filesystem_unittest.cc index f7d25da..17c72d6 100644 --- a/payload_generator/ext2_filesystem_unittest.cc +++ b/payload_generator/ext2_filesystem_unittest.cc
@@ -189,8 +189,8 @@ } TEST_F(Ext2FilesystemTest, LoadSettingsWorksTest) { - base::FilePath path = test_utils::GetBuildArtifactsPath().Append( - "gen/disk_ext2_ue_settings.img"); + base::FilePath path = + test_utils::GetBuildArtifactsPath().Append("gen/disk_ext2_unittest.img"); unique_ptr<Ext2Filesystem> fs = Ext2Filesystem::CreateFromFile(path.value()); ASSERT_NE(nullptr, fs.get());
diff --git a/sample_images/generate_images.sh b/sample_images/generate_images.sh index c9936c1..b461175 100755 --- a/sample_images/generate_images.sh +++ b/sample_images/generate_images.sh
@@ -150,6 +150,22 @@ exit 1 EOF + # A program that succeeds if it is suspended during the first 5 minutes. + sudo tee "${mntdir}"/bin/postinst_suspend >/dev/null <<EOF +#!/etc/../bin/sh +trap "{ echo Got a SIGCONT; exit 0; }" CONT +# Signal that we are ready to receive the signal by redirecting our stdin to +# /dev/zero, the test can detect that. +exec </dev/zero +# Allow the signal handler to run every 100 ms. +i=3000 +while [ \$i -ge 0 ]; do + sleep 0.1 + i=\$((i-1)) +done +exit 1 +EOF + # A postinstall bash program. sudo tee "${mntdir}"/bin/self_check_context >/dev/null <<EOF #!/etc/../bin/sh @@ -193,7 +209,7 @@ sudo mount "${filename}" "${mntdir}" -o loop case "${kind}" in - ue_settings) + unittest) add_files_ue_settings "${mntdir}" "${block_size}" add_files_postinstall "${mntdir}" "${block_size}" ;; @@ -223,7 +239,7 @@ generate_image disk_ext2_1k default 16777216 1024 generate_image disk_ext2_4k default 16777216 4096 generate_image disk_ext2_4k_empty empty $((1024 * 4096)) 4096 - generate_image disk_ext2_ue_settings ue_settings $((1024 * 4096)) 4096 + generate_image disk_ext2_unittest unittest $((1024 * 4096)) 4096 # Generate the tarball and delete temporary images. echo "Packing tar file sample_images.tar.bz2"
diff --git a/sample_images/sample_images.tar.bz2 b/sample_images/sample_images.tar.bz2 index d77ae93..20a698b 100644 --- a/sample_images/sample_images.tar.bz2 +++ b/sample_images/sample_images.tar.bz2 Binary files differ