Merge "logd: Fix ClearUidLogs() when writer_active_ is true"

2 files changed, 32 insertions, 8 deletions

diff --git a/logd/SerializedLogChunk.cpp b/logd/SerializedLogChunk.cpp
index de641d62a..e4d89451d 100644
--- a/logd/SerializedLogChunk.cpp
+++ b/logd/SerializedLogChunk.cpp
@@ -25,12 +25,10 @@ SerializedLogChunk::~SerializedLogChunk() {
 }
 
 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 @@ bool SerializedLogChunk::ClearUidLogs(uid_t uid, log_id_t log_id, LogStatistics*
     // 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 f10b9c673..3b451252d 100644
--- a/logd/SerializedLogChunkTest.cpp
+++ b/logd/SerializedLogChunkTest.cpp
@@ -281,3 +281,27 @@ TEST_P(UidClearTest, save_beginning_and_end) {
 }
 
 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();
+}