fix: resolve audit findings across cache, hooks, input, and logging#106
Conversation
- 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
|
Warning Review limit reached
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 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 configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis 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. ChangesConcurrency Safety and Robustness Hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
include/DetourModKit/async_logger.hpp (1)
267-270: ⚡ Quick winUse 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
📒 Files selected for processing (22)
.github/workflows/coverage-pages.yml.github/workflows/pr-check.ymlAGENTS.mddocs/tests/README.mdinclude/DetourModKit/async_logger.hppinclude/DetourModKit/config.hppinclude/DetourModKit/config_watcher.hppinclude/DetourModKit/hook_manager.hppinclude/DetourModKit/memory.hppsrc/async_logger.cppsrc/hook_manager.cppsrc/input.cppsrc/input_intercept.cppsrc/input_intercept.hppsrc/logger.cppsrc/memory.cppsrc/profiler.cpptests/test_async_logger.cpptests/test_hook_integration.cpptests/test_input_intercept.cpptests/test_memory.cpptests/test_profiler.cpp
Convert the new get_reentrancy_guard and timestamp_format member comments to Doxygen doc blocks and brace-initialize timestamp_format, per AGENTS.md.
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
invalidate_rangenow evicts multi-page cached regions reliably by scanning shards by containment instead of a single per-page key probe.with_*callback no longer risks recursive shared-lock UB on the non-recursive mutex.ctestfor 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_uncheckedlower-bound difference versusplausible_userspace_ptris intentional and now documented.Summary by CodeRabbit
Bug Fixes
New Features