From 451694e29d4865f41d1097eba0f0c8d7551567d4 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Tue, 10 Jul 2018 21:07:23 -0700 Subject: [PATCH] liblp: Make it easier to test UpdatePartitionTable. This change makes the internal UpdatePartitionTable function more testable by parameterizing its write functions. It also adds two tests, one of which exposes a flaw in the current implementation. Bug: 79173901 Test: liblp_test gtest Change-Id: I3c4112794b97d577a27f035baeac2d42ac75f552 --- fs_mgr/liblp/Android.bp | 3 ++ fs_mgr/liblp/include/liblp/writer.h | 4 ++ fs_mgr/liblp/io_test.cpp | 82 +++++++++++++++++++++++++++++ fs_mgr/liblp/writer.cpp | 22 +++++--- 4 files changed, 105 insertions(+), 6 deletions(-) diff --git a/fs_mgr/liblp/Android.bp b/fs_mgr/liblp/Android.bp index f59fa8468..1050cf560 100644 --- a/fs_mgr/liblp/Android.bp +++ b/fs_mgr/liblp/Android.bp @@ -46,6 +46,9 @@ cc_library_static { cc_test { name: "liblp_test", defaults: ["fs_mgr_defaults"], + cppflags: [ + "-Wno-unused-parameter", + ], static_libs: [ "libbase", "liblog", diff --git a/fs_mgr/liblp/include/liblp/writer.h b/fs_mgr/liblp/include/liblp/writer.h index 6e3c9cb55..f4d1ad7ee 100644 --- a/fs_mgr/liblp/include/liblp/writer.h +++ b/fs_mgr/liblp/include/liblp/writer.h @@ -17,6 +17,7 @@ #ifndef LIBLP_WRITER_H #define LIBLP_WRITER_H +#include #include "metadata_format.h" namespace android { @@ -43,6 +44,9 @@ bool UpdatePartitionTable(const std::string& block_device, const LpMetadata& met bool FlashPartitionTable(int fd, const LpMetadata& metadata, uint32_t slot_number); bool UpdatePartitionTable(int fd, const LpMetadata& metadata, uint32_t slot_number); +bool UpdatePartitionTable(int fd, const LpMetadata& metadata, uint32_t slot_number, + std::function writer); + // Helper function to serialize geometry and metadata to a normal file, for // flashing or debugging. bool WriteToImageFile(const char* file, const LpMetadata& metadata); diff --git a/fs_mgr/liblp/io_test.cpp b/fs_mgr/liblp/io_test.cpp index 29d8ca5b5..c3f8f369d 100644 --- a/fs_mgr/liblp/io_test.cpp +++ b/fs_mgr/liblp/io_test.cpp @@ -393,3 +393,85 @@ TEST(liblp, ImageFiles) { unique_ptr imported = ReadFromImageFile(fd); ASSERT_NE(imported, nullptr); } + +class BadWriter { + public: + // When requested, write garbage instead of the requested bytes, then + // return false. + bool operator()(int fd, const std::string& blob) { + if (++write_count_ == fail_on_write_) { + std::unique_ptr new_data = std::make_unique(blob.size()); + memset(new_data.get(), 0xe5, blob.size()); + EXPECT_TRUE(android::base::WriteFully(fd, new_data.get(), blob.size())); + return false; + } else { + return android::base::WriteFully(fd, blob.data(), blob.size()); + } + } + void FailOnWrite(int number) { + fail_on_write_ = number; + write_count_ = 0; + } + + private: + int fail_on_write_ = 0; + int write_count_ = 0; +}; + +// Test that an interrupted flash operation on the "primary" copy of metadata +// is not fatal. +TEST(liblp, FlashPrimaryMetadataFailure) { + // Initial state. + unique_fd fd = CreateFlashedDisk(); + ASSERT_GE(fd, 0); + + BadWriter writer; + + // Read and write it back. + writer.FailOnWrite(1); + unique_ptr imported = ReadMetadata(fd, 0); + ASSERT_NE(imported, nullptr); + ASSERT_FALSE(UpdatePartitionTable(fd, *imported.get(), 0, writer)); + + // We should still be able to read the backup copy. + imported = ReadMetadata(fd, 0); + ASSERT_NE(imported, nullptr); + + // Flash again, this time fail the backup copy. We should still be able + // to read the primary. + writer.FailOnWrite(2); + ASSERT_FALSE(UpdatePartitionTable(fd, *imported.get(), 0, writer)); + imported = ReadMetadata(fd, 0); + ASSERT_NE(imported, nullptr); +} + +// Test that an interrupted flash operation on the "backup" copy of metadata +// is not fatal. +TEST(liblp, FlashBackupMetadataFailure) { + // Initial state. + unique_fd fd = CreateFlashedDisk(); + ASSERT_GE(fd, 0); + + BadWriter writer; + + // Read and write it back. + writer.FailOnWrite(2); + unique_ptr imported = ReadMetadata(fd, 0); + ASSERT_NE(imported, nullptr); + ASSERT_FALSE(UpdatePartitionTable(fd, *imported.get(), 0, writer)); + + // We should still be able to read the primary copy. + imported = ReadMetadata(fd, 0); + ASSERT_NE(imported, nullptr); + + // Flash again, this time fail the primary copy. We should still be able + // to read the primary. + // + // TODO(dvander): This is currently not handled correctly. liblp does not + // guarantee both copies are in sync before the update. The ASSERT_EQ + // will change to an ASSERT_NE when this is fixed. + writer.FailOnWrite(1); + ASSERT_FALSE(UpdatePartitionTable(fd, *imported.get(), 0, writer)); + imported = ReadMetadata(fd, 0); + ASSERT_EQ(imported, nullptr); +} diff --git a/fs_mgr/liblp/writer.cpp b/fs_mgr/liblp/writer.cpp index e3dba47da..36c7b5a43 100644 --- a/fs_mgr/liblp/writer.cpp +++ b/fs_mgr/liblp/writer.cpp @@ -130,8 +130,13 @@ static bool ValidateAndSerializeMetadata(int fd, const LpMetadata& metadata, std return true; } +static bool DefaultWriter(int fd, const std::string& blob) { + return android::base::WriteFully(fd, blob.data(), blob.size()); +} + static bool WriteMetadata(int fd, const LpMetadataGeometry& geometry, uint32_t slot_number, - const std::string& blob) { + const std::string& blob, + std::function writer) { // Make sure we're writing to a valid metadata slot. if (slot_number >= geometry.metadata_slot_count) { LERROR << "Invalid logical partition metadata slot number."; @@ -144,7 +149,7 @@ static bool WriteMetadata(int fd, const LpMetadataGeometry& geometry, uint32_t s PERROR << __PRETTY_FUNCTION__ << "lseek failed: offset " << primary_offset; return false; } - if (!android::base::WriteFully(fd, blob.data(), blob.size())) { + if (!writer(fd, blob)) { PERROR << __PRETTY_FUNCTION__ << "write " << blob.size() << " bytes failed"; return false; } @@ -161,7 +166,7 @@ static bool WriteMetadata(int fd, const LpMetadataGeometry& geometry, uint32_t s << " is within logical partition bounds, sector " << geometry.last_logical_sector; return false; } - if (!android::base::WriteFully(fd, blob.data(), blob.size())) { + if (!writer(fd, blob)) { PERROR << __PRETTY_FUNCTION__ << "backup write " << blob.size() << " bytes failed"; return false; } @@ -197,10 +202,11 @@ bool FlashPartitionTable(int fd, const LpMetadata& metadata, uint32_t slot_numbe } // Write metadata to the correct slot, now that geometry is in place. - return WriteMetadata(fd, metadata.geometry, slot_number, metadata_blob); + return WriteMetadata(fd, metadata.geometry, slot_number, metadata_blob, DefaultWriter); } -bool UpdatePartitionTable(int fd, const LpMetadata& metadata, uint32_t slot_number) { +bool UpdatePartitionTable(int fd, const LpMetadata& metadata, uint32_t slot_number, + std::function writer) { // Before writing geometry and/or logical partition tables, perform some // basic checks that the geometry and tables are coherent, and will fit // on the given block device. @@ -221,7 +227,7 @@ bool UpdatePartitionTable(int fd, const LpMetadata& metadata, uint32_t slot_numb LERROR << "Incompatible geometry in new logical partition metadata"; return false; } - return WriteMetadata(fd, geometry, slot_number, blob); + return WriteMetadata(fd, geometry, slot_number, blob, writer); } bool FlashPartitionTable(const std::string& block_device, const LpMetadata& metadata, @@ -244,6 +250,10 @@ bool UpdatePartitionTable(const std::string& block_device, const LpMetadata& met return UpdatePartitionTable(fd, metadata, slot_number); } +bool UpdatePartitionTable(int fd, const LpMetadata& metadata, uint32_t slot_number) { + return UpdatePartitionTable(fd, metadata, slot_number, DefaultWriter); +} + bool WriteToImageFile(int fd, const LpMetadata& input) { std::string geometry = SerializeGeometry(input.geometry); std::string padding(LP_METADATA_GEOMETRY_SIZE - geometry.size(), '\0');