Skip to content

fix(core): do not latch speedhack installed after a failed attach - #29

Merged
ResurrectedTrader merged 1 commit into
mainfrom
fix/hook-real-contract
Sep 17, 2026
Merged

ResurrectedTrader merged 1 commit into
mainfrom
fix/hook-real-contract

Conversation

@ResurrectedTrader

Copy link
Copy Markdown
Owner

Two small correctness issues in core, both found while auditing the hooking layer. Neither fires today; both are traps for the next caller.

speedhack::Install latched its flag on a failed attach

bool expected = false;
if (!isInstalled.compare_exchange_strong(expected, true)) {
    return;
}
if (const int32_t err = detour::AttachAll(HOOKS); err != 0) {
    Log().error(...);          // flag stays true
}

The flag is claimed before the attach. AttachAll is all-or-nothing — a failed commit has Detours roll every rewritten pointer back, so nothing is installed — but the flag then said otherwise. Two consequences:

  • Remove() passes its own CAS and calls DetachAll on eighteen slots that were never attached.
  • Any later Install() returns immediately, so the failure is permanent rather than retryable.

Now the flag is handed back on failure, so the state matches what is actually installed.

Nothing retries Install() today, so this is latent rather than a live bug.

Hook<Fn>'s Real() contract was wider than the code

The class comment claimed Real() reaches the original "whether or not the slot is attached". That is true for a slot built from an address — it has one from construction — but not for the other two constructors:

  • Hook(Target::Export(...), replacement) has no address until an attach resolves the export.
  • explicit Hook(Fn replacement) has no target at all until SetTarget.

In both cases Real() is null. The same applies after pointing a detached slot at an export, which drops the address it was holding — correctly, since the target moved and where is not known until the attach.

I narrowed the comment rather than widening the code. A slot that has not been told where its target lives has nothing truthful to return, and inventing a fallback would hide the mistake at the call site instead of surfacing it. Nothing calls Real() on an unresolved slot today.

Verification

clang-format clean, clang-tidy clean (118 files, full no-cache run), Win32 build 0 warnings / 0 errors.

🤖 Generated with Claude Code

…d say when Real() works

speedhack::Install sets its installed flag before attaching, and left it set
when the attach failed. The batch is all-or-nothing, so a failure leaves
nothing installed - but the flag then made Remove() try to unwind an install
that never happened, and turned any later Install into a no-op. Hand the flag
back on failure so the state matches reality.

Hook<Fn> claimed Real() reaches the original whether or not the slot is
attached. That holds for a slot built from an address, which has one from
construction, but not for one built from an export name or from the
replacement alone: those have no address until an attach resolves them, and
Real() is null until then. The same is true after pointing a detached slot at
an export, which drops the address it had. Narrow the claim to what the code
does rather than widen the code to fit the claim - nothing calls Real() on an
unresolved slot today, and a slot that has not been told where its target is
has nothing truthful to return.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ResurrectedTrader
ResurrectedTrader merged commit 3d7ed51 into main Sep 17, 2026
1 check passed
@ResurrectedTrader
ResurrectedTrader deleted the fix/hook-real-contract branch September 17, 2026 21:17
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