Don't rollback if the other partition isn't valid.
Before we start a rollback to the other OS slot, validate the GPT flags show
it as bootable. This should prevent us from attempting a rollback if an
update has been attempted and failed, or is currently in progress. Such
a rollback would always fail, since the other partition would be left in
a partially modified state.
Piggyback:
Move sanity test in hardware that was added to the wrong method.
Undid some unittest changes that were decided against after the fact.
BUG=chromium:267054
TEST=Unittests
Manual Update Rollbacks (with/without flags on other partition)
Change-Id: Ide6b0673855ba2e4b05a0db93413a1a9f2ece2a9
Reviewed-on: https://chromium-review.googlesource.com/176755
Reviewed-by: David Zeuthen <zeuthen@chromium.org>
Commit-Queue: Don Garrett <dgarrett@chromium.org>
Tested-by: Don Garrett <dgarrett@chromium.org>
diff --git a/delta_performer_unittest.cc b/delta_performer_unittest.cc
index d2a3ebd..80aa064 100644
--- a/delta_performer_unittest.cc
+++ b/delta_performer_unittest.cc
@@ -1162,7 +1162,7 @@
DeltaPerformer *performer = new DeltaPerformer(&prefs,
&mock_system_state,
&install_plan);
- FakeHardware& fake_hardware = mock_system_state.get_mock_hardware().fake();
+ FakeHardware* fake_hardware = mock_system_state.get_fake_hardware();
string temp_dir;
EXPECT_TRUE(utils::MakeTempDirectory("/tmp/PublicKeyFromResponseTests.XXXXXX",
@@ -1172,46 +1172,46 @@
EXPECT_EQ(0, System(StringPrintf("touch %s", existing_file.c_str())));
// Non-official build, non-existing public-key, key in response -> true
- fake_hardware.SetIsOfficialBuild(false);
+ fake_hardware->SetIsOfficialBuild(false);
performer->public_key_path_ = non_existing_file;
install_plan.public_key_rsa = "VGVzdAo="; // result of 'echo "Test" | base64'
EXPECT_TRUE(performer->GetPublicKeyFromResponse(&key_path));
EXPECT_FALSE(key_path.empty());
EXPECT_EQ(unlink(key_path.value().c_str()), 0);
// Same with official build -> false
- fake_hardware.SetIsOfficialBuild(true);
+ fake_hardware->SetIsOfficialBuild(true);
EXPECT_FALSE(performer->GetPublicKeyFromResponse(&key_path));
// Non-official build, existing public-key, key in response -> false
- fake_hardware.SetIsOfficialBuild(false);
+ fake_hardware->SetIsOfficialBuild(false);
performer->public_key_path_ = existing_file;
install_plan.public_key_rsa = "VGVzdAo="; // result of 'echo "Test" | base64'
EXPECT_FALSE(performer->GetPublicKeyFromResponse(&key_path));
// Same with official build -> false
- fake_hardware.SetIsOfficialBuild(true);
+ fake_hardware->SetIsOfficialBuild(true);
EXPECT_FALSE(performer->GetPublicKeyFromResponse(&key_path));
// Non-official build, non-existing public-key, no key in response -> false
- fake_hardware.SetIsOfficialBuild(false);
+ fake_hardware->SetIsOfficialBuild(false);
performer->public_key_path_ = non_existing_file;
install_plan.public_key_rsa = "";
EXPECT_FALSE(performer->GetPublicKeyFromResponse(&key_path));
// Same with official build -> false
- fake_hardware.SetIsOfficialBuild(true);
+ fake_hardware->SetIsOfficialBuild(true);
EXPECT_FALSE(performer->GetPublicKeyFromResponse(&key_path));
// Non-official build, existing public-key, no key in response -> false
- fake_hardware.SetIsOfficialBuild(false);
+ fake_hardware->SetIsOfficialBuild(false);
performer->public_key_path_ = existing_file;
install_plan.public_key_rsa = "";
EXPECT_FALSE(performer->GetPublicKeyFromResponse(&key_path));
// Same with official build -> false
- fake_hardware.SetIsOfficialBuild(true);
+ fake_hardware->SetIsOfficialBuild(true);
EXPECT_FALSE(performer->GetPublicKeyFromResponse(&key_path));
// Non-official build, non-existing public-key, key in response
// but invalid base64 -> false
- fake_hardware.SetIsOfficialBuild(false);
+ fake_hardware->SetIsOfficialBuild(false);
performer->public_key_path_ = non_existing_file;
install_plan.public_key_rsa = "not-valid-base64";
EXPECT_FALSE(performer->GetPublicKeyFromResponse(&key_path));
diff --git a/filesystem_copier_action_unittest.cc b/filesystem_copier_action_unittest.cc
index 8b6e627..3e88b42 100644
--- a/filesystem_copier_action_unittest.cc
+++ b/filesystem_copier_action_unittest.cc
@@ -17,6 +17,7 @@
#include "update_engine/filesystem_copier_action.h"
#include "update_engine/filesystem_iterator.h"
+#include "update_engine/mock_hardware.h"
#include "update_engine/mock_system_state.h"
#include "update_engine/omaha_hash_calculator.h"
#include "update_engine/test_utils.h"
@@ -41,6 +42,8 @@
}
void TearDown() {
}
+
+ MockSystemState mock_system_state_;
};
class FilesystemCopierActionTestDelegate : public ActionProcessorDelegate {
@@ -119,7 +122,10 @@
bool terminate_early,
bool use_kernel_partition,
int verify_hash) {
- MockSystemState mock_system_state;
+ // We need MockHardware to verify MarkUnbootable calls, but don't want
+ // warnings about other usages.
+ testing::NiceMock<MockHardware> mock_hardware;
+ mock_system_state_.set_hardware(&mock_hardware);
GMainLoop *loop = g_main_loop_new(g_main_context_default(), FALSE);
@@ -197,13 +203,13 @@
}
}
- EXPECT_CALL(mock_system_state.get_mock_hardware(),
+ EXPECT_CALL(mock_hardware,
MarkKernelUnbootable(a_dev)).Times(use_kernel_partition ? 1 : 0);
ActionProcessor processor;
ObjectFeederAction<InstallPlan> feeder_action;
- FilesystemCopierAction copier_action(&mock_system_state,
+ FilesystemCopierAction copier_action(&mock_system_state_,
use_kernel_partition,
verify_hash != 0);
ObjectCollectorAction<InstallPlan> collector_action;
@@ -276,8 +282,7 @@
LOG(INFO) << "Verifying bootable flag on: " << a_dev;
bool bootable;
- EXPECT_TRUE(mock_system_state.get_mock_hardware().fake().
- IsKernelBootable(a_dev, &bootable));
+ EXPECT_TRUE(mock_hardware.fake().IsKernelBootable(a_dev, &bootable));
// We should always mark a partition as unbootable if it's a kernel
// partition, but never if it's anything else.
EXPECT_EQ(bootable, !use_kernel_partition);
@@ -302,12 +307,11 @@
TEST_F(FilesystemCopierActionTest, MissingInputObjectTest) {
ActionProcessor processor;
- MockSystemState mock_system_state;
FilesystemCopierActionTest2Delegate delegate;
processor.set_delegate(&delegate);
- FilesystemCopierAction copier_action(&mock_system_state, false, false);
+ FilesystemCopierAction copier_action(&mock_system_state_, false, false);
ObjectCollectorAction<InstallPlan> collector_action;
BondActions(&copier_action, &collector_action);
@@ -322,7 +326,6 @@
TEST_F(FilesystemCopierActionTest, ResumeTest) {
ActionProcessor processor;
- MockSystemState mock_system_state;
FilesystemCopierActionTest2Delegate delegate;
processor.set_delegate(&delegate);
@@ -331,7 +334,7 @@
const char* kUrl = "http://some/url";
InstallPlan install_plan(false, true, kUrl, 0, "", 0, "", "", "", "");
feeder_action.set_obj(install_plan);
- FilesystemCopierAction copier_action(&mock_system_state, false, false);
+ FilesystemCopierAction copier_action(&mock_system_state_, false, false);
ObjectCollectorAction<InstallPlan> collector_action;
BondActions(&feeder_action, &copier_action);
@@ -349,7 +352,6 @@
TEST_F(FilesystemCopierActionTest, NonExistentDriveTest) {
ActionProcessor processor;
- MockSystemState mock_system_state;
FilesystemCopierActionTest2Delegate delegate;
processor.set_delegate(&delegate);
@@ -366,7 +368,7 @@
"/no/such/file",
"");
feeder_action.set_obj(install_plan);
- FilesystemCopierAction copier_action(&mock_system_state, false, false);
+ FilesystemCopierAction copier_action(&mock_system_state_, false, false);
ObjectCollectorAction<InstallPlan> collector_action;
BondActions(&copier_action, &collector_action);
@@ -403,7 +405,6 @@
}
TEST_F(FilesystemCopierActionTest, RunAsRootDetermineFilesystemSizeTest) {
- MockSystemState mock_system_state;
string img;
EXPECT_TRUE(utils::MakeTempFile("/tmp/img.XXXXXX", &img, NULL));
ScopedPathUnlinker img_unlinker(img);
@@ -416,7 +417,7 @@
for (int i = 0; i < 2; ++i) {
bool is_kernel = i == 1;
- FilesystemCopierAction action(&mock_system_state, is_kernel, false);
+ FilesystemCopierAction action(&mock_system_state_, is_kernel, false);
EXPECT_EQ(kint64max, action.filesystem_size_);
{
int fd = HANDLE_EINTR(open(img.c_str(), O_RDONLY));
diff --git a/hardware.cc b/hardware.cc
index d547c2d..6213739 100644
--- a/hardware.cc
+++ b/hardware.cc
@@ -51,12 +51,6 @@
bool Hardware::IsKernelBootable(const std::string& kernel_device,
bool* bootable) {
-
- if (kernel_device == BootKernelDevice()) {
- LOG(ERROR) << "Refusing to mark current kernel as unbootable.";
- return false;
- }
-
CgptAddParams params;
memset(¶ms, '\0', sizeof(params));
@@ -78,6 +72,11 @@
bool Hardware::MarkKernelUnbootable(const std::string& kernel_device) {
LOG(INFO) << "MarkPartitionUnbootable: " << kernel_device;
+ if (kernel_device == BootKernelDevice()) {
+ LOG(ERROR) << "Refusing to mark current kernel as unbootable.";
+ return false;
+ }
+
string root_dev = utils::RootDevice(kernel_device);
string partition_number_str = utils::PartitionNumber(kernel_device);
uint32_t partition_number = atoi(partition_number_str.c_str());
diff --git a/mock_system_state.h b/mock_system_state.h
index c3fb32b..193438b 100644
--- a/mock_system_state.h
+++ b/mock_system_state.h
@@ -15,7 +15,7 @@
#include "update_engine/mock_p2p_manager.h"
#include "update_engine/mock_payload_state.h"
#include "update_engine/clock.h"
-#include "update_engine/mock_hardware.h"
+#include "update_engine/fake_hardware.h"
#include "update_engine/prefs_mock.h"
#include "update_engine/system_state.h"
@@ -96,8 +96,8 @@
hardware_ = hardware;
}
- inline MockHardware& get_mock_hardware() {
- return default_hardware_;
+ inline FakeHardware* get_fake_hardware() {
+ return &default_hardware_;
}
inline void set_prefs(PrefsInterface* prefs) {
@@ -145,7 +145,7 @@
// These are the other object we own.
Clock default_clock_;
- MockHardware default_hardware_;
+ FakeHardware default_hardware_;
OmahaRequestParams default_request_params_;
// These are pointers to objects which caller can override.
diff --git a/payload_state_unittest.cc b/payload_state_unittest.cc
index 6616684..eb231cb 100644
--- a/payload_state_unittest.cc
+++ b/payload_state_unittest.cc
@@ -1360,8 +1360,8 @@
prefs.Init(FilePath(temp_dir));
mock_system_state.set_prefs(&prefs);
- FakeHardware& fake_hardware = mock_system_state.get_mock_hardware().fake();
- fake_hardware.SetBootDevice("/dev/sda3");
+ FakeHardware* fake_hardware = mock_system_state.get_fake_hardware();
+ fake_hardware->SetBootDevice("/dev/sda3");
EXPECT_TRUE(payload_state.Initialize(&mock_system_state));
SetupPayloadStateWith2Urls("Hash3141", true, &payload_state, &response);
@@ -1410,8 +1410,8 @@
prefs.Init(FilePath(temp_dir));
mock_system_state.set_prefs(&prefs);
- FakeHardware& fake_hardware = mock_system_state.get_mock_hardware().fake();
- fake_hardware.SetBootDevice("/dev/sda3");
+ FakeHardware* fake_hardware = mock_system_state.get_fake_hardware();
+ fake_hardware->SetBootDevice("/dev/sda3");
EXPECT_TRUE(payload_state.Initialize(&mock_system_state));
SetupPayloadStateWith2Urls("Hash3141", true, &payload_state, &response);
@@ -1422,7 +1422,7 @@
payload_state.ExpectRebootInNewVersion("Version:12345678");
// Change the BootDevice to a different one, no metric should be sent.
- fake_hardware.SetBootDevice("/dev/sda5");
+ fake_hardware->SetBootDevice("/dev/sda5");
EXPECT_CALL(*mock_system_state.mock_metrics_lib(), SendToUMA(
"Installer.RebootToNewPartitionAttempt", _, _, _, _))
@@ -1431,7 +1431,7 @@
// A second reboot in eiher partition should not send a metric.
payload_state.ReportFailedBootIfNeeded();
- fake_hardware.SetBootDevice("/dev/sda3");
+ fake_hardware->SetBootDevice("/dev/sda3");
payload_state.ReportFailedBootIfNeeded();
EXPECT_TRUE(utils::RecursiveUnlinkDir(temp_dir));
diff --git a/update_attempter.cc b/update_attempter.cc
index ea307f5..292d816 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -729,6 +729,23 @@
install_plan.kernel_install_path =
utils::KernelDeviceOfBootDevice(install_plan.install_path);
+
+ // Check to see if the kernel we want to rollback too is bootable.
+ LOG(INFO) << "Validating there is something to rollback too at: "
+ << install_plan.kernel_install_path;
+ bool rollback_bootable;
+ if (!system_state_->hardware()->IsKernelBootable(
+ install_plan.kernel_install_path,
+ &rollback_bootable)) {
+ LOG(ERROR) << "Unable to read GPT kernel flags.";
+ return false;
+ }
+
+ if (!rollback_bootable) {
+ LOG(ERROR) << "There is no valid OS to rollback too.";
+ return false;
+ }
+
install_plan.powerwash_required = powerwash;
if (powerwash) {
// Enterprise-enrolled devices have an empty owner in their device policy.
diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index 10af01e..0ec41c3 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -95,11 +95,14 @@
void UpdateTestStart();
void UpdateTestVerify();
- void RollbackTestStart(bool enterprise_rollback, bool stable_channel);
+ void RollbackTestStart(bool enterprise_rollback,
+ bool stable_channel,
+ bool valid_slot);
void RollbackTestVerify();
static gboolean StaticUpdateTestStart(gpointer data);
static gboolean StaticUpdateTestVerify(gpointer data);
static gboolean StaticRollbackTestStart(gpointer data);
+ static gboolean StaticInvalidSlotRollbackTestStart(gpointer data);
static gboolean StaticEnterpriseRollbackTestStart(gpointer data);
static gboolean StaticStableChannelRollbackTestStart(gpointer data);
static gboolean StaticRollbackTestVerify(gpointer data);
@@ -331,19 +334,28 @@
}
gboolean UpdateAttempterTest::StaticRollbackTestStart(gpointer data) {
- reinterpret_cast<UpdateAttempterTest*>(data)->RollbackTestStart(false, false);
+ reinterpret_cast<UpdateAttempterTest*>(data)->RollbackTestStart(
+ false, false, true);
+ return FALSE;
+}
+
+gboolean UpdateAttempterTest::StaticInvalidSlotRollbackTestStart(
+ gpointer data) {
+ reinterpret_cast<UpdateAttempterTest*>(data)->RollbackTestStart(
+ false, false, false);
return FALSE;
}
gboolean UpdateAttempterTest::StaticEnterpriseRollbackTestStart(gpointer data) {
- reinterpret_cast<UpdateAttempterTest*>(data)->RollbackTestStart(true, false);
+ reinterpret_cast<UpdateAttempterTest*>(data)->RollbackTestStart(
+ true, false, true);
return FALSE;
}
gboolean UpdateAttempterTest::StaticStableChannelRollbackTestStart(
gpointer data) {
reinterpret_cast<UpdateAttempterTest*>(data)->RollbackTestStart(
- false, true);
+ false, true, true);
return FALSE;
}
@@ -455,7 +467,7 @@
}
void UpdateAttempterTest::RollbackTestStart(
- bool enterprise_rollback, bool stable_channel) {
+ bool enterprise_rollback, bool stable_channel, bool valid_slot) {
// Create a device policy so that we can change settings.
policy::MockDevicePolicy* device_policy = new policy::MockDevicePolicy();
attempter_.policy_provider_.reset(new policy::PolicyProvider(device_policy));
@@ -464,12 +476,19 @@
EXPECT_CALL(mock_system_state_, device_policy()).WillRepeatedly(
Return(device_policy));
+ if (!valid_slot) {
+ string rollback_kernel = "/dev/sda2";
+ LOG(INFO) << "Test Mark Unbootable: " << rollback_kernel;
+ mock_system_state_.get_fake_hardware()->MarkKernelUnbootable(
+ rollback_kernel);
+ }
+
string install_path = "/dev/sda3";
bool is_rollback_allowed = false;
// We only allow rollback on devices that are neither enterprise enrolled or
- // not on the stable channel.
- if (!(enterprise_rollback || stable_channel)) {
+ // not on the stable channel, and which have a valid slot to rollback too.
+ if (!(enterprise_rollback || stable_channel) && valid_slot) {
is_rollback_allowed = true;
}
@@ -477,14 +496,16 @@
if (stable_channel) {
attempter_.omaha_request_params_->set_current_channel(
string("stable-channel"));
- } else if (enterprise_rollback) {
+ }
+
+ if (enterprise_rollback) {
// We return an empty owner as this is an enterprise.
- EXPECT_CALL(*device_policy, GetOwner(_)).WillOnce(
+ EXPECT_CALL(*device_policy, GetOwner(_)).WillRepeatedly(
DoAll(SetArgumentPointee<0>(std::string("")),
Return(true)));
} else {
// We return a fake owner as this is an owned consumer device.
- EXPECT_CALL(*device_policy, GetOwner(_)).WillOnce(
+ EXPECT_CALL(*device_policy, GetOwner(_)).WillRepeatedly(
DoAll(SetArgumentPointee<0>(std::string("fake.mail@fake.com")),
Return(true)));
}
@@ -539,6 +560,14 @@
loop_ = NULL;
}
+TEST_F(UpdateAttempterTest, InvalidSlotRollbackTest) {
+ loop_ = g_main_loop_new(g_main_context_default(), FALSE);
+ g_idle_add(&StaticInvalidSlotRollbackTestStart, this);
+ g_main_loop_run(loop_);
+ g_main_loop_unref(loop_);
+ loop_ = NULL;
+}
+
TEST_F(UpdateAttempterTest, StableChannelRollbackTest) {
loop_ = g_main_loop_new(g_main_context_default(), FALSE);
g_idle_add(&StaticStableChannelRollbackTestStart, this);