Skip to content

fix(scanner): tighten prologue fallback to require unique match - #80

Merged
tkhquang merged 1 commit into
mainfrom
fix/scanner-prologue-fallback-uniqueness
May 22, 2026
Merged

fix(scanner): tighten prologue fallback to require unique match#80
tkhquang merged 1 commit into
mainfrom
fix/scanner-prologue-fallback-uniqueness

Conversation

@tkhquang

@tkhquang tkhquang commented May 22, 2026

Copy link
Copy Markdown
Owner

Summary

  • Raise kPrologueFallbackMinTailLiterals from 5 to 10 so the rebuilt E9 ?? ?? ?? ?? pattern carries enough literal context to disambiguate from incidental near-JMPs in a multi-MB .text section.
  • Raise kPrologueFallbackMaxHits from 4 to 1 so a sibling-mod inline-hook recovery only proceeds when the rewritten pattern resolves to exactly one site. Two-or-more is now NoMatch instead of silently picking the first.
  • Update docs/misc/aob-signatures.md with the new thresholds, the rationale for each, and a safety note distinguishing sibling inline-hook recovery from game-patch survival (with resolve_cascade as the strict alternative).

Tests

  • PrologueFallbackHitFindsHookedPrologue: tail extended to 10 bytes to satisfy the new floor.
  • PrologueFallbackRejectsAmbiguousTail: seeds 2 copies (was 5) with an 11-byte unique tail to trip the tightened uniqueness ceiling.
  • New PrologueFallbackRejectsNineByteTail: boundary regression that a 9-byte literal tail surfaces PrologueFallbackNotApplicable.
  • New PrologueFallbackRejectsExactlyTwoMatches: boundary regression that exactly two matches now resolve to NoMatch.

Risk

Cold-path tightening. Behavior changes only when the cascade missed and the fallback would previously have engaged on a short or ambiguous tail; in those cases the API now returns PrologueFallbackNotApplicable or NoMatch instead of an unstable address.

Summary by CodeRabbit

  • Bug Fixes

    • Tightened validation thresholds for fallback signature matching to reduce false positives and improve accuracy.
  • Tests

    • Updated test coverage to validate stricter acceptance criteria and boundary conditions for signature fallback resolution.

Review Change Stack

Raise kPrologueFallbackMinTailLiterals from 5 to 10 and
kPrologueFallbackMaxHits from 4 to 1 so the rewritten near-JMP pattern
must resolve to exactly one site backed by ten literal tail bytes.
Adds boundary regressions for the nine-byte tail and two-match cases.
@tkhquang tkhquang self-assigned this May 22, 2026
@coderabbitai

coderabbitai Bot commented May 22, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

This PR tightens the prologue-fallback pattern matching by raising the minimum literal tail requirement from 5 to 10 bytes and lowering the maximum hit count from 4 to 1, with matching documentation clarifications and expanded test coverage including new regression tests.

Changes

Prologue-Fallback Threshold and Guardrail Tightening

Layer / File(s) Summary
Documentation: fallback contract and guardrails
docs/misc/aob-signatures.md
Updated section 6.4 to specify prologue-fallback acceptance criteria: exactly one resolved match across executable regions, E9 displacement inside loaded module, and minimum 10 literal tail bytes (raised from 5). Added safety guidance on intended use cases and risks of structural code changes.
Implementation: threshold constant adjustments
src/scanner.cpp
Increased kPrologueFallbackMinTailLiterals from 5 to 10 and decreased kPrologueFallbackMaxHits from 4 to 1, directly enforcing the tightened applicability and ambiguity rejection thresholds.
Test coverage: fallback behavior and regression tests
tests/test_scanner.cpp
Expanded unique tail literals in the primary fallback test, adjusted ambiguity conditions to trigger at two matches, and added two new regression tests: one verifying 9-byte tails are rejected as PrologueFallbackNotApplicable, another confirming exactly two rebuilt matches are rejected as NoMatch.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • tkhquang/DetourModKit#69: Prior adjustment to prologue-fallback uniqueness logic in src/scanner.cpp with corresponding test updates, also rejecting ambiguous fallback candidates as NoMatch.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: tightening the prologue fallback mechanism to require a unique match by increasing minimum tail literals from 5 to 10 and maximum hits from 4 to 1.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tests/test_scanner.cpp (1)

