Browse Source

LibCoredump: Copy out the FooInfo structs to an aligned address

We were handing out unaligned pointers to these, which made UBSAN super
mad, copy them out to avoid that.
Ali Mohammad Pur 3 years ago
parent
commit
e72521f286

+ 1 - 1
Userland/Applications/CrashReporter/main.cpp

@@ -153,7 +153,7 @@ ErrorOr<int> serenity_main(Main::Arguments arguments)
 
 
     Vector<String> memory_regions;
     Vector<String> memory_regions;
     coredump->for_each_memory_region_info([&](auto& memory_region_info) {
     coredump->for_each_memory_region_info([&](auto& memory_region_info) {
-        memory_regions.append(String::formatted("{:p} - {:p}: {}", memory_region_info.region_start, memory_region_info.region_end, (const char*)memory_region_info.region_name));
+        memory_regions.append(String::formatted("{:p} - {:p}: {}", memory_region_info.region_start, memory_region_info.region_end, memory_region_info.region_name));
         return IterationDecision::Continue;
         return IterationDecision::Continue;
     });
     });
 
 

+ 4 - 4
Userland/Libraries/LibCoredump/Backtrace.cpp

@@ -17,7 +17,7 @@
 
 
 namespace Coredump {
 namespace Coredump {
 
 
-ELFObjectInfo const* Backtrace::object_info_for_region(ELF::Core::MemoryRegionInfo const& region)
+ELFObjectInfo const* Backtrace::object_info_for_region(MemoryRegionInfo const& region)
 {
 {
     auto path = region.object_name();
     auto path = region.object_name();
     if (!path.starts_with('/') && Core::File::looks_like_shared_library(path))
     if (!path.starts_with('/') && Core::File::looks_like_shared_library(path))
@@ -99,8 +99,8 @@ Backtrace::~Backtrace()
 
 
 void Backtrace::add_entry(const Reader& coredump, FlatPtr ip)
 void Backtrace::add_entry(const Reader& coredump, FlatPtr ip)
 {
 {
-    auto* ip_region = coredump.region_containing(ip);
-    if (!ip_region) {
+    auto ip_region = coredump.region_containing(ip);
+    if (!ip_region.has_value()) {
         m_entries.append({ ip, {}, {}, {} });
         m_entries.append({ ip, {}, {}, {} });
         return;
         return;
     }
     }
@@ -116,7 +116,7 @@ void Backtrace::add_entry(const Reader& coredump, FlatPtr ip)
     // the PT_LOAD header for the .text segment isn't the first one
     // the PT_LOAD header for the .text segment isn't the first one
     // in the object file.
     // in the object file.
     auto region = coredump.first_region_for_object(object_name);
     auto region = coredump.first_region_for_object(object_name);
-    auto* object_info = object_info_for_region(*region);
+    auto object_info = object_info_for_region(*region);
     if (!object_info)
     if (!object_info)
         return;
         return;
 
 

+ 1 - 1
Userland/Libraries/LibCoredump/Backtrace.h

@@ -45,7 +45,7 @@ public:
 
 
 private:
 private:
     void add_entry(const Reader&, FlatPtr ip);
     void add_entry(const Reader&, FlatPtr ip);
-    ELFObjectInfo const* object_info_for_region(ELF::Core::MemoryRegionInfo const&);
+    ELFObjectInfo const* object_info_for_region(MemoryRegionInfo const&);
 
 
     bool m_skip_loader_so { false };
     bool m_skip_loader_so { false };
     ELF::Core::ThreadInfo m_thread_info;
     ELF::Core::ThreadInfo m_thread_info;

+ 12 - 12
Userland/Libraries/LibCoredump/Reader.cpp

@@ -135,8 +135,8 @@ bool Reader::NotesEntryIterator::at_end() const
 
 
 Optional<FlatPtr> Reader::peek_memory(FlatPtr address) const
 Optional<FlatPtr> Reader::peek_memory(FlatPtr address) const
 {
 {
-    const auto* region = region_containing(address);
-    if (!region)
+    auto region = region_containing(address);
+    if (!region.has_value())
         return {};
         return {};
 
 
     FlatPtr offset_in_region = address - region->region_start;
     FlatPtr offset_in_region = address - region->region_start;
@@ -167,12 +167,12 @@ const JsonObject Reader::process_info() const
     // FIXME: Maybe just cache this on the Reader instance after first access.
     // FIXME: Maybe just cache this on the Reader instance after first access.
 }
 }
 
 
-ELF::Core::MemoryRegionInfo const* Reader::first_region_for_object(StringView object_name) const
+Optional<MemoryRegionInfo> Reader::first_region_for_object(StringView object_name) const
 {
 {
-    ELF::Core::MemoryRegionInfo const* ret = nullptr;
+    Optional<MemoryRegionInfo> ret;
     for_each_memory_region_info([&ret, &object_name](auto& region_info) {
     for_each_memory_region_info([&ret, &object_name](auto& region_info) {
         if (region_info.object_name() == object_name) {
         if (region_info.object_name() == object_name) {
-            ret = &region_info;
+            ret = region_info;
             return IterationDecision::Break;
             return IterationDecision::Break;
         }
         }
         return IterationDecision::Continue;
         return IterationDecision::Continue;
@@ -180,12 +180,12 @@ ELF::Core::MemoryRegionInfo const* Reader::first_region_for_object(StringView ob
     return ret;
     return ret;
 }
 }
 
 
-const ELF::Core::MemoryRegionInfo* Reader::region_containing(FlatPtr address) const
+Optional<MemoryRegionInfo> Reader::region_containing(FlatPtr address) const
 {
 {
-    const ELF::Core::MemoryRegionInfo* ret = nullptr;
-    for_each_memory_region_info([&ret, address](const ELF::Core::MemoryRegionInfo& region_info) {
+    Optional<MemoryRegionInfo> ret;
+    for_each_memory_region_info([&ret, address](const auto& region_info) {
         if (region_info.region_start <= address && region_info.region_end >= address) {
         if (region_info.region_start <= address && region_info.region_end >= address) {
-            ret = &region_info;
+            ret = region_info;
             return IterationDecision::Break;
             return IterationDecision::Break;
         }
         }
         return IterationDecision::Continue;
         return IterationDecision::Continue;
@@ -272,8 +272,8 @@ HashMap<String, String> Reader::metadata() const
 const Reader::LibraryData* Reader::library_containing(FlatPtr address) const
 const Reader::LibraryData* Reader::library_containing(FlatPtr address) const
 {
 {
     static HashMap<String, OwnPtr<LibraryData>> cached_libs;
     static HashMap<String, OwnPtr<LibraryData>> cached_libs;
-    auto* region = region_containing(address);
-    if (!region)
+    auto region = region_containing(address);
+    if (!region.has_value())
         return {};
         return {};
 
 
     auto name = region->object_name();
     auto name = region->object_name();
@@ -300,7 +300,7 @@ const Reader::LibraryData* Reader::library_containing(FlatPtr address) const
 void Reader::for_each_library(Function<void(LibraryInfo)> func) const
 void Reader::for_each_library(Function<void(LibraryInfo)> func) const
 {
 {
     HashTable<String> libraries;
     HashTable<String> libraries;
-    for_each_memory_region_info([&](ELF::Core::MemoryRegionInfo const& region) {
+    for_each_memory_region_info([&](auto const& region) {
         auto name = region.object_name();
         auto name = region.object_name();
         if (name.is_null() || libraries.contains(name))
         if (name.is_null() || libraries.contains(name))
             return IterationDecision::Continue;
             return IterationDecision::Continue;

+ 38 - 4
Userland/Libraries/LibCoredump/Reader.h

@@ -6,6 +6,7 @@
 
 
 #pragma once
 #pragma once
 
 
+#include <AK/ByteReader.h>
 #include <AK/HashMap.h>
 #include <AK/HashMap.h>
 #include <AK/Noncopyable.h>
 #include <AK/Noncopyable.h>
 #include <AK/OwnPtr.h>
 #include <AK/OwnPtr.h>
@@ -15,6 +16,24 @@
 
 
 namespace Coredump {
 namespace Coredump {
 
 
+struct MemoryRegionInfo {
+    ELF::Core::NotesEntryHeader header;
+    uint64_t region_start;
+    uint64_t region_end;
+    uint16_t program_header_index;
+    StringView region_name;
+
+    StringView object_name() const
+    {
+        if (region_name.contains("Loader.so"))
+            return "Loader.so"sv;
+        auto maybe_colon_index = region_name.find(':');
+        if (!maybe_colon_index.has_value())
+            return {};
+        return region_name.substring_view(0, *maybe_colon_index);
+    }
+};
+
 class Reader {
 class Reader {
     AK_MAKE_NONCOPYABLE(Reader);
     AK_MAKE_NONCOPYABLE(Reader);
     AK_MAKE_NONMOVABLE(Reader);
     AK_MAKE_NONMOVABLE(Reader);
@@ -40,8 +59,8 @@ public:
     const ELF::Image& image() const { return m_coredump_image; }
     const ELF::Image& image() const { return m_coredump_image; }
 
 
     Optional<FlatPtr> peek_memory(FlatPtr address) const;
     Optional<FlatPtr> peek_memory(FlatPtr address) const;
-    ELF::Core::MemoryRegionInfo const* first_region_for_object(StringView object_name) const;
-    const ELF::Core::MemoryRegionInfo* region_containing(FlatPtr address) const;
+    Optional<MemoryRegionInfo> first_region_for_object(StringView object_name) const;
+    Optional<MemoryRegionInfo> region_containing(FlatPtr address) const;
 
 
     struct LibraryData {
     struct LibraryData {
         String name;
         String name;
@@ -104,7 +123,20 @@ void Reader::for_each_memory_region_info(Func func) const
     for (; !it.at_end(); it.next()) {
     for (; !it.at_end(); it.next()) {
         if (it.type() != ELF::Core::NotesEntryHeader::Type::MemoryRegionInfo)
         if (it.type() != ELF::Core::NotesEntryHeader::Type::MemoryRegionInfo)
             continue;
             continue;
-        auto& memory_region_info = *bit_cast<const ELF::Core::MemoryRegionInfo*>(it.current());
+        ELF::Core::MemoryRegionInfo raw_memory_region_info;
+        ReadonlyBytes raw_data {
+            it.current(),
+            sizeof(raw_memory_region_info),
+        };
+        ByteReader::load(raw_data.data(), raw_memory_region_info);
+
+        MemoryRegionInfo memory_region_info {
+            raw_memory_region_info.header,
+            raw_memory_region_info.region_start,
+            raw_memory_region_info.region_end,
+            raw_memory_region_info.program_header_index,
+            { bit_cast<const char*>(raw_data.offset_pointer(raw_data.size())) },
+        };
         IterationDecision decision = func(memory_region_info);
         IterationDecision decision = func(memory_region_info);
         if (decision == IterationDecision::Break)
         if (decision == IterationDecision::Break)
             return;
             return;
@@ -118,7 +150,9 @@ void Reader::for_each_thread_info(Func func) const
     for (; !it.at_end(); it.next()) {
     for (; !it.at_end(); it.next()) {
         if (it.type() != ELF::Core::NotesEntryHeader::Type::ThreadInfo)
         if (it.type() != ELF::Core::NotesEntryHeader::Type::ThreadInfo)
             continue;
             continue;
-        auto& thread_info = *bit_cast<const ELF::Core::ThreadInfo*>(it.current());
+        ELF::Core::ThreadInfo thread_info;
+        ByteReader::load(bit_cast<const u8*>(it.current()), thread_info);
+
         IterationDecision decision = func(thread_info);
         IterationDecision decision = func(thread_info);
         if (decision == IterationDecision::Break)
         if (decision == IterationDecision::Break)
             return;
             return;