Revert "Revert "AU: do not copy filesystem during full updates""
This reverts commit d1cd325c3135d88498483da811b594ba6b91ce42
The problem that caused all autotests to fail with the original CL has now been rectified; lab devservers were updated to send the correct delta flag in their omaha response.
Change-Id: I664afb33f72856572baaa658cbd473c07271af36
Reviewed-on: https://gerrit.chromium.org/gerrit/56600
Reviewed-by: Gilad Arnold <[email protected]>
Tested-by: Gilad Arnold <[email protected]>
Commit-Queue: Gilad Arnold <[email protected]>
diff --git a/delta_performer.cc b/delta_performer.cc
index 360272f..5615801 100644
--- a/delta_performer.cc
+++ b/delta_performer.cc
@@ -383,6 +383,11 @@
if (result == kMetadataParseInsufficientData) {
return true;
}
+
+ // Checks the integrity of the payload manifest.
+ if ((*error = ValidateManifest()) != kErrorCodeSuccess)
+ return false;
+
// Remove protobuf and header info from buffer_, so buffer_ contains
// just data blobs
DiscardBufferHeadBytes(manifest_metadata_size_);
@@ -831,6 +836,30 @@
return kErrorCodeSuccess;
}
+ErrorCode DeltaPerformer::ValidateManifest() {
+ // Ensure that a full update does not contain old partition hashes, which is
+ // indicative of a delta.
+ //
+ // TODO(garnold) in general, the presence of an old partition hash should be
+ // the sole indicator for a delta update, as we would generally like update
+ // payloads to be self contained and not assume an Omaha response to tell us
+ // that. However, since this requires some massive reengineering of the update
+ // flow (making filesystem copying happen conditionally only *after*
+ // downloading and parsing of the update manifest) we'll put it off for now.
+ // See chromium-os:7597 for further discussion.
+ if (install_plan_->is_full_update &&
+ (manifest_.has_old_kernel_info() || manifest_.has_old_rootfs_info())) {
+ LOG(ERROR) << "Purported full payload contains old partition "
+ "hash(es), aborting update";
+ return kErrorCodePayloadMismatchedType;
+ }
+
+ // TODO(garnold) we should be adding more and more manifest checks, such as
+ // partition boundaries etc (see chromium-os:37661).
+
+ return kErrorCodeSuccess;
+}
+
ErrorCode DeltaPerformer::ValidateOperationHash(
const DeltaArchiveManifest_InstallOperation& operation) {
diff --git a/delta_performer.h b/delta_performer.h
index 1f8f5cb..33e39d7 100644
--- a/delta_performer.h
+++ b/delta_performer.h
@@ -193,6 +193,10 @@
bool CanPerformInstallOperation(
const DeltaArchiveManifest_InstallOperation& operation);
+ // Checks the integrity of the payload manifest. Returns true upon success,
+ // false otherwise.
+ ErrorCode ValidateManifest();
+
// Validates that the hash of the blobs corresponding to the given |operation|
// matches what's specified in the manifest in the payload.
// Returns kErrorCodeSuccess on match or a suitable error code otherwise.
diff --git a/download_action_unittest.cc b/download_action_unittest.cc
index 3911d55..e937d71 100644
--- a/download_action_unittest.cc
+++ b/download_action_unittest.cc
@@ -131,6 +131,7 @@
OmahaHashCalculator::OmahaHashOfBytes(&data[1], data.size() - 1);
uint64_t size = data.size();
InstallPlan install_plan(false,
+ false,
"",
size,
hash,
@@ -252,7 +253,8 @@
// takes ownership of passed in HttpFetcher
ObjectFeederAction<InstallPlan> feeder_action;
- InstallPlan install_plan(false, "", 0, "", 0, "", temp_file.GetPath(), "");
+ InstallPlan install_plan(false, false, "", 0, "", 0, "",
+ temp_file.GetPath(), "");
feeder_action.set_obj(install_plan);
PrefsMock prefs;
DownloadAction download_action(&prefs, NULL,
@@ -354,6 +356,7 @@
// takes ownership of passed in HttpFetcher
InstallPlan install_plan(false,
+ false,
"",
1,
OmahaHashCalculator::OmahaHashOfString("x"),
@@ -395,7 +398,7 @@
DirectFileWriter writer;
// takes ownership of passed in HttpFetcher
- InstallPlan install_plan(false, "", 0, "", 0, "", path, "");
+ InstallPlan install_plan(false, false, "", 0, "", 0, "", path, "");
ObjectFeederAction<InstallPlan> feeder_action;
feeder_action.set_obj(install_plan);
PrefsMock prefs;
diff --git a/error_code.h b/error_code.h
index db023f7..aab0917 100644
--- a/error_code.h
+++ b/error_code.h
@@ -15,7 +15,7 @@
kErrorCodeOmahaResponseHandlerError = 3,
kErrorCodeFilesystemCopierError = 4,
kErrorCodePostinstallRunnerError = 5,
- kErrorCodeSetBootableFlagError = 6, // TODO(petkov): Unused. Recycle?
+ kErrorCodePayloadMismatchedType = 6,
kErrorCodeInstallDeviceOpenError = 7,
kErrorCodeKernelDeviceOpenError = 8,
kErrorCodeDownloadTransferError = 9,
diff --git a/filesystem_copier_action.cc b/filesystem_copier_action.cc
index 7024b67..3e5160e 100644
--- a/filesystem_copier_action.cc
+++ b/filesystem_copier_action.cc
@@ -69,8 +69,11 @@
return;
}
install_plan_ = GetInputObject();
- if (!verify_hash_ && install_plan_.is_resume) {
+ if (!verify_hash_ &&
+ (install_plan_.is_resume || install_plan_.is_full_update)) {
// No copy or hash verification needed. Done!
+ LOG(INFO) << "filesystem copying skipped: "
+ << (install_plan_.is_resume ? "resumed" : "full") << " update";
if (HasOutputPipe())
SetOutputObject(install_plan_);
abort_action_completer.set_code(kErrorCodeSuccess);
diff --git a/filesystem_copier_action_unittest.cc b/filesystem_copier_action_unittest.cc
index 3ffed88..0db1640 100644
--- a/filesystem_copier_action_unittest.cc
+++ b/filesystem_copier_action_unittest.cc
@@ -311,7 +311,7 @@
ObjectFeederAction<InstallPlan> feeder_action;
const char* kUrl = "http://some/url";
- InstallPlan install_plan(true, kUrl, 0, "", 0, "", "", "");
+ InstallPlan install_plan(false, true, kUrl, 0, "", 0, "", "", "");
feeder_action.set_obj(install_plan);
FilesystemCopierAction copier_action(false, false);
ObjectCollectorAction<InstallPlan> collector_action;
@@ -337,6 +337,7 @@
ObjectFeederAction<InstallPlan> feeder_action;
InstallPlan install_plan(false,
+ false,
"",
0,
"",
diff --git a/install_plan.cc b/install_plan.cc
index 59bd5da..a17cb52 100644
--- a/install_plan.cc
+++ b/install_plan.cc
@@ -13,6 +13,7 @@
namespace chromeos_update_engine {
InstallPlan::InstallPlan(bool is_resume,
+ bool is_full_update,
const string& url,
uint64_t payload_size,
const string& payload_hash,
@@ -21,6 +22,7 @@
const string& install_path,
const string& kernel_install_path)
: is_resume(is_resume),
+ is_full_update(is_full_update),
download_url(url),
payload_size(payload_size),
payload_hash(payload_hash),
@@ -34,6 +36,7 @@
powerwash_required(false) {}
InstallPlan::InstallPlan() : is_resume(false),
+ is_full_update(false), // play it safe.
payload_size(0),
metadata_size(0),
kernel_size(0),
@@ -44,6 +47,7 @@
bool InstallPlan::operator==(const InstallPlan& that) const {
return ((is_resume == that.is_resume) &&
+ (is_full_update == that.is_full_update) &&
(download_url == that.download_url) &&
(payload_size == that.payload_size) &&
(payload_hash == that.payload_hash) &&
@@ -59,7 +63,8 @@
void InstallPlan::Dump() const {
LOG(INFO) << "InstallPlan: "
- << (is_resume ? ", resume" : ", new_update")
+ << (is_resume ? "resume" : "new_update")
+ << ", payload type: " << (is_full_update ? "full" : "delta")
<< ", url: " << download_url
<< ", payload size: " << payload_size
<< ", payload hash: " << payload_hash
diff --git a/install_plan.h b/install_plan.h
index fc33f25..d6ff9e0 100644
--- a/install_plan.h
+++ b/install_plan.h
@@ -16,6 +16,7 @@
struct InstallPlan {
InstallPlan(bool is_resume,
+ bool is_full_update,
const std::string& url,
uint64_t payload_size,
const std::string& payload_hash,
@@ -34,6 +35,7 @@
void Dump() const;
bool is_resume;
+ bool is_full_update;
std::string download_url; // url to download from
uint64_t payload_size; // size of the payload
diff --git a/omaha_response_handler_action.cc b/omaha_response_handler_action.cc
index c85711b..c876d94 100644
--- a/omaha_response_handler_action.cc
+++ b/omaha_response_handler_action.cc
@@ -70,6 +70,7 @@
kPrefsUpdateCheckResponseHash, response.hash))
<< "Unable to save the update check response hash.";
}
+ install_plan_.is_full_update = !response.is_delta_payload;
TEST_AND_RETURN(GetInstallDev(
(!boot_device_.empty() ? boot_device_ : utils::BootDevice()),
diff --git a/payload_state.cc b/payload_state.cc
index 3de9e04..0636f56 100644
--- a/payload_state.cc
+++ b/payload_state.cc
@@ -189,6 +189,7 @@
case kErrorCodeDownloadInvalidMetadataSignature:
case kErrorCodeDownloadOperationHashMissingError:
case kErrorCodeDownloadMetadataSignatureMissingError:
+ case kErrorCodePayloadMismatchedType:
IncrementUrlIndex();
break;
@@ -239,7 +240,6 @@
break;
case kErrorCodeSuccess: // success code
- case kErrorCodeSetBootableFlagError: // unused
case kErrorCodeUmaReportedMax: // not an error code
case kErrorCodeOmahaRequestHTTPResponseBase: // aggregated already
case kErrorCodeDevModeFlag: // not an error code
diff --git a/utils.cc b/utils.cc
index be98a71..a6d8d91 100644
--- a/utils.cc
+++ b/utils.cc
@@ -848,8 +848,8 @@
return "kErrorCodeFilesystemCopierError";
case kErrorCodePostinstallRunnerError:
return "kErrorCodePostinstallRunnerError";
- case kErrorCodeSetBootableFlagError:
- return "kErrorCodeSetBootableFlagError";
+ case kErrorCodePayloadMismatchedType:
+ return "kErrorCodePayloadMismatchedType";
case kErrorCodeInstallDeviceOpenError:
return "kErrorCodeInstallDeviceOpenError";
case kErrorCodeKernelDeviceOpenError:
