Fix AccessViolationException when scanning Unity 6 modules with protected memory pages - #267
Conversation
FindSignatureInModule did a raw linear byte walk over the entire loaded module. On Unity 6000.x the GameAssembly image maps regions as PAGE_NOACCESS / guard pages, so dereferencing them throws a process-fatal AccessViolationException before the caller's signature-exhaustion fallback can run (observed registering injected types on Priconne, Unity 6000.0.58f2). Temporarily flip every committed region of the module to PAGE_EXECUTE_READWRITE for the duration of the scan via VirtualQuery/VirtualProtect, then restore the original protections in a finally block. Non-committed (MEM_FREE/MEM_RESERVE) regions are skipped. Mirrors the approach used by downstream forks that already support Unity 6.
Per @ds5678's review on #267: VirtualProtect/VirtualQuery are kernel32 (Windows-only) and PAGE_NOACCESS is a Windows page state. Run the make-readable / restore dance only when OperatingSystem.IsWindows(); on other platforms fall back to the direct FindSignatureInBlock scan (its prior behaviour), so the kernel32 P/Invokes are never called off-Windows. Also satisfies the CA1416 platform-compatibility analyzer.
ds5678
left a comment
ds5678
left a comment
There was a problem hiding this comment.
Also, another user in Discord gave feedback, which I've summarized below:
Modifying page protections is bad and should be avoided. You're already using VirtualQuery, so you should be able to skip the portions that aren't readable.
Per @ds5678's review on #267 (relaying Discord feedback): modifying page protections (VirtualProtect to PAGE_EXECUTE_READWRITE) is dangerous and unnecessary. VirtualQuery already enumerates the module's regions, so skip the non-readable / guard / uncommitted ones during the signature scan rather than forcing them readable. Removes VirtualProtect and the RWX flip entirely; keeps the Windows-only guard (VirtualQuery is kernel32) with the plain whole-module scan as the off-Windows fallback.
| private const uint PAGE_READABLE = 0x02 | 0x04 | 0x08 | 0x20 | 0x40 | 0x80; | ||
|
|
||
| [DllImport("kernel32.dll", SetLastError = true)] | ||
| internal static extern int VirtualQuery(IntPtr lpAddress, out MEMORY_BASIC_INFORMATION lpBuffer, uint dwLength); |
There was a problem hiding this comment.
Add a package reference to TerraFX.Interop.Windows and remove this.
The per-region scan could read up to mask.Length-1 bytes past a readable region into the adjacent guard / PAGE_NOACCESS page -> a fatal AccessViolationException. Cap FindSignatureInBlock's loop at blockSize - mask.Length so every read stays inside the region. Per @ds5678's review: GetModuleRegions now returns the list instead of an out parameter, and its comment is XML documentation.
|
My comment about TerraFX still needs resolved. |
|
Thanks for the review. Bit of context so it's clear where this comes from: I'm not a programmer - I maintain this for a real game (Princess Connect Re:Dive, Unity 6 / IL2CPP) and test every change on the actual game, so this AccessViolation is one I genuinely hit, not something inferred from reading code. I do use AI to help write the changes, so please call out anything that looks off. On TerraFX: Il2CppInterop.Runtime targets net6.0, and TerraFX.Interop.Windows dropped net6 after 10.0.22621.2 (newer releases are net8+). So I can pin 10.0.22621.2, or bump the target framework if you'd rather use a current TerraFX - your call. If you'd prefer not to add the dependency on net6, the existing VirtualQuery P/Invoke is small and does the job. Let me know which and I'll update the PR. |
|
The version of TerraFX can be bumped later when we're no longer on .NET 6, so using the last .NET 6 version is fine. |
…voke Replace the hand-written kernel32 VirtualQuery DllImport and MEMORY_BASIC_INFORMATION struct with the TerraFX.Interop.Windows bindings, pinned to 10.0.22621.2 (the last release targeting .NET 6). The readable-region scan is unchanged.
|
Done -- swapped the manual VirtualQuery P/Invoke and MEMORY_BASIC_INFORMATION for TerraFX.Interop.Windows 10.0.22621.2 (the last .NET 6 release, as you suggested). The readable-region scan is unchanged. Thanks for the reviews. |
The TerraFX MEMORY_BASIC_INFORMATION fields are annotated for Windows 6.1+. Mark GetModuleRegions with the matching SupportedOSPlatform attribute and guard the call with OperatingSystem.IsWindowsVersionAtLeast(6, 1) so the platform-compatibility analyzer is satisfied.
Drop the manual MEM_COMMIT/PAGE_GUARD/PAGE_READABLE consts and the `using static Windows` in favour of the constants TerraFX already exposes (MEM.*, PAGE.*), qualify VirtualQuery, and default-init the MEMORY_BASIC_INFORMATION. The readable mask moves inside the windows6.1 guard so the const fold stays on a supported-platform path.
ce69b03 to
ab6318c
Compare
|
Thanks for the review - addressed all three in ab6318c:
One note on placement: the readable-protection mask had to move inside the |
I've asked someone for a second opinion, but otherwise I approve of this pull request. Thank you for working on it.
I actually prefer that location, for encapsulation reasons. |
2 participants
Problem
On Unity 6000.x (and some Unity 2021.3+ builds), GameAssembly.dll maps portions of its loaded image as PAGE_NOACCESS or guard pages. The raw linear byte walk in FindSignatureInBlock dereferences those protected addresses and throws System.AccessViolationException - a corrupted-state exception that is process-fatal and cannot be caught by a normal ry/catch. BepInEx terminates silently before any plugin loads.
Fixes #178. Also addresses the root cause behind #215 and #239.
Root Cause
csharp // MemoryUtils.FindSignatureInBlock - before this fix for (long address = 0; address < blockSize; address++) if (*(byte*)(address + block + offset) != ...) // ? AV here on guard pageThe scan walks the entire module range (ModuleMemorySize ? 181 MB for Priconne) without checking whether each page is actually committed and readable.
Fix
Before scanning, enumerate the module's virtual memory regions via VirtualQuery, temporarily set every committed region to PAGE_EXECUTE_READWRITE, run the scan, then restore each region's original protection in a inally block.
Non-committed regions (MEM_FREE / MEM_RESERVE) are skipped - VirtualProtect rejects them and they hold no scannable bytes.
csharp public static nint FindSignatureInModule(ProcessModule module, SignatureDefinition sigDef) { GetModuleRegions(module, out var protectedRegions); SetModuleRegions(protectedRegions, PAGE_EXECUTE_READWRITE); nint ptr; try { ptr = FindSignatureInBlock(...); } finally { SetModuleRegions(protectedRegions); // always restore } ... }Relation to #122
PR #122 (krulci) tackles a similar problem but is substantially larger: it also adds a runtime metadata dump path and was put on hold pending architectural changes. This PR is intentionally narrower - scan safety only, no dump, no game-specific code - so it can land independently.
Tested
Verified on: