fix(ffmpeg): heap bounds, bad packet, checks at open, other fixes - #5406
Open
lgritz wants to merge 2 commits into
Open
fix(ffmpeg): heap bounds, bad packet, checks at open, other fixes#5406lgritz wants to merge 2 commits into
lgritz wants to merge 2 commits into
Conversation
This is a security audit of the FFmpeg reader (specs/002-format-security-audit). Gray movies read heap memory after the end of the buffer. The GRAY8 and GRAY16 code asked swscale for a gray frame with one plane, but it left nchannels at 3. Each scanline copy then read three times too many bytes. Now nchannels is 1. This was the intent when we added the gray code (AcademySoftwareFoundation#2349). It also makes 16-bit gray files readable, which they were not before. A decoder can change the frame size in the middle of a stream. But we make the scale context and the RGB buffer one time, from the header. A smaller frame thus caused a read after its end. Now read_frame() refuses a frame whose size or pixel format is different from the ImageSpec. It also scales from the spec. Nothing examined the frame size at open. A file of 6 KB can declare a frame of 1.1 GB, and cause an allocation of 2.4 GB. Now check_open() and check_compression_ratio() refuse such a file. The ratio floor stays at 1 GB, because a correct movie with one frame can reach a ratio of 13000:1. read_frame() gave an uninitialized AVPacket to av_read_frame(), which does not initialize it when it fails. The code then read bad values from the packet, and it could continue forever on a bad stream. Now the code uses av_packet_alloc(), and it stops when the decoder is empty. There are smaller changes. Examine the result of av_frame_alloc(). Correct a bad nb_frames and start_time. Protect time_stamp() against a time base of zero. Do the frame-count arithmetic in double, because two int64 times can overflow. Report a decode failure, and do not give back the last good frame. Remove an unnecessary av_free() after avformat_close_input(). New fixtures come from testsuite/ffmpeg/src/make_malformed_movies.py: the gray8 and gray16 tests, a decompression bomb, a truncated stream, and a change of size in the middle of a stream. Assisted-by: Claude Code / claude-opus-5 Signed-off-by: Larry Gritz <lg@larrygritz.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request hardens the FFmpeg reader against malformed media, grayscale buffer overreads, excessive allocations, and decoder failures.
Changes:
- Validates frame geometry, formats, allocation sizes, and compression ratios.
- Improves grayscale handling, packet processing, timestamps, and decode failure handling.
- Adds malformed-media regression fixtures, test commands, and updated reference outputs.
Reviewed changes
Copilot reviewed 6 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Summary | Final review comments |
|---|---|---|
testsuite/ffmpeg/src/make_malformed_movies.py |
Generates malformed-media test fixtures. | No final comments. |
testsuite/ffmpeg/run.py |
Runs FFmpeg regression tests. | No final comments. |
testsuite/ffmpeg/ref/out-ffmpeg8.1.txt |
Updated FFmpeg 8.1 reference output. | No final comments. |
testsuite/ffmpeg/ref/out-ffmpeg8.0.txt |
Updated FFmpeg 8.0 reference output. | No final comments. |
testsuite/ffmpeg/ref/out-ffmpeg6.1.txt |
Updated FFmpeg 6.1 reference output. | No final comments. |
src/ffmpeg.imageio/ffmpeginput.cpp |
Implements FFmpeg reader safety and validation fixes. | Four unresolved moderate issues: convert stream start_time to seconds; correct seek timestamp units; count frames only from video packets; properly drain delayed codecs at EOF. |
Suppressed comments (1)
testsuite/ffmpeg/run.py:27
- This does not assert that either grayscale fixture was successfully read: as the resolution-change test notes,
oiiotool --hashexits 0 when a frame read fails. A regression that fails the read instead of exercising the scanline copy would therefore still pass this test; use a command whose exit status reflects read failure or add an explicit success assertion.
command = command + oiiotool ("src/" + f + " --hash")
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+800
to
+801
| int current_frame = safe_int((pts - double(m_start_time)) * fps() | ||
| + 0.5); |
| double scale = fps() * time_base; | ||
| if (!(scale > 0) || !(time_base > 0)) | ||
| return 0; | ||
| int64_t timestamp = safe_int64(static_cast<double>(frame) / scale); |
Comment on lines
+369
to
+371
| int64_t current_pts = safe_int64( | ||
| av_q2d(stream->time_base) | ||
| * (double(pkt.pts) - double(first_pts)) * fps()); |
Comment on lines
+777
to
+779
| if (ret < 0) { | ||
| if (!m_codec_cap_delay || ret == AVERROR_EOF) | ||
| break; |
Four bugs in how the reader turns timestamps into frame numbers. All predate this branch; the new "could not decode" error made them visible, because before they returned stale or blank pixels instead. read_frame() compared a pts in seconds against m_start_time, which is a count of time base ticks. Any movie whose stream does not start at zero was therefore undecodable, in every frame. Scale the start time to seconds before the subtraction. time_stamp() produced a value in the stream time base, and seek() gave it to av_seek_frame() with a stream index of -1, which wants AV_TIME_BASE units. It also added the format start time, which is already in AV_TIME_BASE units, after multiplying it by AV_TIME_BASE again. Seeking therefore landed at the start of the file, or far past the end. Seek on the video stream instead, and keep the whole calculation in that stream's time base. The frame counting pass treated every demuxed packet as a video packet. An audio track that runs longer than the video thus added frames that no decode will ever produce. Count video packets only. Codecs that buffer frames, h264 among them, need a flush at the end of the stream to give up the last few. The loop broke out on AVERROR_EOF before reaching the flush path, so the tail of every such movie was unreadable. Flush at end of stream, and let receive_frame() keep pulling frames after the decoder stops taking input. Also: open() left the demuxer at the end of the file, so reading frame 1 first found nothing. Seek back, and start m_last_decoded_pos at -1 so the first read always seeks. The frame counting pass now allocates its packet rather than using the deprecated av_init_packet(). Drop m_offset_time and m_last_search_pos, which nothing has read since 2015. New fixtures in testsuite/ffmpeg/src, from make_test_movies.py (renamed from make_malformed_movies.py, which no longer describes all of them): bframes.mp4, audio-track.mkv and start-offset.mkv. Each is ten frames of well-formed video, and each fails without the fixes above. Assisted-by: Claude Code / claude-opus-5 Signed-off-by: Larry Gritz <lg@larrygritz.com>
lgritz
changed the title
Collaborator
Author
|
Asking for the Copilot review was helpful -- it uncovered 4 additional bugs, only one of which was related to the changed code at all, and three of which were actually bugs all along (though in some cases their specific symptoms changed as a result of the other changes of this PR). All fixed now and amended to this PR. I changed the PR title and description to cover all the changes, both original and new. New test cases added as well. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is from an audit of the FFmpeg reader, looking for bugs and security issues.
Grayscale movies read heap memory after the end of the buffer. The GRAY8 and GRAY16 code asked swscale for a gray frame with one plane, but it left nchannels at 3. Each scanline copy then read three times too many bytes. Now nchannels is 1. This was the intent when we added the gray code (#2349). It also makes 16-bit gray files readable, which they were not before.
A decoder can change the frame size in the middle of a stream. But we make the scale context and the RGB buffer one time, from the header. A smaller frame thus caused a read after its end. Now read_frame() refuses a frame whose size or pixel format is different from the ImageSpec. It also scales from the spec.
Nothing examined the frame size at open. A file of 6 KB can declare a frame of 1.1 GB, and cause an allocation of 2.4 GB. Now check_open() and check_compression_ratio() refuse such a file. The ratio floor stays at 1 GB, because a correct movie with one frame can reach a ratio of 13000:1.
read_frame() gave an uninitialized AVPacket to av_read_frame(), which does not initialize it when it fails. The code then read bad values from the packet, and it could continue forever on a bad stream. Now the code uses av_packet_alloc(), and it stops when the decoder is empty.
A failed decode gave back the pixels of the last good frame. Now it is an error.
read_frame() compared a pts in seconds against m_start_time, a count of time base ticks. Every frame of a movie whose stream does not start at zero was thus undecodable. Now the start time is scaled to seconds first.
time_stamp() returned stream time base units, and seek() gave them to av_seek_frame() with a stream index of -1, which wants AV_TIME_BASE units. It also added the format start time, already in AV_TIME_BASE units, after multiplying it by AV_TIME_BASE again. Seeks landed at the start of the file, or far past the end. Now we seek on the video stream, and the whole calculation stays in that stream's time base.
The frame count pass read every demuxed packet as a video packet. An audio track longer than the video thus added frames that no decode can produce. Now only video packets count.
Codecs that buffer frames, h264 among them, need a flush at the end of the stream to give up the last few. The loop broke on AVERROR_EOF before it reached the flush path, so the tail of every such movie was unreadable. Now it flushes at end of stream, and receive_frame() keeps pulling frames after the decoder stops taking input.
open() left the demuxer at the end of the file, so a first read of frame 1 found nothing. Now open() seeks back, and the first read always seeks.
There are smaller changes. Examine the result of av_frame_alloc(). Correct a bad nb_frames and start_time. Protect time_stamp() against a time base of zero. Do the frame-count arithmetic in double, because two int64 times can overflow. Remove an unnecessary av_free() after avformat_close_input(). Replace the deprecated av_init_packet(). Drop m_offset_time and m_last_search_pos, which nothing has read since 2015.
New test fixtures come from testsuite/ffmpeg/src/make_test_movies.py, renamed from make_malformed_movies.py because it no longer describes all of them. The malformed ones are a decompression bomb, a truncated stream, and a change of size in the middle of a stream. The correct ones are the gray8 and gray16 movies, and three more of ten frames each, all of which fail without the fixes above: bframes.mp4, h264 with B-frames; audio-track.mkv, whose audio runs longer than its video; and start-offset.mkv, whose first frame is at 5 seconds.
Assisted-by: Claude Code / claude-opus-5