From 37fe9119526c03ca064166130cc983c55b9a9e86 Mon Sep 17 00:00:00 2001 From: Exverge Date: Sat, 26 Sep 2026 16:02:35 +0200 Subject: [PATCH] [common/sparse_large_vector] correct Win32 exception handler (#4481) - [x] I have read and followed the [Contribution Guidelines](https://git.eden-emu.dev/eden-emu/eden/src/branch/master/CONTRIBUTING.md#code-contributions). - [x] I have read and followed the [AI Policy](https://git.eden-emu.dev/eden-emu/eden/src/branch/master/docs/policies/AI.md) - [x] I have read and followed the [Coding Guidelines](https://git.eden-emu.dev/eden-emu/eden/src/branch/master/docs/policies/Coding.md) to the best of my ability. ------------------- Adds mutex to prevent multiple threads from accessing the vector at the same time and corrects the Windows exception handler to use the correct faulting address used by the exception handler (previously used the instruction address instead of the fault address) and properly shifts the stored values for the handler. Fixes weird compiler-specific bugs on Windows Co-authored-by: bruno Reviewed-on: https://git.eden-emu.dev/eden-emu/eden/pulls/4481 Reviewed-by: lizzie Reviewed-by: MaranBr --- src/common/sparse_large_vector.cpp | 46 ++++++++++++++++++++---------- src/common/sparse_large_vector.h | 27 +++++++++++------- 2 files changed, 47 insertions(+), 26 deletions(-) diff --git a/src/common/sparse_large_vector.cpp b/src/common/sparse_large_vector.cpp index 745575a880..eee8ff6d18 100644 --- a/src/common/sparse_large_vector.cpp +++ b/src/common/sparse_large_vector.cpp @@ -6,6 +6,7 @@ // SPDX-License-Identifier: GPL-2.0-or-later #ifdef _WIN32 +#include #include #include #else @@ -20,32 +21,36 @@ namespace Common { #ifdef _WIN32 static std::vector> vector_regions {}; +static std::mutex vector_regions_mutex {}; // Workaround for handling non-commited memory accessed by Dynarmic; usually result of an error static LONG WINAPI FakePageFaultHandler(PEXCEPTION_POINTERS info) { DWORD code = info->ExceptionRecord->ExceptionCode; - u64 exception_addr = reinterpret_cast(info->ExceptionRecord->ExceptionAddress); + u64 exception_addr = reinterpret_cast(info->ExceptionRecord->ExceptionInformation[1]); - if (code != EXCEPTION_ACCESS_VIOLATION) { + if (code != EXCEPTION_ACCESS_VIOLATION || info->ExceptionRecord->ExceptionInformation[0] == 1) { // Not our problem return EXCEPTION_CONTINUE_SEARCH; } u64 addr = 0, addr2 = 0; - for (auto region: vector_regions) { - auto addr_shifted = exception_addr >> HostPageBits; - if (region.first <= addr_shifted && addr_shifted <= region.second) { - addr = addr_shifted; - } + { + std::lock_guard lock(vector_regions_mutex); + for (auto region: vector_regions) { + auto addr_shifted = exception_addr >> HostPageBits; + if (region.first <= addr_shifted && addr_shifted <= region.second) { + addr = addr_shifted; + } - // Page-boundary accesses - if (auto addr_ = (exception_addr + 0x40) >> HostPageBits; addr_ != addr_shifted && region.first <= addr_ && addr_ <= region.second) { - addr2 = addr_; - } + // Page-boundary accesses + if (auto addr_ = (exception_addr + 0x40) >> HostPageBits; addr_ != addr_shifted && region.first <= addr_ && addr_ <= region.second) { + addr2 = addr_; + } - if (addr != 0 || addr2 != 0) { - break; + if (addr != 0 || addr2 != 0) { + break; + } } } @@ -77,8 +82,16 @@ bool CommitVectorPage(uintptr_t addr, bool write) noexcept { auto res = VirtualQuery(reinterpret_cast(addr), &info, sizeof(info)); if (res == 0) { LOG_CRITICAL(HW_Memory, "Failed to query large buffer region at {:#x} with error {}, will try committing anyway", addr, GetLastError()); + } else if (info.State == MEM_COMMIT) { + DWORD old_protect {}; + auto perm = write ? PAGE_READWRITE : PAGE_READONLY; + if (!VirtualProtect(reinterpret_cast(addr), HostPageSize, perm, &old_protect)) { + LOG_ERROR(HW_Memory, "Failed to change permissions of large buffer region at {:#x}, error {}", addr, GetLastError()); + return false; + } + return true; } else if (info.State != MEM_RESERVE) { - LOG_ERROR(HW_Memory, "Tried to commit an unreserved large buffer region at {:#x} that is not mapped or is already committed (state {:#x})", addr, info.State); + LOG_ERROR(HW_Memory, "Tried to commit an unreserved large buffer region at {:#x} that is not mapped (state {:#x})", addr, info.State); return false; } @@ -123,7 +136,8 @@ void* AllocateMemoryPages(std::size_t size) noexcept { void* base = VirtualAlloc(nullptr, size, MEM_RESERVE, PAGE_READWRITE); if (base != nullptr) { - vector_regions.emplace_back(reinterpret_cast(base), reinterpret_cast(base) + size); + std::lock_guard lock(vector_regions_mutex); + vector_regions.emplace_back(reinterpret_cast(base) >> HostPageBits, (reinterpret_cast(base) + size) >> HostPageBits); static std::once_flag flag; std::call_once(flag, []() { AddVectoredExceptionHandler(1, FakePageFaultHandler); }); @@ -149,6 +163,8 @@ void FreeMemoryPages(void* base, [[maybe_unused]] std::size_t size) noexcept { if (!base) return; #ifdef _WIN32 + std::lock_guard lock(vector_regions_mutex); + std::erase_if(vector_regions, [base](const auto& r) {return r.first == reinterpret_cast(base); }); ASSERT(VirtualFree(base, 0, MEM_RELEASE)); #else ASSERT(munmap(base, size) == 0); diff --git a/src/common/sparse_large_vector.h b/src/common/sparse_large_vector.h index 86f0be7866..8496c28cef 100644 --- a/src/common/sparse_large_vector.h +++ b/src/common/sparse_large_vector.h @@ -28,9 +28,9 @@ constexpr u64 HostPageBits = 12; constexpr u64 HostPageMask = ~(HostPageSize - 1); bool CommitVectorPage(uintptr_t addr, bool write) noexcept; #else -const u64 HostPageSize = sysconf(_SC_PAGESIZE); -const u64 HostPageBits = std::countr_zero(HostPageSize); -const u64 HostPageMask = ~(HostPageSize - 1); +inline const u64 HostPageSize = sysconf(_SC_PAGESIZE); +inline const u64 HostPageBits = std::countr_zero(HostPageSize); +inline const u64 HostPageMask = ~(HostPageSize - 1); #endif void* AllocateMemoryPages(std::size_t size) noexcept; @@ -81,8 +81,8 @@ public: UNREACHABLE_MSG("Out of bounds RW access on SparseLargeVector @ {}", index); } - if (!IsCommittedPage(index)) { - CommitPage(index); + if (!IsCommittedPage(index) && !CommitPage(index)) { + UNREACHABLE_MSG("Cannot access SparseLargeVector index {} with RW permission", index); } return base_ptr[index]; } @@ -103,9 +103,8 @@ public: LOG_CRITICAL(Common_Memory, "Out of bounds write on SparseLargeVector @ {}", index); return; } - if (!IsCommittedPage(index)) - CommitPage(index); - base_ptr[index] = value; + if (IsCommittedPage(index) || CommitPage(index)) + base_ptr[index] = value; } void ZeroRegion(std::size_t start, std::size_t end_) noexcept { @@ -177,16 +176,22 @@ private: return (val >> (page & 63)) & 1; } - constexpr void CommitPage(std::size_t index) noexcept { + constexpr bool CommitPage(std::size_t index) noexcept { auto page_index = (index * sizeof(T)) >> HostPageBits; auto page = reinterpret_cast(base_ptr + index) & HostPageMask; #if defined(_WIN32) - CommitVectorPage(page, true); + if (!CommitVectorPage(page, true)) { + return false; + } #else - mprotect(reinterpret_cast(page), HostPageSize, PROT_READ | PROT_WRITE); + if (mprotect(reinterpret_cast(page), HostPageSize, PROT_READ | PROT_WRITE) != 0) { + LOG_ERROR(Common_Memory, "Failed to commit large buffer region at index {}, error {}", index, strerror(errno)); + return false; + } #endif committed_pages[page_index >> 6].fetch_or(1ULL << (page_index & 63), std::memory_order_release); + return true; } constexpr void DecommitPage(std::size_t index) noexcept {