Do not skip copying in place block if source corrupted
In VAB(both plain and VABC), we will skip copying some blocks if the
source and target extents are exactly the same. However, if these source
blocks are corrupted then we would miss the oppoturnity to correct them
using FEC.
To address this issue, attempt to FEC correct the corrupted blocks and
write these blocks back to source partition.
Test: th
Bug: 276846805
Change-Id: Ic1e0276a310cff0be3695ef5f3a2c99437c29159
diff --git a/payload_consumer/fec_file_descriptor.cc b/payload_consumer/fec_file_descriptor.cc
index 3fee196..56edc24 100644
--- a/payload_consumer/fec_file_descriptor.cc
+++ b/payload_consumer/fec_file_descriptor.cc
@@ -34,7 +34,7 @@
return false;
}
- fec_status status;
+ fec_status status{};
if (!fh_.get_status(status)) {
LOG(ERROR) << "Couldn't load ECC status";
fh_.close();
diff --git a/payload_consumer/partition_writer.cc b/payload_consumer/partition_writer.cc
index d7d8bea..2ec05f5 100644
--- a/payload_consumer/partition_writer.cc
+++ b/payload_consumer/partition_writer.cc
@@ -32,18 +32,14 @@
#include <base/strings/string_util.h>
#include <base/strings/stringprintf.h>
-#include "update_engine/common/terminator.h"
+#include "update_engine/common/error_code.h"
#include "update_engine/common/utils.h"
-#include "update_engine/payload_consumer/bzip_extent_writer.h"
#include "update_engine/payload_consumer/cached_file_descriptor.h"
-#include "update_engine/payload_consumer/extent_reader.h"
#include "update_engine/payload_consumer/extent_writer.h"
#include "update_engine/payload_consumer/file_descriptor_utils.h"
#include "update_engine/payload_consumer/install_operation_executor.h"
#include "update_engine/payload_consumer/install_plan.h"
#include "update_engine/payload_consumer/mount_history.h"
-#include "update_engine/payload_consumer/payload_constants.h"
-#include "update_engine/payload_consumer/xz_extent_writer.h"
#include "update_engine/payload_generator/extent_utils.h"
namespace chromeos_update_engine {
@@ -232,22 +228,23 @@
// decide it the operation should be skipped.
const PartitionUpdate& partition = partition_update_;
- InstallOperation buf;
- const bool should_optimize = dynamic_control_->OptimizeOperation(
- partition.partition_name(), operation, &buf);
- const InstallOperation& optimized = should_optimize ? buf : operation;
-
// Invoke ChooseSourceFD with original operation, so that it can properly
// verify source hashes. Optimized operation might contain a smaller set of
// extents, or completely empty.
auto source_fd = ChooseSourceFD(operation, error);
- if (source_fd == nullptr) {
- LOG(ERROR) << "Unrecoverable source hash mismatch found on partition "
- << partition.partition_name()
- << " extents: " << ExtentsToString(operation.src_extents());
+ if (*error != ErrorCode::kSuccess || source_fd == nullptr) {
+ LOG(WARNING) << "Source hash mismatch detected for extents "
+ << operation.src_extents() << " on partition "
+ << partition.partition_name() << " @ " << source_path_;
+
return false;
}
+ InstallOperation buf;
+ const bool should_optimize = dynamic_control_->OptimizeOperation(
+ partition.partition_name(), operation, &buf);
+ const InstallOperation& optimized = should_optimize ? buf : operation;
+
auto writer = CreateBaseExtentWriter();
return install_op_executor_.ExecuteSourceCopyOperation(
optimized, std::move(writer), source_fd);
@@ -340,8 +337,9 @@
// Log remount history if this device is an ext4 partition.
LogMountHistory(source_fd);
-
- *error = ErrorCode::kDownloadStateInitializationError;
+ if (error) {
+ *error = ErrorCode::kDownloadStateInitializationError;
+ }
return false;
}
return true;
diff --git a/payload_consumer/partition_writer_unittest.cc b/payload_consumer/partition_writer_unittest.cc
index 4910594..32324b6 100644
--- a/payload_consumer/partition_writer_unittest.cc
+++ b/payload_consumer/partition_writer_unittest.cc
@@ -210,8 +210,7 @@
op.set_src_sha256_hash(src_hash.data(), src_hash.size());
ErrorCode error = ErrorCode::kSuccess;
- ASSERT_EQ(writer_.verified_source_fd_.source_ecc_fd_,
- writer_.ChooseSourceFD(op, &error));
+ ASSERT_NE(writer_.ChooseSourceFD(op, &error), nullptr);
ASSERT_EQ(ErrorCode::kSuccess, error);
// Verify that the fake_fec was actually used.
ASSERT_EQ(1U, fake_fec->GetReadOps().size());
diff --git a/payload_consumer/vabc_partition_writer.cc b/payload_consumer/vabc_partition_writer.cc
index 17b7d50..b00ff70 100644
--- a/payload_consumer/vabc_partition_writer.cc
+++ b/payload_consumer/vabc_partition_writer.cc
@@ -95,7 +95,7 @@
}
copy_blocks_.AddExtent(cow_op.dst_extent());
}
- LOG(INFO) << "Partition `" << partition_update.partition_name() << " has "
+ LOG(INFO) << "Partition `" << partition_update.partition_name() << "` has "
<< copy_blocks_.blocks() << " copy blocks";
}
diff --git a/payload_consumer/verified_source_fd.cc b/payload_consumer/verified_source_fd.cc
index 002bd07..f35b6a9 100644
--- a/payload_consumer/verified_source_fd.cc
+++ b/payload_consumer/verified_source_fd.cc
@@ -26,11 +26,17 @@
#include <base/strings/string_util.h>
#include <base/strings/stringprintf.h>
+#include "update_engine/common/error_code.h"
+#include "update_engine/common/hash_calculator.h"
#include "update_engine/common/utils.h"
-#include "update_engine/payload_consumer/fec_file_descriptor.h"
+#include "update_engine/payload_consumer/extent_writer.h"
+#include "update_engine/payload_consumer/file_descriptor.h"
#include "update_engine/payload_consumer/file_descriptor_utils.h"
-#include "update_engine/payload_consumer/mount_history.h"
#include "update_engine/payload_consumer/partition_writer.h"
+#include "update_engine/update_metadata.pb.h"
+#if USE_FEC
+#include "update_engine/payload_consumer/fec_file_descriptor.h"
+#endif
namespace chromeos_update_engine {
using std::string;
@@ -45,7 +51,7 @@
return false;
#if USE_FEC
- FileDescriptorPtr fd(new FecFileDescriptor());
+ auto fd = std::make_shared<FecFileDescriptor>();
if (!fd->Open(source_path_.c_str(), O_RDONLY, 0)) {
PLOG(ERROR) << "Unable to open ECC source partition " << source_path_;
source_ecc_open_failure_ = true;
@@ -60,12 +66,25 @@
return !source_ecc_open_failure_;
}
+bool VerifiedSourceFd::WriteBackCorrectedSourceBlocks(
+ const std::vector<unsigned char>& source_data,
+ const google::protobuf::RepeatedPtrField<Extent>& extents) {
+ auto fd = std::make_shared<EintrSafeFileDescriptor>();
+ TEST_AND_RETURN_FALSE_ERRNO(fd->Open(source_path_.c_str(), O_RDWR));
+ DirectExtentWriter writer(fd);
+ TEST_AND_RETURN_FALSE(writer.Init(extents, block_size_));
+ return writer.Write(source_data.data(), source_data.size());
+}
+
FileDescriptorPtr VerifiedSourceFd::ChooseSourceFD(
const InstallOperation& operation, ErrorCode* error) {
if (source_fd_ == nullptr) {
LOG(ERROR) << "ChooseSourceFD fail: source_fd_ == nullptr";
return nullptr;
}
+ if (error) {
+ *error = ErrorCode::kSuccess;
+ }
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
@@ -74,6 +93,9 @@
if (OpenCurrentECCPartition() &&
fd_utils::ReadAndHashExtents(
source_ecc_fd_, operation.src_extents(), block_size_, nullptr)) {
+ if (error) {
+ *error = ErrorCode::kDownloadOperationHashMissingError;
+ }
return source_ecc_fd_;
}
return source_fd_;
@@ -87,6 +109,9 @@
source_hash == expected_source_hash) {
return source_fd_;
}
+ if (error) {
+ *error = ErrorCode::kDownloadOperationHashMismatch;
+ }
// 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()) {
@@ -103,11 +128,23 @@
<< base::HexEncode(expected_source_hash.data(),
expected_source_hash.size());
- if (fd_utils::ReadAndHashExtents(
- source_ecc_fd_, operation.src_extents(), block_size_, &source_hash) &&
- PartitionWriter::ValidateSourceHash(
+ std::vector<unsigned char> source_data;
+ if (!utils::ReadExtents(
+ source_ecc_fd_, operation.src_extents(), &source_data, block_size_)) {
+ return nullptr;
+ }
+ if (!HashCalculator::RawHashOfData(source_data, &source_hash)) {
+ return nullptr;
+ }
+ if (PartitionWriter::ValidateSourceHash(
source_hash, operation, source_ecc_fd_, error)) {
source_ecc_recovered_failures_++;
+ if (WriteBackCorrectedSourceBlocks(source_data, operation.src_extents())) {
+ if (error) {
+ *error = ErrorCode::kSuccess;
+ }
+ return source_fd_;
+ }
return source_ecc_fd_;
}
return nullptr;
diff --git a/payload_consumer/verified_source_fd.h b/payload_consumer/verified_source_fd.h
index f7d0620..6d859b9 100644
--- a/payload_consumer/verified_source_fd.h
+++ b/payload_consumer/verified_source_fd.h
@@ -39,6 +39,9 @@
[[nodiscard]] bool Open();
private:
+ bool WriteBackCorrectedSourceBlocks(
+ const std::vector<unsigned char>& source_data,
+ const google::protobuf::RepeatedPtrField<Extent>& extents);
bool OpenCurrentECCPartition();
const size_t block_size_;
const std::string source_path_;