From 568cde5e233633da1cc199406374bb996236b3cf Mon Sep 17 00:00:00 2001 From: Linus Groh Date: Wed, 13 Jan 2021 23:59:22 +0100 Subject: [PATCH] Kernel+LibELF+LibCoreDump+CrashReporter: Use JSON for ProcessInfo This is in preparation of adding (much) more process information to coredumps. As we can only have one null-terminated char[] of arbitrary length in each struct it's now a single JSON blob, which is a great fit: easily extensible in the future and allows for key/value pairs and even nested objects, which will be used e.g. for the process environment, for example. --- Kernel/CoreDump.cpp | 14 +++---- Userland/Applications/CrashReporter/main.cpp | 9 ++-- Userland/Libraries/LibCoreDump/Reader.cpp | 43 ++++++++++++++++++-- Userland/Libraries/LibCoreDump/Reader.h | 11 ++++- Userland/Libraries/LibELF/CoreDump.h | 16 ++++++-- 5 files changed, 72 insertions(+), 21 deletions(-) diff --git a/Kernel/CoreDump.cpp b/Kernel/CoreDump.cpp index 5a8cccc489..feaf75a1f3 100644 --- a/Kernel/CoreDump.cpp +++ b/Kernel/CoreDump.cpp @@ -219,15 +219,15 @@ ByteBuffer CoreDump::create_notes_process_data() const ELF::Core::ProcessInfo info {}; info.header.type = ELF::Core::NotesEntryHeader::Type::ProcessInfo; - info.pid = m_process->pid().value(); - info.termination_signal = m_process->termination_signal(); - process_data.append((void*)&info, sizeof(info)); - auto executable_path = String::empty(); - if (auto executable = m_process->executable()) - executable_path = executable->absolute_path(); - process_data.append(executable_path.characters(), executable_path.length() + 1); + JsonObject process_obj; + process_obj.set("pid", m_process->pid().value()); + process_obj.set("termination_signal", m_process->termination_signal()); + process_obj.set("executable_path", m_process->executable() ? m_process->executable()->absolute_path() : String::empty()); + + auto json_data = process_obj.to_string(); + process_data.append(json_data.characters(), json_data.length() + 1); return process_data; } diff --git a/Userland/Applications/CrashReporter/main.cpp b/Userland/Applications/CrashReporter/main.cpp index eea07b2c69..668ccd234c 100644 --- a/Userland/Applications/CrashReporter/main.cpp +++ b/Userland/Applications/CrashReporter/main.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) 2020, Linus Groh + * Copyright (c) 2020-2021, Linus Groh * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -96,11 +96,10 @@ int main(int argc, char** argv) warnln("Could not open coredump '{}'", coredump_path); return 1; } - auto& process_info = coredump->process_info(); backtrace = build_backtrace(*coredump); - executable_path = String(process_info.executable_path); - pid = process_info.pid; - termination_signal = process_info.termination_signal; + executable_path = coredump->process_executable_path(); + pid = coredump->process_pid(); + termination_signal = coredump->process_termination_signal(); } auto app = GUI::Application::construct(argc, argv); diff --git a/Userland/Libraries/LibCoreDump/Reader.cpp b/Userland/Libraries/LibCoreDump/Reader.cpp index e12611bd46..d489520af5 100644 --- a/Userland/Libraries/LibCoreDump/Reader.cpp +++ b/Userland/Libraries/LibCoreDump/Reader.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include @@ -88,7 +89,7 @@ void Reader::NotesEntryIterator::next() switch (type()) { case ELF::Core::NotesEntryHeader::Type::ProcessInfo: { const auto* current = reinterpret_cast(m_current); - m_current = reinterpret_cast(current->executable_path + strlen(current->executable_path) + 1); + m_current = reinterpret_cast(current->json_data + strlen(current->json_data) + 1); break; } case ELF::Core::NotesEntryHeader::Type::ThreadInfo: { @@ -127,14 +128,24 @@ Optional Reader::peek_memory(FlatPtr address) const return *(const uint32_t*)(®ion_data[offset_in_region]); } -const ELF::Core::ProcessInfo& Reader::process_info() const +const JsonObject Reader::process_info() const { + const ELF::Core::ProcessInfo* process_info_notes_entry = nullptr; for (NotesEntryIterator it((const u8*)m_coredump_image.program_header(m_notes_segment_index).raw_data()); !it.at_end(); it.next()) { if (it.type() != ELF::Core::NotesEntryHeader::Type::ProcessInfo) continue; - return reinterpret_cast(*it.current()); + process_info_notes_entry = reinterpret_cast(it.current()); + break; } - ASSERT_NOT_REACHED(); + if (!process_info_notes_entry) + return {}; + auto process_info_json_value = JsonValue::from_string(process_info_notes_entry->json_data); + if (!process_info_json_value.has_value()) + return {}; + if (!process_info_json_value.value().is_object()) + return {}; + return process_info_json_value.value().as_object(); + // FIXME: Maybe just cache this on the Reader instance after first access. } const ELF::Core::MemoryRegionInfo* Reader::region_containing(FlatPtr address) const @@ -155,6 +166,30 @@ const Backtrace Reader::backtrace() const return Backtrace(*this); } +int Reader::process_pid() const +{ + auto process_info = this->process_info(); + auto pid = process_info.get("pid"); + return pid.to_number(); +} + +u8 Reader::process_termination_signal() const +{ + auto process_info = this->process_info(); + auto termination_signal = process_info.get("termination_signal"); + auto signal_number = termination_signal.to_number(); + if (signal_number <= SIGINVAL || signal_number >= NSIG) + return SIGINVAL; + return (u8)signal_number; +} + +String Reader::process_executable_path() const +{ + auto process_info = this->process_info(); + auto executable_path = process_info.get("executable_path"); + return executable_path.as_string_or({}); +} + const HashMap Reader::metadata() const { const ELF::Core::Metadata* metadata_notes_entry = nullptr; diff --git a/Userland/Libraries/LibCoreDump/Reader.h b/Userland/Libraries/LibCoreDump/Reader.h index 9ac411a204..dbba53fa4d 100644 --- a/Userland/Libraries/LibCoreDump/Reader.h +++ b/Userland/Libraries/LibCoreDump/Reader.h @@ -44,8 +44,6 @@ public: static OwnPtr create(const String&); ~Reader(); - const ELF::Core::ProcessInfo& process_info() const; - template void for_each_memory_region_info(Func func) const; @@ -66,6 +64,10 @@ public: const LibraryData* library_containing(FlatPtr address) const; const Backtrace backtrace() const; + + int process_pid() const; + u8 process_termination_signal() const; + String process_executable_path() const; const HashMap metadata() const; private: @@ -86,6 +88,11 @@ private: const u8* start { nullptr }; }; + // Private as we don't need anyone poking around in this JsonObject + // manually - we know very well what should be included and expose that + // as getters with the appropriate (non-JsonValue) types. + const JsonObject process_info() const; + NonnullRefPtr m_coredump_file; ELF::Image m_coredump_image; ssize_t m_notes_segment_index { -1 }; diff --git a/Userland/Libraries/LibELF/CoreDump.h b/Userland/Libraries/LibELF/CoreDump.h index 4978430306..58ba114195 100644 --- a/Userland/Libraries/LibELF/CoreDump.h +++ b/Userland/Libraries/LibELF/CoreDump.h @@ -50,9 +50,14 @@ struct [[gnu::packed]] NotesEntry { struct [[gnu::packed]] ProcessInfo { NotesEntryHeader header; - int pid; - u8 termination_signal; - char executable_path[]; // Null terminated + // Information is stored as JSON blob to allow arbitrary + // number and length of strings/objects/arrays. + // + // Keys: + // - "pid" (int) + // - "termination_signal" (u8) + // - "executable_path" (String) + char json_data[]; // Null terminated }; struct [[gnu::packed]] ThreadInfo { @@ -81,6 +86,11 @@ struct [[gnu::packed]] MemoryRegionInfo { struct [[gnu::packed]] Metadata { NotesEntryHeader header; + // Arbitrary metadata, set via SC_set_coredump_metadata. + // Limited to 16 entries and 16 KiB keys/values by the kernel. + // + // Well-known keys: + // - "assertion": Used by LibC's __assertion_failed() to store assertion info char json_data[]; // Null terminated };