Skip to content

fix: resolve audit findings across cache, hooks, input, and logging#106

Merged
tkhquang merged 2 commits into
mainfrom
fix/audit-remediation-followups
Jun 11, 2026
Merged

fix: resolve audit findings across cache, hooks, input, and logging#106
tkhquang merged 2 commits into
mainfrom
fix/audit-remediation-followups

Conversation

@tkhquang

@tkhquang tkhquang commented Jun 11, 2026

Copy link
Copy Markdown
Owner

Summary

Correctness follow-ups from the latest independent audit, plus a CI test-isolation fix. Every change is covered by new tests and the full MinGW suite passes under per-process isolation.

Changes

  • memory: invalidate_range now evicts multi-page cached regions reliably by scanning shards by containment instead of a single per-page key probe.
  • hook_manager: read-only query accessors are reentrancy-guard-aware, so calling them from inside a with_* callback no longer risks recursive shared-lock UB on the non-recursive mutex.
  • input: the mouse-wheel notch backlog is capped so sustained fast scrolling cannot queue phantom notches without bound.
  • logger: the async sink honors the configured timestamp format and reports real delivery status on the shutdown path.
  • profiler: guards the QueryPerformanceFrequency result and clamps backwards durations to zero.
  • ci: the MinGW test suite runs via ctest for per-process isolation, removing a single-process cross-suite flake (matches the release and MSVC jobs).

Notes

No public API changes; behavior on the read and hot paths is unchanged. The read_ptr_unchecked lower-bound difference versus plausible_userspace_ptr is intentional and now documented.

Summary by CodeRabbit

  • Bug Fixes

    • Async logger validates file stream state before writing.
    • Profiler clamps invalid timing data; hook manager safely handles exceptions during hook operations.
    • Memory cache invalidation improved for multi-page regions; reentrancy guards prevent deadlocks in hook queries.
    • Wheel input backlog capped for stability.
  • New Features

    • Async logger supports configurable timestamp formatting.
    • Profiler validates system timing frequency with fallback handling.

- memory: invalidate_range scans all shards by containment so multi-page regions are evicted
- hook_manager: reentrancy-aware query accessors avoid recursive shared-lock UB from callbacks
- input: cap the wheel-notch backlog to bound replay under sustained scrolling
- logger: async sink honors the configured timestamp format and reports real delivery status
- profiler: guard QPC frequency and clamp backwards durations
- ci: run the MinGW test suite via ctest for per-process isolation
@tkhquang tkhquang self-assigned this Jun 11, 2026
@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@tkhquang, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 41 minutes and 19 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more credits in the billing tab to continue.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: eaecb8af-ddf2-46a5-9ce8-a1b4cff062d4

📥 Commits

Reviewing files that changed from the base of the PR and between d59617d and a196018.

📒 Files selected for processing (2)
  • include/DetourModKit/async_logger.hpp
  • include/DetourModKit/hook_manager.hpp
📝 Walkthrough

Walkthrough

This PR hardens concurrency safety and robustness across six independent systems: HookManager query reentrancy, async logger timestamp configuration consistency, input wheel pulse backlog capping, memory cache invalidation completeness, profiler tick validation, and test runner infrastructure.

Changes

Concurrency Safety and Robustness Hardening

Layer / File(s) Summary
HookManager reentrant query accessors
include/DetourModKit/hook_manager.hpp, src/hook_manager.cpp, tests/test_hook_integration.cpp
get_reentrancy_guard() becomes const to allow const query methods to check the thread-local guard. New lock_hooks_shared_reentrant() helper acquires a shared lock without deadlock by returning disengaged when the thread already holds the mutex in a callback. Five read-only query paths now use this helper. apply_vmt_hook wraps the VmtHook apply call in exception handling to prevent exceptions escaping the locked region.
Async logger timestamp configuration
include/DetourModKit/async_logger.hpp, src/async_logger.cpp, src/logger.cpp, tests/test_async_logger.cpp
AsyncLoggerConfig gains a configurable timestamp_format field. AsyncLogger::enqueue, write_batch, and handle_overflow now format timestamps using the config string instead of a hard-coded value. Logger::enable_async_mode overrides the caller's timestamp format with the owning Logger's to keep async and sync logging consistent.
Input wheel pulse backlog capping
src/input_intercept.hpp, src/input_intercept.cpp, src/input.cpp, docs/tests/README.md, tests/test_input_intercept.cpp
Introduces MAX_WHEEL_PENDING=16 constant to cap unconsumed wheel notches per direction. New add_wheel_notches() helper increments pending counts while clamping negative values to zero and saturating at the cap. InputPoller::poll_loop delegates per-cycle wheel accumulation to this helper. Tests verify cap enforcement and post-saturation draining.
Memory cache invalidation refactor
src/memory.cpp, tests/test_memory.cpp
Reworks Memory::invalidate_range to scan all cache shards for overlapping entries instead of probing only specific page-aligned keys. New evict_overlapping_entries_in_shard helper linearly scans shards and removes all overlapping entries from LRU index and sorted range structure. invalidate_range_internal now iterates all shards with bounded try-lock retries, fixing the issue where entries in multiple shards could be missed.
Profiler tick and frequency validation
src/profiler.cpp, tests/test_profiler.cpp
Profiler constructor guards QueryPerformanceFrequency, falling back to 10 MHz if the call fails or returns zero. Profiler::record clamps negative or backwards tick deltas to zero before converting to microseconds, avoiding unsigned wraparound.
Test runner and workflow updates
.github/workflows/coverage-pages.yml, .github/workflows/pr-check.yml
Updated to use ctest --preset mingw-debug instead of direct DetourModKit_tests.exe invocation. Leverages CTest's per-test process isolation while preserving coverage data accumulation for gcovr. pr-check timeout increased from 5 to 12 minutes.
Design constraints and API documentation
AGENTS.md, include/DetourModKit/config.hpp, include/DetourModKit/config_watcher.hpp, include/DetourModKit/memory.hpp
New C++ design constraints document reentrancy-safe const-accessor locking, cache key consistency, bounded queue growth, and logger config alignment. Config::disable_auto_reload() and ConfigWatcher::stop() documentation clarified to explain pending debounced changes may still trigger final callbacks. Memory::read_ptr_unchecked documentation contrasts its exclusive lower bound semantics.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • tkhquang/DetourModKit#77: Adds HookManager::is_target_already_hooked() query accessor, directly using the same shared-lock reentrant pattern introduced in this PR.
  • tkhquang/DetourModKit#9: Prior memory cache concurrency/LRU correctness fixes in src/memory.cpp; this PR's invalidation refactor builds on and extends that same subsystem.
  • tkhquang/DetourModKit#6: Prior async logger robustness refactor of AsyncLogger::enqueue behavior; this PR extends timestamp configuration and failure-handling patterns in the same code paths.
