#173 Unknown chunk writing location problem.
The libpng 1.5.2 documentation states that the location of a png_unknown_chunk can be set to 0, PNG_HAVE_IHDR, PNG_HAVE_PLTE or PNG_AFTER_IDAT. However, there is some disparity between checks in each of png_write_info_before_PLTE(), png_write_info() and png_write_end(), which are the three functions that make a call to write unknown chunks.
png_write_info_before_PLTE() checks to make sure that up->location && !(up->location & PNG_HAVE_PLTE) && !(up->location & PNG_HAVE_IDAT).
png_write_info() checks to make sure that up->location && (up->location & PNG_HAVE_PLTE) && !(up->location & PNG_HAVE_IDAT).
png_write_end() checks to make sure that up->location && (up->location & PNG_AFTER_IDAT).
The issue is that these checks are not mutually exclusive. If the unknown chunk location is set to PNG_AFTER_IDAT, it gets written both before the PLTE chunk and after the IDAT chunk. Furthermore, the PNG_AFTER_IDAT bit only gets set in png_ptr->mode after png_write_end() gets called, which is odd. Surely after png_write_end() is called, the mode should be something like PNG_HAVE_IEND (seeing as the IEND chunk has been written)?
My suggestion is to do away with PNG_AFTER_IDAT and use PNG_HAVE_IDAT instead.
There is one thing that is incorrect in the above statement - the PNG_AFTER_IDAT is set just before the IEND chunk is written.
Thus, my suggestion is probably just to change the allowed unknown chunk locations to PNG_HAVE_IHDR, PNG_HAVE_PLTE and PNG_HAVE_IDAT instead, and update the checks made in png_write_end() to reflect that.
There is indeed a bug -- PNG_AFTER_IDAT and the others are defined
in pngpriv.h in libpng-1.5.2. They need to be moved back into
png.h where applications can see them.
PNG_HAVE_IDAT means we have written
the first IDAT chunk. Using this would risk
writing unknown chunks in between IDATs.
That makes sense. Perhaps, then, the checks in png_write_info_before_PLTE() and png_write_info() need to be updated to ensure that !(up->location & PNG_AFTER_IDAT)?
Right at the moment, if you set the location of the unknown chunk to PNG_AFTER_IDAT, it gets written twice, once after the IHDR chunk and once after the IDAT chunks.
The macros PNG_AFTER_IDAT, etc., that are needed
by applications that call png_set_unknown_chunks()
will be visible to applications in 1.5.3 (currently only in
the GIT head, will be in 1.5.3beta07 shortly).
How are you testing this now, since the
macros aren't visible to your application? Are you
simply using the constants 0x01, 0x02, or 0x08?
(which would be fine). I'll try to replicate
(and fix) the behavior you are observing tonight.
Your suggested bugfix looks right.
I pushed your suggested change to the HEAD
of the git "devel" branch.
Your proposed bugfix is in libpng-1.5.3beta07. Please test it and let us know if the problem is solved.
When I was testing, I tried both the constant 0x08 and the preprocessor definitions by simply including pngpriv.h.
I just checked out libpng 1.5.3beta07, and it looks like the changes you made have fixed the problem. I also like the fact that the PNG_AFTER_IDAT... preprocessor definitions are now in png.h, it makes life easier.
Thanks!
Fixed in libpng-1.5.4, 1.4.8, 1.2.45, and 1.0.55. Thanks, ../glennrp