Possible off-by-one errors in png_do_check_palette_indexes
Adam Richter <[email protected]> Sun, 30 Aug 2020 11:38:15 -0700
| Newsgroups | gmane.comp.graphics.png.devel |
|---|---|
| Message-ID | <CAGn-TggPyfKdt1-Ob7_rarsRqU5D_2zqT=zFHQ7+K-+3FtHX9w@mail.gmail.com> |
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