From 0f4ab63101bb0abadea64e1e8aa7eb1098915a8c Mon Sep 17 00:00:00 2001
From: clamp <15528374+DJ-clamp@users.noreply.github.com>
Date: Tue, 18 Aug 2026 23:16:12 +0800
Subject: [PATCH] patchmanager: fix signature patches reporting neither on or
 off (#873)

## Link to GitHub Issue or related Pull Request, if one exists
None

## Description of change
Patch Manager showed valid signature patches as broken: **"Bad patch;
patch is neither on or off"** instead of Enabled/Disabled, so they could
not be toggled.

`SignaturePatch::to_memory` had two bugs. In the configurator it cached
a file offset as `data_offset_ptr`, so status checks `memcmp`'d a fake
address. It also passed JSON `offset` into `find_pattern` while still
indexing the signature/replacement from 0, which mis-aligned every patch
with `offset != 0`. This change locates the signature start, applies
`offset` afterward, compares only the replacement window, and leaves the
pointer null so `is_patch_active` re-resolves from `data_offset`.

Not proposed as built-in patches. The JSON below is the reproduction
case: each entry uses `offset > 0` and a replacement shorter than the
signature.

## Testing
Reproduced in Patch Manager against `bm2dx.dll` using the signature JSON
below. Before the fix, every patch reported "neither on or off". After
the fix, each patch locates, shows Disabled/Enabled, and toggling writes
only the replacement bytes at `signature_match + offset`.

## Demo
<details>
<summary>Signature JSON used to reproduce (offset + short
replacement)</summary>

```json
[
  {
    "info": "streaming / getcm patches (type=signature)",
    "gameCode": "LDJ",
    "notes": "Each site uses a unique signature (usage=0 only)."
  },
  {
    "type": "group",
    "id": "streaming-getcm",
    "name": "Streaming getcm",
    "description": "Enable all children so streaming.common merges without Banner FS and getcm can fire without visiting Test Mode.",
    "gameCode": "LDJ"
  },
  {
    "name": "Streaming: merge common without Banner",
    "description": "NOP jz in streaming.common callback so CM work table is filled even when Banner FS is still null.",
    "caution": "Required. Without this, early common responses are discarded and getcm stays empty.",
    "gameCode": "LDJ",
    "type": "signature",
    "group": "streaming-getcm",
    "dllName": "bm2dx.dll",
    "signature": "E8????????4885C00F84????????488D0D????????488D15????????41B848080000",
    "replacement": "909090909090",
    "offset": 8,
    "usage": 0
  },
  {
    "name": "Streaming: scheduler without Banner (common)",
    "description": "NOP jz after Banner getter on the common branch of the periodic scheduler.",
    "caution": "Enable with the getcm scheduler sibling. Unique via imul of common-interval dword.",
    "gameCode": "LDJ",
    "type": "signature",
    "group": "streaming-getcm",
    "dllName": "bm2dx.dll",
    "signature": "E8????????4885C0742469057E791C0AE8030000",
    "replacement": "9090",
    "offset": 8,
    "usage": 0
  },
  {
    "name": "Streaming: scheduler without Banner (getcm)",
    "description": "NOP jz after Banner getter on the getcm branch of the periodic scheduler.",
    "caution": "Enable with the common scheduler sibling. Unique via imul of getcm-interval dword. Without this, getcm never schedules while Banner FS is null.",
    "gameCode": "LDJ",
    "type": "signature",
    "group": "streaming-getcm",
    "dllName": "bm2dx.dll",
    "signature": "E8????????4885C07424690574B91D0AE8030000",
    "replacement": "9090",
    "offset": 8,
    "usage": 0
  },
  {
    "name": "Streaming: fall into getcm after common",
    "description": "NOP jmp-after-common so the same scheduler tick can evaluate getcm instead of returning early.",
    "caution": "Pair with Banner scheduler skips (or a live Banner FS).",
    "gameCode": "LDJ",
    "type": "signature",
    "group": "streaming-getcm",
    "dllName": "bm2dx.dll",
    "signature": "891D????????E9????????E8????????4885C074246905",
    "replacement": "9090909090",
    "offset": 6,
    "usage": 0
  },
  {
    "name": "Streaming: getcm interval 1s #1",
    "description": "Default getcm poll interval 1800s to 1s (first init store).",
    "caution": "Enable #1 and #2 together. Trailing BF3C000000 distinguishes this init site.",
    "gameCode": "LDJ",
    "type": "signature",
    "group": "streaming-getcm",
    "dllName": "bm2dx.dll",
    "signature": "C705????????18150000C705????????08070000C705????????201C0000BF3C000000",
    "replacement": "01000000",
    "offset": 16,
    "usage": 0
  },
  {
    "name": "Streaming: getcm interval 1s #2",
    "description": "Default getcm poll interval 1800s to 1s (second init store).",
    "caution": "Enable #1 and #2 together. Trailing 448925 distinguishes this init site.",
    "gameCode": "LDJ",
    "type": "signature",
    "group": "streaming-getcm",
    "dllName": "bm2dx.dll",
    "signature": "C705????????18150000C705????????08070000C705????????201C0000448925",
    "replacement": "01000000",
    "offset": 16,
    "usage": 0
  }
]
```
</details>

Co-authored-by: Cursor <cursoragent@cursor.com>
---
 src/spice2x/patcher/runtime.cpp | 133 ++++++++++++++++++++------------
 1 file changed, 84 insertions(+), 49 deletions(-)

diff --git a/src/spice2x/patcher/runtime.cpp b/src/spice2x/patcher/runtime.cpp
index 769b999..34a3a28 100644
--- a/src/spice2x/patcher/runtime.cpp
+++ b/src/spice2x/patcher/runtime.cpp
@@ -669,6 +669,11 @@ namespace patcher {
         signature.erase(std::remove(signature.begin(), signature.end(), ' '), signature.end());
         replacement.erase(std::remove(replacement.begin(), replacement.end(), ' '), replacement.end());
 
+        if (signature.empty() || (signature.length() % 2) != 0
+            || replacement.empty() || (replacement.length() % 2) != 0) {
+            return {.fatal_error = true};
+        }
+
         // build pattern
         std::string pattern_str(signature);
         strreplace(pattern_str, "??", "00");
@@ -717,13 +722,26 @@ namespace patcher {
         }
         std::string replace_mask_str = replace_mask.str();
 
-        // find offset
+        // replacement applies at signature_match + offset; must stay inside the signature
+        const size_t sig_len = signature_mask_str.length();
+        const size_t repl_len = replace_mask_str.length();
+        if (offset > sig_len || repl_len == 0 || offset + repl_len > sig_len) {
+            log_warning("patchmanager",
+                        "signature patch '{}': offset {} + replacement {} exceeds signature {}",
+                        patch->name, offset, repl_len, sig_len);
+            return {.fatal_error = true};
+        }
+
+        // Locate signature *start* (find_pattern offset arg = 0). JSON "offset" is applied
+        // below. Old code passed JSON offset into find_pattern but still indexed
+        // signature/replacement from 0 → mis-aligned when offset != 0.
         uint64_t data_offset = 0;
-        uint8_t *data_offset_ptr = nullptr;
-        uintptr_t data_offset_ptr_base = 0;
+        uint8_t *read_ptr = nullptr;
+        HMODULE module = nullptr;
+        bool module_free = false;
+
         if (cfg::CONFIGURATOR_STANDALONE) {
 
-            // load file into dll map if missing
             auto it = DLL_MAP.find(dll_name);
             if (it == DLL_MAP.end()) {
                 DLL_MAP[dll_name] =
@@ -732,16 +750,21 @@ namespace patcher {
                 it = DLL_MAP.find(dll_name);
             }
 
-            // find pattern
-            data_offset = find_pattern(*it->second, 0, pattern_bin.get(), signature_mask_str.c_str(), offset, usage);
-            data_offset_ptr = reinterpret_cast<uint8_t *>(data_offset);
-            data_offset_ptr_base = (uintptr_t) it->second->data();
+            // base=0 → file offset of signature start; 0 also means "not found"
+            const intptr_t match = find_pattern(
+                    *it->second, 0, pattern_bin.get(), signature_mask_str.c_str(), 0, usage);
+            if (match == 0) {
+                return {.fatal_error = true};
+            }
+            data_offset = static_cast<uint64_t>(match) + offset;
+            if (data_offset + repl_len > it->second->size()) {
+                return {.fatal_error = true};
+            }
+            read_ptr = it->second->data() + data_offset;
 
         } else {
 
-            // get module
-            auto module = libutils::try_module(dll_path);
-            bool module_free = false;
+            module = libutils::try_module(dll_path);
             if (!module) {
                 module = libutils::try_library(dll_path);
                 if (module) {
@@ -751,59 +774,71 @@ namespace patcher {
                 }
             }
 
-            // find pattern
-            data_offset_ptr = reinterpret_cast<uint8_t *>(
-                    find_pattern(module, pattern_bin.get(), signature_mask_str.c_str(), offset, usage));
+            const intptr_t match_va = find_pattern(
+                    module, pattern_bin.get(), signature_mask_str.c_str(), 0, usage);
+            auto *match_ptr = reinterpret_cast<uint8_t *>(match_va);
+            if (match_ptr == nullptr) {
+                if (module_free) {
+                    FreeLibrary(module);
+                }
+                return {.fatal_error = true};
+            }
 
-            // convert back to offset
-            data_offset = libutils::rva2offset(dll_path, (intptr_t) (data_offset_ptr - (uint8_t*) module));
+            const intptr_t file_off = libutils::rva2offset(
+                    dll_path, (intptr_t) (match_ptr - (uint8_t *) module));
+            if (file_off < 0) {
+                if (module_free) {
+                    FreeLibrary(module);
+                }
+                return {.fatal_error = true};
+            }
+            data_offset = static_cast<uint64_t>(file_off) + offset;
+            read_ptr = match_ptr + offset;
+        }
 
-            // clean
-            if (module_free) {
-                FreeLibrary(module);
+        // Build disabled/enabled for the replacement window only.
+        std::shared_ptr<uint8_t[]> data_disabled(new uint8_t[repl_len]);
+        std::shared_ptr<uint8_t[]> data_enabled(new uint8_t[repl_len]);
+        {
+            memutils::VProtectGuard data_guard(read_ptr, repl_len);
+            for (size_t i = 0; i < repl_len; ++i) {
+                const size_t si = static_cast<size_t>(offset) + i;
+                if (signature_mask_str[si] != 'X') {
+                    data_disabled.get()[i] = read_ptr[i];
+                } else {
+                    data_disabled.get()[i] = pattern_bin.get()[si];
+                }
+                if (replace_mask_str[i] != 'X') {
+                    data_enabled.get()[i] = read_ptr[i];
+                } else {
+                    data_enabled.get()[i] = replace_data_bin.get()[i];
+                }
             }
         }
 
-        // check pointers
-        if (data_offset_ptr == nullptr) {
-            return {.fatal_error = true};
+        // Drop temporary LoadLibrary mapping only after reads above.
+        if (module_free) {
+            FreeLibrary(module);
         }
 
-        // get disabled/enabled data
-        size_t data_len = std::max(signature_mask_str.length(), replace_mask_str.length());
-        std::shared_ptr<uint8_t[]> data_disabled(new uint8_t[data_len]);
-        std::shared_ptr<uint8_t[]> data_enabled(new uint8_t[data_len]);
-        memutils::VProtectGuard data_guard(data_offset_ptr + data_offset_ptr_base, data_len);
-        for (size_t i = 0; i < data_len; ++i) {
-            if (i >= signature_mask_str.length() || signature_mask_str[i] != 'X') {
-                data_disabled.get()[i] = (data_offset_ptr + data_offset_ptr_base)[i];
-            } else {
-                data_disabled.get()[i] = pattern_bin.get()[i];
-            }
-        }
-        for (size_t i = 0; i < data_len; ++i) {
-            if (i >= replace_mask_str.length() || replace_mask_str[i] != 'X') {
-                data_enabled.get()[i] = (data_offset_ptr + data_offset_ptr_base)[i];
-            } else {
-                data_enabled.get()[i] = replace_data_bin.get()[i];
-            }
-        }
-
-        // log edit
         log_misc("patchmanager", "found {}: {:#08X}: {} -> {}",
                  patch->name, data_offset,
-                 bin2hex(data_disabled.get(), data_len),
-                 bin2hex(data_enabled.get(), data_len));
+                 bin2hex(data_disabled.get(), repl_len),
+                 bin2hex(data_enabled.get(), repl_len));
 
-        // build patch
+        // BUGFIX: never cache a pointer here.
+        // - standalone used to store (uint8_t*)file_offset, so is_patch_active skipped
+        //   re-resolve and memcmp'd a fake address → "neither on or off".
+        // - in-game used to keep a pointer that FreeLibrary may invalidate.
+        // is_patch_active / apply_patch re-resolve from data_offset when ptr is nullptr.
         return MemoryPatch {
                 .dll_name = dll_name,
                 .data_disabled = std::move(data_disabled),
-                .data_disabled_len = data_len,
+                .data_disabled_len = repl_len,
                 .data_enabled = std::move(data_enabled),
-                .data_enabled_len = data_len,
+                .data_enabled_len = repl_len,
                 .data_offset = data_offset,
-                .data_offset_ptr = data_offset_ptr,
+                .data_offset_ptr = nullptr,
         };
     }