From 1fdbdbe14805a2e18345385cc17ab5494794b344 Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Mon, 22 Jun 2020 08:28:45 -0700 Subject: [PATCH 1/2] logd: join() the SerializedLogBuffer deleter thread Logd never deletes SerializedLogBuffer, so it seemed reasonable to detach the deleter thread, however unit tests and fuzzers do delete SerializedLogBuffer, so we must safely join the deleter thread in the destructor. This simplifies the deleter thread code and ensures that only one deleter thread will be running at a time. Test: fuzzing works Change-Id: I69c7447109898a1bb7038a03337cadacb1213281 --- logd/SerializedLogBuffer.cpp | 64 ++++++++++++++++++++++++++---------- logd/SerializedLogBuffer.h | 9 ++++- 2 files changed, 55 insertions(+), 18 deletions(-) diff --git a/logd/SerializedLogBuffer.cpp b/logd/SerializedLogBuffer.cpp index 70b800f9c..ea9feec27 100644 --- a/logd/SerializedLogBuffer.cpp +++ b/logd/SerializedLogBuffer.cpp @@ -16,8 +16,9 @@ #include "SerializedLogBuffer.h" +#include + #include -#include #include #include @@ -31,7 +32,11 @@ SerializedLogBuffer::SerializedLogBuffer(LogReaderList* reader_list, LogTags* ta Init(); } -SerializedLogBuffer::~SerializedLogBuffer() {} +SerializedLogBuffer::~SerializedLogBuffer() { + if (deleter_thread_.joinable()) { + deleter_thread_.join(); + } +} void SerializedLogBuffer::Init() { log_id_for_each(i) { @@ -119,15 +124,44 @@ void SerializedLogBuffer::MaybePrune(log_id_t log_id) { } } +void SerializedLogBuffer::StartDeleterThread() { + if (deleter_thread_running_) { + return; + } + if (deleter_thread_.joinable()) { + deleter_thread_.join(); + } + auto new_thread = std::thread([this] { DeleterThread(); }); + deleter_thread_.swap(new_thread); + deleter_thread_running_ = true; +} + // Decompresses the chunks, call LogStatistics::Subtract() on each entry, then delete the chunks and // the list. Note that the SerializedLogChunk objects have been removed from logs_ and their // references have been deleted from any SerializedFlushToState objects, so this can be safely done -// without holding lock_. It is done in a separate thread to avoid delaying the writer thread. The -// lambda for the thread takes ownership of the 'chunks' list and thus when the thread ends and the -// lambda is deleted, the objects are deleted. -void SerializedLogBuffer::DeleteLogChunks(std::list&& chunks, log_id_t log_id) { - auto delete_thread = std::thread{[chunks = std::move(chunks), log_id, this]() mutable { - for (auto& chunk : chunks) { +// without holding lock_. It is done in a separate thread to avoid delaying the writer thread. +void SerializedLogBuffer::DeleterThread() { + prctl(PR_SET_NAME, "logd.deleter"); + while (true) { + std::list local_chunks_to_delete; + log_id_t log_id; + { + auto lock = std::lock_guard{lock_}; + log_id_for_each(i) { + if (!chunks_to_delete_[i].empty()) { + local_chunks_to_delete = std::move(chunks_to_delete_[i]); + chunks_to_delete_[i].clear(); + log_id = i; + break; + } + } + if (local_chunks_to_delete.empty()) { + deleter_thread_running_ = false; + return; + } + } + + for (auto& chunk : local_chunks_to_delete) { chunk.IncReaderRefCount(); int read_offset = 0; while (read_offset < chunk.write_offset()) { @@ -137,8 +171,7 @@ void SerializedLogBuffer::DeleteLogChunks(std::list&& chunks } chunk.DecReaderRefCount(false); } - }}; - delete_thread.detach(); + } } void SerializedLogBuffer::NotifyReadersOfPrune( @@ -164,13 +197,9 @@ bool SerializedLogBuffer::Prune(log_id_t log_id, size_t bytes_to_free, uid_t uid } } + StartDeleterThread(); + auto& log_buffer = logs_[log_id]; - - std::list chunks_to_prune; - auto prune_chunks = android::base::make_scope_guard([&chunks_to_prune, log_id, this] { - DeleteLogChunks(std::move(chunks_to_prune), log_id); - }); - auto it = log_buffer.begin(); while (it != log_buffer.end()) { if (oldest != nullptr && it->highest_sequence_number() >= oldest->start()) { @@ -193,7 +222,8 @@ bool SerializedLogBuffer::Prune(log_id_t log_id, size_t bytes_to_free, uid_t uid } } else { size_t buffer_size = it_to_prune->PruneSize(); - chunks_to_prune.splice(chunks_to_prune.end(), log_buffer, it_to_prune); + chunks_to_delete_[log_id].splice(chunks_to_delete_[log_id].end(), log_buffer, + it_to_prune); if (buffer_size >= bytes_to_free) { return true; } diff --git a/logd/SerializedLogBuffer.h b/logd/SerializedLogBuffer.h index 346f51f68..421d41997 100644 --- a/logd/SerializedLogBuffer.h +++ b/logd/SerializedLogBuffer.h @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -60,7 +61,9 @@ class SerializedLogBuffer final : public LogBuffer { REQUIRES_SHARED(lock_); void NotifyReadersOfPrune(log_id_t log_id, const std::list::iterator& chunk) REQUIRES(reader_list_->reader_threads_lock()); - void DeleteLogChunks(std::list&& chunks, log_id_t log_id); + + void StartDeleterThread() REQUIRES(lock_); + void DeleterThread(); LogReaderList* reader_list_; LogTags* tags_; @@ -70,5 +73,9 @@ class SerializedLogBuffer final : public LogBuffer { std::list logs_[LOG_ID_MAX] GUARDED_BY(lock_); RwLock lock_; + std::list chunks_to_delete_[LOG_ID_MAX] GUARDED_BY(lock_); + std::thread deleter_thread_ GUARDED_BY(lock_); + bool deleter_thread_running_ GUARDED_BY(lock_) = false; + std::atomic sequence_ = 1; }; From f2774a04cabc28a851bfd8ca389acf61c6206fe2 Mon Sep 17 00:00:00 2001 From: Tom Cherry Date: Fri, 19 Jun 2020 14:01:16 -0700 Subject: [PATCH 2/2] logd: add fuzzer for SerializedLogBuffer and other improvements 1) Add fuzzer for SerializedLogBuffer 2) Enable fuzzing on host 3) Read logs after writing them 4) Silence log tags error on host Test: run these fuzzers Change-Id: Id5f0394546ecbccf5281e3d8855853be90dee3f0 --- logd/LogTags.cpp | 2 + logd/fuzz/Android.bp | 28 +++++++--- logd/fuzz/log_buffer_log_fuzzer.cpp | 59 +++++++++++++++++++--- logd/fuzz/serialized_log_buffer_fuzzer.cpp | 19 +++++++ 4 files changed, 94 insertions(+), 14 deletions(-) create mode 100644 logd/fuzz/serialized_log_buffer_fuzzer.cpp diff --git a/logd/LogTags.cpp b/logd/LogTags.cpp index 8e18f9d46..1b7107f8a 100644 --- a/logd/LogTags.cpp +++ b/logd/LogTags.cpp @@ -276,7 +276,9 @@ void LogTags::ReadFileEventLogTags(const char* filename, bool warn) { cp++; } } else if (warn) { +#ifdef __ANDROID__ LOG(ERROR) << "Cannot read " << filename; +#endif } } diff --git a/logd/fuzz/Android.bp b/logd/fuzz/Android.bp index 9834ff058..587eedca1 100644 --- a/logd/fuzz/Android.bp +++ b/logd/fuzz/Android.bp @@ -13,11 +13,9 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -cc_fuzz { - name: "log_buffer_log_fuzzer", - srcs: [ - "log_buffer_log_fuzzer.cpp", - ], + +cc_defaults { + name: "log_fuzzer_defaults", static_libs: [ "libbase", "libcutils", @@ -25,9 +23,25 @@ cc_fuzz { "liblog", "liblogd", "libcutils", - "libsysutils", "libz", "libzstd", ], - cflags: ["-Werror"], + cflags: ["-Wextra"], + host_supported: true, +} + +cc_fuzz { + name: "log_buffer_log_fuzzer", + defaults: ["log_fuzzer_defaults"], + srcs: [ + "log_buffer_log_fuzzer.cpp", + ], +} + +cc_fuzz { + name: "serialized_log_buffer_fuzzer", + defaults: ["log_fuzzer_defaults"], + srcs: [ + "serialized_log_buffer_fuzzer.cpp", + ], } diff --git a/logd/fuzz/log_buffer_log_fuzzer.cpp b/logd/fuzz/log_buffer_log_fuzzer.cpp index a7a17921a..1dc996c12 100644 --- a/logd/fuzz/log_buffer_log_fuzzer.cpp +++ b/logd/fuzz/log_buffer_log_fuzzer.cpp @@ -15,10 +15,13 @@ */ #include +#include + #include "../ChattyLogBuffer.h" #include "../LogReaderList.h" #include "../LogReaderThread.h" #include "../LogStatistics.h" +#include "../SerializedLogBuffer.h" // We don't want to waste a lot of entropy on messages #define MAX_MSG_LENGTH 5 @@ -27,7 +30,20 @@ #define MIN_TAG_ID 1000 #define TAG_MOD 10 -namespace android { +#ifndef __ANDROID__ +unsigned long __android_logger_get_buffer_size(log_id_t) { + return 1024 * 1024; +} + +bool __android_logger_valid_buffer_size(unsigned long) { + return true; +} +#endif + +char* android::uidToName(uid_t) { + return strdup("fake"); +} + struct LogInput { public: log_id_t log_id; @@ -79,9 +95,13 @@ int write_log_messages(const uint8_t** pdata, size_t* data_left, LogBuffer* log_ return 1; } -char* uidToName(uid_t) { - return strdup("fake"); -} +class NoopWriter : public LogWriter { + public: + NoopWriter() : LogWriter(0, true) {} + bool Write(const logger_entry&, const char*) override { return true; } + + std::string name() const override { return "noop_writer"; } +}; extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { // We want a random tag length and a random remaining message length @@ -89,11 +109,18 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { return 0; } + android::base::SetMinimumLogSeverity(android::base::ERROR); + LogReaderList reader_list; LogTags tags; PruneList prune_list; LogStatistics stats(true); - LogBuffer* log_buffer = new ChattyLogBuffer(&reader_list, &tags, &prune_list, &stats); + std::unique_ptr log_buffer; +#ifdef FUZZ_SERIALIZED + log_buffer.reset(new SerializedLogBuffer(&reader_list, &tags, &stats)); +#else + log_buffer.reset(new ChattyLogBuffer(&reader_list, &tags, &prune_list, &stats)); +#endif size_t data_left = size; const uint8_t** pdata = &data; @@ -102,12 +129,30 @@ extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) { log_id_for_each(i) { log_buffer->SetSize(i, 10000); } while (data_left >= sizeof(LogInput) + 2 * sizeof(uint8_t)) { - if (!write_log_messages(pdata, &data_left, log_buffer, &stats)) { + if (!write_log_messages(pdata, &data_left, log_buffer.get(), &stats)) { return 0; } } + // Read out all of the logs. + { + auto lock = std::unique_lock{reader_list.reader_threads_lock()}; + std::unique_ptr test_writer(new NoopWriter()); + std::unique_ptr log_reader( + new LogReaderThread(log_buffer.get(), &reader_list, std::move(test_writer), true, 0, + kLogMaskAll, 0, {}, 1, {})); + reader_list.reader_threads().emplace_back(std::move(log_reader)); + } + + // Wait until the reader has finished. + while (true) { + usleep(50); + auto lock = std::unique_lock{reader_list.reader_threads_lock()}; + if (reader_list.reader_threads().size() == 0) { + break; + } + } + log_id_for_each(i) { log_buffer->Clear(i, 0); } return 0; } -} // namespace android diff --git a/logd/fuzz/serialized_log_buffer_fuzzer.cpp b/logd/fuzz/serialized_log_buffer_fuzzer.cpp new file mode 100644 index 000000000..d4795b07d --- /dev/null +++ b/logd/fuzz/serialized_log_buffer_fuzzer.cpp @@ -0,0 +1,19 @@ +/* + * Copyright 2020 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#define FUZZ_SERIALIZED + +#include "log_buffer_log_fuzzer.cpp"