Skip to content

fix(ffmpeg): heap reads after the end, bad packet, checks at open #5406

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lgritz wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(ffmpeg): heap reads after the end, bad packet, checks at open #5406

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
198 changes: 146 additions & 52 deletions src/ffmpeg.imageio/ffmpeginput.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,13 +74,37 @@ receive_frame(AVCodecContext* avctx, AVFrame* picture, AVPacket* avpkt)


#include <OpenImageIO/color.h>
#include <OpenImageIO/filesystem.h>
#include <OpenImageIO/fmath.h>
#include <OpenImageIO/imageio.h>
#include <climits>
#include <cmath>
#include <iostream>
#include <mutex>

OIIO_PLUGIN_NAMESPACE_BEGIN


// Timestamps, frame rates and time bases all come from the file, so the
// arithmetic below can produce NaN, infinity, or values outside the integer
// range. Convert through these helpers to keep that out of undefined
// behavior. safe_int64's range is well under the type's limits so that a sum
// of two results can't overflow either.
static int64_t
safe_int64(double v)
{
return std::isfinite(v) ? int64_t(clamp(v, -4.0e18, 4.0e18)) : 0;
}


static int
safe_int(double v)
{
return std::isfinite(v) ? int(clamp(v, double(INT_MIN), double(INT_MAX)))
: 0;
}


