From 2e5f296bfa04c5a4f885ebad790339641691e4bd Mon Sep 17 00:00:00 2001 From: John Bowler Date: Tue, 24 Jun 2025 14:18:37 -0700 Subject: [PATCH] fix: Prevent unknown chunks from causing out-of-place IEND errors PNG_AFTER_IDAT was not set by the IDAT read code if unknown chunk handling was turned on. This was hidden in the current tests by checks within the text handling chunks. (For example, pngtest.png has a zTXt chunk after IDAT.) This change modifies both the sequential and the progressive reader to reliably set PNG_AFTER_IDAT when the first non-IDAT chunk is seen and before that chunk is processed. The change is minimalist; PNG_HAVE_CHUNK_AFTER_IDAT can probably be removed and replaced with PNG_AFTER_IDAT. Making the latter change is something to be considered in libpng2. Co-authored-by: Cosmin Truta Signed-off-by: John Bowler Signed-off-by: Cosmin Truta --- pngpread.c | 8 ++++++++ pngread.c | 7 ++++++- pngrutil.c | 12 ------------ 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/pngpread.c b/pngpread.c index 0a3e822cf..37aa432ae 100644 --- a/pngpread.c +++ b/pngpread.c @@ -229,6 +229,14 @@ png_push_read_chunk(png_structrp png_ptr, png_inforp info_ptr) png_benign_error(png_ptr, "Too many IDATs found"); } + else if ((png_ptr->mode & PNG_HAVE_IDAT) != 0) + { + /* These flags must be set consistently for all non-IDAT chunks, + * including the unknown chunks. + */ + png_ptr->mode |= PNG_HAVE_CHUNK_AFTER_IDAT | PNG_AFTER_IDAT; + } + if (chunk_name == png_IHDR) { if (png_ptr->push_length != 13) diff --git a/pngread.c b/pngread.c index a0dbf2ae2..212afb7d2 100644 --- a/pngread.c +++ b/pngread.c @@ -702,7 +702,12 @@ png_read_end(png_structrp png_ptr, png_inforp info_ptr) png_uint_32 chunk_name = png_ptr->chunk_name; if (chunk_name != png_IDAT) - png_ptr->mode |= PNG_HAVE_CHUNK_AFTER_IDAT; + { + /* These flags must be set consistently for all non-IDAT chunks, + * including the unknown chunks. + */ + png_ptr->mode |= PNG_HAVE_CHUNK_AFTER_IDAT | PNG_AFTER_IDAT; + } if (chunk_name == png_IEND) png_handle_chunk(png_ptr, info_ptr, length); diff --git a/pngrutil.c b/pngrutil.c index 7d9a00186..e7c7bbe48 100644 --- a/pngrutil.c +++ b/pngrutil.c @@ -2412,10 +2412,6 @@ png_handle_tEXt(png_structrp png_ptr, png_inforp info_ptr, png_uint_32 length) } #endif - /* TODO: this doesn't work and shouldn't be necessary. */ - if ((png_ptr->mode & PNG_HAVE_IDAT) != 0) - png_ptr->mode |= PNG_AFTER_IDAT; - buffer = png_read_buffer(png_ptr, length+1); if (buffer == NULL) @@ -2486,10 +2482,6 @@ png_handle_zTXt(png_structrp png_ptr, png_inforp info_ptr, png_uint_32 length) } #endif - /* TODO: should not be necessary. */ - if ((png_ptr->mode & PNG_HAVE_IDAT) != 0) - png_ptr->mode |= PNG_AFTER_IDAT; - /* Note, "length" is sufficient here; we won't be adding * a null terminator later. The limit check in png_handle_chunk should be * sufficient. @@ -2606,10 +2598,6 @@ png_handle_iTXt(png_structrp png_ptr, png_inforp info_ptr, png_uint_32 length) } #endif - /* TODO: should not be necessary. */ - if ((png_ptr->mode & PNG_HAVE_IDAT) != 0) - png_ptr->mode |= PNG_AFTER_IDAT; - buffer = png_read_buffer(png_ptr, length+1); if (buffer == NULL)