2035-2036: ⚡ Quick win

Move byte-template comments above the lines and state rationale

These inline trailing comments are “what” comments; convert them to line-above “why” comments to match project comment conventions.

Suggested edit
-    constexpr std::uint8_t kAmbiguousTemplate[] = {
-        0xE9, 0x00, 0x00, 0x00, 0x00,                                     // JMP rel32
-        0xA5, 0xB6, 0xC7, 0xD8, 0xE9, 0xFA, 0x0B, 0x1C, 0x2D, 0x3E, 0x4F, // unique 11-byte tail
-    };
+    constexpr std::uint8_t kAmbiguousTemplate[] = {
+        // Starts with a hook-shaped E9 rel32 prologue so fallback rebuilding can match this layout.
+        0xE9, 0x00, 0x00, 0x00, 0x00,
+        // Tail is intentionally uncommon to avoid incidental matches in executable pages.
+        0xA5, 0xB6, 0xC7, 0xD8, 0xE9, 0xFA, 0x0B, 0x1C, 0x2D, 0x3E, 0x4F,
+    };

-    constexpr std::uint8_t kTemplate[] = {
-        0xE9, 0x00, 0x00, 0x00, 0x00,                                     // JMP rel32
-        0x71, 0x82, 0x93, 0xA4, 0xB5, 0xC6, 0xD7, 0xE8, 0xF9, 0x0A, 0x1B, // unique 11-byte tail
-    };
+    constexpr std::uint8_t kTemplate[] = {
+        // Uses E9 rel32 prologue shape to exercise fallback rewrite matching.
+        0xE9, 0x00, 0x00, 0x00, 0x00,
+        // Tail uniqueness keeps the test focused on the two seeded matches.
+        0x71, 0x82, 0x93, 0xA4, 0xB5, 0xC6, 0xD7, 0xE8, 0xF9, 0x0A, 0x1B,
+    };

As per coding guidelines: Inline comments (//) used inside function bodies must explain why, not what. Place on the line above the code they describe.

Also applies to: 2099-2100

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_scanner.cpp` around lines 2035 - 2036, Change the inline trailing
“what” comments on the byte array lines (the lines containing the JMP rel32
sequence "0xE9, 0x00, 0x00, 0x00, 0x00" and the unique tail "0xA5, 0xB6, 0xC7,
0xD8, 0xE9, 0xFA, 0x0B, 0x1C, 0x2D, 0x3E, 0x4F") so that the descriptive text
moves to a separate line above each statement and explains why that byte pattern
is present (test intent/ rationale) rather than what the bytes are; update the
two occurrences mentioned (current block and the one around the 2099–2100
region) to follow the project convention of line-above “why” comments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/test_scanner.cpp`:
- Around line 2035-2036: Change the inline trailing “what” comments on the byte
array lines (the lines containing the JMP rel32 sequence "0xE9, 0x00, 0x00,
0x00, 0x00" and the unique tail "0xA5, 0xB6, 0xC7, 0xD8, 0xE9, 0xFA, 0x0B, 0x1C,
0x2D, 0x3E, 0x4F") so that the descriptive text moves to a separate line above
each statement and explains why that byte pattern is present (test intent/
rationale) rather than what the bytes are; update the two occurrences mentioned
(current block and the one around the 2099–2100 region) to follow the project
convention of line-above “why” comments.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9d9dfa0d-6685-445c-96fa-8be8b76f00a1

📥 Commits

Reviewing files that changed from the base of the PR and between eeb330d and e3ca36b.

📒 Files selected for processing (3)
  • docs/misc/aob-signatures.md
  • src/scanner.cpp
  • tests/test_scanner.cpp

@tkhquang
tkhquang merged commit 37e5c2f into main May 22, 2026
2 checks passed
@tkhquang
tkhquang deleted the fix/scanner-prologue-fallback-uniqueness branch May 22, 2026 14:51
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.

1 participant