PR #24063 opened by Théo Valette (theovalette) URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/24063 Patch URL: https://code.ffmpeg.org/FFmpeg/FFmpeg/pulls/24063.patch
The PNG parser returns a complete image when it reaches the IEND chunk and its CRC. If bytes remain, it buffers them while looking for another PNG signature. At EOF, the generic parser flush returns that signature-less buffer as a second packet, which the decoder rejects as an invalid PNG. The same data also prefixes the next image when the png_pipe demuxer loops. Track whether the parser has completed an image. After that point, discard signature-less trailing bytes at EOF and skip interstitial bytes before a subsequent valid PNG or MNG signature. Preserve the existing EOF flush for an invalid or truncated first packet. Regression coverage directly exercises trailing data, interstitial data, a signature split across parser calls, and the existing invalid-input EOF flush. Tests performed: - `make -j$(nproc) fate-png-parser` - `make -j$(nproc) fate-libavcodec` with the documented minimal configuration - `make -j$(nproc) fate` with the documented minimal configuration - `make -j$(nproc) fate-png-parser` with `--toolchain=gcc-asan` - Manual ffprobe and decode checks with the issue attachment - Two-frame `-loop 1` and `-stream_loop 1` checks with the issue attachment - `git diff --check` - `tools/patcheck` on the generated format-patch The configured `make fate` run warned that it covered only the enabled subset because no external `SAMPLES` directory was specified. The existing sample-based `fate-pngparser` test was therefore unavailable. The new regression test is self-contained and passed in both the normal and sanitizer builds. Fixes #23103. Sample: https://code.ffmpeg.org/attachments/8ab0ff3e-04d1-49fa-90ec-a9bdfc6d4354 From 497a1507f43e1ed47e625d60935f0573c67a0408 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9o=20Valette?= <[email protected]> Date: Sun, 9 Aug 2026 18:48:10 +0200 Subject: [PATCH] avcodec/png_parser: Ignore trailing data after IEND MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After returning an image through its IEND CRC, the parser buffers the following bytes while searching for another PNG signature. The generic EOF flush then exposes signature-less trailing bytes as another packet. Discard such data after a completed frame, while preserving the EOF flush for an invalid first packet. Also skip interstitial data before a subsequent valid signature so image-pipe looping remains valid. Fixes #23103. Signed-off-by: Théo Valette <[email protected]> --- libavcodec/Makefile | 1 + libavcodec/png_parser.c | 35 +++++++- libavcodec/tests/png_parser.c | 165 ++++++++++++++++++++++++++++++++++ tests/fate/libavcodec.mak | 5 ++ 4 files changed, 204 insertions(+), 2 deletions(-) create mode 100644 libavcodec/tests/png_parser.c diff --git a/libavcodec/Makefile b/libavcodec/Makefile index e464811af6..baf6d8f688 100644 --- a/libavcodec/Makefile +++ b/libavcodec/Makefile @@ -1392,6 +1392,7 @@ TESTPROGS-$(CONFIG_IDCTDSP) += dct TESTPROGS-$(CONFIG_DXV_ENCODER) += hashtable TESTPROGS-$(CONFIG_MJPEG_ENCODER) += mjpegenc_huffman TESTPROGS-$(CONFIG_MPEGVIDEO) += mpeg12framerate +TESTPROGS-$(CONFIG_PNG_PARSER) += png_parser TESTPROGS-$(CONFIG_H264_METADATA_BSF) += h264_levels TESTPROGS-$(CONFIG_HEVC_METADATA_BSF) += h265_levels TESTPROGS-$(CONFIG_RANGECODER) += rangecoder diff --git a/libavcodec/png_parser.c b/libavcodec/png_parser.c index fbeae1103b..7a1ac5fdad 100644 --- a/libavcodec/png_parser.c +++ b/libavcodec/png_parser.c @@ -33,6 +33,7 @@ typedef struct PNGParseContext { uint32_t chunk_pos; ///< position inside current chunk uint32_t chunk_length; ///< length of the current chunk uint32_t remaining_size; ///< remaining size of the current chunk + int parsed_frame; } PNGParseContext; static int png_parse(AVCodecParserContext *s, AVCodecContext *avctx, @@ -41,6 +42,7 @@ static int png_parse(AVCodecParserContext *s, AVCodecContext *avctx, { PNGParseContext *ppc = s->priv_data; int next = END_NOT_FOUND; + int skipped = 0; int i = 0; s->pict_type = AV_PICTURE_TYPE_NONE; @@ -48,6 +50,13 @@ static int png_parse(AVCodecParserContext *s, AVCodecContext *avctx, *poutbuf_size = 0; *poutbuf = NULL; + /* Do not return signature-less trailing data as another frame. */ + if (!buf_size && ppc->parsed_frame && !ppc->pc.frame_start_found) { + ppc->pc.index = 0; + ppc->pc.state64 = 0; + return 0; + } + if (!ppc->pc.frame_start_found) { uint64_t state64 = ppc->pc.state64; for (; i < buf_size; i++) { @@ -55,6 +64,24 @@ static int png_parse(AVCodecParserContext *s, AVCodecContext *avctx, if (state64 == PNGSIG || state64 == MNGSIG) { i++; ppc->pc.frame_start_found = 1; + if (ppc->parsed_frame) { + /* Drop data between the previous frame and this one. */ + if (i >= 8) { + skipped = i - 8; + ppc->pc.index = 0; + buf += skipped; + buf_size -= skipped; + i = 8; + } else { + int signature_size = 8 - i; + int signature_pos = ppc->pc.index - signature_size; + + memmove(ppc->pc.buffer, + ppc->pc.buffer + signature_pos, + signature_size); + ppc->pc.index = signature_size; + } + } break; } } @@ -102,13 +129,17 @@ static int png_parse(AVCodecParserContext *s, AVCodecContext *avctx, flush: if (ff_combine_frame(&ppc->pc, next, &buf, &buf_size) < 0) - return buf_size; + return skipped + buf_size; ppc->chunk_pos = ppc->pc.frame_start_found = 0; + if (next != END_NOT_FOUND) { + ppc->parsed_frame = 1; + ppc->pc.state64 = 0; + } *poutbuf = buf; *poutbuf_size = buf_size; - return next; + return skipped + next; } const FFCodecParser ff_png_parser = { diff --git a/libavcodec/tests/png_parser.c b/libavcodec/tests/png_parser.c new file mode 100644 index 0000000000..e2cfeea0c2 --- /dev/null +++ b/libavcodec/tests/png_parser.c @@ -0,0 +1,165 @@ +/* + * This file is part of FFmpeg. + * + * FFmpeg is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * FFmpeg is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with FFmpeg; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include <stdint.h> +#include <stdio.h> +#include <string.h> + +#include "libavcodec/avcodec.h" + +#define PNG_SIZE 20 + +static const uint8_t png[PNG_SIZE + AV_INPUT_BUFFER_PADDING_SIZE] = { + 0x89, 'P', 'N', 'G', 0x0d, 0x0a, 0x1a, 0x0a, + 0, 0, 0, 0, 'I', 'E', 'N', 'D', 0, 0, 0, 0, +}; + +static int parse(AVCodecParserContext *parser, AVCodecContext *avctx, + const uint8_t *buf, int size, + int expected_consumed, int expected_size, + const uint8_t *expected) +{ + uint8_t *out; + int out_size; + int consumed = av_parser_parse2(parser, avctx, &out, &out_size, buf, size, + AV_NOPTS_VALUE, AV_NOPTS_VALUE, -1); + + if (consumed != expected_consumed || out_size != expected_size || + expected_size && memcmp(out, expected, expected_size)) { + fprintf(stderr, "consumed %d (expected %d), output %d " + "(expected %d)\n", consumed, expected_consumed, + out_size, expected_size); + return 1; + } + return 0; +} + +static int test_trailing_data(void) +{ + static const uint8_t input[PNG_SIZE + 3 + AV_INPUT_BUFFER_PADDING_SIZE] = { + 0x89, 'P', 'N', 'G', 0x0d, 0x0a, 0x1a, 0x0a, + 0, 0, 0, 0, 'I', 'E', 'N', 'D', 0, 0, 0, 0, + 1, 2, 3, + }; + AVCodecParserContext *parser = av_parser_init(AV_CODEC_ID_PNG); + AVCodecContext *avctx = avcodec_alloc_context3(NULL); + int ret; + + if (!parser || !avctx) { + av_parser_close(parser); + avcodec_free_context(&avctx); + return 1; + } + avctx->codec_id = AV_CODEC_ID_PNG; + + ret = parse(parser, avctx, input, PNG_SIZE + 3, + PNG_SIZE, PNG_SIZE, png); + ret |= parse(parser, avctx, input + PNG_SIZE, 3, 3, 0, NULL); + ret |= parse(parser, avctx, NULL, 0, 0, 0, NULL); + av_parser_close(parser); + avcodec_free_context(&avctx); + return ret; +} + +static int test_interstitial_data(void) +{ + static const uint8_t input[2 * PNG_SIZE + 3 + + AV_INPUT_BUFFER_PADDING_SIZE] = { + 0x89, 'P', 'N', 'G', 0x0d, 0x0a, 0x1a, 0x0a, + 0, 0, 0, 0, 'I', 'E', 'N', 'D', 0, 0, 0, 0, + 1, 2, 3, + 0x89, 'P', 'N', 'G', 0x0d, 0x0a, 0x1a, 0x0a, + 0, 0, 0, 0, 'I', 'E', 'N', 'D', 0, 0, 0, 0, + }; + AVCodecParserContext *parser = av_parser_init(AV_CODEC_ID_PNG); + AVCodecContext *avctx = avcodec_alloc_context3(NULL); + int ret; + + if (!parser || !avctx) { + av_parser_close(parser); + avcodec_free_context(&avctx); + return 1; + } + avctx->codec_id = AV_CODEC_ID_PNG; + + ret = parse(parser, avctx, input, 2 * PNG_SIZE + 3, + PNG_SIZE, PNG_SIZE, png); + ret |= parse(parser, avctx, input + PNG_SIZE, PNG_SIZE + 3, + PNG_SIZE + 3, PNG_SIZE, png); + ret |= parse(parser, avctx, NULL, 0, 0, 0, NULL); + av_parser_close(parser); + avcodec_free_context(&avctx); + return ret; +} + +static int test_split_signature(void) +{ + static const uint8_t prefix[5 + AV_INPUT_BUFFER_PADDING_SIZE] = { + 1, 2, 3, 0x89, 'P', + }; + AVCodecParserContext *parser = av_parser_init(AV_CODEC_ID_PNG); + AVCodecContext *avctx = avcodec_alloc_context3(NULL); + int ret; + + if (!parser || !avctx) { + av_parser_close(parser); + avcodec_free_context(&avctx); + return 1; + } + avctx->codec_id = AV_CODEC_ID_PNG; + + ret = parse(parser, avctx, png, PNG_SIZE, PNG_SIZE, PNG_SIZE, png); + ret |= parse(parser, avctx, prefix, 5, 5, 0, NULL); + ret |= parse(parser, avctx, png + 2, PNG_SIZE - 2, + PNG_SIZE - 2, PNG_SIZE, png); + ret |= parse(parser, avctx, NULL, 0, 0, 0, NULL); + av_parser_close(parser); + avcodec_free_context(&avctx); + return ret; +} + +static int test_existing_eof_flush(void) +{ + static const uint8_t invalid[3 + AV_INPUT_BUFFER_PADDING_SIZE] = { + 1, 2, 3, + }; + AVCodecParserContext *parser = av_parser_init(AV_CODEC_ID_PNG); + AVCodecContext *avctx = avcodec_alloc_context3(NULL); + int ret; + + if (!parser || !avctx) { + av_parser_close(parser); + avcodec_free_context(&avctx); + return 1; + } + avctx->codec_id = AV_CODEC_ID_PNG; + + ret = parse(parser, avctx, invalid, 3, 3, 0, NULL); + ret |= parse(parser, avctx, NULL, 0, 0, 3, invalid); + av_parser_close(parser); + avcodec_free_context(&avctx); + return ret; +} + +int main(void) +{ + return test_trailing_data() || + test_interstitial_data() || + test_split_signature() || + test_existing_eof_flush(); +} diff --git a/tests/fate/libavcodec.mak b/tests/fate/libavcodec.mak index e2d616e307..530fcb9e30 100644 --- a/tests/fate/libavcodec.mak +++ b/tests/fate/libavcodec.mak @@ -91,6 +91,11 @@ fate-rangecoder: libavcodec/tests/rangecoder$(EXESUF) fate-rangecoder: CMD = run libavcodec/tests/rangecoder$(EXESUF) fate-rangecoder: CMP = null +FATE_LIBAVCODEC-$(CONFIG_PNG_PARSER) += fate-png-parser +fate-png-parser: libavcodec/tests/png_parser$(EXESUF) +fate-png-parser: CMD = run libavcodec/tests/png_parser$(EXESUF) +fate-png-parser: CMP = null + FATE_LIBAVCODEC-yes += fate-mathops fate-mathops: libavcodec/tests/mathops$(EXESUF) fate-mathops: CMD = run libavcodec/tests/mathops$(EXESUF) -- 2.52.0 _______________________________________________ ffmpeg-devel mailing list -- [email protected] To unsubscribe send an email to [email protected]