class FFmpegInput final : public ImageInput {
public:
FFmpegInput();
Expand Down Expand Up @@ -120,6 +144,7 @@ class FFmpegInput final : public ImageInput {
AVFrame* m_frame = nullptr;
AVFrame* m_rgb_frame = nullptr;
size_t m_stride; // scanline width in bytes, a.k.a. scanline stride
AVPixelFormat m_decoded_pix_format; // what the decoder promised at open
AVPixelFormat m_dst_pix_format;
SwsContext* m_sws_rgb_context = nullptr;
AVRational m_frame_rate;
Expand All @@ -133,19 +158,21 @@ class FFmpegInput final : public ImageInput {
bool m_offset_time;
bool m_codec_cap_delay;
bool m_read_frame;
bool m_frame_valid; // did the last read_frame() actually decode?
int64_t m_start_time;

// init to initialize state
void init(void)
{
m_filename.clear();
m_format_context = nullptr;
m_codec_context = nullptr;
m_codec = nullptr;
m_frame = nullptr;
m_rgb_frame = nullptr;
m_sws_rgb_context = nullptr;
m_stride = 0;
m_format_context = nullptr;
m_codec_context = nullptr;
m_codec = nullptr;
m_frame = nullptr;
m_rgb_frame = nullptr;
m_sws_rgb_context = nullptr;
m_stride = 0;
m_decoded_pix_format = AV_PIX_FMT_NONE;
m_rgb_buffer.clear();
m_video_indexes.clear();
m_video_stream = -1;
Expand All @@ -155,6 +182,7 @@ class FFmpegInput final : public ImageInput {
m_last_decoded_pos = 0;
m_offset_time = true;
m_read_frame = false;
m_frame_valid = false;
m_codec_cap_delay = false;
m_subimage = 0;
m_start_time = 0;
Expand Down Expand Up @@ -315,31 +343,48 @@ FFmpegInput::open(const std::string& name, ImageSpec& spec)
AVStream* stream = m_format_context->streams[m_video_stream];
m_frame_rate = av_guess_frame_rate(m_format_context, stream, NULL);

m_frames = stream->nb_frames;
// nb_frames and start_time come from the file. Clamp them to a range the
// rest of this reader can handle: subimage indices are int, and an
// unset start_time (AV_NOPTS_VALUE == INT64_MIN) would blow up the frame
// arithmetic in read_frame().
m_frames = OIIO::clamp(stream->nb_frames, int64_t(0), int64_t(INT_MAX));
m_start_time = stream->start_time;
if (m_start_time == int64_t(AV_NOPTS_VALUE))
m_start_time = 0;
if (!m_frames) {
seek(0);
AVPacket pkt;
av_init_packet(&pkt);
av_read_frame(m_format_context, &pkt);
int64_t first_pts = pkt.pts;
int64_t max_pts = 0;
if (first_pts == int64_t(AV_NOPTS_VALUE))
first_pts = 0;
int64_t max_pts = 0;
av_packet_unref(&pkt); //because seek(int) uses m_format_context
seek(1 << 29);
av_init_packet(&pkt); //Is this needed?
while (stream && av_read_frame(m_format_context, &pkt) >= 0) {
int64_t current_pts = static_cast<int64_t>(
av_q2d(stream->time_base) * (pkt.pts - first_pts) * fps());
// Do the difference in double: both timestamps are untrusted and
// subtracting them as int64 can overflow.
int64_t current_pts = safe_int64(
av_q2d(stream->time_base)
* (double(pkt.pts) - double(first_pts)) * fps());
if (current_pts > max_pts) {
max_pts = current_pts + 1;
}
av_packet_unref(&pkt); //Always free before format_context usage
}
m_frames = max_pts;
m_frames = std::min(max_pts, int64_t(INT_MAX));
}
m_frame = av_frame_alloc();
m_rgb_frame = av_frame_alloc();
if (!m_frame || !m_rgb_frame) {
errorfmt("\"{}\" could not allocate FFmpeg frame", file_name);
close();
return false;
}

m_decoded_pix_format = m_codec_context->pix_fmt;
AVPixelFormat src_pix_format;
switch (m_codec_context->pix_fmt) { // deprecation warning for YUV formats
case AV_PIX_FMT_YUVJ420P: src_pix_format = AV_PIX_FMT_YUV420P; break;
Expand Down Expand Up @@ -418,6 +463,7 @@ FFmpegInput::open(const std::string& name, ImageSpec& spec)
case AV_PIX_FMT_GRAY8:
case AV_PIX_FMT_MONOWHITE:
case AV_PIX_FMT_MONOBLACK:
nchannels = 1;
datatype = TypeUInt8;
m_dst_pix_format = AV_PIX_FMT_GRAY8;
break;
Expand All @@ -430,6 +476,7 @@ FFmpegInput::open(const std::string& name, ImageSpec& spec)
case AV_PIX_FMT_GRAY12LE:
case AV_PIX_FMT_GRAY16BE:
case AV_PIX_FMT_GRAY16LE:
nchannels = 1;
datatype = TypeUInt16;
m_dst_pix_format = AV_PIX_FMT_GRAY16;
break;
Expand Down Expand Up @@ -496,8 +543,16 @@ FFmpegInput::open(const std::string& name, ImageSpec& spec)
default: break;
}

m_spec = ImageSpec(m_codec_context->width, m_codec_context->height,
nchannels, datatype);
m_spec = ImageSpec(m_codec_context->width, m_codec_context->height,
nchannels, datatype);
// The frame dimensions come from the container/codec headers, which are
// untrusted. FFmpeg caps the pixel count at only about 2.7e8, which still
// leaves room for a tiny file to declare a multi-GB frame.
if (!check_open(m_spec, { 0, 1 << 16, 0, 1 << 16, 0, 1, 0, 4 })
|| !check_compression_ratio(m_spec, Filesystem::file_size(name))) {
close();
return false;
}
m_stride = (size_t)(m_spec.scanline_bytes());

int rgb_buffer_size
Expand Down Expand Up @@ -592,8 +647,9 @@ FFmpegInput::seek_subimage(int subimage, int miplevel)
if (subimage == m_subimage) {
return true;
}
m_subimage = subimage;
m_read_frame = false;
m_subimage = subimage;
m_read_frame = false;
m_frame_valid = false;
return true;
}

Expand Down Expand Up @@ -633,6 +689,14 @@ FFmpegInput::read_native_scanline(int subimage, int miplevel, int y, int /*z*/,
if (!m_read_frame) {
read_frame(m_subimage);
}
if (!m_frame_valid) {
// Nothing was decoded for this subimage. Without this check we would
// hand back whatever the previously decoded frame left in the buffer.
if (!has_error())
errorfmt("Could not decode frame {} of \"{}\"", m_subimage,
m_filename);
return false;
}
if (m_spec.format == TypeUInt8 || m_spec.nchannels == 1) {
if (m_rgb_frame->data[0]) {
memcpy(data, m_rgb_frame->data[0] + y * m_rgb_frame->linesize[0],
Expand Down Expand Up @@ -676,8 +740,8 @@ FFmpegInput::close(void)
if (m_codec_context)
avcodec_free_context(&m_codec_context);
if (m_format_context) {
// Frees the context and everything it owns, and nulls the pointer.
avformat_close_input(&m_format_context);
av_free(m_format_context); // will free m_codec and m_codec_context
}
if (m_frame)
av_frame_free(&m_frame); // free after close input
Expand All @@ -697,61 +761,87 @@ FFmpegInput::read_frame(int frame)
if (m_last_decoded_pos + 1 != frame) {
seek(frame);
}
AVPacket pkt;
int finished = 0;
int ret = 0;
while ((ret = av_read_frame(m_format_context, &pkt)) == 0
|| m_codec_cap_delay) {
if (ret == AVERROR_EOF) {
break;
m_read_frame = true;
m_frame_valid = false;
// Allocate the packet rather than using a stack AVPacket: av_read_frame
// is not guaranteed to initialize it on failure, and the flush path below
// would then unref an uninitialized packet.
AVPacket* pkt = av_packet_alloc();
if (!pkt) {
errorfmt("Could not allocate FFmpeg packet");
return;
}
bool flushing = false;
while (true) {
int ret = av_read_frame(m_format_context, pkt);
if (ret < 0) {
if (!m_codec_cap_delay || ret == AVERROR_EOF)
break;
// The codec buffers delayed frames, so keep going with flush
// packets (data == null, size == 0), but stop as soon as the
// decoder runs dry -- otherwise a stream that keeps returning
// the same error would spin here forever.
flushing = true;
av_packet_unref(pkt);
pkt->stream_index = m_video_stream;
}
if (pkt.stream_index == m_video_stream) {
if (ret < 0 && m_codec_cap_delay) {
pkt.data = NULL;
pkt.size = 0;
}

finished = receive_frame(m_codec_context, m_frame, &pkt);
if (pkt->stream_index == m_video_stream) {
int finished = receive_frame(m_codec_context, m_frame, pkt);
if (flushing && !finished)
break;

double pts = 0;
if (static_cast<int64_t>(m_frame->pts) != int64_t(AV_NOPTS_VALUE)) {
pts = av_q2d(
m_format_context->streams[m_video_stream]->time_base)
* m_frame->pts;
* double(m_frame->pts);
}

int current_frame = int((pts - m_start_time) * fps() + 0.5f); //???
int current_frame = safe_int((pts - double(m_start_time)) * fps()
+ 0.5);
//current_frame = m_frame->display_picture_number;
m_last_search_pos = current_frame;

if (current_frame == frame && finished) {
// A decoder may change frame geometry or pixel format
// mid-stream, but the scaling context and RGB buffer were
// sized from the header when we opened the file.
if (m_frame->width != m_spec.width
|| m_frame->height != m_spec.height
|| m_frame->format != m_decoded_pix_format) {
errorfmt("\"{}\" frame {} does not match the {}x{} format "
"declared by the header",
m_filename, frame, m_spec.width, m_spec.height);
break;
}
// Use the spec dimensions, which are what m_rgb_buffer and
// the scaling context were sized for -- the decoder can move
// m_codec_context->width/height out from under us.
int fill_ret = avpicture_fill(m_rgb_frame, &m_rgb_buffer[0],
m_dst_pix_format,
m_codec_context->width,
m_codec_context->height);
m_dst_pix_format, m_spec.width,
m_spec.height);
if (fill_ret < 0) {
errorfmt("Error filling FFmpeg RGB frame");
av_packet_unref(&pkt);
break;
}
int scale_ret = sws_scale(
m_sws_rgb_context,
static_cast<uint8_t const* const*>(m_frame->data),
m_frame->linesize, 0, m_codec_context->height,
m_rgb_frame->data, m_rgb_frame->linesize);
int scale_ret = sws_scale(m_sws_rgb_context,
static_cast<uint8_t const* const*>(
m_frame->data),
m_frame->linesize, 0, m_spec.height,
m_rgb_frame->data,
m_rgb_frame->linesize);
if (scale_ret <= 0) {
errorfmt("Error converting FFmpeg frame");
av_packet_unref(&pkt);
break;
}
m_last_decoded_pos = current_frame;
av_packet_unref(&pkt);
m_frame_valid = true;
break;
}
}
av_packet_unref(&pkt);
av_packet_unref(pkt);
}
m_read_frame = true;
av_packet_free(&pkt);
}


Expand Down Expand Up @@ -791,15 +881,19 @@ FFmpegInput::seek(int frame)
int64_t
FFmpegInput::time_stamp(int frame) const
{
int64_t timestamp = static_cast<int64_t>(
(static_cast<double>(frame)
/ (fps()
* av_q2d(m_format_context->streams[m_video_stream]->time_base))));
// A corrupt header can give us a zero or degenerate time base, which
// would make the divisions below produce inf/NaN.
double time_base = av_q2d(
m_format_context->streams[m_video_stream]->time_base);
double scale = fps() * time_base;
if (!(scale > 0) || !(time_base > 0))
return 0;
int64_t timestamp = safe_int64(static_cast<double>(frame) / scale);
if (static_cast<int64_t>(m_format_context->start_time)
!= int64_t(AV_NOPTS_VALUE)) {
timestamp += static_cast<int64_t>(
timestamp += safe_int64(
static_cast<double>(m_format_context->start_time) * AV_TIME_BASE
/ av_q2d(m_format_context->streams[m_video_stream]->time_base));
/ time_base);
}
return timestamp;
}
Expand Down
14 changes: 14 additions & 0 deletions testsuite/ffmpeg/ref/out-ffmpeg6.1.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,3 +110,17 @@ ref/vp9_rec2100_pq.mkv : 384 x 216, 3 channel, uint10 FFmpeg movie
oiio:ColorSpace: "pq_rec2020_display"
oiio:Movie: 1
oiio:subimages: 2
oiiotool WARNING : oiiotool produced no output. Did you forget -o?
src/gray8.avi : 128 x 64, 1 channel, uint8 FFmpeg movie
oiiotool WARNING : oiiotool produced no output. Did you forget -o?
src/gray16.avi : 128 x 64, 1 channel, uint16 FFmpeg movie
oiiotool WARNING : oiiotool produced no output. Did you forget -o?
oiiotool ERROR: read : "src/bomb-16384x9000.avi": FFmpeg movie header claims a 1125 MB image from a 6592 byte file; probably a corrupt or malicious header
Full command line was:
> oiiotool --info --hash src/bomb-16384x9000.avi
oiiotool ERROR: read : "src/truncated.avi" could not open input
Full command line was:
> oiiotool --info --hash src/truncated.avi
src/resolution-change.mkv : 320 x 240, 3 channel, uint8 FFmpeg movie (4 subimages)
SHA-1: "src/resolution-change.mkv" frame 0 does not match the 320x240 format declared by the header
iinfo ERROR: "src/resolution-change.mkv" : "src/resolution-change.mkv" frame 0 does not match the 320x240 format declared by the header
14 changes: 14 additions & 0 deletions testsuite/ffmpeg/ref/out-ffmpeg8.0.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,3 +110,17 @@ ref/vp9_rec2100_pq.mkv : 384 x 216, 3 channel, uint10 FFmpeg movie
oiio:ColorSpace: "pq_rec2020_display"
oiio:Movie: 1
oiio:subimages: 2
oiiotool WARNING : oiiotool produced no output. Did you forget -o?
src/gray8.avi : 128 x 64, 1 channel, uint8 FFmpeg movie
oiiotool WARNING : oiiotool produced no output. Did you forget -o?
src/gray16.avi : 128 x 64, 1 channel, uint16 FFmpeg movie
oiiotool WARNING : oiiotool produced no output. Did you forget -o?
oiiotool ERROR: read : "src/bomb-16384x9000.avi": FFmpeg movie header claims a 1125 MB image from a 6592 byte file; probably a corrupt or malicious header
Full command line was:
> oiiotool --info --hash src/bomb-16384x9000.avi
oiiotool ERROR: read : "src/truncated.avi" could not open input
Full command line was:
> oiiotool --info --hash src/truncated.avi
src/resolution-change.mkv : 320 x 240, 3 channel, uint8 FFmpeg movie (4 subimages)
SHA-1: "src/resolution-change.mkv" frame 0 does not match the 320x240 format declared by the header
iinfo ERROR: "src/resolution-change.mkv" : "src/resolution-change.mkv" frame 0 does not match the 320x240 format declared by the header
Loading
Loading