fix(screenshot): bounds-check staging texture mapped region#25
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
No actionable suggestions for changed features. |
|
✅ A pre-release build is available for this PR: |
|
Seems functional; unlikely to get a tester so will merge as soon as ci confirmed stable. |
alandtse
force-pushed
the
fix/screenshot-staging-memcpy
branch
2 times, most recently
from
451c7c6 to
fb800fc
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens the screenshot worker’s CPU readback path to prevent out-of-bounds reads when copying from a mapped D3D11 staging texture into a DirectXTex ScratchImage, addressing an access violation reported in ScreenshotFeature.cpp.
Changes:
- Adds validation for
Map()results (nullpData/ zeroRowPitch) before proceeding. - Computes a safe row count based on mapped region sizing (
DepthPitch/RowPitch) and clamps per-row copy size to the smaller of source/destination pitches. - Refactors the copy loop to use a cached source pointer and bounded
memcpy.
Two Copilot findings on PR #25: - ScratchImage::Initialize2D leaves the pixel buffer uninitialized. When the driver mapped a short region (rowsToCopy < height or bytesPerRow < destImage->rowPitch) the unfilled bytes encoded as whatever was on the heap, producing nondeterministic/corrupted screenshots from SaveToWICFile. Zero-fill the pixel buffer up front so any uncopied bytes encode as deterministic black. - File used std::min via transitive include — make the dependency explicit by adding <algorithm>. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two Copilot findings on PR #25: - ScratchImage::Initialize2D leaves the pixel buffer uninitialized. When the driver mapped a short region (rowsToCopy < height or bytesPerRow < destImage->rowPitch) the unfilled bytes encoded as whatever was on the heap, producing nondeterministic/corrupted screenshots from SaveToWICFile. Zero-fill the pixel buffer up front so any uncopied bytes encode as deterministic black. - File used std::min via transitive include — make the dependency explicit by adding <algorithm>. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
alandtse
force-pushed
the
fix/screenshot-staging-memcpy
branch
from
9b94c67 to
7f357eb
Compare
Worker crashed inside rep movsb at line 84 of ScreenshotFeature.cpp when memcpy ran past the driver-mapped region of a staging texture (crash 2026-05-19 01:20:48, fault at source +0x0FFF page boundary on a 2560-wide R8G8B8A8 capture). The previous loop assumed mapped.RowPitch * destImage->height covered the full subresource and that destImage->rowPitch never exceeded mapped.RowPitch. Drivers can return a smaller mapped region (alignment quirks, partial subresource exposure) or smaller row pitch than DirectXTex computes for the destination. Cap rows by mapped.DepthPitch / mapped.RowPitch (falling back to the old assumption only when DepthPitch is zero), copy the smaller of the two row pitches, and reject the call if Map succeeded with a null pData or zero RowPitch.
Empty commit to trigger CodeQL on this PR. The default-setup CodeQL workflow only fires on synchronize events, not reopened, so a push is required to gate the PR on the new code scan. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Two Copilot findings on PR #25: - ScratchImage::Initialize2D leaves the pixel buffer uninitialized. When the driver mapped a short region (rowsToCopy < height or bytesPerRow < destImage->rowPitch) the unfilled bytes encoded as whatever was on the heap, producing nondeterministic/corrupted screenshots from SaveToWICFile. Zero-fill the pixel buffer up front so any uncopied bytes encode as deterministic black. - File used std::min via transitive include — make the dependency explicit by adding <algorithm>. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
alandtse
force-pushed
the
fix/screenshot-staging-memcpy
branch
from
7f357eb to
4ec9de2
Compare
Summary
Crash 2026-05-19 01:20:48 —
EXCEPTION_ACCESS_VIOLATIONinsiderep movsbatScreenshotFeature.cpp:84. The worker thread's per-rowmemcpyran past the driver-mapped region of the staging texture; the source pointer hit unreadable memory at a page boundary (0x7FFA329B3DFF).Root cause
PopulateScratchImageFromStagingTextureassumed:mapped.RowPitch * destImage->heightalways covered the full mapped subresource.destImage->rowPitchwas always<= mapped.RowPitch.Neither is guaranteed. Drivers can return a smaller mapped region for alignment reasons, and DirectXTex can compute a destination row pitch that exceeds the source pitch on some formats. Either case made the inner
memcpystep past valid memory.Fix
Map()returns success with a nullpDataor zeroRowPitch.mapped.DepthPitch / mapped.RowPitch(falling back to the prior assumption only whenDepthPitch == 0).destImage->rowPitchandmapped.RowPitchper row.No-op for the common case where the driver returns expected values.
Test plan