🚥 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 PR title accurately captures the main objectives: addressing audit findings across cache, hooks, input, and logging components.
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.

Actionable comments posted: 1

🧹 Nitpick comments (1)
include/DetourModKit/async_logger.hpp (1)

267-270: ⚡ Quick win

Use header-compliant member docs and brace initialization for timestamp_format.

Please switch the field comment to Doxygen-style and use brace initialization for the default value.

Suggested patch
-        // strftime-style date/time format for the async sink, kept in sync with the
-        // synchronous Logger by Logger::enable_async_mode so both sinks emit identical
-        // timestamps; the trailing ".<ms>" is appended by the writer, not this format.
-        std::string timestamp_format = "%Y-%m-%d %H:%M:%S";
+        /// strftime-style date/time format for the async sink. Logger::enable_async_mode
+        /// synchronizes this with the synchronous Logger format; ".<ms>" is appended by writer code.
+        std::string timestamp_format{"%Y-%m-%d %H:%M:%S"};

As per coding guidelines, header member docs should use /////** */, and member defaults in declarations should use brace initialization.

🤖 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 `@include/DetourModKit/async_logger.hpp` around lines 267 - 270, Convert the
inline comment above the timestamp_format field into a header-compliant Doxygen
comment (use /// or /** */) and change the default initialization to
brace-style; specifically update the member declaration for timestamp_format to
use brace initialization (timestamp_format{"%Y-%m-%d %H:%M:%S"}) and replace the
existing C-style comment with a Doxygen comment that documents that this is a
strftime-style date/time format and that the trailing ".<ms>" is appended by the
writer.

Source: Coding guidelines

🤖 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.

Inline comments:
In `@include/DetourModKit/hook_manager.hpp`:
- Around line 1119-1121: Replace the two leading // comment lines above the
declaration of get_reentrancy_guard() with a proper declaration-level doc
comment (e.g. a Doxygen /** ... */ block or ///< style) placed immediately above
the declaration; the doc comment should state that the accessor is const so
read-only query accessors can consult the guard and that the counter is
thread-local and independent of object state, preserving the same explanatory
text but using the repository's doc-comment style instead of inline // comments.

---

Nitpick comments:
In `@include/DetourModKit/async_logger.hpp`:
- Around line 267-270: Convert the inline comment above the timestamp_format
field into a header-compliant Doxygen comment (use /// or /** */) and change the
default initialization to brace-style; specifically update the member
declaration for timestamp_format to use brace initialization
(timestamp_format{"%Y-%m-%d %H:%M:%S"}) and replace the existing C-style comment
with a Doxygen comment that documents that this is a strftime-style date/time
format and that the trailing ".<ms>" is appended by the writer.
🪄 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: a6102318-a5e7-49fc-8a7c-d8f385757c13

📥 Commits

Reviewing files that changed from the base of the PR and between 322c9d3 and d59617d.

📒 Files selected for processing (22)
  • .github/workflows/coverage-pages.yml
  • .github/workflows/pr-check.yml
  • AGENTS.md
  • docs/tests/README.md
  • include/DetourModKit/async_logger.hpp
  • include/DetourModKit/config.hpp
  • include/DetourModKit/config_watcher.hpp
  • include/DetourModKit/hook_manager.hpp
  • include/DetourModKit/memory.hpp
  • src/async_logger.cpp
  • src/hook_manager.cpp
  • src/input.cpp
  • src/input_intercept.cpp
  • src/input_intercept.hpp
  • src/logger.cpp
  • src/memory.cpp
  • src/profiler.cpp
  • tests/test_async_logger.cpp
  • tests/test_hook_integration.cpp
  • tests/test_input_intercept.cpp
  • tests/test_memory.cpp
  • tests/test_profiler.cpp

Comment thread include/DetourModKit/hook_manager.hpp Outdated
Convert the new get_reentrancy_guard and timestamp_format member comments to
Doxygen doc blocks and brace-initialize timestamp_format, per AGENTS.md.
@tkhquang tkhquang merged commit d683aa9 into main Jun 11, 2026
4 checks passed
@tkhquang tkhquang deleted the fix/audit-remediation-followups branch June 11, 2026 12:46
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