From c6c062250ec1b6a4243e658f6eeb2e3b1f572fc3 Mon Sep 17 00:00:00 2001 From: Florian Egger Date: Mon, 7 Sep 2026 10:17:59 +0200 Subject: [PATCH] fix(codec): fix decoder packet leak and extradata over-read Allocate packet payloads with av_new_packet so they are freed through the owning AVBufferRef instead of leaking on every decode. Pad extradata with AV_INPUT_BUFFER_PADDING_SIZE for FFmpeg's bitstream parsers. Reject oversized frames before the int cast, handle unexpected EOF in the EAGAIN-retry loops, and add the missing include. Document the valgrind codec check in docs/RUNBOOK.md. Validated with valgrind: 0 bytes definitely lost, 0 invalid reads. --- docs/RUNBOOK.md | 35 +++++++++++++++++++++++++++++++++++ src/codec/ffmpeg_decoder.cpp | 24 ++++++++++++++++++------ src/codec/ffmpeg_encoder.cpp | 9 ++++++++- 3 files changed, 61 insertions(+), 7 deletions(-) create mode 100644 docs/RUNBOOK.md diff --git a/docs/RUNBOOK.md b/docs/RUNBOOK.md new file mode 100644 index 0000000..a66c710 --- /dev/null +++ b/docs/RUNBOOK.md @@ -0,0 +1,35 @@ +# Runbook — build and validation commands + +Reusable validation steps for this repository. Run the relevant ones before +considering a change complete. + +## Build and test + +```sh +meson setup build # once +meson compile -C build +meson test -C build --print-errorlogs +``` + +## Memory and correctness checks (codec changes) + +Run the codec round-trip test under valgrind after touching the encoder or +decoder: + +```sh +valgrind --leak-check=full --errors-for-leak-kinds=definite \ + build/tests/test_codec_roundtrip +``` + +Expected: no "definitely lost" bytes and no "Invalid read/write" errors. +FFmpeg may keep some "still reachable" allocations at exit; that is normal. + +This check caught two real defects in the Phase 2 decoder: a per-packet +payload leak and an unpadded extradata buffer over-read. Keep using it. + +## Formatting + +```sh +find include src tests -type f \( -name '*.cpp' -o -name '*.h' \) \ + -exec clang-format --dry-run --Werror {} + +``` \ No newline at end of file diff --git a/src/codec/ffmpeg_decoder.cpp b/src/codec/ffmpeg_decoder.cpp index 675db11..bf3a16e 100644 --- a/src/codec/ffmpeg_decoder.cpp +++ b/src/codec/ffmpeg_decoder.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -43,15 +44,20 @@ class FfmpegDecoder final : public Decoder { return CodecError{"failed to allocate AVPacket"}; } - packet->pts = av_rescale_q( - static_cast(frame.capture_timestamp_ns), AVRational{1, 1'000'000'000}, ctx_->time_base); + if (frame.data.size() > + static_cast(std::numeric_limits::max() - AV_INPUT_BUFFER_PADDING_SIZE)) { + return CodecError{"encoded frame is too large"}; + } - packet->data = static_cast(av_malloc(frame.data.size() + AV_INPUT_BUFFER_PADDING_SIZE)); - if (packet->data == nullptr) { + // av_new_packet wires up the owning AVBufferRef so the payload is + // freed with the packet, and zeroes the padding FFmpeg decoders + // require after the bitstream. + if (av_new_packet(packet.get(), static_cast(frame.data.size())) < 0) { return CodecError{"failed to allocate packet data"}; } std::memcpy(packet->data, frame.data.data(), frame.data.size()); - packet->size = static_cast(frame.data.size()); + packet->pts = av_rescale_q( + static_cast(frame.capture_timestamp_ns), AVRational{1, 1'000'000'000}, ctx_->time_base); std::vector out; int send_ret = avcodec_send_packet(ctx_.get(), packet.get()); @@ -70,6 +76,9 @@ class FfmpegDecoder final : public Decoder { if (received.status == ReceiveStatus::Again) { return CodecError{"decoder stalled before producing output"}; } + if (received.status == ReceiveStatus::Eof) { + return CodecError{"decoder reached end of stream before accepting the packet"}; + } send_ret = avcodec_send_packet(ctx_.get(), packet.get()); } @@ -218,11 +227,14 @@ CodecResult> DecoderFactory::create(const DecoderConfig if (config.extradata.size() > static_cast(std::numeric_limits::max())) { return CodecError{"decoder extradata is too large"}; } - ctx->extradata = static_cast(av_malloc(config.extradata.size())); + // FFmpeg bitstream parsers may read past the end of extradata, so the + // buffer must include the padding they require. + ctx->extradata = static_cast(av_malloc(config.extradata.size() + AV_INPUT_BUFFER_PADDING_SIZE)); if (ctx->extradata == nullptr) { return CodecError{"failed to allocate decoder extradata"}; } std::memcpy(ctx->extradata, config.extradata.data(), config.extradata.size()); + std::memset(ctx->extradata + config.extradata.size(), 0, AV_INPUT_BUFFER_PADDING_SIZE); ctx->extradata_size = static_cast(config.extradata.size()); } diff --git a/src/codec/ffmpeg_encoder.cpp b/src/codec/ffmpeg_encoder.cpp index 0ba6d65..cdf4d26 100644 --- a/src/codec/ffmpeg_encoder.cpp +++ b/src/codec/ffmpeg_encoder.cpp @@ -126,6 +126,9 @@ class FfmpegEncoder final : public Encoder { if (received.status == ReceiveStatus::Again) { return CodecError{"encoder stalled before producing output"}; } + if (received.status == ReceiveStatus::Eof) { + return CodecError{"encoder reached end of stream before accepting the frame"}; + } send_ret = avcodec_send_frame(ctx_.get(), input.get()); } @@ -154,6 +157,10 @@ class FfmpegEncoder final : public Encoder { if (received.status == ReceiveStatus::Again) { return CodecError{"encoder stalled during flush"}; } + if (received.status == ReceiveStatus::Eof) { + // The encoder is already fully drained. + return out; + } ret = avcodec_send_frame(ctx_.get(), nullptr); } @@ -183,7 +190,7 @@ class FfmpegEncoder final : public Encoder { ReceiveResult receive_one_packet() { AvPacketPtr packet(av_packet_alloc(), AvPacketDeleter{}); if (packet == nullptr) { - return {ReceiveStatus::Error, nullptr, "failed to allocate RTP packet"}; + return {ReceiveStatus::Error, nullptr, "failed to allocate AVPacket"}; } int ret = avcodec_receive_packet(ctx_.get(), packet.get());