Recover source hash failures on other source operations.
In addition to SOURCE_COPY, now SOURCE_BSDIFF, BROTLI_BSDIFF and
PUFFDIFF will also recover source hash failures using error correction
data.
Bug: 34284069
Test: update_engine_unittests
Change-Id: I536d0675e3550aa142b77b4c8bf0a9f70a789652
diff --git a/payload_consumer/delta_performer.cc b/payload_consumer/delta_performer.cc
index 3d8a044..a986b07 100644
--- a/payload_consumer/delta_performer.cc
+++ b/payload_consumer/delta_performer.cc
@@ -1138,6 +1138,56 @@
return true;
}
+FileDescriptorPtr DeltaPerformer::ChooseSourceFD(
+ const InstallOperation& operation, ErrorCode* error) {
+ if (!operation.has_src_sha256_hash()) {
+ // When the operation doesn't include a source hash, we attempt the error
+ // corrected device first since we can't verify the block in the raw device
+ // at this point, but we first need to make sure all extents are readable
+ // since the error corrected device can be shorter or not available.
+ if (OpenCurrentECCPartition() &&
+ fd_utils::ReadAndHashExtents(
+ source_ecc_fd_, operation.src_extents(), block_size_, nullptr)) {
+ return source_ecc_fd_;
+ }
+ return source_fd_;
+ }
+
+ brillo::Blob source_hash;
+ brillo::Blob expected_source_hash(operation.src_sha256_hash().begin(),
+ operation.src_sha256_hash().end());
+ if (fd_utils::ReadAndHashExtents(
+ source_fd_, operation.src_extents(), block_size_, &source_hash) &&
+ source_hash == expected_source_hash) {
+ return source_fd_;
+ }
+ // We fall back to use the error corrected device if the hash of the raw
+ // device doesn't match or there was an error reading the source partition.
+ if (!OpenCurrentECCPartition()) {
+ // The following function call will return false since the source hash
+ // mismatches, but we still want to call it so it prints the appropriate
+ // log message.
+ ValidateSourceHash(source_hash, operation, source_fd_, error);
+ return nullptr;
+ }
+ LOG(WARNING) << "Source hash from RAW device mismatched: found "
+ << base::HexEncode(source_hash.data(), source_hash.size())
+ << ", expected "
+ << base::HexEncode(expected_source_hash.data(),
+ expected_source_hash.size());
+
+ if (fd_utils::ReadAndHashExtents(
+ source_ecc_fd_, operation.src_extents(), block_size_, &source_hash) &&
+ ValidateSourceHash(source_hash, operation, source_ecc_fd_, error)) {
+ // At this point reading from the the error corrected device worked, but
+ // reading from the raw device failed, so this is considered a recovered
+ // failure.
+ source_ecc_recovered_failures_++;
+ return source_ecc_fd_;
+ }
+ return nullptr;
+}
+
bool DeltaPerformer::ExtentsToBsdiffPositionsString(
const RepeatedPtrField<Extent>& extents,
uint64_t block_size,
@@ -1280,17 +1330,12 @@
if (operation.has_dst_length())
TEST_AND_RETURN_FALSE(operation.dst_length() % block_size_ == 0);
- if (operation.has_src_sha256_hash()) {
- brillo::Blob source_hash;
- TEST_AND_RETURN_FALSE(fd_utils::ReadAndHashExtents(
- source_fd_, operation.src_extents(), block_size_, &source_hash));
- TEST_AND_RETURN_FALSE(
- ValidateSourceHash(source_hash, operation, source_fd_, error));
- }
+ FileDescriptorPtr source_fd = ChooseSourceFD(operation, error);
+ TEST_AND_RETURN_FALSE(source_fd != nullptr);
auto reader = std::make_unique<DirectExtentReader>();
TEST_AND_RETURN_FALSE(
- reader->Init(source_fd_, operation.src_extents(), block_size_));
+ reader->Init(source_fd, operation.src_extents(), block_size_));
auto src_file = std::make_unique<BsdiffExtentFile>(
std::move(reader),
utils::BlocksInExtents(operation.src_extents()) * block_size_);
@@ -1397,17 +1442,12 @@
TEST_AND_RETURN_FALSE(buffer_offset_ == operation.data_offset());
TEST_AND_RETURN_FALSE(buffer_.size() >= operation.data_length());
- if (operation.has_src_sha256_hash()) {
- brillo::Blob source_hash;
- TEST_AND_RETURN_FALSE(fd_utils::ReadAndHashExtents(
- source_fd_, operation.src_extents(), block_size_, &source_hash));
- TEST_AND_RETURN_FALSE(
- ValidateSourceHash(source_hash, operation, source_fd_, error));
- }
+ FileDescriptorPtr source_fd = ChooseSourceFD(operation, error);
+ TEST_AND_RETURN_FALSE(source_fd != nullptr);
auto reader = std::make_unique<DirectExtentReader>();
TEST_AND_RETURN_FALSE(
- reader->Init(source_fd_, operation.src_extents(), block_size_));
+ reader->Init(source_fd, operation.src_extents(), block_size_));
puffin::UniqueStreamPtr src_stream(new PuffinExtentStream(
std::move(reader),
utils::BlocksInExtents(operation.src_extents()) * block_size_));
diff --git a/payload_consumer/delta_performer.h b/payload_consumer/delta_performer.h
index 57bad26..38d2c43 100644
--- a/payload_consumer/delta_performer.h
+++ b/payload_consumer/delta_performer.h
@@ -175,6 +175,7 @@
friend class DeltaPerformerIntegrationTest;
FRIEND_TEST(DeltaPerformerTest, BrilloMetadataSignatureSizeTest);
FRIEND_TEST(DeltaPerformerTest, BrilloParsePayloadMetadataTest);
+ FRIEND_TEST(DeltaPerformerTest, ChooseSourceFDTest);
FRIEND_TEST(DeltaPerformerTest, UsePublicKeyFromResponse);
// Parse and move the update instructions of all partitions into our local
@@ -229,6 +230,13 @@
bool PerformPuffDiffOperation(const InstallOperation& operation,
ErrorCode* error);
+ // For a given operation, choose the source fd to be used (raw device or error
+ // correction device) based on the source operation hash.
+ // Returns nullptr if the source hash mismatch cannot be corrected, and set
+ // the |error| accordingly.
+ FileDescriptorPtr ChooseSourceFD(const InstallOperation& operation,
+ ErrorCode* error);
+
// Extracts the payload signature message from the blob on the |operation| if
// the offset matches the one specified by the manifest. Returns whether the
// signature was extracted.
diff --git a/payload_consumer/delta_performer_unittest.cc b/payload_consumer/delta_performer_unittest.cc
index 4585789..c3e4fdb 100644
--- a/payload_consumer/delta_performer_unittest.cc
+++ b/payload_consumer/delta_performer_unittest.cc
@@ -20,6 +20,7 @@
#include <inttypes.h>
#include <time.h>
+#include <memory>
#include <string>
#include <vector>
@@ -631,7 +632,7 @@
// Test that the error-corrected file descriptor is used to read the partition
// since the source partition doesn't match the operation hash.
-TEST_F(DeltaPerformerTest, ErrorCorrectionSourceCopyWhenNoHashFallbackTest) {
+TEST_F(DeltaPerformerTest, ErrorCorrectionSourceCopyFallbackTest) {
const size_t kCopyOperationSize = 4 * 4096;
string source_path;
EXPECT_TRUE(utils::MakeTempFile("Source-XXXXXX", &source_path, nullptr));
@@ -657,7 +658,7 @@
// Test that the error-corrected file descriptor is used to read a partition
// when no hash is available for SOURCE_COPY but it falls back to the normal
// file descriptor when the size of the error corrected one is too small.
-TEST_F(DeltaPerformerTest, ErrorCorrectionSourceCopyFallbackTest) {
+TEST_F(DeltaPerformerTest, ErrorCorrectionSourceCopyWhenNoHashFallbackTest) {
const size_t kCopyOperationSize = 4 * 4096;
string source_path;
EXPECT_TRUE(utils::MakeTempFile("Source-XXXXXX", &source_path, nullptr));
@@ -683,6 +684,40 @@
EXPECT_EQ(0U, GetSourceEccRecoveredFailures());
}
+TEST_F(DeltaPerformerTest, ChooseSourceFDTest) {
+ const size_t kSourceSize = 4 * 4096;
+ string source_path;
+ EXPECT_TRUE(utils::MakeTempFile("Source-XXXXXX", &source_path, nullptr));
+ ScopedPathUnlinker path_unlinker(source_path);
+ // Write invalid data to the source image, which doesn't match the expected
+ // hash.
+ brillo::Blob invalid_data(kSourceSize, 0x55);
+ EXPECT_TRUE(utils::WriteFile(
+ source_path.c_str(), invalid_data.data(), invalid_data.size()));
+
+ performer_.source_fd_ = std::make_shared<EintrSafeFileDescriptor>();
+ performer_.source_fd_->Open(source_path.c_str(), O_RDONLY);
+ performer_.block_size_ = 4096;
+
+ // Setup the fec file descriptor as the fake stream, which matches
+ // |expected_data|.
+ FakeFileDescriptor* fake_fec = SetFakeECCFile(kSourceSize);
+ brillo::Blob expected_data = FakeFileDescriptorData(kSourceSize);
+
+ InstallOperation op;
+ *(op.add_src_extents()) = ExtentForRange(0, kSourceSize / 4096);
+ brillo::Blob src_hash;
+ EXPECT_TRUE(HashCalculator::RawHashOfData(expected_data, &src_hash));
+ op.set_src_sha256_hash(src_hash.data(), src_hash.size());
+
+ ErrorCode error = ErrorCode::kSuccess;
+ EXPECT_EQ(performer_.source_ecc_fd_, performer_.ChooseSourceFD(op, &error));
+ EXPECT_EQ(ErrorCode::kSuccess, error);
+ // Verify that the fake_fec was actually used.
+ EXPECT_EQ(1U, fake_fec->GetReadOps().size());
+ EXPECT_EQ(1U, GetSourceEccRecoveredFailures());
+}
+
TEST_F(DeltaPerformerTest, ExtentsToByteStringTest) {
uint64_t test[] = {1, 1, 4, 2, 0, 1};
static_assert(arraysize(test) % 2 == 0, "Array size uneven");
diff --git a/payload_consumer/file_descriptor_utils.cc b/payload_consumer/file_descriptor_utils.cc
index b1902de..ebfb977 100644
--- a/payload_consumer/file_descriptor_utils.cc
+++ b/payload_consumer/file_descriptor_utils.cc
@@ -96,10 +96,7 @@
const RepeatedPtrField<Extent>& extents,
uint64_t block_size,
brillo::Blob* hash_out) {
- TEST_AND_RETURN_FALSE(hash_out != nullptr);
- TEST_AND_RETURN_FALSE(
- CommonHashExtents(source, extents, nullptr, block_size, hash_out));
- return true;
+ return CommonHashExtents(source, extents, nullptr, block_size, hash_out);
}
} // namespace fd_utils
diff --git a/payload_consumer/file_descriptor_utils_unittest.cc b/payload_consumer/file_descriptor_utils_unittest.cc
index 79d2184..48e610f 100644
--- a/payload_consumer/file_descriptor_utils_unittest.cc
+++ b/payload_consumer/file_descriptor_utils_unittest.cc
@@ -175,10 +175,10 @@
EXPECT_FALSE(fd_utils::ReadAndHashExtents(source_, extents, 4, &hash_out));
}
-// Test that if hash_out is null, then it should fail.
+// Test that if hash_out is null, it still works.
TEST_F(FileDescriptorUtilsTest, ReadAndHashExtentsWithoutHashingTest) {
auto extents = CreateExtentList({{0, 5}});
- EXPECT_FALSE(fd_utils::ReadAndHashExtents(source_, extents, 4, nullptr));
+ EXPECT_TRUE(fd_utils::ReadAndHashExtents(source_, extents, 4, nullptr));
}
// Tests that it can calculate the hash properly.