marfrit/libva-v4l2-request-fourier
1
0
Fork 1
Code Issues 2 Pull Requests Actions Packages Projects Releases Wiki Activity

picture: bounds-check codec_store_buffer slice writes against source_size (#13) #14

Merged
marfrit merged 1 commits from claude-noether/libva-v4l2-request-fourier:noether/codec-store-buffer-bounds-check-13 into master 2026-05-21 10:17:16 +00:00
Conversation 0 Commits 1 Files Changed 1
+43 -7
marfrit commented 2026-05-21 10:16:37 +00:00
Owner
Copy Link
Copy Source

Fixes the unbounded memcpys in codec_store_buffer's VASliceDataBufferType branch reported in #13.

What changes

Three append sites in src/picture.c now check slices_size + needed <= source_size before writing:

  1. H.264 Annex-B start code (3 bytes)
  2. VP8 uncompressed-header pad (3 or 10 bytes)
  3. The slice payload itself (buffer_object->size * buffer_object->count)

If any check fails, RenderPicture returns VA_STATUS_ERROR_ALLOCATION_FAILED and request_log records the over-budget request. The slices_size / slices_count state is left consistent (we only advance after the successful copy), so the surface is recoverable; libavcodec re-creates the surface at the new resolution on the next BeginPicture.

Why this is sufficient (and why it's only the floor)

source_data is the mmap of an OUTPUT pool slot whose size is set by v4l2_query_buffer at request_pool_init time — i.e. the kernel driver's negotiated sizeimage from S_FMT. That number is fixed for the life of the pool, so an SPS-driven resolution upshift mid-stream that produces a slice larger than the pool slot's sizeimage will overflow.

Issue #13 also suggests a better-fix track: re-create surfaces (and re-S_FMT / re-init the OUTPUT pool) on resolution change, or grow source_data on demand. Both are larger refactors; this PR is the memory-safety floor that should land first.

Notes on the C scoping

Wrapped the case VASliceDataBufferType: body in a { ... } block to allow declaring size_t cap = surface_object->source_size; and size_t need; at the top of the case without C90 declaration-after-statement noise. No behavioural change for the other case arms.

Test plan

  • Build via Gitea Actions (libva-v4l2-request-fourier-aarch64 + -debian jobs).
  • On higgs: confirm mpv --hwdec=vaapi-copy bbb_1080p30_h264.mp4 against a 720p-default daedalus session no longer SIGSEGVs in memcpy, and instead surfaces a clean VA_STATUS_ERROR_ALLOCATION_FAILED to libavcodec; request_log shows the would overflow OUTPUT buffer line with the over-budget delta.
  • Regress-check the 5 in-spec codecs (rkvdec H.264 / HEVC / VP9, hantro MPEG-2 / VP8) on fresnel — slices_size + payload <= source_size should hold trivially for any session that wasn't already on the verge of corruption.
Fixes the unbounded `memcpy`s in `codec_store_buffer`'s `VASliceDataBufferType` branch reported in #13. ## What changes Three append sites in `src/picture.c` now check `slices_size + needed <= source_size` before writing: 1. H.264 Annex-B start code (3 bytes) 2. VP8 uncompressed-header pad (3 or 10 bytes) 3. The slice payload itself (`buffer_object->size * buffer_object->count`) If any check fails, `RenderPicture` returns `VA_STATUS_ERROR_ALLOCATION_FAILED` and `request_log` records the over-budget request. The `slices_size` / `slices_count` state is left consistent (we only advance after the successful copy), so the surface is recoverable; libavcodec re-creates the surface at the new resolution on the next `BeginPicture`. ## Why this is sufficient (and why it's only the floor) `source_data` is the mmap of an OUTPUT pool slot whose size is set by `v4l2_query_buffer` at `request_pool_init` time — i.e. the kernel driver's negotiated `sizeimage` from `S_FMT`. That number is fixed for the life of the pool, so an SPS-driven resolution upshift mid-stream that produces a slice larger than the pool slot's `sizeimage` will overflow. Issue #13 also suggests a better-fix track: re-create surfaces (and re-`S_FMT` / re-init the OUTPUT pool) on resolution change, or grow `source_data` on demand. Both are larger refactors; this PR is the memory-safety floor that should land first. ## Notes on the C scoping Wrapped the `case VASliceDataBufferType:` body in a `{ ... }` block to allow declaring `size_t cap = surface_object->source_size;` and `size_t need;` at the top of the case without C90 declaration-after-statement noise. No behavioural change for the other `case` arms. ## Test plan - [ ] Build via Gitea Actions (`libva-v4l2-request-fourier-aarch64` + `-debian` jobs). - [ ] On higgs: confirm `mpv --hwdec=vaapi-copy bbb_1080p30_h264.mp4` against a 720p-default daedalus session no longer SIGSEGVs in `memcpy`, and instead surfaces a clean `VA_STATUS_ERROR_ALLOCATION_FAILED` to libavcodec; `request_log` shows the `would overflow OUTPUT buffer` line with the over-budget delta. - [ ] Regress-check the 5 in-spec codecs (rkvdec H.264 / HEVC / VP9, hantro MPEG-2 / VP8) on fresnel — `slices_size + payload <= source_size` should hold trivially for any session that wasn't already on the verge of corruption.
marfrit added 1 commit 2026-05-21 10:16:37 +00:00
picture: bounds-check codec_store_buffer slice writes against source_size bfcb286031 
surface_object->source_data points at an OUTPUT-pool mmap of fixed
size source_size, negotiated by v4l2_query_buffer at request_pool_init
time (kernel sizeimage at S_FMT).  codec_store_buffer's
VASliceDataBufferType branch appended to it at three sites (H.264 Annex-B
start code, VP8 uncompressed-header pad, slice payload) without
consulting that capacity — a stream-level resolution upshift would walk
past the mmap and SIGSEGV inside the memcpy (mpv --hwdec=vaapi-copy on
the daedalus path, issue #13) or corrupt adjacent heap (Firefox RDD).

Add a check at each append site that fails the RenderPicture call with
VA_STATUS_ERROR_ALLOCATION_FAILED when slices_size+payload exceeds
source_size, and logs the over-budget request for postmortem.
libavcodec recreates the surface at the new dimensions on the next
BeginPicture, so a refused upshift slice is recoverable.

Doesn't address the root cause (surfaces should be re-created on
resolution change, or source_data should be grown on demand) but
removes the memory-safety hazard while the larger refactor waits.

Closes marfrit/libva-v4l2-request-fourier#13.
marfrit merged commit 2860d75afe into master 2026-05-21 10:17:16 +00:00
marfrit deleted branch noether/codec-store-buffer-bounds-check-13 2026-05-21 10:17:16 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
EWONTFIX
Triaged 2026-05-18: scoped out indefinitely; reopen-able if real need emerges. (HEVC-on-ampere first batch.)
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claude-noether (Claude (noether)) marfrit (Markus Fritsche)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: marfrit/libva-v4l2-request-fourier#14
Delete Branch "claude-noether/libva-v4l2-request-fourier:noether/codec-store-buffer-bounds-check-13"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?