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 <limits> include. Document the valgrind codec check in docs/RUNBOOK.md. Validated with valgrind: 0 bytes definitely lost, 0 invalid reads.
This commit is contained in:
@@ -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 {} +
|
||||
```
|
||||
@@ -5,6 +5,7 @@
|
||||
#include <array>
|
||||
#include <cstdint>
|
||||
#include <cstring>
|
||||
#include <limits>
|
||||
#include <optional>
|
||||
#include <string>
|
||||
#include <vector>
|
||||
@@ -43,15 +44,20 @@ class FfmpegDecoder final : public Decoder {
|
||||
return CodecError{"failed to allocate AVPacket"};
|
||||
}
|
||||
|
||||
packet->pts = av_rescale_q(
|
||||
static_cast<int64_t>(frame.capture_timestamp_ns), AVRational{1, 1'000'000'000}, ctx_->time_base);
|
||||
if (frame.data.size() >
|
||||
static_cast<std::size_t>(std::numeric_limits<int>::max() - AV_INPUT_BUFFER_PADDING_SIZE)) {
|
||||
return CodecError{"encoded frame is too large"};
|
||||
}
|
||||
|
||||
packet->data = static_cast<uint8_t*>(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<int>(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<int>(frame.data.size());
|
||||
packet->pts = av_rescale_q(
|
||||
static_cast<int64_t>(frame.capture_timestamp_ns), AVRational{1, 1'000'000'000}, ctx_->time_base);
|
||||
|
||||
std::vector<DecodedFrame> 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<std::unique_ptr<Decoder>> DecoderFactory::create(const DecoderConfig
|
||||
if (config.extradata.size() > static_cast<std::size_t>(std::numeric_limits<int>::max())) {
|
||||
return CodecError{"decoder extradata is too large"};
|
||||
}
|
||||
ctx->extradata = static_cast<uint8_t*>(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<uint8_t*>(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<int>(config.extradata.size());
|
||||
}
|
||||
|
||||
|
||||
@@ -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());
|
||||
|
||||
Reference in New Issue
Block a user