Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 2adbece1 authored by Tom Cherry's avatar Tom Cherry
Browse files

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
parent 29e7b2af

logd/SerializedLogChunk.cpp

+8 −8
Original line number Diff line number Diff line
@@ -25,12 +25,10 @@ SerializedLogChunk::~SerializedLogChunk() { 
}



void SerializedLogChunk::Compress() {

    if (compressed_log_.size() == 0) {

    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();

    }

              << " 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,10 +87,12 @@ 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;

        if (!writer_active_) {

            compressed_log_.Resize(0);

            Compress();

        }

    }



    DecReaderRefCount();

logd/SerializedLogChunkTest.cpp

+24 −0
Original line number Diff line number Diff line
@@ -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();

}