From db25e8e32d0c080d7b4e3baad4efe132410a5ee6 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Fri, 2 Apr 2021 16:04:22 -0700 Subject: [PATCH] libsnapshot: Fix premature truncation in CowWriter. When opening in append mode, we ftruncate() the COW. This has three side effects: (1) If the COW is never modified, or Finalized(), the state of the COW will have changed. Ideally it should only change on an explicit write operation. (2) Data after the current cluster will be accidentally thrown away. (3) The ending "cluster" op will be thrown away if the current cluster was incomplete, and thus the last valid label could be invalidated. Bug: 183985866 Test: cow_api_test Change-Id: I3c9a38553b7492a3d6e71d177d75ddb1b6490dfe --- fs_mgr/libsnapshot/cow_api_test.cpp | 88 +++++++++++++++++++++++++++++ fs_mgr/libsnapshot/cow_writer.cpp | 15 +++-- 2 files changed, 99 insertions(+), 4 deletions(-) diff --git a/fs_mgr/libsnapshot/cow_api_test.cpp b/fs_mgr/libsnapshot/cow_api_test.cpp index 5d632206d..a4cf9b4a7 100644 --- a/fs_mgr/libsnapshot/cow_api_test.cpp +++ b/fs_mgr/libsnapshot/cow_api_test.cpp @@ -25,6 +25,10 @@ #include #include +using testing::AssertionFailure; +using testing::AssertionResult; +using testing::AssertionSuccess; + namespace android { namespace snapshot { @@ -781,6 +785,90 @@ TEST_F(CowTest, AppendAfterFinalize) { ASSERT_TRUE(reader.Parse(cow_->fd)); } +AssertionResult WriteDataBlock(CowWriter* writer, uint64_t new_block, std::string data) { + data.resize(writer->options().block_size, '\0'); + if (!writer->AddRawBlocks(new_block, data.data(), data.size())) { + return AssertionFailure() << "Failed to add raw block"; + } + return AssertionSuccess(); +} + +AssertionResult CompareDataBlock(CowReader* reader, const CowOperation& op, + const std::string& data) { + CowHeader header; + reader->GetHeader(&header); + + std::string cmp = data; + cmp.resize(header.block_size, '\0'); + + StringSink sink; + if (!reader->ReadData(op, &sink)) { + return AssertionFailure() << "Failed to read data block"; + } + if (cmp != sink.stream()) { + return AssertionFailure() << "Data blocks did not match, expected " << cmp << ", got " + << sink.stream(); + } + + return AssertionSuccess(); +} + +TEST_F(CowTest, ResumeMidCluster) { + CowOptions options; + options.cluster_ops = 7; + auto writer = std::make_unique(options); + ASSERT_TRUE(writer->Initialize(cow_->fd)); + + ASSERT_TRUE(WriteDataBlock(writer.get(), 1, "Block 1")); + ASSERT_TRUE(WriteDataBlock(writer.get(), 2, "Block 2")); + ASSERT_TRUE(WriteDataBlock(writer.get(), 3, "Block 3")); + ASSERT_TRUE(writer->AddLabel(1)); + ASSERT_TRUE(writer->Finalize()); + ASSERT_TRUE(WriteDataBlock(writer.get(), 4, "Block 4")); + ASSERT_EQ(lseek(cow_->fd, 0, SEEK_SET), 0); + + writer = std::make_unique(options); + ASSERT_TRUE(writer->InitializeAppend(cow_->fd, 1)); + ASSERT_TRUE(WriteDataBlock(writer.get(), 4, "Block 4")); + ASSERT_TRUE(WriteDataBlock(writer.get(), 5, "Block 5")); + ASSERT_TRUE(WriteDataBlock(writer.get(), 6, "Block 6")); + ASSERT_TRUE(WriteDataBlock(writer.get(), 7, "Block 7")); + ASSERT_TRUE(WriteDataBlock(writer.get(), 8, "Block 8")); + ASSERT_TRUE(writer->AddLabel(2)); + ASSERT_TRUE(writer->Finalize()); + ASSERT_EQ(lseek(cow_->fd, 0, SEEK_SET), 0); + + CowReader reader; + ASSERT_TRUE(reader.Parse(cow_->fd)); + + auto iter = reader.GetOpIter(); + size_t num_replace = 0; + size_t max_in_cluster = 0; + size_t num_in_cluster = 0; + size_t num_clusters = 0; + while (!iter->Done()) { + const auto& op = iter->Get(); + + num_in_cluster++; + max_in_cluster = std::max(max_in_cluster, num_in_cluster); + + if (op.type == kCowReplaceOp) { + num_replace++; + + ASSERT_EQ(op.new_block, num_replace); + ASSERT_TRUE(CompareDataBlock(&reader, op, "Block " + std::to_string(num_replace))); + } else if (op.type == kCowClusterOp) { + num_in_cluster = 0; + num_clusters++; + } + + iter->Next(); + } + ASSERT_EQ(num_replace, 8); + ASSERT_EQ(max_in_cluster, 7); + ASSERT_EQ(num_clusters, 2); +} + } // namespace snapshot } // namespace android diff --git a/fs_mgr/libsnapshot/cow_writer.cpp b/fs_mgr/libsnapshot/cow_writer.cpp index 59f6d6f92..5c7cc1681 100644 --- a/fs_mgr/libsnapshot/cow_writer.cpp +++ b/fs_mgr/libsnapshot/cow_writer.cpp @@ -232,10 +232,6 @@ bool CowWriter::OpenForAppend(uint64_t label) { // Free reader so we own the descriptor position again. reader = nullptr; - // Remove excess data - if (!Truncate(next_op_pos_)) { - return false; - } if (lseek(fd_.get(), next_op_pos_, SEEK_SET) < 0) { PLOG(ERROR) << "lseek failed"; return false; @@ -403,6 +399,17 @@ bool CowWriter::Finalize() { return false; } + // Remove excess data, if we're in append mode and threw away more data + // than we wrote before. + off_t offs = lseek(fd_.get(), 0, SEEK_CUR); + if (offs < 0) { + PLOG(ERROR) << "Failed to lseek to find current position"; + return false; + } + if (!Truncate(offs)) { + return false; + } + // Reposition for additional Writing if (extra_cluster) { current_cluster_size_ = continue_cluster_size;