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 <ctruta@gmail.com>
Signed-off-by: John Bowler <jbowler@acm.org>
Signed-off-by: Cosmin Truta <ctruta@gmail.com>
This commit is contained in:
John Bowler 2025-06-24 14:18:37 -07:00 committed by Cosmin Truta
parent 4266c75f40
commit 2e5f296bfa
3 changed files with 14 additions and 13 deletions

View File

@ -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)

View File

@ -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);

View File

@ -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)