Fix segfaults and use-after-frees

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,12 +15,21 @@
 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, 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
 
-        try:
+        if isinstance(owner, bytes):
             self.ptr = owner
-        except TypeError:
-            pass
-        else:
             self.length = len(owner)
             return
 

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")
 

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]

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)
         )

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})"

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: ...

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

av/sidedata/encparams.pxd

@@ -8,4 +8,5 @@ cdef class VideoEncParams(SideData):
 
 
 cdef class VideoBlockParams:
+    cdef VideoEncParams parent
     cdef lib.AVVideoBlockParams *ptr

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
         )

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

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

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:

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

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")

tests/test_packet.py

@@ -65,6 +65,21 @@ 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_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)

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())