Re: Possible off-by-one errors in png_do_check_palette_indexes

Hi, everyone.

Looking at the code more carefully, it appears to me that the
only thing that png_do_check_palette_indexes() is used for is
to generate warning message if the image contains a reference
to a palette entry that is larger than the palette.  So, ignoring the
last pixel of each row may cause this warning message not to
be generated when it should, but should not otherwise change
the behavior of a program, unless that client relies on the value
of png_ptr->num_palette_max (which png_do_check_palette_indexes
sets).  That field name does not occur in any of the subdirectory
trees of libpng (at least for the master branch and, therefore, the
libpng16 branch).

So, probably this bug (if I am right that it is a bug) has not
resulted in any lost pixel data.

Adam

On Sun, Aug 30, 2020 at 11:38 AM Adam Richter <[email protected]> wrote:
>
> In my message with the patches that I posted yesterday, I forgot to
> mention that I spotted what I think is an off-by-one error in
> png_do_check_palette_indexes() in pngtrans.c, line 711, at the
> initialization of the variable rp:
>
>       png_bytep rp = png_ptr->row_buf + row_info->rowbytes - 1;
>
> I believe that the "- 1" should be removed.  The purpose of this line
> is to make rp point to the last byte of data in the row, but
> png_ptr->row_buf starts with an extra byte at the beginning, which is
> not counted in row_info->rowbytes.  The extra byte, part of an
> optimization for the cases where data is to be written without
> filtering, appears to be taken into account in the rest of the
> function.
>
> The last patch in the message that I posted yesterday, work in
> progress about attempting to avoid data copying if multiple lines are
> presented, includes changes to that function that also should fix this
> along the way.
>
> The purpose of that function is basically to record the maximum pixel
> value seen so far.  If the maximum pixel value occurs only in the
> right-most column, the result without this change can be less than it
> should be.  I am not sure what the consequences of that are.
>
> By the way, looking at that function more closely as I write this
> message, it appears to me that there is a performance bug.  At the
> very beginning of the function, it looks like there is a mistake in
> the test intended to skip the whole function if the maximum pixel
> value has already been seen, another off-by-one error, line 701 in the
> sourceforge git master branch version of the file:
>
> void /* PRIVATE */
> png_do_check_palette_indexes(png_structrp png_ptr, png_row_infop row_info)
> {
>    if (png_ptr->num_palette < (1 << row_info->bit_depth) &&
>                                               ^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> That expression should be "((1 << row_info->bit_depth) - 1)".
>
> There appear to be other optimization opportunities in the function.
> I plan to submit a patch to address at least what I think are the
> errors without requiring integration of my other changes unless anyone
> beats me to it or requests otherwise.
>
> Adam