memory: Add Read32OrNullopt to better detect unmapped memory execution

src/common/CMakeLists.txt

@@ -59,6 +59,7 @@ add_library(citra_common STATIC
     microprofile.cpp
     microprofile.h
     microprofileui.h
+    optional_helper.h
     param_package.cpp
     param_package.h
     play_time_manager.cpp

src/common/optional_helper.h

@@ -0,0 +1,42 @@
+// Copyright Citra Emulator Project / Azahar Emulator Project
+// Licensed under GPLv2 or any later version
+// Refer to the license.txt file included.
+
+#pragma once
+
+#include <optional>
+#include <type_traits>
+
+namespace detail {
+template <typename T>
+struct is_optional_trait : std::false_type {
+    using value_type = T;
+};
+
+template <typename T>
+struct is_optional_trait<std::optional<T>> : std::true_type {
+    using value_type = T;
+};
+} // namespace detail
+
+/**
+ * Returns true if T is a std::optional, false otherwise.
+ * For example:
+ * using Test1 = u32;
+ * using Test2 = std::optional<u32>;
+ * is_optional_type<Test1> -> false
+ * is_optional_type<Test2> -> true
+ */
+template <typename T>
+inline constexpr bool is_optional_type = detail::is_optional_trait<T>::value;
+
+/**
+ * Provides the inner type of T if it is a std::optional, or T itself if it is not.
+ * For example:
+ * using Test1 = u32;
+ * using Test2 = std::optional<u32>;
+ * optional_value_type<Test1> -> u32
+ * optional_value_type<Test2> -> u32
+ */
+template <typename T>
+using optional_inner_or_type = typename detail::is_optional_trait<T>::value_type;

src/core/arm/dynarmic/arm_dynarmic.cpp

@@ -37,18 +37,7 @@ public:
     ~DynarmicUserCallbacks() = default;
 
     std::optional<std::uint32_t> MemoryReadCode(VAddr vaddr) override {
-        constexpr VAddr low_page_limit = 0x1000;
-
-        // This check prevents a common cascading error that results from the
-        // memory system allowing unmapped memory accesses (in some situations,
-        // a vtable is read from an invalid address and execution jumps to the
-        // zero page). On real hw, it's not normally possible to map the zero page.
-        if (vaddr < low_page_limit) [[unlikely]] {
-            LOG_CRITICAL(Debug, "Tried to read code from low address 0x{:08x}", vaddr);
-            return {};
-        }
-
-        return MemoryRead32(vaddr);
+        return memory.Read32OrNullopt(vaddr);
     }
 
     std::uint8_t MemoryRead8(VAddr vaddr) override {
@@ -127,6 +116,23 @@ public:
             return;
         }
 
+        static constexpr auto ExceptionToString = [](Dynarmic::A32::Exception e) -> std::string {
+            switch (e) {
+            case Dynarmic::A32::Exception::UndefinedInstruction:
+                return "UndefinedInstruction";
+            case Dynarmic::A32::Exception::UnpredictableInstruction:
+                return "UnpredictableInstruction";
+            case Dynarmic::A32::Exception::DecodeError:
+                return "DecodeError";
+            case Dynarmic::A32::Exception::NoExecuteFault:
+                return "NoExecuteFault";
+            case Dynarmic::A32::Exception::Breakpoint:
+                return "Breakpoint";
+            default:
+                return fmt::format("Unknown({})", e);
+            }
+        };
+
         parent.SetPC(pc);
 #ifdef ENABLE_GDBSTUB
         if (GDBStub::IsConnected()) {
@@ -138,7 +144,8 @@ public:
             for (int i = 0; i < 16; i++) {
                 error += fmt::format("r{:02d} = {:08X}\n", i, parent.GetReg(i));
             }
-            error += fmt::format("ExceptionRaised(exception = {}, pc = {:08X})", exception, pc);
+            error += fmt::format("ExceptionRaised(exception = {}, pc = {:08X})",
+                                 ExceptionToString(exception), pc);
             parent.system.SetStatus(Core::System::ResultStatus::ErrorCoreExceptionRaised,
                                     error.c_str());
         }

src/core/memory.cpp

@@ -13,6 +13,7 @@
 #include "common/atomic_ops.h"
 #include "common/common_types.h"
 #include "common/logging/log.h"
+#include "common/optional_helper.h"
 #include "common/settings.h"
 #include "common/swap.h"
 #include "core/arm/arm_interface.h"
@@ -564,7 +565,7 @@ void MemorySystem::UnregisterPageTable(std::shared_ptr<PageTable> page_table) {
 }
 
 template <typename T>
-T MemorySystem::UnmappedAccess(const VAddr vaddr, const T value, bool read) {
+void MemorySystem::UnmappedAccess(const VAddr vaddr, const T value, bool read) {
     const std::string mode = (read ? "Read" : "Write");
     const std::string value_str = read ? std::string("") : fmt::format(" 0x{:08X}", value);
     const std::string message = fmt::format("unmapped {}{}{} @ 0x{:08X} at PC 0x{:08X}", mode,
@@ -580,16 +581,20 @@ T MemorySystem::UnmappedAccess(const VAddr vaddr, const T value, bool read) {
     }
 
     LOG_ERROR(HW_Memory, "{}", message);
-    return {};
 }
 
 template <typename T>
 T MemorySystem::Read(const std::shared_ptr<PageTable>& page_table, const VAddr vaddr) {
+    constexpr bool is_optional = is_optional_type<T>;
+    using ReadType = optional_inner_or_type<T>;
+
+    constexpr size_t read_size = sizeof(ReadType);
+
     const u8* page_pointer = page_table->pointers[vaddr >> CITRA_PAGE_BITS];
     if (page_pointer) {
         // NOTE: Avoid adding any extra logic to this fast-path block
-        T value;
-        std::memcpy(&value, &page_pointer[vaddr & CITRA_PAGE_MASK], sizeof(T));
+        ReadType value;
+        std::memcpy(&value, &page_pointer[vaddr & CITRA_PAGE_MASK], read_size);
         return value;
     }
 
@@ -598,20 +603,27 @@ T MemorySystem::Read(const std::shared_ptr<PageTable>& page_table, const VAddr v
     if (vaddr & (1 << 31)) {
         PAddr paddr = (vaddr & ~(1 << 31));
         if ((paddr & 0xF0000000) == Memory::FCRAM_PADDR) { // Check FCRAM region
-            T value;
-            std::memcpy(&value, GetFCRAMPointer(paddr - Memory::FCRAM_PADDR), sizeof(T));
+            ReadType value;
+            std::memcpy(&value, GetFCRAMPointer(paddr - Memory::FCRAM_PADDR), read_size);
             return value;
         } else if ((paddr & 0xF0000000) == 0x10000000 &&
                    paddr >= Memory::IO_AREA_PADDR) { // Check MMIO region
-            return impl->system.GPU().ReadReg(static_cast<VAddr>(paddr) - Memory::IO_AREA_PADDR +
-                                              0x1EC00000);
+            return static_cast<ReadType>(impl->system.GPU().ReadReg(
+                static_cast<VAddr>(paddr) - Memory::IO_AREA_PADDR + 0x1EC00000));
         }
     }
 
     PageType type = page_table->attributes[vaddr >> CITRA_PAGE_BITS];
     switch (type) {
     case PageType::Unmapped: {
-        return UnmappedAccess<T>(vaddr, 0, true);
+
+        UnmappedAccess<ReadType>(vaddr, 0, true);
+
+        if constexpr (is_optional) {
+            return std::nullopt;
+        } else {
+            return T{};
+        }
     }
     case PageType::Memory:
         ASSERT_MSG(false, "Mapped memory page without a pointer @ {:08X}", vaddr);
@@ -621,11 +633,11 @@ T MemorySystem::Read(const std::shared_ptr<PageTable>& page_table, const VAddr v
         ASSERT_MSG(it != page_table->watchpoint_pages_map.end(),
                    "Missing memory for watchpoint page");
 
-        T value;
-        std::memcpy(&value, it->second.memory.GetPtr() + (vaddr & CITRA_PAGE_MASK), sizeof(T));
+        ReadType value;
+        std::memcpy(&value, it->second.memory.GetPtr() + (vaddr & CITRA_PAGE_MASK), read_size);
 
 #ifdef ENABLE_GDBSTUB
-        if (GDBStub::CheckBreakpoint(vaddr, sizeof(T), GDBStub::BreakpointType::Read)) {
+        if (GDBStub::CheckBreakpoint(vaddr, read_size, GDBStub::BreakpointType::Read)) {
             GDBStub::Break(SIGTRAP);
         }
 #endif
@@ -633,20 +645,20 @@ T MemorySystem::Read(const std::shared_ptr<PageTable>& page_table, const VAddr v
         return value;
     }
     [[likely]] case PageType::RasterizerCachedMemory: {
-        RasterizerFlushVirtualRegion(vaddr, sizeof(T), FlushMode::Flush);
+        RasterizerFlushVirtualRegion(vaddr, read_size, FlushMode::Flush);
 
-        T value;
-        std::memcpy(&value, GetPointerForRasterizerCache(vaddr), sizeof(T));
+        ReadType value;
+        std::memcpy(&value, GetPointerForRasterizerCache(vaddr), read_size);
         return value;
     }
     case PageType::RasterizerCachedMemoryWatchpoint: {
-        RasterizerFlushVirtualRegion(vaddr, sizeof(T), FlushMode::Flush);
+        RasterizerFlushVirtualRegion(vaddr, read_size, FlushMode::Flush);
 
-        T value;
-        std::memcpy(&value, GetPointerForRasterizerCache(vaddr), sizeof(T));
+        ReadType value;
+        std::memcpy(&value, GetPointerForRasterizerCache(vaddr), read_size);
 
 #ifdef ENABLE_GDBSTUB
-        if (GDBStub::CheckBreakpoint(vaddr, sizeof(T), GDBStub::BreakpointType::Read)) {
+        if (GDBStub::CheckBreakpoint(vaddr, read_size, GDBStub::BreakpointType::Read)) {
             GDBStub::Break(SIGTRAP);
         }
 #endif
@@ -657,7 +669,11 @@ T MemorySystem::Read(const std::shared_ptr<PageTable>& page_table, const VAddr v
         UNREACHABLE();
     }
 
-    return T{};
+    if constexpr (is_optional) {
+        return std::nullopt;
+    } else {
+        return T{};
+    }
 }
 
 template <typename T>
@@ -1047,6 +1063,14 @@ u64 MemorySystem::Read64(const Kernel::Process& process, VAddr addr) {
     return Read<u64_le>(process.vm_manager.page_table, addr);
 }
 
+std::optional<u32> MemorySystem::Read32OrNullopt(VAddr addr) {
+    return Read<std::optional<u32_le>>(impl->current_page_table, addr);
+}
+
+std::optional<u32> MemorySystem::Read32OrNullopt(const Kernel::Process& process, VAddr addr) {
+    return Read<std::optional<u32_le>>(process.vm_manager.page_table, addr);
+}
+
 void MemorySystem::ReadBlock(const Kernel::Process& process, const VAddr src_addr,
                              void* dest_buffer, const std::size_t size) {
     return impl->ReadBlockImpl<false>(process, src_addr, dest_buffer, size);

src/core/memory.h

@@ -5,11 +5,13 @@
 #pragma once
 #include <array>
 #include <cstddef>
+#include <optional>
 #include <string>
 #include <boost/serialization/array.hpp>
 #include <boost/serialization/vector.hpp>
 #include "common/common_types.h"
 #include "common/memory_ref.h"
+#include "common/swap.h"
 
 namespace Kernel {
 class Process;
@@ -387,6 +389,29 @@ public:
      */
     u64 Read64(const Kernel::Process& process, VAddr addr);
 
+    /**
+     * Reads a 32-bit unsigned value from the current process' address space
+     * at the given virtual address. If the address is invalid std::nullopt
+     * is returned instead.
+     *
+     * @param addr The virtual address to read the 32-bit value from.
+     *
+     * @returns the read 32-bit unsigned value or std::nullopt.
+     */
+    std::optional<u32> Read32OrNullopt(VAddr addr);
+
+    /**
+     * Reads a 32-bit unsigned value from the process' address space
+     * at the given virtual address. If the address is invalid std::nullopt
+     * is returned instead.
+     *
+     * @param process The process to read from.
+     * @param addr The virtual address to read the 32-bit value from.
+     *
+     * @returns the read 32-bit unsigned value or std::nullopt.
+     */
+    std::optional<u32> Read32OrNullopt(const Kernel::Process& process, VAddr addr);
+
     /**
      * Writes an 8-bit unsigned integer to the given virtual address in
      * the current process' address space.
@@ -682,7 +707,7 @@ public:
 
 private:
     template <typename T>
-    T UnmappedAccess(const VAddr vaddr, const T value, bool read);
+    void UnmappedAccess(const VAddr vaddr, const T value, bool read);
 
     template <typename T>
     T Read(const std::shared_ptr<PageTable>& page_table, const VAddr vaddr);