From d4022f4f517b1da5ace565e736b4efe77a9af81c Mon Sep 17 00:00:00 2001 From: WyattBlue Date: Sat, 24 Oct 2026 22:06:43 -0400 Subject: [PATCH 1/2] Fix segfaults and use-after-frees - `demux()`/`decode()` resumed after the container closed read from the freed format context; check the container is open on every iteration - A Python file whose `read(n)` returned more than `n` bytes overflowed FFmpeg's I/O buffer; raise `ValueError` instead - `Packet(bytearray)` referenced the bytearray without a buffer export, so resizing it left the packet reading freed memory; only take the no-copy path for immutable `bytes` and `str` - `Dictionary` iteration held an entry pointer that inserts invalidated; snapshot the keys first - `VideoBlockParams` didn't keep its `VideoEncParams` alive - `HWConfig` and the side data wrappers checked their sentinel in `__init__`, so `__new__` left a NULL pointer; check in `__cinit__` Also type `Packet(input)` and `Dictionary(...)` to match what they accept. --- av/buffer.py | 7 +++---- av/codec/hwaccel.py | 2 +- av/container/input.py | 5 +++++ av/container/pyio.py | 4 ++++ av/dictionary.py | 6 +++++- av/dictionary.pyi | 3 +++ av/packet.pyi | 5 ++++- av/sidedata/encparams.pxd | 1 + av/sidedata/encparams.py | 4 +++- av/sidedata/motionvectors.py | 6 +++--- av/sidedata/sidedata.py | 2 +- tests/test_decode.py | 20 ++++++++++++++++++-- tests/test_dictionary.py | 9 +++++++++ tests/test_open.py | 10 ++++++++++ tests/test_packet.py | 12 ++++++++++++ tests/test_python_io.py | 9 +++++++++ 16 files changed, 91 insertions(+), 14 deletions(-) diff --git a/av/buffer.py b/av/buffer.py index 3fefceee4..31fe997dc 100644 --- a/av/buffer.py +++ b/av/buffer.py @@ -15,11 +15,10 @@ class ByteSource: def __cinit__(self, owner): self.owner = owner - try: + # Only immutable objects may be referenced without a buffer export; a + # bytearray could be resized, freeing the memory self.ptr points to. + if isinstance(owner, (bytes, str)): self.ptr = owner - except TypeError: - pass - else: self.length = len(owner) return diff --git a/av/codec/hwaccel.py b/av/codec/hwaccel.py index aaca6fb87..5aaf8ac3e 100644 --- a/av/codec/hwaccel.py +++ b/av/codec/hwaccel.py @@ -56,7 +56,7 @@ def wrap_hwconfig(ptr: cython.pointer[cython.const[lib.AVCodecHWConfig]]) -> HWC @cython.final @cython.cclass class HWConfig: - def __init__(self, sentinel): + def __cinit__(self, sentinel): if sentinel is not _cinit_sentinel: raise RuntimeError("Cannot instantiate CodecContext") diff --git a/av/container/input.py b/av/container/input.py index c3070c58f..c0d8deca3 100644 --- a/av/container/input.py +++ b/av/container/input.py @@ -199,6 +199,10 @@ def demux(self, *args, **kwargs): raise MemoryError("Could not allocate packet") while True: + # The container may have been closed while this generator was + # suspended at a yield. + self._assert_open() + # Reset the read buffer with cython.nogil: lib.av_packet_unref(read_packet) @@ -231,6 +235,7 @@ def demux(self, *args, **kwargs): # Flush! for i in range(nb_streams): + self._assert_open() if include_stream[i]: packet = Packet() packet._stream = self.streams[i] diff --git a/av/container/pyio.py b/av/container/pyio.py index 0f7af6eb7..de9ea52dc 100644 --- a/av/container/pyio.py +++ b/av/container/pyio.py @@ -103,6 +103,10 @@ def pyio_read_gil(opaque: cython.p_void, buf: Buf, buf_size: cython.int) -> cyth try: self = cython.cast(PyIOFile, opaque) res = self.fread(buf_size) + if len(res) > buf_size: + raise ValueError( + f"read({buf_size}) returned {len(res)} bytes; expected at most {buf_size}" + ) memcpy( buf, cython.cast(cython.p_void, cython.cast(cython.p_char, res)), len(res) ) diff --git a/av/dictionary.py b/av/dictionary.py index c9bd76acb..2b33707db 100644 --- a/av/dictionary.py +++ b/av/dictionary.py @@ -33,12 +33,16 @@ def __len__(self): return err_check(lib.av_dict_count(self.ptr)) def __iter__(self): + # Collect the keys up front: setting or deleting an item while a lazy + # iterator holds an entry pointer would leave it dangling. + keys: list = [] element: cython.pointer[lib.AVDictionaryEntry] = cython.NULL while True: element = lib.av_dict_get(self.ptr, "", element, lib.AV_DICT_IGNORE_SUFFIX) if element == cython.NULL: break - yield element.key + keys.append(element.key) + return iter(keys) def __repr__(self): return f"av.Dictionary({dict(self)!r})" diff --git a/av/dictionary.pyi b/av/dictionary.pyi index 5f67d1f88..818f0b7e1 100644 --- a/av/dictionary.pyi +++ b/av/dictionary.pyi @@ -3,6 +3,9 @@ from typing import final @final class Dictionary: + def __init__( + self, *args: Mapping[str, str] | Iterable[tuple[str, str]], **kwargs: str + ) -> None: ... def __getitem__(self, key: str, /) -> str: ... def __setitem__(self, key: str, value: str, /) -> None: ... def __delitem__(self, key: str, /) -> None: ... diff --git a/av/packet.pyi b/av/packet.pyi index 0d8116d65..4887d9736 100644 --- a/av/packet.pyi +++ b/av/packet.pyi @@ -1,3 +1,4 @@ +from collections.abc import Buffer as _BufferLike from collections.abc import Iterator from fractions import Fraction from typing import Generic, Literal, TypeVar, final, overload @@ -103,7 +104,9 @@ class Packet(Buffer, Generic[StreamT]): is_trusted: bool is_disposable: bool - def __init__(self: Packet[Stream], input: int | bytes | None = None) -> None: ... + def __init__( + self: Packet[Stream], input: int | str | _BufferLike | None = None + ) -> None: ... def rescale_ts(self, time_base: AVRational) -> None: ... # Overloads that return the same type as the stream's decode method diff --git a/av/sidedata/encparams.pxd b/av/sidedata/encparams.pxd index 031b59a42..6869ad00c 100644 --- a/av/sidedata/encparams.pxd +++ b/av/sidedata/encparams.pxd @@ -8,4 +8,5 @@ cdef class VideoEncParams(SideData): cdef class VideoBlockParams: + cdef VideoEncParams parent cdef lib.AVVideoBlockParams *ptr diff --git a/av/sidedata/encparams.py b/av/sidedata/encparams.py index 22d88deb8..ff51db7b6 100644 --- a/av/sidedata/encparams.py +++ b/av/sidedata/encparams.py @@ -133,10 +133,12 @@ def qp_map(self): @cython.final @cython.cclass class VideoBlockParams: - def __init__(self, video_enc_params: VideoEncParams, idx: cython.int) -> None: + def __cinit__(self, video_enc_params: VideoEncParams, idx: cython.int): if idx < 0 or idx >= video_enc_params.nb_blocks: raise ValueError("Expected idx in range [0, nb_blocks)") + # self.ptr points into the parent's side data, so keep it alive. + self.parent = video_enc_params base: cython.pointer[uint8_t] = cython.cast( cython.pointer[uint8_t], video_enc_params.ptr.data ) diff --git a/av/sidedata/motionvectors.py b/av/sidedata/motionvectors.py index f48aded22..15add7a40 100644 --- a/av/sidedata/motionvectors.py +++ b/av/sidedata/motionvectors.py @@ -9,8 +9,8 @@ @cython.final @cython.cclass class MotionVectors(SideData): - def __init__(self, sentinel, frame: Frame, index: cython.int): - SideData.__init__(self, sentinel, frame, index) + def __cinit__(self, sentinel, frame: Frame, index: cython.int): + # SideData.__cinit__ has already run with the same arguments. self._len = self.ptr.size // cython.sizeof(lib.AVMotionVector) def __repr__(self): @@ -73,7 +73,7 @@ class MotionVector: Motion vectors describe the motion of a block of pixels between frames. """ - def __init__(self, sentinel, parent: MotionVectors, index: cython.int): + def __cinit__(self, sentinel, parent: MotionVectors, index: cython.int): if sentinel is not _cinit_bypass_sentinel: raise RuntimeError("cannot manually instantiate MotionVector") self.parent = parent diff --git a/av/sidedata/sidedata.py b/av/sidedata/sidedata.py index 12eff435c..bc1b5d2f4 100644 --- a/av/sidedata/sidedata.py +++ b/av/sidedata/sidedata.py @@ -96,7 +96,7 @@ def get_display_rotation(frame: Frame) -> cython.int: @cython.cclass class SideData(Buffer): - def __init__(self, sentinel, frame: Frame, index: cython.int): + def __cinit__(self, sentinel, frame: Frame, index: cython.int): if sentinel is not _cinit_bypass_sentinel: raise RuntimeError("cannot manually instantiate SideData") self.frame = frame diff --git a/tests/test_decode.py b/tests/test_decode.py index b0265ffca..27aa39486 100644 --- a/tests/test_decode.py +++ b/tests/test_decode.py @@ -1,4 +1,5 @@ import functools +import gc import io import os import pathlib @@ -9,8 +10,10 @@ import pytest import av -from av.sidedata.encparams import VideoEncParams -from av.sidedata.sidedata import Type +from av.codec.hwaccel import HWConfig +from av.sidedata.encparams import VideoBlockParams, VideoEncParams +from av.sidedata.motionvectors import MotionVector, MotionVectors +from av.sidedata.sidedata import SideData, Type from av.subtitles.subtitle import SubtitleSet from .common import TestCase, fate_suite @@ -53,6 +56,16 @@ def make_h264_test_video(path: str) -> None: output_container.close() +@pytest.mark.parametrize( + "cls", + [HWConfig, SideData, MotionVectors, MotionVector, VideoEncParams, VideoBlockParams], +) +def test_wrapper_cannot_be_created_empty(cls: type[object]) -> None: + # Skipping the constructor used to leave a NULL pointer that getters read. + with pytest.raises(TypeError): + cls.__new__(cls) + + class TestDecode(TestCase): def test_decode_stream_without_codec_context(self) -> None: buffer = io.BytesIO() @@ -228,6 +241,9 @@ def test_decoded_video_enc_params(self) -> None: first_block = video_enc_params.block_params(0) assert video_enc_params.qp + first_block.delta_qp == 29 + + # The block points into the side data, so it must keep it alive. + assert video_enc_params in gc.get_referents(first_block) return def test_decoded_video_enc_params_no_flag(self) -> None: diff --git a/tests/test_dictionary.py b/tests/test_dictionary.py index 14e698d73..e31d38653 100644 --- a/tests/test_dictionary.py +++ b/tests/test_dictionary.py @@ -24,3 +24,12 @@ def test_dictionary_non_ascii() -> None: assert d["café"] == "naïve 日本語 🎵" assert "café" in d assert list(d) == ["café"] + + +def test_dictionary_mutate_while_iterating() -> None: + d = Dictionary({"a": "1", "b": "2"}) + # Iteration used to hold an entry pointer that inserts could invalidate. + for key in d: + for i in range(100): + d[f"{key}{i}"] = "x" + assert len(d) == 202 diff --git a/tests/test_open.py b/tests/test_open.py index bb8e1582e..5ce8f2308 100644 --- a/tests/test_open.py +++ b/tests/test_open.py @@ -57,6 +57,16 @@ def test_container_no_close() -> None: gc.collect() +def test_decode_after_close() -> None: + with av.open(fate_suite("h264/interlaced_crop.mp4")) as container: + frames = container.decode(video=0) + next(frames) + + # Resuming the generator used to read from the freed format context. + with pytest.raises(AssertionError, match="not open"): + next(frames) + + def test_output_container_is_closed_after_close(tmp_path: Path) -> None: path = str(tmp_path / "out.mp4") container = av.open(path, "w") diff --git a/tests/test_packet.py b/tests/test_packet.py index 7ba2bec93..9f5180804 100644 --- a/tests/test_packet.py +++ b/tests/test_packet.py @@ -65,6 +65,18 @@ def test_data_packet_bytes(self) -> None: class TestProperties: + def test_bytearray_input_is_locked(self) -> None: + # The packet references the bytearray's memory, so resizing it while + # the packet is alive would leave the packet pointing at freed memory. + data = bytearray(b"abc") + packet = av.Packet(data) + with pytest.raises(BufferError): + data.extend(b"d") + assert bytes(packet) == b"abc" + + del packet + data.extend(b"d") + def test_rescale_ts(self) -> None: packet = av.Packet() packet.time_base = fractions.Fraction(1, 1000) diff --git a/tests/test_python_io.py b/tests/test_python_io.py index d52e234a7..eb1419392 100644 --- a/tests/test_python_io.py +++ b/tests/test_python_io.py @@ -182,6 +182,15 @@ def test_reading_from_buffer(self) -> None: buf = BytesIO(fh.read()) read(buf, seekable=True) + def test_read_returning_too_much(self) -> None: + class Greedy: + def read(self, n: int) -> bytes: + return bytes(n + 1) + + # This used to overflow FFmpeg's I/O buffer. + with pytest.raises(ValueError, match="expected at most"): + av.open(Greedy(), format="mpegts") + def test_reading_from_buffer_no_seek(self) -> None: with open(fate_suite("mpeg2/mpeg2_field_encoding.ts"), "rb") as fh: buf = ReadOnlyBuffer(fh.read()) From 71f0798e16e395fff4e4bcc0dc692985cfe7810b Mon Sep 17 00:00:00 2001 From: WyattBlue Date: Sat, 24 Oct 2026 22:07:35 -0400 Subject: [PATCH 2/2] Use the UTF-8 byte length of str input MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ByteSource` took `len()` of a str, which counts characters, so `Packet("né")` and `CodecContext.extradata = "né"` dropped the last byte of any non-ASCII text. Get the pointer and byte length together from `PyUnicode_AsUTF8AndSize`; the str owns the buffer, and for ASCII it is the str's own data, so nothing is copied. --- av/buffer.py | 13 ++++++++++++- tests/test_packet.py | 3 +++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/av/buffer.py b/av/buffer.py index 31fe997dc..b8739225d 100644 --- a/av/buffer.py +++ b/av/buffer.py @@ -6,6 +6,7 @@ PyObject_CheckBuffer, PyObject_GetBuffer, ) +from cython.cimports.cpython.unicode import PyUnicode_AsUTF8AndSize from cython.cimports.libc.string import memcpy @@ -14,10 +15,20 @@ class ByteSource: def __cinit__(self, owner): self.owner = owner + size: cython.Py_ssize_t # Only immutable objects may be referenced without a buffer export; a # bytearray could be resized, freeing the memory self.ptr points to. - if isinstance(owner, (bytes, str)): + if isinstance(owner, str): + # The str owns this UTF-8 buffer. len() would count characters, not + # encoded bytes. + self.ptr = cython.cast( + cython.p_uchar, PyUnicode_AsUTF8AndSize(owner, cython.address(size)) + ) + self.length = size + return + + if isinstance(owner, bytes): self.ptr = owner self.length = len(owner) return diff --git a/tests/test_packet.py b/tests/test_packet.py index 9f5180804..b318b4b8b 100644 --- a/tests/test_packet.py +++ b/tests/test_packet.py @@ -77,6 +77,9 @@ def test_bytearray_input_is_locked(self) -> None: del packet data.extend(b"d") + def test_str_input_is_utf8(self) -> None: + assert bytes(av.Packet("né")) == "né".encode() + def test_rescale_ts(self) -> None: packet = av.Packet() packet.time_base = fractions.Fraction(1, 1000)