feat: mod authoring helpers#68
Merged
Merged
Conversation
- Scanner::resolve_cascade and resolve_cascade_with_prologue_fallback for multi-candidate AOB resolution with RIP and hooked-prologue recovery, guarded by literal-tail and module-bounds checks - HookManager cross-module duplicate-hook detection (E9 rel32, EB rel8, FF 25 rip-indirect), HookError::TargetAlreadyHookedInProcess - Config::register_press_combo with InputBindingGuard RAII and live update via InputManager::update_binding_combos - Filesystem::get_runtime_directory_utf8 UTF-8 sibling (cached) - DMK::Bootstrap DllMain lifecycle helper with loader-lock-safe detach and request_shutdown pre-unload contract - DMK::StoppableWorker RAII jthread wrapper with loader-lock-aware destruction (pin-then-detach) - Logger::configure timestamp_fmt default argument Adds 13 test cases (bootstrap, worker, scanner cascade hard paths, hook duplicate detection, concurrent input rebind). 932 tests pass.
tkhquang
changed the title
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 31 minutes and 3 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR introduces DLL lifecycle management ( Changes
Sequence Diagram(s)sequenceDiagram
participant DLL as DLL Module
participant Boot as Bootstrap
participant Log as Logger
participant Worker as StoppableWorker
participant Init as User Init/Shutdown
participant DMK as DMK_Shutdown
DLL->>Boot: on_dll_attach(hMod, ModInfo, init_fn, shutdown_fn)
Boot->>Boot: Validate process name, enforce instance mutex
Boot->>Log: Configure async logger
Log-->>Boot: Ready
Boot->>Worker: Start lifecycle worker
Worker->>Init: Run init_fn() in background
Init-->>Worker: init complete
Worker->>Worker: Wait on shutdown event
Note over DLL,Init: DLL remains loaded, worker spinning...
DLL->>Boot: request_shutdown() or on_dll_detach
Boot->>Worker: Signal shutdown event
Worker->>Init: Run shutdown_fn()
Init-->>Worker: cleanup complete
Worker->>DMK: Call DMK_Shutdown()
DMK-->>Boot: Shutdown complete
Boot->>DLL: Detach (close handles, release mutex)
sequenceDiagram
participant Scanner as Scanner::resolve_cascade
participant Parser as AOB Parser
participant Memory as Process Memory
participant Decoder as x86_decode (E9/EB/FF25)
participant Result as ResolveHit / ResolveError
Scanner->>Scanner: Validate candidates not empty
loop For each AddrCandidate
Scanner->>Parser: Parse AOB pattern
alt Pattern parse fails
Scanner->>Result: Return AllPatternsInvalid
else Pattern valid
Scanner->>Memory: Scan executable regions for match
alt No match found
Scanner->>Result: Continue to next candidate
else Match found at Address
Scanner->>Decoder: Decode via ResolveMode (Direct/RipRelative)
Decoder-->>Scanner: Resolved address
Scanner->>Result: Return ResolveHit { address, winning_name }
end
end
end
alt All candidates exhausted without match
Scanner->>Result: Return NoMatch error
end
sequenceDiagram
participant Config as Config::register_press_combo
participant InputMgr as InputManager
participant InputPoller as InputPoller (polling thread)
participant Guard as InputBindingGuard
participant Handler as User on_press Callback
Config->>InputMgr: Update current_combos in config
Config->>InputMgr: Call update_binding_combos(name, combos)
alt InputManager running (poller active)
InputMgr->>InputPoller: Take bindings_rw_mutex (exclusive)
InputPoller->>InputPoller: Swap keys/modifiers, recompute cache
InputPoller-->>InputMgr: Unlock
else InputManager not started yet
InputMgr->>InputMgr: Update pending_bindings_
end
Config->>InputMgr: Register InputManager press callback
InputMgr-->>Config: Handler installed with atomic enabled flag
Config-->>Guard: Return InputBindingGuard(name, enabled_flag_ptr)
Note over InputPoller,Handler: During polling cycle...
InputPoller->>InputPoller: Take bindings_rw_mutex (shared)
InputPoller->>InputPoller: Poll registered bindings
InputPoller-->>InputPoller: Release mutex, collect pending callbacks
InputPoller->>Handler: Call on_press() if enabled_flag is true
Handler-->>InputPoller: Callback complete
Note over Guard: Later - Guard destruction or release()
Guard->>Guard: Set enabled_flag to false atomically
Guard->>Handler: Callback disabled (no further invocations)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
134-134:⚠️ Potential issue | 🟡 MinorDoc inconsistency: Filesystem bullet still says "wide-string API" only.
Line 23 of the features table correctly advertises "wide-string and UTF-8 APIs", but the bullet inside the collapsible "Format, Filesystem, Math, and Version Utilities" section at line 134 still reads
Module directory resolution (wide-string API).— this should be updated to match (e.g. "wide-string and UTF-8 APIs") so users discovering the newget_runtime_directory_utf8()from the detail section see it too.Proposed fix
-- **Filesystem** (`filesystem.hpp`): Module directory resolution (wide-string API). +- **Filesystem** (`filesystem.hpp`): Module directory resolution (wide-string and UTF-8 APIs).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 134, Update the README bullet in the "Format, Filesystem, Math, and Version Utilities" section so it matches the features table by changing the Filesystem entry from "Module directory resolution (wide-string API)" to indicate both APIs (e.g., "Module directory resolution (wide-string and UTF-8 APIs)"); mention the header name `filesystem.hpp` and the new API symbol `get_runtime_directory_utf8()` in the revised text so readers can discover the UTF-8 API from the detailed section.
🧹 Nitpick comments (11)
tests/test_filesystem.cpp (1)
104-115: Consider consolidating into the existingFilesystemTestsuite for consistency.The new tests use a separate suite name
FilesystemUtf8while every other test in this file usesFilesystemTest(e.g.FilesystemTest.GetRuntimeDirectory_CachedResult). Aligning toFilesystemTest.GetRuntimeDirectoryUtf8_*would keep the GTest output grouping consistent and simplify filtering. Also, aNoTrailingSeparator/IsAbsoluteassertion analogous to the wide-string tests would strengthen coverage of the UTF-8 variant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_filesystem.cpp` around lines 104 - 115, Rename and merge the two new tests into the existing FilesystemTest suite and expand assertions: change TEST(FilesystemUtf8, ReturnsNonEmpty) to TEST_F(FilesystemTest, GetRuntimeDirectoryUtf8_ReturnsNonEmpty) and TEST(FilesystemUtf8, Cached) to TEST_F(FilesystemTest, GetRuntimeDirectoryUtf8_Cached) (or use TEST with suite name FilesystemTest if other tests are non-fixture), call Filesystem::get_runtime_directory_utf8() as before, assert non-empty and equality for caching, and add the same additional checks used for the wide-string tests (e.g., NoTrailingSeparator and IsAbsolute assertions referencing the runtime directory UTF-8 result) so the UTF-8 tests mirror the coverage and naming convention of FilesystemTest.GetRuntimeDirectory_CachedResult.tests/test_config.cpp (1)
1286-1313: InputBindingGuard tests look solid; one small suggestion.The three new tests cover default-constructed inactivity, idempotent
release(), and move-transfer. One edge worth adding cheaply: verify that destroying an active guard also clears the shared flag (RAII contract), e.g.:TEST(InputBindingGuard, DestructorReleasesFlag) { auto flag = std::make_shared<std::atomic<bool>>(true); { Config::InputBindingGuard g("b", flag); } EXPECT_FALSE(flag->load()); }Given the PR summary describes
InputBindingGuardas RAII, covering the destructor path prevents regressions where only explicitrelease()clears the flag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_config.cpp` around lines 1286 - 1313, Add a test that verifies InputBindingGuard's destructor clears the shared flag to enforce the RAII contract: create a shared_ptr<std::atomic<bool>> initialized true, construct a scoped Config::InputBindingGuard (use a distinguishing test name like "DestructorReleasesFlag"), let it go out of scope, and then assert the flag->load() is false; this complements the existing tests for release() and move semantics and ensures implicit destruction also calls the same cleanup as release().src/filesystem.cpp (1)
144-169:to_utf8logic is correct; consider tightening the overflow fallback.Two minor observations:
wide.size() > INT_MAXreturning"."is defensive but effectively unreachable given Windows path limits (MAX_MODULE_PATH = 32768upstream). Fine as-is.- Returning
"."whenWideCharToMultiBytefails on a non-empty wide path silently masks a real failure (e.g. lone surrogate). The wide resolver already logs to stderr on failure; mirroring that here would aid diagnosability:if (needed <= 0) { + std::cerr << "[DMK Filesystem WARNING] WideCharToMultiByte(CP_UTF8) sizing failed. Error: " + << GetLastError() << '\n'; return "."; }Not blocking — caching means the failure would only fire once per process.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/filesystem.cpp` around lines 144 - 169, In to_utf8, don't silently return "." on WideCharToMultiByte failures — emit a diagnostic to stderr (like the wide resolver does) including context (the failing function name to_utf8 and the problematic wide string or its length) and the system error (GetLastError or formatted message) before returning the fallback; leave the INT_MAX check as-is. Locate the failure branches in to_utf8 (the two places that return "." when needed <= 0 or written <= 0) and add the stderr logging there so callers have actionable diagnostics.tests/test_logger.cpp (1)
1517-1536: Minor: new test does not clean up under exception and leaks the counter helper across fixtures.The
TEST(LoggerConfigureOverload, ...)is a free test (notLoggerTestfixture), so ifflush()/info()throws, the temp log file leaks. Wrapping theremovein a try/catch (as done elsewhere in this file) and optionally asserting content-before-remove would match the surrounding style. Not a blocker.Proposed tweak
- EXPECT_TRUE(std::filesystem::exists(path)); - std::filesystem::remove(path); + EXPECT_TRUE(std::filesystem::exists(path)); + try { std::filesystem::remove(path); } + catch (const std::filesystem::filesystem_error &) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_logger.cpp` around lines 1517 - 1536, The test should guarantee cleanup of the temporary log file even if Logger::get_instance().info() or flush() throws and avoid leaking the temp file/counter state across failures: wrap the file removal in a try/catch (mirroring other tests), check EXPECT_TRUE(std::filesystem::exists(path)) or assert file content before remove, and in the catch block attempt std::filesystem::remove(path) (or ignore errors) so the temp file created by make_logger_overload_path() is always removed; modify the TEST(LoggerConfigureOverload...) to perform removal inside a safe try/catch around the remove call while leaving make_logger_overload_path(), Logger::configure, Logger::get_instance().info(), and flush() usage unchanged.include/DetourModKit/config.hpp (1)
5-16: Redundant forward declaration ofLogger.
"DetourModKit/logger.hpp"is now included at line 5, which makes theclass Logger;forward declaration on line 16 redundant.♻️ Proposed cleanup
namespace DetourModKit { - class Logger; - /** * `@namespace` Config🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/DetourModKit/config.hpp` around lines 5 - 16, The file contains a redundant forward declaration "class Logger;" because DetourModKit/logger.hpp is already included; remove the forward declaration to avoid duplication. Locate the forward declaration token "class Logger;" in the config.hpp header and delete that single line, leaving the existing `#include` "DetourModKit/logger.hpp" and any usages of Logger intact (no other code changes needed).src/config.cpp (1)
606-611: Avoid spuriousupdate_binding_comboswarning during initial registration.
register_key_combo(line 606) immediately fires its setter with the parsed default combos. That setter callsInputManager::update_binding_combos(binding_name_str, ...)before the binding is registered viaregister_press(line 611). Per the implementation insrc/input.cpp:713-768(update_binding_combos), that lookup falls through the "name not found" branch and emits a Debug-level log on everyregister_press_combocall. Functionally harmless, but noisy.Consider gating the forwarding call on registration having actually happened, e.g. by flipping a shared flag after
register_pressreturns and letting the setter check it, or by reordering soregister_pressruns first andregister_key_combois registered after (the initial INI load still picks up the correct value sinceload()invokes each item's setter). Example:♻️ Proposed fix
auto enabled_flag = std::make_shared<std::atomic<bool>>(true); auto current_combos = std::make_shared<KeyComboList>(parse_key_combo_list(std::string(default_value))); std::string binding_name_str(input_binding_name); + auto binding_live = std::make_shared<std::atomic<bool>>(false); - register_key_combo(section, ini_key, log_name, [current_combos, binding_name_str](const KeyComboList &combos) + register_key_combo(section, ini_key, log_name, [current_combos, binding_name_str, binding_live](const KeyComboList &combos) { *current_combos = combos; - InputManager::get_instance().update_binding_combos(binding_name_str, combos); }, default_value); + if (binding_live->load(std::memory_order_acquire)) + { + InputManager::get_instance().update_binding_combos(binding_name_str, combos); + } }, default_value); InputManager::get_instance().register_press( binding_name_str, *current_combos, [enabled_flag, cb = std::move(on_press)]() { if (cb && enabled_flag->load(std::memory_order_acquire)) { cb(); } }); + binding_live->store(true, std::memory_order_release);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.cpp` around lines 606 - 611, The setter passed to register_key_combo is calling InputManager::update_binding_combos(binding_name_str, ...) before the binding is registered via InputManager::register_press, causing noisy "name not found" debug logs; fix by ensuring the forwarding only runs after registration: either register the press first (call InputManager::get_instance().register_press(...) before register_key_combo) or add a small shared flag/atomic captured by the setter (e.g., bool registered = false captured by reference) that you flip to true immediately after register_press returns, and have the setter early-return unless registered is true; reference the lambda passed into register_key_combo, InputManager::update_binding_combos, register_press, and binding_name_str to locate the exact spots to change.src/x86_decode.hpp (1)
47-72:decode_ff25_indirectcorrectly assumes x86-64 RIP-relative semantics; no fix needed for current scope.The instruction semantics claim is technically accurate:
FF 25 disp32is RIP-relative[RIP+disp32]on x86-64 (long mode) and absolute[disp32]on x86-32. However, DetourModKit is explicitly built for x86-64 only:
- CMakePresets.json defines only native-arch presets (mingw-debug, mingw-release, msvc-debug, msvc-release) with no
-m32or 32-bit explicit flags- CMakeLists.txt has no 32-bit build logic or architecture detection
- README emphasizes x86-64 features (RIP resolution) and Windows 10+ targeting
- The
#ifdef _WIN64code in platform.hpp is Windows API compatibility (PEB access methods), not for 32-bit library buildsThe
decode_ff25_indirectfunction correctly implements x86-64 semantics for the current, 64-bit-only project. If future 32-bit support is planned, the proposed#ifdef _WIN64conditional would be appropriate, but it is not needed now.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/x86_decode.hpp` around lines 47 - 72, The decode_ff25_indirect function correctly assumes x86-64 RIP-relative semantics; do not change the logic, but make the intent explicit: keep decode_ff25_indirect as-is and add a short comment above it stating it implements x86-64 RIP-relative FF 25 disp32 semantics (or add a compile-time guard such as static_assert(sizeof(void*)==8) / `#ifdef` _WIN64) to document/guard the 64-bit-only assumption for future readers.src/input.cpp (2)
180-183: Redundanthas_gamepad_bindings_store in the constructor.Line 180 stores the gamepad-binding flag, and then
recompute_modifier_caches_locked()on line 182 stores it again at line 201. The first store is dead. Either drop it, or makerecompute_modifier_caches_lockednot touchhas_gamepad_bindings_(leaving the constructor as the sole initializer andupdate_combosre-deriving only when bindings change). Current behavior is correct but mildly confusing.♻️ Suggested cleanup
- has_gamepad_bindings_.store(scan_for_gamepad_bindings(bindings_), std::memory_order_relaxed); name_index_.reserve(bindings_.size()); recompute_modifier_caches_locked();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/input.cpp` around lines 180 - 183, The initial store to has_gamepad_bindings_ using scan_for_gamepad_bindings(bindings_) in the constructor is redundant because recompute_modifier_caches_locked() reassigns has_gamepad_bindings_ again; remove the first store in the constructor (leave recompute_modifier_caches_locked() as the single place that sets has_gamepad_bindings_) so initialization is not done twice and the code is clearer.
344-351: Allocatependingoutside the hot loop to amortize cost.
PendingCallbackis declared and a freshstd::vector<PendingCallback> pendingis default-constructed every poll cycle (~every 5 ms). Moving the vector out of thewhile (!stop_token.stop_requested())loop and callingpending.clear()at the top of each iteration (plus a one-timereserve(bindings_.size())) keeps the capacity around and avoids repeated heap churn on the poll thread. Minor, but it is the per-tick path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/input.cpp` around lines 344 - 351, The PendingCallback type is fine but the std::vector<PendingCallback> pending is being constructed each poll cycle inside the while (!stop_token.stop_requested()) loop; move the declaration of pending outside that loop so it is only constructed once, call pending.clear() at the top of each iteration, and after construction do a one-time pending.reserve(bindings_.size()) to keep capacity stable and avoid per-tick allocations; adjust any uses of pending in the loop accordingly.src/bootstrap.cpp (1)
223-229:request_shutdownreadsg_shutdown_eventwithout synchronization.
g_shutdown_eventis written on the DllMain thread insideon_dll_attach(line 206) and cleared inon_dll_detach(line 316), butrequest_shutdown()is documented as "Safe to call from any thread" (seeinclude/DetourModKit/bootstrap.hpp:110). The read of the plainHANDLEglobal is technically racy with respect to those writes. In practice Win32 thread creation inserts a release barrier, so callers invokingrequest_shutdown()from the worker-facing side after attach completes will observe the event; and the documented contract is to call this beforeFreeLibrary, which also precludes racingon_dll_detach. Still, modeling these handles asstd::atomic<HANDLE>(or guarding with a small mutex) would make the lifetime invariants explicit and silence TSan-style tools.Does any caller (test or mod scaffolding) invoke
request_shutdownfrom a non-worker thread whileon_dll_detachis concurrently running? If yes, the race is real and should be hardened; if not, the contract in the header is sufficient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bootstrap.cpp` around lines 223 - 229, The global g_shutdown_event is accessed without synchronization in request_shutdown(), which races with writes in on_dll_attach/on_dll_detach; change the global to std::atomic<HANDLE> (or protect with a small mutex) and update all accesses: in on_dll_attach use atomic.store(handle, std::memory_order_release), in on_dll_detach use atomic.exchange(nullptr, std::memory_order_acq_rel) (close the old handle only if non-null), and in request_shutdown load the handle with std::memory_order_acquire and call SetEvent only if the loaded HANDLE is non-null; update include/DetourModKit/bootstrap.hpp contract if needed. This makes the lifetime explicit and removes the TSan race while preserving the documented "safe to call from any thread" behavior.src/scanner.cpp (1)
811-825:count_pattern_hits_boundedre-walks the full address space per iteration.Each
scan_executable_regions(pattern, n)call restartsVirtualQueryfrom address 0 and re-scans every committed executable region up to the Nth hit. Calling it forn = 1..max_hits+1(5 times withkPrologueFallbackMaxHits = 4) redoes all prior work on each iteration — effectively O(max_hits² × regions). Given this only runs on cascade failure with tinymax_hits, it is not a practical issue, but exposing a dedicatedcount_pattern_hits(pattern, cap)inScannerthat walks regions once and stops atcap + 1would make intent clearer and avoid the quadratic behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scanner.cpp` around lines 811 - 825, The current count_pattern_hits_bounded repeatedly calls DetourModKit::Scanner::scan_executable_regions(pattern, n) which restarts region scanning for every n; replace this quadratic behavior by adding or changing to a single-pass implementation that walks executable regions once, uses the CompiledPattern to search within each region (advancing an offset after each match), increments hits and stops when hits > max_hits (i.e., cap+1), then returns hits; update or add a Scanner method (e.g., Scanner::count_pattern_hits_bounded or Scanner::count_pattern_hits(pattern, cap)) that performs this single-pass region iteration instead of repeated calls to scan_executable_regions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/DetourModKit/bootstrap.hpp`:
- Around line 71-74: Add the [[nodiscard]] attribute to the on_dll_attach
declaration and its definition/implementation so callers cannot silently ignore
its BOOL result; update the prototype BOOL on_dll_attach(...) and the
corresponding function definition (the implementation symbol on_dll_attach) to
both be marked [[nodiscard]] to keep signature parity and avoid compiler
warnings/errors.
- Line 15: This public header currently includes <windows.h> and exposes Win32
types (e.g., HMODULE, BOOL/DllMain-facing signatures) which pollutes consumers;
either remove the include and replace Win32 types with neutral aliases (use
void* for OS handles and int for BOOL-returning functions) in bootstrap.hpp and
confine all actual Windows API usage and `#include` <windows.h> to the .cpp
implementation, or if the DllMain-facing API intentionally requires Windows
types, add an explicit `#ifdef` _WIN32 guard and a short comment explaining the
deliberate carve-out; update any symbols that reference HMODULE/BOOL in
bootstrap.hpp to the new typedefs or guarded signatures so the umbrella
DetourModKit.hpp no longer transits <windows.h>.
In `@src/bootstrap.cpp`:
- Line 31: Initialize the uninitialized stack arrays by changing the
declarations for exe_path and mutex_name to use brace-initialization;
specifically, update the declaration of char exe_path[MAX_PATH] and wchar_t
mutex_name[128] to use {} so they are zero-initialized before being passed to
GetModuleFileNameA and wsprintfW (references: exe_path, mutex_name,
GetModuleFileNameA, wsprintfW).
In `@src/hook_manager.cpp`:
- Around line 58-97: detect_existing_inline_hook currently flags any
cross-module JMP as HookedByOtherModule; change the heuristic to ignore known
legitimate thunks such as import/IAT indirect jumps and compiler/linker
forwarder stubs. After calling decode_prehook_destination in
detect_existing_inline_hook, examine the original instruction bytes at
target_address (e.g., detect opcode patterns FF 25, E9/Eb patterns) and for FF
25 check the RIP/displacement/memory operand address — if that memory operand
points into the target module's import/IAT/data section or to a known forwarder
thunk within the destination module (use
VirtualQuery/GetModuleHandleEx/GetModuleInformation to map addresses to
modules/sections), treat the target as not already hooked (set result.state =
PrehookState::Unhooked) instead of HookedByOtherModule; otherwise keep the
existing cross-module hook classification. Ensure you reference
decode_prehook_destination, detect_existing_inline_hook, PrehookDetection and
PrehookState when applying the change.
In `@src/scanner.cpp`:
- Around line 680-693: The RipRelative branch in resolve_candidate_match
currently does a reinterpret_cast to const int32_t* and dereferences it, which
risks alignment/aliasing UB and lacks a readability check; change this to
compute the target pointer (match_addr + c.disp_offset), call
Memory::is_readable(ptr, sizeof(int32_t)) and if that check fails return a safe
sentinel (e.g., 0) to reject the candidate gracefully, otherwise copy the 4
bytes into a local std::int32_t via std::memcpy and use that value when
computing the final address (maintain use of c.instr_end_offset and keep the
Direct branch unchanged); mirror the approach used by
Scanner::resolve_rip_relative for consistency.
In `@tests/test_bootstrap.cpp`:
- Around line 153-161: TearDown currently calls Bootstrap::request_shutdown()
and sig.wait_for_shutdown(...) but never invokes
Bootstrap::on_dll_detach(FALSE), leaving globals like g_shutdown_event,
g_worker_thread, g_instance_mutex, and g_detach_called set and causing future
Bootstrap::on_dll_attach() calls to short-circuit; after the existing
sig.wait_for_shutdown(kTestTimeout) completes in TearDown, call
Bootstrap::on_dll_detach(FALSE) to perform the full cleanup so the fixture is
self-contained and safe for subsequent tests.
In `@tests/test_scanner.cpp`:
- Around line 1936-1979: The test currently allows
resolve_cascade_with_prologue_fallback() to return NoMatch; instead make the
buffer contain a valid E9 rel32 that points from the call site (kOffset+5) to
the intended synthetic destination so the prologue fallback is actually
exercised, then assert fallback.has_value() unconditionally. Concretely: compute
rel32 = (uintptr_t)(buf.base + kOffset + 5 + rel) targetting the unique tail
(use synthetic_dest logic but write the computed rel into buf.base + kOffset +
1), remove the if/else that accepts NoMatch, and replace it with
ASSERT_TRUE(fallback.has_value()) followed by EXPECT_EQ(fallback->address,
reinterpret_cast<std::uintptr_t>(buf.base) + kOffset); this uses
resolve_cascade_with_prologue_fallback, Scanner::AddrCandidate ("cands"), and
the local fallback variable to locate the change.
---
Outside diff comments:
In `@README.md`:
- Line 134: Update the README bullet in the "Format, Filesystem, Math, and
Version Utilities" section so it matches the features table by changing the
Filesystem entry from "Module directory resolution (wide-string API)" to
indicate both APIs (e.g., "Module directory resolution (wide-string and UTF-8
APIs)"); mention the header name `filesystem.hpp` and the new API symbol
`get_runtime_directory_utf8()` in the revised text so readers can discover the
UTF-8 API from the detailed section.
---
Nitpick comments:
In `@include/DetourModKit/config.hpp`:
- Around line 5-16: The file contains a redundant forward declaration "class
Logger;" because DetourModKit/logger.hpp is already included; remove the forward
declaration to avoid duplication. Locate the forward declaration token "class
Logger;" in the config.hpp header and delete that single line, leaving the
existing `#include` "DetourModKit/logger.hpp" and any usages of Logger intact (no
other code changes needed).
In `@src/bootstrap.cpp`:
- Around line 223-229: The global g_shutdown_event is accessed without
synchronization in request_shutdown(), which races with writes in
on_dll_attach/on_dll_detach; change the global to std::atomic<HANDLE> (or
protect with a small mutex) and update all accesses: in on_dll_attach use
atomic.store(handle, std::memory_order_release), in on_dll_detach use
atomic.exchange(nullptr, std::memory_order_acq_rel) (close the old handle only
if non-null), and in request_shutdown load the handle with
std::memory_order_acquire and call SetEvent only if the loaded HANDLE is
non-null; update include/DetourModKit/bootstrap.hpp contract if needed. This
makes the lifetime explicit and removes the TSan race while preserving the
documented "safe to call from any thread" behavior.
In `@src/config.cpp`:
- Around line 606-611: The setter passed to register_key_combo is calling
InputManager::update_binding_combos(binding_name_str, ...) before the binding is
registered via InputManager::register_press, causing noisy "name not found"
debug logs; fix by ensuring the forwarding only runs after registration: either
register the press first (call InputManager::get_instance().register_press(...)
before register_key_combo) or add a small shared flag/atomic captured by the
setter (e.g., bool registered = false captured by reference) that you flip to
true immediately after register_press returns, and have the setter early-return
unless registered is true; reference the lambda passed into register_key_combo,
InputManager::update_binding_combos, register_press, and binding_name_str to
locate the exact spots to change.
In `@src/filesystem.cpp`:
- Around line 144-169: In to_utf8, don't silently return "." on
WideCharToMultiByte failures — emit a diagnostic to stderr (like the wide
resolver does) including context (the failing function name to_utf8 and the
problematic wide string or its length) and the system error (GetLastError or
formatted message) before returning the fallback; leave the INT_MAX check as-is.
Locate the failure branches in to_utf8 (the two places that return "." when
needed <= 0 or written <= 0) and add the stderr logging there so callers have
actionable diagnostics.
In `@src/input.cpp`:
- Around line 180-183: The initial store to has_gamepad_bindings_ using
scan_for_gamepad_bindings(bindings_) in the constructor is redundant because
recompute_modifier_caches_locked() reassigns has_gamepad_bindings_ again; remove
the first store in the constructor (leave recompute_modifier_caches_locked() as
the single place that sets has_gamepad_bindings_) so initialization is not done
twice and the code is clearer.
- Around line 344-351: The PendingCallback type is fine but the
std::vector<PendingCallback> pending is being constructed each poll cycle inside
the while (!stop_token.stop_requested()) loop; move the declaration of pending
outside that loop so it is only constructed once, call pending.clear() at the
top of each iteration, and after construction do a one-time
pending.reserve(bindings_.size()) to keep capacity stable and avoid per-tick
allocations; adjust any uses of pending in the loop accordingly.
In `@src/scanner.cpp`:
- Around line 811-825: The current count_pattern_hits_bounded repeatedly calls
DetourModKit::Scanner::scan_executable_regions(pattern, n) which restarts region
scanning for every n; replace this quadratic behavior by adding or changing to a
single-pass implementation that walks executable regions once, uses the
CompiledPattern to search within each region (advancing an offset after each
match), increments hits and stops when hits > max_hits (i.e., cap+1), then
returns hits; update or add a Scanner method (e.g.,
Scanner::count_pattern_hits_bounded or Scanner::count_pattern_hits(pattern,
cap)) that performs this single-pass region iteration instead of repeated calls
to scan_executable_regions.
In `@src/x86_decode.hpp`:
- Around line 47-72: The decode_ff25_indirect function correctly assumes x86-64
RIP-relative semantics; do not change the logic, but make the intent explicit:
keep decode_ff25_indirect as-is and add a short comment above it stating it
implements x86-64 RIP-relative FF 25 disp32 semantics (or add a compile-time
guard such as static_assert(sizeof(void*)==8) / `#ifdef` _WIN64) to document/guard
the 64-bit-only assumption for future readers.
In `@tests/test_config.cpp`:
- Around line 1286-1313: Add a test that verifies InputBindingGuard's destructor
clears the shared flag to enforce the RAII contract: create a
shared_ptr<std::atomic<bool>> initialized true, construct a scoped
Config::InputBindingGuard (use a distinguishing test name like
"DestructorReleasesFlag"), let it go out of scope, and then assert the
flag->load() is false; this complements the existing tests for release() and
move semantics and ensures implicit destruction also calls the same cleanup as
release().
In `@tests/test_filesystem.cpp`:
- Around line 104-115: Rename and merge the two new tests into the existing
FilesystemTest suite and expand assertions: change TEST(FilesystemUtf8,
ReturnsNonEmpty) to TEST_F(FilesystemTest,
GetRuntimeDirectoryUtf8_ReturnsNonEmpty) and TEST(FilesystemUtf8, Cached) to
TEST_F(FilesystemTest, GetRuntimeDirectoryUtf8_Cached) (or use TEST with suite
name FilesystemTest if other tests are non-fixture), call
Filesystem::get_runtime_directory_utf8() as before, assert non-empty and
equality for caching, and add the same additional checks used for the
wide-string tests (e.g., NoTrailingSeparator and IsAbsolute assertions
referencing the runtime directory UTF-8 result) so the UTF-8 tests mirror the
coverage and naming convention of
FilesystemTest.GetRuntimeDirectory_CachedResult.
In `@tests/test_logger.cpp`:
- Around line 1517-1536: The test should guarantee cleanup of the temporary log
file even if Logger::get_instance().info() or flush() throws and avoid leaking
the temp file/counter state across failures: wrap the file removal in a
try/catch (mirroring other tests), check
EXPECT_TRUE(std::filesystem::exists(path)) or assert file content before remove,
and in the catch block attempt std::filesystem::remove(path) (or ignore errors)
so the temp file created by make_logger_overload_path() is always removed;
modify the TEST(LoggerConfigureOverload...) to perform removal inside a safe
try/catch around the remove call while leaving make_logger_overload_path(),
Logger::configure, Logger::get_instance().info(), and flush() usage unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9d82f1ce-3a62-4681-b1cf-9c3c8fd9aa48
📒 Files selected for processing (30)
AGENTS.mdCMakeLists.txtREADME.mddocs/misc/aob-signatures.mdinclude/DetourModKit.hppinclude/DetourModKit/bootstrap.hppinclude/DetourModKit/config.hppinclude/DetourModKit/filesystem.hppinclude/DetourModKit/hook_manager.hppinclude/DetourModKit/input.hppinclude/DetourModKit/logger.hppinclude/DetourModKit/scanner.hppinclude/DetourModKit/worker.hppsrc/bootstrap.cppsrc/config.cppsrc/filesystem.cppsrc/hook_manager.cppsrc/input.cppsrc/logger.cppsrc/scanner.cppsrc/worker.cppsrc/x86_decode.hpptests/test_bootstrap.cpptests/test_config.cpptests/test_filesystem.cpptests/test_hook_manager.cpptests/test_input.cpptests/test_logger.cpptests/test_scanner.cpptests/test_worker.cpp
💤 Files with no reviewable changes (1)
- src/logger.cpp
- [[nodiscard]] on Bootstrap::on_dll_attach - Zero-init exe_path and mutex_name stack arrays - Harden RipRelative disp read with is_readable + memcpy - TearDown in test_bootstrap calls on_dll_detach for full cleanup - Make prologue-fallback scanner test deterministic (real E9 rel32) - README: Filesystem bullet lists both wide and UTF-8 APIs - Remove redundant class Logger forward decl in config.hpp - Drop redundant has_gamepad_bindings_ store in InputPoller ctor - Hoist PendingCallback + pending vector out of poll loop - static_assert(sizeof(void*)==8) on decode_ff25_indirect - New test: InputBindingGuard.DestructorReleasesFlag
tkhquang
added a commit
that referenced
this pull request
Apr 23, 2026
- logger: emplace AsyncLogger into the leak vector unconditionally under loader lock; use_count() is not a reliable proxy for 'no other owners' when concurrent log() callers hold temporaries obtained before exchange() - profiler: add static_assert + guardrail comment forbidding load-then-store RMW on the seqlock sequence counter - profiler: correct docs to describe what const char (&)[N] actually guarantees (rejects pointer decay, accepts any array) - test_hook_manager: add pre-release EXPECT_FALSE(shutdown_returned) to catch premature-return regressions directly - test_scanner: replace leak-prone raw VirtualAlloc/Protect/Free with existing ExecBuffer RAII helper - test_profiler: rename the concurrent-wrap test to match what it verifies and add per-event dur range check for torn-sample detection - README + hot-reload guide: document Mod Bootstrap and StoppableWorker helpers added in #68
tkhquang
added a commit
that referenced
this pull request
Apr 23, 2026
* fix: harden profiler, hook manager, and logger for v3.1.0 Address v3.1.0 audit findings: switch profiler seqlock to fetch_add(acq_rel), serialize HookManager destructor via mutator_gate with loader-lock module pinning, make logger shutdown leak vector append-only, enforce scanner prologue fallback hit cap, raise input cardinality mismatch to warning, scope scanner -O2 downgrade to Release only, and make ScopedProfile compile-time literal-only. * fix: address v3.1.0 audit follow-ups - logger: emplace AsyncLogger into the leak vector unconditionally under loader lock; use_count() is not a reliable proxy for 'no other owners' when concurrent log() callers hold temporaries obtained before exchange() - profiler: add static_assert + guardrail comment forbidding load-then-store RMW on the seqlock sequence counter - profiler: correct docs to describe what const char (&)[N] actually guarantees (rejects pointer decay, accepts any array) - test_hook_manager: add pre-release EXPECT_FALSE(shutdown_returned) to catch premature-return regressions directly - test_scanner: replace leak-prone raw VirtualAlloc/Protect/Free with existing ExecBuffer RAII helper - test_profiler: rename the concurrent-wrap test to match what it verifies and add per-event dur range check for torn-sample detection - README + hot-reload guide: document Mod Bootstrap and StoppableWorker helpers added in #68
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds six helpers that absorb patterns mod authors have been reimplementing:
Scanner::resolve_cascade+resolve_cascade_with_prologue_fallbackfor multi-candidate AOB resolution with RIP and hooked-prologue recoveryHookManagercross-module duplicate-hook detection (E9 / EB / FF 25)Config::register_press_combowithInputBindingGuardRAII and live-update viaInputManager::update_binding_combosFilesystem::get_runtime_directory_utf8cached UTF-8 siblingDMK::BootstrapDllMain lifecycle helper with loader-lock-safe detach andrequest_shutdownpre-unload APIDMK::StoppableWorkerRAIIstd::jthreadwrapper with loader-lock-aware teardownPlus
Logger::configuregains a defaulttimestamp_fmtargument.Backwards compatibility
No breaking changes. Two new enum values appended:
HookError::TargetAlreadyHookedInProcess,ResolveError::PrologueFallbackNotApplicable.Test plan
Summary by CodeRabbit
Release Notes v3.1.0
New Features
StoppableWorker—RAII thread abstraction for background tasksImprovements
Logger::configure()timestamp format parameter now optionalDocumentation