From 2adbece108c0f29dd417216173176c016440e1e5 Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Wed, 9 Sep 2020 20:13:18 +0000 Subject: 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 --- logd/SerializedLogChunkTest.cpp | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) (limited to 'logd/SerializedLogChunkTest.cpp') 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(); +} -- cgit v1.2.3