fix: harden scan-path reads and loader-lock teardown#91
Conversation
Harden the scan path and subsystem teardown for the v3.4.0 cut: - scanner: read RIP displacement and prologue bytes via Memory::seh_read instead of is_readable + raw deref, closing a TOCTOU host-crash window during scans - input: skip hold-release callbacks on the loader-lock detach path so user callbacks never run under the loader lock - platform: validate the PEB loader-lock pointer before deref and fail safe - config: register_atomic is now one constrained template (was explicit specializations); logger const accessors are noexcept - bump version to 3.4.0 and retarget the version test
|
Warning Review limit reached
More reviews will be available in 30 minutes and 37 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ 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)
📝 WalkthroughWalkthroughVersion 3.4.0 release introducing template API refinements, logger exception-safety declarations, and defensive memory-read patterns. Loader-lock shutdown and scanner symbol resolution now use fault-guarded reads to prevent races and crashes in Windows loader contexts. ChangesDetourModKit 3.4.0 Release
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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: 2
🧹 Nitpick comments (1)
include/DetourModKit/logger.hpp (1)
122-122: ⚡ Quick winConsider adding
[[nodiscard]]to these status queries.Both
is_async_mode_enabled()(aboolstatus query) andget_log_level()(a status query) discard usefully signal an accidentally-ignored return. Since these declarations are already being touched, adding[[nodiscard]]aligns them with the rest of the public surface.♻️ Proposed change
- bool is_async_mode_enabled() const noexcept; + [[nodiscard]] bool is_async_mode_enabled() const noexcept;- LogLevel get_log_level() const noexcept + [[nodiscard]] LogLevel get_log_level() const noexcept { return current_log_level_.load(std::memory_order_acquire); }As per coding guidelines: "Apply
[[nodiscard]]to factory functions, status queries,boolsuccess/failure returns".Also applies to: 144-147
🤖 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/logger.hpp` at line 122, Mark the status-query functions as [[nodiscard]] so callers cannot silently ignore their return values: add the [[nodiscard]] attribute to the declarations of is_async_mode_enabled() and get_log_level() (and any other status-query/public surface functions noted in the review) in logger.hpp; keep their signatures otherwise (e.g., bool is_async_mode_enabled() const noexcept and the get_log_level() declaration) to align with the coding guideline for factory/status-query returns.
🤖 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 `@src/input.cpp`:
- Around line 297-311: The shutdown path detaches poll_thread_ under
is_loader_lock_held() but may destroy the InputPoller while the detached
poll_loop() still accesses members (cv_mutex_, cv_, poll_interval_), causing a
use-after-free; update InputManager::shutdown() so when local_poller->shutdown()
runs while is_loader_lock_held() is true you intentionally keep the InputPoller
alive (e.g., move local_poller into a static container) instead of letting it be
destroyed, or centralize the is_loader_lock_held() check into
InputManager::shutdown() to leak the shared_ptr and prevent destruction until
process exit, ensuring the detached thread cannot access freed members.
In `@tests/test_version.cpp`:
- Around line 9-33: The tests in VersionTest are plain TEST cases but must be
converted to a fixture-based suite: create a class VersionTest : public
::testing::Test with public SetUp() and TearDown() (can be empty or used for
cleanup), change each TEST(VersionTest, ...) to TEST_F(VersionTest, ...) and
keep the same test bodies that assert DMK_VERSION_MAJOR/MINOR/PATCH,
DMK_VERSION_STRING and DMK_VERSION_AT_LEAST checks; ensure the fixture class
name is exactly VersionTest so the TEST_F references resolve.
---
Nitpick comments:
In `@include/DetourModKit/logger.hpp`:
- Line 122: Mark the status-query functions as [[nodiscard]] so callers cannot
silently ignore their return values: add the [[nodiscard]] attribute to the
declarations of is_async_mode_enabled() and get_log_level() (and any other
status-query/public surface functions noted in the review) in logger.hpp; keep
their signatures otherwise (e.g., bool is_async_mode_enabled() const noexcept
and the get_log_level() declaration) to align with the coding guideline for
factory/status-query returns.
🪄 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: f94e9b83-06c4-4d5f-a339-285591e85b69
📒 Files selected for processing (8)
CMakeLists.txtinclude/DetourModKit/config.hppinclude/DetourModKit/logger.hppsrc/input.cppsrc/logger.cppsrc/platform.hppsrc/scanner.cpptests/test_version.cpp
…lock The loader-lock detach path detaches the poll jthread, but InputManager::shutdown() then dropped the last reference and freed InputPoller members the detached poll_loop still reads (use-after-free). Leak the poller into a nothrow heap cell under loader lock so it outlives the detached thread, matching the leak-on-loader-lock discipline used for Logger and ConfigWatcher. Also mark the const Logger status queries (get_log_level, is_async_mode_enabled, is_enabled) [[nodiscard]].
1 participant
Summary
Hardening pass for the v3.4.0 cut. Strictly additive on v3.3.0; no public API breaks.
is_likely_function_prologuenow go throughMemory::seh_readinstead ofis_readable+ a raw dereference, closing a TOCTOU window where a page could unmap/reprotect between the check and the read and fault the host during a scan.shutdown()no longer callsrelease_active_holds(), so user hold-release callbacks never fire under the loader lock (deadlock risk duringFreeLibrary). The join path still releases, race-free.is_loader_lock_held()validates the PEB loader-lockCRITICAL_SECTIONpointer is committed/readable before dereferencing it, and fails safe to "held" on any uncertainty (a wrongfalsewould deadlock the loader on unload).register_atomicis now a single constrained template (requires+if constexpr) instead of in-class explicit specializations; sameint/bool/floatsupport, standard-conforming.get_log_level()andis_async_mode_enabled()are nownoexcept.3.4.0inCMakeLists.txtand retargettests/test_version.cpp.Summary by CodeRabbit
Chores
Bug Fixes
Improvements