fix(memory): harden read_ptr_unsafe under ASan, stabilize concurrent guarded-read tests#120
Merged
Conversation
…entry point read_ptr_unsafe kept a private __try/std::memcpy that never gained the safety properties seh_read_bytes did: the low/null source floor, the (src + size) wrap rejection, and the __movsb copy that avoids MSVC routing std::memcpy through the ASan interceptor. Under AddressSanitizer a wrapping source range aborted the process (negative-size-param) before SEH could turn it into a 0 return, and foreign reads were mis-handled by the interceptor. Delegate to seh_read_bytes so a single implementation owns every guarded-read safety property and the two cannot diverge again.
… tests GuardedReadsAreThreadIsolatedUnderConcurrency and GuardedReadsSurviveConcurrentShutdown obtained a non-readable address by MEM_RELEASE'ing a region, then read it from many threads expecting the guard to fault to 0. A released VA can be recycled and remapped by the allocations that spawning those threads triggers (thread stacks/TEBs, more so under AddressSanitizer), so a read could land on live memory and the assertion flaked on about one read in many thousands. Hold a committed PAGE_NOACCESS page until teardown instead: it faults deterministically and cannot be recycled. Document the pitfall in AGENTS.md.
|
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 (3)
📝 WalkthroughWalkthroughThis PR refactors ChangesGuarded-read unification and test stabilization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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}
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
The AddressSanitizer probe was failing two MemoryTest cases. Both are addressed here, and the guard itself is shown correct rather than worked around.
read_ptr_unsafe kept a private __try/std::memcpy that never gained the hardening seh_read_bytes received: the low/null source floor, the (src + size) wrap rejection, and the __movsb copy that bypasses the MSVC ASan memcpy interceptor. Under ASan a wrapping source range aborted the process (negative-size-param) before SEH could turn it into a 0 return, and foreign reads were mis-handled by the interceptor. It now delegates to seh_read_bytes, so a single implementation owns every guarded-read safety property and the two cannot diverge again. The MinGW path is behaviourally unchanged (it already routed through the same fault guard).
The two concurrent guarded-read tests obtained a non-readable address by releasing a region with MEM_RELEASE, then read it from many threads expecting the guard to fault to 0. A released VA can be recycled and remapped by the allocations that spawning the reader threads triggers (thread stacks/TEBs, more so under ASan), so a read could land on live memory and the assertion flaked on roughly one read in many thousands. They now hold a committed PAGE_NOACCESS page until teardown, which faults deterministically and cannot be recycled, so they exercise the guard's per-thread fault isolation without that artifact. AGENTS.md documents the pitfall so it is not reintroduced.
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests