fix: resolve concurrency bugs, improve safety, and add SSE2/sanitizer support#24
Merged
fix: resolve concurrency bugs, improve safety, and add SSE2/sanitizer support#24
Conversation
📝 WalkthroughWalkthroughBumps project version to 2.0.0; adds an opt-in sanitizers CMake flag; introduces SSE2-accelerated AOB scanning; and applies broad concurrency, noexcept, and API signature changes across logger, async-logger, config, hook, input, and memory subsystems, with related tests and CMake tweaks. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
tkhquang
force-pushed
the
fix/pre-release-bugs-3
branch
from
562838e to
85a57b6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
README.md (1)
10-10: Qualify the SSE2 claim as build-dependent.
src/scanner.cpponly enables the vectorized path when the target/compiler exposes SSE2, so this sentence overstates the guarantee for non-SSE2 x86 builds. “On SSE2-capable builds” would match the implementation.💬 Suggested wording
-* **AOB Scanner:** Find array-of-bytes (signatures) in memory with wildcard support and SSE2-accelerated pattern verification for patterns >= 16 bytes. Supports `|` offset markers for targeting a specific instruction within a wider pattern (e.g., `"48 8B 88 B8 00 00 00 | 48 89 4C 24 68"` sets the offset to byte 7) and nth-occurrence matching (1-based) for patterns that hit multiple locations. Includes RIP-relative instruction resolution for extracting absolute addresses from x86-64 code. +* **AOB Scanner:** Find array-of-bytes (signatures) in memory with wildcard support and SSE2-accelerated pattern verification on SSE2-capable builds for patterns >= 16 bytes. Supports `|` offset markers for targeting a specific instruction within a wider pattern (e.g., `"48 8B 88 B8 00 00 00 | 48 89 4C 24 68"` sets the offset to byte 7) and nth-occurrence matching (1-based) for patterns that hit multiple locations. Includes RIP-relative instruction resolution for extracting absolute addresses from x86-64 code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 10, Update the README description of "AOB Scanner" to qualify the SSE2 acceleration as build-dependent by changing the claim that it "SSE2-accelerated pattern verification" to something like "SSE2-accelerated pattern verification on SSE2-capable builds" or "when compiled with SSE2 support", so the wording matches the implementation in src/scanner.cpp that only enables the vectorized path when SSE2 is available; keep the rest of the sentence (wildcard support, `|` offset markers, nth-occurrence matching, RIP-relative resolution) unchanged.src/hook_manager.cpp (1)
122-232: Deferred logging pattern avoids holding mutex during logging.The refactoring to collect log entries in a vector and emit them after releasing the lock is a good concurrency improvement. This prevents potential deadlocks if the logger itself needs synchronization.
One observation: the
DeferredLogstruct is defined identically in bothcreate_inline_hookandcreate_mid_hook. Consider moving it to class scope or an anonymous namespace to avoid duplication.♻️ Optional: Extract DeferredLog to reduce duplication
+namespace { + struct DeferredLog { + std::string msg; + LogLevel level; + }; +} + std::expected<std::string, HookError> HookManager::create_inline_hook(...) { - struct DeferredLog - { - std::string msg; - LogLevel level; - }; // ... rest of implementation🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hook_manager.cpp` around lines 122 - 232, The DeferredLog struct is duplicated in create_inline_hook and create_mid_hook; extract it to a shared scope (e.g., move struct DeferredLog into the class definition of HookManager or into an anonymous namespace above the methods) and update both functions to use that single definition (referencing DeferredLog, create_inline_hook, and create_mid_hook) to remove duplication and keep the deferred-logging behavior unchanged.tests/test_logger.cpp (1)
942-965: Timestamp format verification is reasonable but fragile.The test correctly verifies the log message appears and checks for expected date format structure. However, the offset-based checks (Lines 962-963) assume the timestamp always starts exactly at
pos, which depends on other content not appearing before the timestamp.Consider a more robust approach using a regex or at least finding the closing bracket position:
💡 Suggested improvement
// Instead of fixed offsets, verify the pattern more robustly auto bracket_end = content.find(']', pos); if (bracket_end != std::string::npos) { std::string timestamp = content.substr(pos + 1, bracket_end - pos - 1); // Verify format: YYYY-MM-DD HH:MM:SS (19 chars minimum) EXPECT_GE(timestamp.size(), 19u); EXPECT_EQ(timestamp[4], '-'); EXPECT_EQ(timestamp[7], '-'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_logger.cpp` around lines 942 - 965, The timestamp verification in TEST_F(LoggerTest, TimestampFormat_StrftimeOutput) is fragile because it uses fixed offsets from content.find("[20"); update the test to locate the closing bracket (']') after the opening '[' (found at pos), extract the timestamp substring between them, assert its minimum length (e.g., >= 19), and then check the expected separators at timestamp[4] and timestamp[7] (and other positions as needed) instead of using fixed offsets from pos; this change keeps the checks resilient to any preceding log content while still using the same Logger::get_instance(), logger.info call, and test_log_file_ reading flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/config.cpp`:
- Around line 397-405: The race occurs because the new CallbackConfigItem is
published (getRegisteredConfigItems().push_back) before invoking
setter(default_value), allowing Config::load() to see the item and later be
overwritten; fix by creating the CallbackConfigItem (e.g.,
make_unique<CallbackConfigItem<int>>(...)) but do NOT push it into
getRegisteredConfigItems() until after you've invoked setter(default_value) on
the new item (or alternatively set and check an initialization flag that load()
respects). Concretely: construct the unique_ptr locally, call
setter(default_value) while the item is still unpublished, then acquire the
mutex and push_back the unique_ptr into getRegisteredConfigItems(); apply the
same pattern to the other overloads (the blocks around lines 412-420, 427-435,
442-450, 459-467) and/or implement a load-initialized flag checked by
Config::load().
- Around line 262-268: The lambda returned by take_deferred_apply currently
captures this, allowing the callback to observe mutated current_value or
dangling storage; instead, make the callback self-contained by capturing setter
and current_value by value (e.g., copy setter and current_value into the lambda)
so the closure invokes the copied setter(copied_current_value) without
referencing this or any members and thus is safe across later load() calls or
registry changes.
In `@src/logger.cpp`:
- Around line 69-75: The lifecycle flag shutdown_called_ is not sufficient for
mutual exclusion between configure()/reconfigure() and shutdown(); serialize
configure()/reconfigure() and shutdown() with a single lifecycle mutex so they
cannot interleave (prevent configure clearing shutdown_called_ then entering
reconfigure() while shutdown_internal() is running). Modify configure(),
reconfigure(), and shutdown()/shutdown_internal() to acquire the same lifecycle
lock (e.g., add/std::mutex lifecycle_mutex or reuse an existing lifecycle lock)
around the state mutation and stream open/close operations (the
shutdown_called_.store(false) in configure(), the check/CAS and
shutdown_internal() path in shutdown(), and the body of reconfigure()) so
open/close of the file stream and updates to prefix/file_name/timestamp_fmt are
performed under that lock.
- Around line 89-95: The fast-path in configure() incorrectly returns when
log_prefix_, log_file_name_, and timestamp_format_ match, ignoring whether the
underlying stream is closed; update the early-return condition to also verify
the stream is usable (e.g., check the file stream/member like
log_stream_.is_open() or an equivalent "open/healthy" flag) so that after
shutdown_internal() or a prior open failure configure() will attempt to reopen
via open() instead of skipping; adjust the condition in configure() that
compares log_prefix_, log_file_name_, and timestamp_format_ to include this
stream-state check.
- Around line 289-295: The fixed-size char buf[128] causes valid timestamps to
be rejected when strftime needs more space; change the timestamp formatting path
(the code using timestamp_format_ and timeinfo_struct) to allocate a dynamic
buffer and retry until std::strftime returns a non-zero length (e.g., start with
128 bytes and double on failure) or use an std::ostringstream/std::put_time
fallback to produce the formatted string without a hard cap, then return the
resulting std::string; ensure you preserve the existing error branch only if an
actual formatting failure occurs after reasonable retries.
In `@src/memory.cpp`:
- Around line 1004-1007: The current unconditional construction of
MemoryUtilsCacheInternal::ActiveReaderGuard increments s_activeReaders before
checking s_cacheInitialized, which causes fallback calls to block shutdown;
modify DetourModKit::Memory::is_readable() and
DetourModKit::Memory::is_writable() to perform the cheap hot-path checks first
(e.g., if s_shardCount == 0 or !s_cacheInitialized) and only construct/enter
ActiveReaderGuard after those checks pass, so the reader count is incremented
only when the cache/shards are actually in use.
---
Nitpick comments:
In `@README.md`:
- Line 10: Update the README description of "AOB Scanner" to qualify the SSE2
acceleration as build-dependent by changing the claim that it "SSE2-accelerated
pattern verification" to something like "SSE2-accelerated pattern verification
on SSE2-capable builds" or "when compiled with SSE2 support", so the wording
matches the implementation in src/scanner.cpp that only enables the vectorized
path when SSE2 is available; keep the rest of the sentence (wildcard support,
`|` offset markers, nth-occurrence matching, RIP-relative resolution) unchanged.
In `@src/hook_manager.cpp`:
- Around line 122-232: The DeferredLog struct is duplicated in
create_inline_hook and create_mid_hook; extract it to a shared scope (e.g., move
struct DeferredLog into the class definition of HookManager or into an anonymous
namespace above the methods) and update both functions to use that single
definition (referencing DeferredLog, create_inline_hook, and create_mid_hook) to
remove duplication and keep the deferred-logging behavior unchanged.
In `@tests/test_logger.cpp`:
- Around line 942-965: The timestamp verification in TEST_F(LoggerTest,
TimestampFormat_StrftimeOutput) is fragile because it uses fixed offsets from
content.find("[20"); update the test to locate the closing bracket (']') after
the opening '[' (found at pos), extract the timestamp substring between them,
assert its minimum length (e.g., >= 19), and then check the expected separators
at timestamp[4] and timestamp[7] (and other positions as needed) instead of
using fixed offsets from pos; this change keeps the checks resilient to any
preceding log content while still using the same Logger::get_instance(),
logger.info call, and test_log_file_ reading flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 525d8e04-c5cb-49f8-b88d-2becd821de7f
📒 Files selected for processing (18)
CMakeLists.txtREADME.mdinclude/DetourModKit.hppinclude/DetourModKit/async_logger.hppinclude/DetourModKit/hook_manager.hppinclude/DetourModKit/input.hppinclude/DetourModKit/logger.hppsrc/async_logger.cppsrc/config.cppsrc/hook_manager.cppsrc/input.cppsrc/logger.cppsrc/memory.cppsrc/scanner.cpptests/CMakeLists.txttests/test_async_logger.cpptests/test_logger.cpptests/test_scanner.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/hook_manager.cpp`:
- Around line 186-227: The success path is not exception-safe: expensive
allocations and side-effects (std::format/push_back, assigning
*original_trampoline) happen before committing the managed hook to m_hooks, so a
thrown exception can leave a live hook with dangling state or misreported
UnknownError; to fix, build and reserve the DeferredLog vector and all formatted
messages (including status_message and auto-enable warning) before
creating/moving the InlineHook into m_hooks, only call m_hooks.emplace(...) once
all formatting succeeds, and assign *original_trampoline after emplace returns
successfully (use sh_inline_hook_ptr and InlineHook construction locally, then
move into m_hooks); apply the same changes for the inline path and the similar
block around lines 331-371 so that logs are prepared and trampoline assignment
happens post-commit.
- Around line 131-160: After acquiring the lock in create_inline_hook (and
similarly in create_mid_hook) check
m_shutdown_called.load(std::memory_order_acquire) and return an appropriate
std::unexpected(HookError::ShutdownInProgress) with a deferred log if shutdown
has begun; place this guard immediately after the
std::unique_lock<std::shared_mutex> lock(m_hooks_mutex) so no hook creation
proceeds while shutdown is in progress, and add the same check in the other
analogous block(s) that create hooks to prevent recreation after shutdown drains
m_hooks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7a0d3151-0b82-46ea-a58e-c5e476bda3e2
📒 Files selected for processing (5)
README.mdsrc/config.cppsrc/hook_manager.cppsrc/logger.cpptests/test_logger.cpp
✅ Files skipped from review due to trivial changes (2)
- README.md
- tests/test_logger.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- src/logger.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/hook_manager.cpp (1)
198-203: Minor log message formatting inconsistency.The disabled case produces
"Successfully created (created disabled) inline hook..."with a double space and redundant "created" wording.💡 Suggested fix for clearer log messages
- std::string status_message = (initial_status == HookStatus::Active) ? "and enabled" : " (created disabled)"; + const char* status_message = (initial_status == HookStatus::Active) ? "enabled" : "disabled"; std::vector<DeferredLog> logs; logs.reserve(2); - logs.push_back({std::format("HookManager: Successfully created {} inline hook '{}' targeting {}.", + logs.push_back({std::format("HookManager: Successfully created inline hook '{}' targeting {} ({}).", - status_message, name, DetourModKit::Format::format_address(target_address)), + name, DetourModKit::Format::format_address(target_address), status_message), LogLevel::Info});Also applies to Line 343 for mid hooks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hook_manager.cpp` around lines 198 - 203, The log message builds status_message with a leading space and redundant "created" for the disabled case; update the assignment of status_message (used when pushing DeferredLog in HookManager) so the active case is "enabled" and the disabled case is "(disabled)" or "disabled" without leading/trailing extra spaces, and ensure the std::format call in the DeferredLog push_back concatenates with a single space only once; apply the same change to the equivalent mid-hook path where status_message is constructed for mid hooks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/hook_manager.cpp`:
- Around line 198-203: The log message builds status_message with a leading
space and redundant "created" for the disabled case; update the assignment of
status_message (used when pushing DeferredLog in HookManager) so the active case
is "enabled" and the disabled case is "(disabled)" or "disabled" without
leading/trailing extra spaces, and ensure the std::format call in the
DeferredLog push_back concatenates with a single space only once; apply the same
change to the equivalent mid-hook path where status_message is constructed for
mid hooks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9c05fe5-fba4-4a11-a11e-9caa3e1dd12e
📒 Files selected for processing (3)
include/DetourModKit/hook_manager.hppsrc/hook_manager.cpptests/test_hook_manager.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- include/DetourModKit/hook_manager.hpp
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 by CodeRabbit
New Features
Documentation
Improvements
Tests