Skip to content

fix(core): make function-call patches survive a re-resolve - #24

Open
Rushaway wants to merge 1 commit into
masterfrom
fix/function-call-patch-robustness
Open

fix(core): make function-call patches survive a re-resolve#24
Rushaway wants to merge 1 commit into
masterfrom
fix/function-call-patch-robustness

Conversation

@Rushaway

Copy link
Copy Markdown
Member

Fixes #23.

The regression

The anchor pattern introduced in #14 ends on the CALL rel32 the patch NOPs out:

// 10: fix server lagging resulting from too many ConMsgs due to packet spam
(unsigned char *)"\x8B\x45\x08\x05\xA0\x2A\x2A\x2A\xFF\x75\xC8\x53\x50\x68\x2A\x2A\x2A\x2A\xE8\x2A\x2A\x2A\x2A",
"xxxxx???xxxxxx????x????",

That trailing E8 rel32 is what identifies the callee — and it is also one of the call sites the occurrence loop overwrites with 90 90 90 90 90. The patch erases its own signature. Once applied, the pattern can never match again in that process, so every later resolve yields functionAddress = 0

[CSSFIXES] Could not find patch signature for symbol: _ZN8CNetChan19ProcessPacketHeaderEP11netpacket_s
[CSSFIXES] Could not find patch signature for symbol: _Z11NET_GetLongiP11netpacket_s

bSuccess = falseSDK_OnLoad() calls SDK_OnUnload() and returns falsethe whole extension fails to load, losing all ~19 patches, the detours and the hooks. With #10/#11 gone, packet spam makes the engine flood ConMsg/Msg again, which is the server lag we were chasing.

This is #13's failure, now deterministic rather than intermittent.

The fix

When the pattern is not found, search again with the trailing five bytes replaced by NOPs. A match there means the patch is already applied → log and skip, instead of failing the load of everything else.

uintptr_t StartAddr = FindPattern(pPatch->pAddress, pPatch->pPatchSignature, pPatch->pPatchPattern, pPatch->range);
if (StartAddr)
    return StartAddr + (PatternLen - 5);

// ... retry with 90 90 90 90 90 in place of the CALL
if (FindPattern(pPatch->pAddress, Signature, Pattern, pPatch->range))
    *pbAlreadyPatched = true;

FindFunctionAddressByPattern(StartAddr, MaxSize) becomes FindFunctionAddressByCall(CallAddr): it takes the call address directly instead of re-deriving it from the mask length, which also removes the unchecked assumption that strlen(pPatchPattern) == sizeof(pPatchSignature) - 1.

Also fixed in the same code path

pPatch->range - ofs wraps range is int, ofs is uintptr_t → unsigned subtraction. FindFunctionCall() can return a match at BaseAddr + MaxSize - 1, making ofs = range + 4; the next scan then covers 0xfffffffc bytes and NOPs call sites in unrelated engine code. Guarded with if (ofs >= (uintptr_t)pPatch->range) break;
continuebreak ofs is unchanged, so continue re-ran the identical search: a failing #10/#11 logged 100 identical errors and did 100 full-range scans
silent resolve failure the resolve step now says whether the pattern was missing or did not end on a CALL rel32
strlen() on binary data PatchLen for a function-call patch is now the constant 5. strlen(pPatch) only returned 5 by luck; a 0x00 patch byte would truncate it for both the write and the restore in SDK_OnUnload()
dangling pPatch->pRestore cleared after the list is freed
32-bit-only rel32 int32_t Offset instead of uint32_t offset read through an int32_t *, so the arithmetic also holds for #20

No patch signature, range or occurrence count is changed — behaviour on a clean engine image is identical.

Testing

No CS:S server here, so I verified the logic offline: FindPattern, FindFunctionCall, FindFunctionAddressByCall, FindPatchAnchorCall and the old FindFunctionAddressByPattern were extracted verbatim from src/extension.cpp into a standalone harness, built -m32 (i386, as srcds runs), and driven with patch #10's real signature and mask against a synthetic function containing several CALL rel32 sites:

sizeof(uintptr_t) = 4

1. Clean engine image
  [ ok ] anchor CALL located at +0x42
  [ ok ] not reported as already patched
  [ ok ] callee resolved to +0x800 (expected +0x800)

2. Applying the patch
  [ ok ] patched 4 call sites (3 plain + the anchor)
  [ ok ] the anchor's own CALL is one of the NOPed sites
  [ ok ] the unrelated callee was left alone

3. Re-running the patcher against the already patched image
   (this is what a second SDK_OnLoad in the same srcds process sees)
  [ ok ] before: pattern no longer matches -> functionAddress = 0 -> "Could not find patch signature" -> whole extension fails to load
  [ ok ] after: no anchor to re-resolve
  [ ok ] after: recognised as already applied -> patch is skipped, load succeeds

4. range/ofs underflow guard
  [ ok ] before: range - ofs wraps to 0xfffffffc bytes of scan
  [ ok ] after: guard stops the loop instead

Still needs a run on a real server to confirm the patches apply as before on a clean load and that the lag is gone. I have not been able to test the already applied path against a live engine.

Not addressed here

Discussed in #23 — a single stale signature still makes SDK_OnLoad() return false and takes the entire extension down. An optional / required flag per patch would turn that into a warning for non-critical patches, but it is a behaviour change that deserves its own PR.

🤖 Generated with Claude Code

The anchor pattern added in #14 ends on the CALL rel32 that the patch NOPs
out, and that call site is one of the sites the occurrence loop overwrites.
The patch therefore erases its own signature: every later resolve in the same
srcds process returns functionAddress = 0, logs "Could not find patch
signature" and, through bSuccess, makes SDK_OnLoad() call SDK_OnUnload() and
return false -- taking every other fix in the extension down with it. That is
the failure from #13 that #14 set out to fix, now deterministic.

When the pattern is not found, search again with the trailing five bytes
replaced by NOPs. A match there means the patch is already applied, so log it
and skip instead of failing the load.

Also in the same code path:

- guard ofs >= range before "pPatch->range - ofs". range is int and ofs is
  uintptr_t, so the subtraction is unsigned and wraps to ~4 GB as soon as a
  match lands in the last bytes of the range, after which the scan runs past
  the function and NOPs call sites in unrelated engine code
- break instead of continue when a patch signature is not found. ofs is
  unchanged, so continue re-ran the identical search and logged the identical
  error occurrences (100) times
- report which step failed instead of leaving functionAddress at 0 silently
- use a constant 5 for a CALL rel32 instead of strlen() on pPatch, which is
  binary data, not a string. A 0x00 patch byte would truncate PatchLen for
  both the write and the restore
- clear pPatch->pRestore after freeing the list so the head does not dangle
- read the rel32 as int32_t so the arithmetic also holds on 64-bit (#20)

Closes #23

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: function-call patches destroy their own signature, failing the whole extension load (regression from #14)

1 participant