Repository navigation
a frame keeps its context and its pixels past the next grab - #69
Conversation
Frame and Cursor held a raw copy of the drmtap_ctx pointer with no lifetime, so safe code could drop the DrmTap first; drmtap_close freed the context and Frame::drop then read drm_fd from freed memory in drmtap_gem_close. Under asan on i915: heap-use-after-free, READ of size 4 at drm_grab.c:77, freed by drmtap_close. The context now lives in an Arc that DrmTap, Frame and Cursor share, and drmtap_close runs when the last one drops. No signature changes, and the auto traits are unchanged: DrmTap is Send, UnwindSafe and RefUnwindSafe but not Sync, Frame and Cursor are neither Send nor Sync. The new hardware test runs clean under asan. Dropping the last Frame or Cursor now runs the close, which on the privilege-helper path stops the helper; the docs and the changelog say so. Fixes #63
A converted frame had its pixels in the context's conversion buffer, and a frame from the helper's pixel path in the context's receive buffer, so the next grab overwrote them and a larger frame freed and reallocated that memory under Frame::data(). On i915 two live frames from grab_mapped returned the same pointer, and a frame grabbed before a switch to a bigger mode was read from freed memory (asan). The wrapper now points the conversion at a buffer the frame owns, through drmtap_set_output_buffer, clears it right after the grab, and copies pixels that are still the context's into one. A dropped frame leaves its buffer to the next grab, so a consumer that drops one frame per grab does not allocate. Buffers start zeroed, and a frame that reports more bytes than it was lent is released with EOVERFLOW instead of being read. A frame mapped straight from the scanout is still a view of it, not a copy. The new C function drmtap_frame_owns_data tells whether frame->data is memory the frame releases. On i915 (Meteor Lake, EGL detile, as root) grab_mapped costs what 0.5.10 did within 0.1 ms at 1920x1080 and 3840x2160. With glibc forced to map every allocation fresh, a new buffer per grab cost 7.6 and 29.4 ms, the reused one 4.7 and 12.1. The process keeps one frame more resident. Fixes #68
drmtap_deswizzle copies 4 bytes per pixel and never checked that a row fits dst_stride. On a 2-byte geometry (60 pixels on a 120-byte stride, dst sized dst_stride * height) the linear, X-tiled and Y-tiled paths all wrote past dst: heap-buffer-overflow under asan. A layout it decodes now returns -EINVAL before writing; one it cannot decode still returns -ENOTSUP. The header now says what each row gets: width * 4 bytes, the rest of the row left as it was, which a caller writing into a rectangle of a wider image relies on. The stride test goes red without any one of the four checks, and the padding test on a write past width * 4.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 48 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (6)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (7)
|
| Layer / File(s) | Summary |
|---|---|
Frame-data ownership query include/drmtap.h, src/drm_grab.c, bindings/rust/libdrmtap-sys/csrc/drmtap.h, bindings/rust/libdrmtap-sys/csrc/drm_grab.c, bindings/rust/libdrmtap-sys/src/lib.rs |
The C API and Rust FFI expose drmtap_frame_owns_data(). It reports whether frame data matches the frame’s private mapped buffer. |
Shared context and frame-owned pixels bindings/rust/libdrmtap/src/lib.rs, bindings/rust/libdrmtap/README.md, CHANGELOG.md |
DrmTap, Frame, and Cursor share the context through Arc<Ctx>. The wrapper copies context-owned pixels into reusable frame buffers and retains each handle’s context. Tests cover pixel independence, handle lifetime, and spare-buffer reuse. |
Deswizzle destination stride
| Layer / File(s) | Summary |
|---|---|
Row-fit checks and API contract src/pixel_convert.c, bindings/rust/libdrmtap-sys/csrc/pixel_convert.c, include/drmtap.h, bindings/rust/libdrmtap-sys/csrc/drmtap.h, CHANGELOG.md |
Supported deswizzle paths return -EINVAL when dst_stride is narrower than width * 4. The API documentation describes row placement and preserved padding. |
Error reporting and stride tests src/drm_grab.c, bindings/rust/libdrmtap-sys/csrc/drm_grab.c, tests/test_deswizzle.c |
CPU deswizzle callers report the format, width, and stride for narrow rows. Tests check that padding remains unchanged, narrow destinations are not modified, and unsupported layouts return -ENOTSUP. |
Priority: ➖ Normal
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix · Severity of issue fixed: Medium
Merge Risk: ⚪ Minimal · up to 1cd19
This change keeps Rust frames and cursors valid after the capture context is dropped. It also keeps frame pixels intact across later grabs, and it makes undersized deswizzle destinations fail with an error instead of overrunning. No concrete merge-blocking issue was found in the supplied review.
Security Architecture Review
Security architecture risk: 🔵 Low · up to 1cd19
The change strengthens captured-data lifetime and rejects undersized output rows. No introduced security issue was established. Remaining uncertainty concerns hardware-dependent failure behavior and privileged-resource shutdown, which now waits for the last retained handle.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
- inferred — The demonstrated change surface is local capture-process memory and retained device/helper resources. The ownership query only inspects a valid captured frame; it adds no capture operation or privilege grant. The inspected capture authority checks are unchanged in the full PR comparison.
Trust Boundaries and Controls
- observed — Pixels received through the helper's reused buffer are classified as borrowed and copied by the Rust wrapper. The existing exact-payload-size check rejects incomplete helper frames before their advertised pixel extent is exposed.
Resilience and Maintainability Implications
- inferred — Context retention and release-before-reuse ordering address the prior dangling-context and borrowed-pixel lifetime conditions. Source inspection supports normal failure and retry containment, but does not establish behavior under forced interruption or hardware-dependent concurrency stress.
🚥 Pre-merge checks | ✅ 3 | ❌ 2
❌ Failed checks (2 warnings)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Out of Scope Changes check | The drmtap_deswizzle stride checks and their tests address a destination-row overrun, not the context lifetime in [#63] or frame-data lifetime and aliasing in [#68]. The summary also reports removal… |
Move the stride-validation and Nvidia deswizzle changes, including related tests and documentation, to a separate change or link an applicable issue. Keep this PR focused on [#63] and [#68]. |
|
| Docstring Coverage | Docstring coverage is 67.65% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 9 files. (2 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title summarizes the main change: frames retain their context and pixels across later grabs. |
| Description check | ✅ Passed | The description explains the context-lifetime fix, frame-pixel ownership, deswizzle validation, and reported performance. |
| Linked Issues check | ✅ Passed | [#63] DrmTap, Frame, and Cursor share an Arc<Ctx>, so the context closes after the last handle drops and the public Rust signatures remain unchanged. The summary reports tests for a frame outl… |
Full details: Out of Scope Changes check
Explanation
The drmtap_deswizzle stride checks and their tests address a destination-row overrun, not the context lifetime in [#63] or frame-data lifetime and aliasing in [#68]. The summary also reports removal of the Nvidia block-linear CPU deswizzler, which changes format support without connection to either linked issue.
Full details: Docstring Coverage
Explanation
Docstring coverage is 67.65% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 34 functions across 9 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks 💡
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
- Commit to this branch
- Create a new PR
- Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
I checked each frame beside the stream
And kept its pixels safe from overwrite
I hopped through rows with careful stride
No padding byte was brushed aside
The context stayed until handles slept
Then off I bounced, content and kept
Comment @coderabbitai help to get the list of available commands.
…t grab Stamps 0.5.11 across the six version sites and dates the changelog section. The wrapper now needs libdrmtap-sys 0.5.11, the first one with drmtap_frame_owns_data.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
fixes #63, fixes #68
1- c538d14: a
FrameorCursorholds the context, so dropping theDrmTapfirst no longer frees what their release reads (asan on i915: heap-use-after-free before, clean after)2- 6fbb7bb:
grab_mappedlends the conversion a buffer the frame owns and copies pixels that stay in context memory into one. two live frames no longer share a pointer, and a frame survives a bigger mode: hdmi-a-2 1280x1024 -> 1920x1080 between two grabs was a heap-use-after-free on 0.5.10 and is clean now. a dropped frame leaves its buffer to the next grab. new c functiondrmtap_frame_owns_data3- 1cd197a:
drmtap_deswizzlereturns -EINVAL when a row of 4-byte pixels does not fitdst_stride(on a 2-byte geometry the linear, x and y paths all wrote past dst under asan). a layout it cannot decode still returns -ENOTSUP, and the rest of each row is still left aloneon i915 meteor lake (compressed scanout, egl detile)
grab_mappedcosts what 0.5.10 did within 0.1 ms, 4.6-4.8 ms at 1080p and 12.0-12.2 ms at 4k, and keeps one frame more resident. no public signature changes.