feat: add MSVC RTTI walker and module-range helpers#85
Merged
Conversation
- DetourModKit::Rtti: type_name_of, type_name_into, vtable_is_type, find_in_pointer_table (with optional caller-owned vtable cache and stride). - DetourModKit::Memory additions: ModuleRange, module_range_for, own_module_range, host_module_range, contains, seh_read<T>, seh_read_bytes. - 62 new tests using a synthetic MSVC RTTI fixture so the walker is validated under both MSVC and MinGW. - Docs: docs/misc/rtti-walker.md plus README and AGENTS updates.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR introduces a complete MSVC RTTI introspection module with foundational SEH-safe memory utilities. The changes add RTTI-aware vtable type name reading, module range queries, and pointer-table scanning via noexcept APIs that return null/false/0 on failure rather than throwing exceptions. ChangesMSVC RTTI Introspection Module with Memory Safety
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/rtti.cpp (1)
18-18: ⚡ Quick winRemove the file-wide
using namespacedirective.Please qualify the DetourModKit symbols here, or use narrow
using DetourModKit::Memory;/using DetourModKit::Rtti;declarations instead. As per coding guidelines,**/*.cpp: Do not addusing namespacedirectives in any file. In implementation files, they are not needed if the header already declares the namespace.🤖 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 `@src/rtti.cpp` at line 18, The file-wide "using namespace DetourModKit;" in rtti.cpp must be removed; instead qualify references to DetourModKit types/functions (e.g., change bare Memory, Rtti, or any other symbols to DetourModKit::Memory, DetourModKit::Rtti) or add narrow using declarations like "using DetourModKit::Memory;" for any frequently used single symbols; update all occurrences in rtti.cpp accordingly so no global using namespace directive remains.tests/test_rtti.cpp (1)
17-17: ⚡ Quick winRemove the file-wide
using namespacedirective.Please qualify the DetourModKit symbols in this test or add narrow
usingdeclarations for the specific namespaces you need. As per coding guidelines,**/*.cpp: Do not addusing namespacedirectives in any file. In implementation files, they are not needed if the header already declares the namespace.🤖 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 `@tests/test_rtti.cpp` at line 17, Remove the file-wide "using namespace DetourModKit;" from tests/test_rtti.cpp; instead qualify all DetourModKit symbols with the DetourModKit:: prefix or add narrow using declarations (e.g., using DetourModKit::Foo; using DetourModKit::Bar;) for only the specific types/functions used in this test so no global namespace import remains.
🤖 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 `@README.md`:
- Line 155: The sentence claiming "failures produce `std::nullopt`" is
misleading because not all RTTI APIs return optionals; update the README
sentence to list the actual return semantics per API (e.g., entry points are
noexcept and SEH-guarded; unreadable pages, missing COLs, and zero RVAs will not
fault but will return `std::nullopt` for functions that return std::optional,
`false` for boolean APIs such as `vtable_is_type`, and `0` for size/length APIs
such as `type_name_into`), and mention these specific symbols (`vtable_is_type`,
`type_name_into`) so readers know which functions map to which return values.
In `@src/rtti.cpp`:
- Around line 81-112: The code trusts COL fields (p_self, p_type_descriptor) and
computes name_addr = image_base + head_opt->p_type_descriptor without validating
that image_base/name_addr lie inside the module that owns vtable; fix by
querying the owning module's base and size (e.g., via GetModuleHandleExW +
GetModuleInformation/VirtualQuery) and reject (return 0) if either the computed
image_base or the final name_addr (image_base + head_opt->p_type_descriptor +
TD_NAME_OFFSET) is outside that module's [base, base+size) range; apply the same
bounds check for the p_self recovery path (COL_SIGNATURE_X64) and add a
regression test that supplies a bogus p_type_descriptor/p_self RVA to ensure
type_name_of() fails rather than reading from another mapped region.
- Around line 122-155: read_name_seh currently copies each successful chunk
straight into out and if a later Memory::seh_read_bytes() fails before a NUL is
found it returns the truncated prefix (contradicting callers' failure contract).
Change the function to accumulate bytes into a temporary buffer (e.g.
std::vector<char> tmp or a fixed-size stack buffer with offset) instead of
memcpy'ing directly into out; only memcpy the accumulated data into out and set
out[written] = '\0' when a NUL is encountered or the full allowed length is
read; if any chunk read fails before a NUL is seen, clear out[0] = '\0' and
return 0. Keep the existing loop logic, PAGE_MASK calculations and variables
(addr, written, max_chars, buf, this_read, copy_len, nul) but defer writing to
out until success is confirmed.
---
Nitpick comments:
In `@src/rtti.cpp`:
- Line 18: The file-wide "using namespace DetourModKit;" in rtti.cpp must be
removed; instead qualify references to DetourModKit types/functions (e.g.,
change bare Memory, Rtti, or any other symbols to DetourModKit::Memory,
DetourModKit::Rtti) or add narrow using declarations like "using
DetourModKit::Memory;" for any frequently used single symbols; update all
occurrences in rtti.cpp accordingly so no global using namespace directive
remains.
In `@tests/test_rtti.cpp`:
- Line 17: Remove the file-wide "using namespace DetourModKit;" from
tests/test_rtti.cpp; instead qualify all DetourModKit symbols with the
DetourModKit:: prefix or add narrow using declarations (e.g., using
DetourModKit::Foo; using DetourModKit::Bar;) for only the specific
types/functions used in this test so no global namespace import remains.
🪄 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: a367701c-866b-4eac-933f-ecd4ecc78c15
📒 Files selected for processing (10)
AGENTS.mdREADME.mddocs/misc/rtti-walker.mdinclude/DetourModKit.hppinclude/DetourModKit/memory.hppinclude/DetourModKit/rtti.hppsrc/memory.cppsrc/rtti.cpptests/test_memory.cpptests/test_rtti.cpp
…ss reads
Reviewer findings:
- resolve_name_site now anchors every COL-derived address (col pointer,
pSelf-recovered image base, computed name buffer) against the vtable's
owning module range via Memory::module_range_for + Memory::contains. A
forged COL can no longer redirect the walker into another loaded module
or unmapped memory.
- read_name_seh accumulates into a stack-local temporary and only memcpys
into the caller buffer when a NUL is seen or accum_cap is filled. A read
fault before either condition now reports clean failure (out NUL-empty,
return 0) instead of a truncated prefix.
- Drop file-wide using namespace from rtti.cpp; wrap definitions in
namespace DetourModKit { ... } so Memory:: and Rtti:: resolve without
the directive.
- Drop file-wide using namespace from test_rtti.cpp; replace with narrow
namespace aliases for Memory and Rtti.
- Move SyntheticVtable storage from heap to a pool inside the test exe's
data segment so module_range_for resolves the test exe's PE range
(required by the new bound-check guard).
- README: rewrite the RTTI return-semantics sentence to list the actual
failure return per API.
New regression tests: ResolveRejectsPoisonedTypeDescriptorRva,
ResolveRejectsPoisonedSelfRva, ResolveRejectsHeapAllocatedVtable.
AGENTS.md requires every closing namespace brace to carry a trailing comment. Two anonymous namespaces added in this branch (seh_read internals and module-range internals) were missing their markers; aligns them with the existing convention (e.g. memory.cpp:826 "anonymous namespace (cache internals)").
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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
DetourModKit::Rttimodule that walks MSVC x64 RTTI from a runtime vtable (vtable[-1] -> COL -> TypeDescriptor) to recover the mangled type name withouttypeidordynamic_cast. Image base is recovered viaCOL.pSelf(canonical IDA/Ghidra approach), withGetModuleHandleExWonly as a fallback. Entry points:type_name_of,type_name_into(zero-alloc),vtable_is_type,find_in_pointer_table(optional caller-owned vtable cache, configurable stride).DetourModKit::Memoryextended with typed SEH-guarded reads (seh_read<T>,seh_read_bytes) and PE module-range queries (ModuleRange,module_range_for,own_module_range,host_module_range,contains). MSVC uses a single__tryframe; MinGW falls back to per-regionVirtualQuery.RttiTest, 29MemoryTest) using a synthetic MSVC RTTI fixture so the walker is validated under both toolchains.docs/misc/rtti-walker.md(ABI layout, usage patterns, perf notes) plus README and AGENTS updates.Test plan
ConfigTestordering failures unrelated to this change excluded)Summary by CodeRabbit
New Features
Documentation
Tests