Re: Endless loop in version 1.6.26

Well, what I was saying is that the change should *NOT* be applied to 1.7
because it already does this via extensions/new APIs which allow the app to
control the behavior.

In 1.6 the original change was, I think, meant to add a feature, "Ignore
errors that change the IDAT while allowing it still to be read."  That
feature needs a new API (it is an API change) or a new version of libpng
with an implicit API change.

Part of the new API from libpng 1.7 could be backported.  The full thing is
actually a macro:

#define png_set_error_action(png_ptr, what, action)\
   (png_setting((png_ptr), PNG_SRW_ERROR_HANDLING, (what), (action)))
   /* Control the handling of 'benign' errors; errors that can be handled in
    * some way.  The action is one of the following values:
    */
#define PNG_IGNORE 0 /* ignore the error; no warning or error message */
#define PNG_WARN   1 /* call png_warning with an appropriate error message
*/
#define PNG_ERROR  2 /* call png_error with the error message */
   /* 'what' is a list (bit mask) of the errors to set: */
#define PNG_BENIGN_ERRORS (1U)
#define PNG_APP_WARNINGS  (2U)
#define PNG_APP_ERRORS    (4U)
#define PNG_IDAT_ERRORS   (8U)
#define PNG_SAFE_ERRORS
(PNG_BENIGN_ERRORS+PNG_APP_WARNINGS+PNG_APP_ERRORS)
#define PNG_ALL_ERRORS    (PNG_SAFE_ERRORS+PNG_IDAT_ERRORS)

The obvious thing to do in 1.6 if it is to remain API compatible with 1.7
is to just define a new API (possible a png_set_option macro):

png_ignore_IDAT_errors(png_ptr);

Then in 1.6 it is "just" necessary to go through and change every relevant
case where the existing inflate code does a png_error into a (warn ?
png_benign_error : png_error) (or, better, a new internal function
png_IDAT_error because the :? meme causes warnings from one of the testing
tools IRC).

The client who wanted the orignal change will need to add a
png_ignore_IDAT_errors() call, but that shouldn't be too much to ask
because the change required an API add in zlib IRC.

This means that the testing required is restricted to those apps which turn
the stuff on.  I don't know if FireFox actually wants this change but it
seems to me that it definitely should be something an app turns on
explicitly (if it happens in a minor release) because, as shown by this
report, it is capable of causing a serious bug.

Some additional points:

1) libpng 1.7 png_inflate probably has many serious bugs because of the
extent of the changes.

2) One advantage of png_error is that it changes a combinatorial test
problem into a linear one.  With png_error any detectable error in a
deflate stream stops the decoding.  However with png_warning different
combinations of errors cause potentially different code paths which all
need to be tested; this is difficult.

3) Point (2) extends into the application; if the output PNG is incomplete
or altered from the original then bytes within the output data may contain
information from the current process.  The code in 1.7 attempts to address
this by changing those bytes to a fixed value.  Even so displaying a PNG
with incorrect data (even black) leads to bug reports; apps may prefer to
be told and not display the PNG at all.  It needs to be an application
choice.

John Bowler <[email protected]>

------------------------------------------------------------------------------
Developer Access Program for Intel Xeon Phi Processors
Access to Intel Xeon Phi processor-based developer platforms.
With one year of Intel Parallel Studio XE.
Training and support from Colfax.
Order your platform today.http://sdm.link/intel

_______________________________________________
png-mng-implement mailing list
png-mng-implement-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/png-mng-implement