logd: Fix ClearUidLogs() when writer_active_ is true
Previously ClearUidLogs() would Compress() the log buffer in all
cases, however that is the wrong behavior when writer_active_ is true
and would leave the SerializedLogChunk object in an invalid state. If
more logs are written to the log, then write_offset() will be higher
than the compressed size of the log, violating a CHECK() when later
decompressing the log.
This change does not call Compress() in ClearUidLogs() if
writer_active_ is true. It upgrades a check in Compress() from a
simple if statement to a CHECK() to prevent against this happening in
the future.
It adds a test that exercises the previously failing path.
Bug: 166187079
Test: unit tests
Change-Id: Ic5fbcf16f724af1c20170b8f4e6e2daadf6a9529
diff --git a/logd/SerializedLogChunk.cpp b/logd/SerializedLogChunk.cpp
index de641d6..e4d8945 100644
--- a/logd/SerializedLogChunk.cpp
+++ b/logd/SerializedLogChunk.cpp
@@ -25,12 +25,10 @@
}
void SerializedLogChunk::Compress() {
- if (compressed_log_.size() == 0) {
- CompressionEngine::GetInstance().Compress(contents_, write_offset_, compressed_log_);
- LOG(INFO) << "Compressed Log, buffer max size: " << contents_.size()
- << " size used: " << write_offset_
- << " compressed size: " << compressed_log_.size();
- }
+ CHECK_EQ(compressed_log_.size(), 0U);
+ CompressionEngine::GetInstance().Compress(contents_, write_offset_, compressed_log_);
+ LOG(INFO) << "Compressed Log, buffer max size: " << contents_.size()
+ << " size used: " << write_offset_ << " compressed size: " << compressed_log_.size();
}
// TODO: Develop a better reference counting strategy to guard against the case where the writer is
@@ -89,9 +87,11 @@
// Clear the old compressed logs and set write_offset_ appropriately to compress the new
// partially cleared log.
if (new_write_offset != write_offset_) {
- compressed_log_.Resize(0);
write_offset_ = new_write_offset;
- Compress();
+ if (!writer_active_) {
+ compressed_log_.Resize(0);
+ Compress();
+ }
}
DecReaderRefCount();
diff --git a/logd/SerializedLogChunkTest.cpp b/logd/SerializedLogChunkTest.cpp
index f10b9c6..3b45125 100644
--- a/logd/SerializedLogChunkTest.cpp
+++ b/logd/SerializedLogChunkTest.cpp
@@ -281,3 +281,27 @@
}
INSTANTIATE_TEST_CASE_P(UidClearTests, UidClearTest, testing::Values(true, false));
+
+// b/166187079: ClearUidLogs() should not compress the log if writer_active_ is true
+TEST(SerializedLogChunk, ClearUidLogs_then_FinishWriting) {
+ static constexpr size_t kChunkSize = 10 * 4096;
+ static constexpr uid_t kUidToClear = 1000;
+ static constexpr uid_t kOtherUid = 1234;
+
+ SerializedLogChunk chunk{kChunkSize};
+ static const char msg1[] = "saved first message";
+ static const char msg2[] = "cleared interior message";
+ static const char msg3[] = "last message stays";
+ ASSERT_NE(nullptr, chunk.Log(1, log_time(), kOtherUid, 1, 2, msg1, sizeof(msg1)));
+ ASSERT_NE(nullptr, chunk.Log(2, log_time(), kUidToClear, 1, 2, msg2, sizeof(msg2)));
+ ASSERT_NE(nullptr, chunk.Log(3, log_time(), kOtherUid, 1, 2, msg3, sizeof(msg3)));
+
+ chunk.ClearUidLogs(kUidToClear, LOG_ID_MAIN, nullptr);
+
+ ASSERT_NE(nullptr, chunk.Log(4, log_time(), kOtherUid, 1, 2, msg3, sizeof(msg3)));
+
+ chunk.FinishWriting();
+ // The next line would violate a CHECK() during decompression with the faulty code.
+ chunk.IncReaderRefCount();
+ chunk.DecReaderRefCount();
+}