From fb5b3ac013b3ea6bf7f32871be75132380abbccd Mon Sep 17 00:00:00 2001 From: John Bowler Date: Sun, 16 Oct 2011 22:52:56 -0500 Subject: [PATCH] [libpng15] Prevent libpng overwriting unused bits at the end of the image when is not byte aligned, while reading. Prior to libpng-1.5.6 libpng would overwrite the end of the image if the row width is not an exact multiple of 8 bits and the image is not interlaced. --- ANNOUNCE | 4 ++++ CHANGES | 4 ++++ pngrutil.c | 37 +++++++++++++++++++++++++++-- pngvalid.c | 70 ++++++++++++++++++++++++++++++++++++++++++++---------- 4 files changed, 101 insertions(+), 14 deletions(-) diff --git a/ANNOUNCE b/ANNOUNCE index ce73b69aa..ca39fb433 100644 --- a/ANNOUNCE +++ b/ANNOUNCE @@ -80,6 +80,10 @@ Version 1.5.6beta06 [October 17, 2011] twice results in inaccuracies that can't be easily fixed. There is now a warning in the code if this is going to happen. Turned on multiple png_read_update_info in pngvalid transform tests. + Prevent libpng overwriting unused bits at the end of the image when when + it is not byte aligned, while reading. Prior to libpng-1.5.6 libpng would + overwrite the end of the image if the row width is not an exact multiple + of 8 bits and the image is not interlaced. Send comments/corrections/commendations to png-mng-implement at lists.sf.net: (subscription required; visit diff --git a/CHANGES b/CHANGES index 40b45509a..a1be5fce2 100644 --- a/CHANGES +++ b/CHANGES @@ -3641,6 +3641,10 @@ Version 1.5.6beta06 [October 17, 2011] twice results in inaccuracies that can't be easily fixed. There is now a warning in the code if this is going to happen. Turned on multiple png_read_update_info in pngvalid transform tests. + Prevent libpng overwriting unused bits at the end of the image when when + it is not byte aligned, while reading. Prior to libpng-1.5.6 libpng would + overwrite the end of the image if the row width is not an exact multiple + of 8 bits and the image is not interlaced. Send comments/corrections/commendations to png-mng-implement at lists.sf.net (subscription required; visit diff --git a/pngrutil.c b/pngrutil.c index e5feb18da..6a228a7e4 100644 --- a/pngrutil.c +++ b/pngrutil.c @@ -2786,6 +2786,9 @@ png_combine_row(png_structp png_ptr, png_bytep dp, int display) png_const_bytep sp = png_ptr->row_buf + 1; png_uint_32 row_width = png_ptr->width; unsigned int pass = png_ptr->pass; + png_bytep end_ptr = 0; + png_byte end_byte = 0; + unsigned int end_mask; png_debug(1, "in png_combine_row"); @@ -2807,6 +2810,26 @@ png_combine_row(png_structp png_ptr, png_bytep dp, int display) if (row_width == 0) png_error(png_ptr, "internal row width error"); + /* Preserve the last byte in cases where only part of it will be overwritten, + * the multiply below may overflow, we don't care because ANSI-C guarantees + * we get the low bits. + */ + end_mask = (pixel_depth * row_width) & 7; + if (end_mask != 0) + { + /* ep == NULL is a flag to say do nothing */ + end_ptr = dp + PNG_ROWBYTES(pixel_depth, row_width) - 1; + end_byte = *end_ptr; +# ifdef PNG_READ_PACKSWAP_SUPPORTED + if (png_ptr->transformations & PNG_PACKSWAP) /* little-endian byte */ + end_mask = 0xff << end_mask; + + else /* big-endian byte */ +# endif + end_mask = 0xff >> end_mask; + /* end_mask is now the bits to *keep* from the destination row */ + } + /* This reduces to a memcpy for non-interlaced images and for the case where * interlacing isn't supported or isn't done (in that case the caller gets a * sequence of interlace pass rows.) @@ -2969,7 +2992,7 @@ png_combine_row(png_structp png_ptr, png_bytep dp, int display) * this. */ if (row_width <= pixels_per_byte) - return; + break; /* May need to restore part of the last byte */ row_width -= pixels_per_byte; ++dp; @@ -3023,6 +3046,10 @@ png_combine_row(png_structp png_ptr, png_bytep dp, int display) /* And simply copy these bytes. Some optimization is possible here, * depending on the value of 'bytes_to_copy'. Speical case the low * byte counts, which we know to be frequent. + * + * Notice that these cases all 'return' rather than 'break' - this + * avoids an unnecessary test on whether to restore the last byte + * below. */ switch (bytes_to_copy) { @@ -3188,9 +3215,11 @@ png_combine_row(png_structp png_ptr, png_bytep dp, int display) bytes_to_copy = row_width; } } + + /* NOT REACHED*/ } /* pixel_depth >= 8 */ - /* NOT REACHED*/ + /* Here if pixel_depth < 8 to check 'end_ptr' below. */ } else #endif @@ -3200,6 +3229,10 @@ png_combine_row(png_structp png_ptr, png_bytep dp, int display) * destination row if it is a partial byte.) */ png_memcpy(dp, sp, PNG_ROWBYTES(pixel_depth, row_width)); + + /* Restore the overwritten bits from the last byte if necessary. */ + if (end_ptr != NULL) + *end_ptr = (png_byte)((end_byte & end_mask) | (*end_ptr & ~end_mask)); } #ifdef PNG_READ_INTERLACING_SUPPORTED diff --git a/pngvalid.c b/pngvalid.c index eb19f5292..434ce5337 100644 --- a/pngvalid.c +++ b/pngvalid.c @@ -433,12 +433,35 @@ pixel_copy(png_bytep toBuffer, png_uint_32 toIndex, memmove(toBuffer+(toIndex>>3), fromBuffer+(fromIndex>>3), pixelSize>>3); } +/* Copy a complete row of pixels, taking into account potential partial + * bytes at the end. + */ +static void +row_copy(png_bytep toBuffer, png_const_bytep fromBuffer, unsigned int bitWidth) +{ + memcpy(toBuffer, fromBuffer, bitWidth >> 3); + + if ((bitWidth & 7) != 0) + { + unsigned int mask; + + toBuffer += bitWidth >> 3; + fromBuffer += bitWidth >> 3; + /* The remaining bits are in the top of the byte, the mask is the bits to + * retain. + */ + mask = 0xff >> (bitWidth & 7); + *toBuffer = (png_byte)((*toBuffer & mask) | (*fromBuffer & ~mask)); + } +} + /* Compare pixels - they are assumed to start at the first byte in the * given buffers. */ static int pixel_cmp(png_const_bytep pa, png_const_bytep pb, png_uint_32 bit_width) { +#if PNG_LIBPNG_VER < 10506 if (memcmp(pa, pb, bit_width>>3) == 0) { png_uint_32 p; @@ -459,6 +482,13 @@ pixel_cmp(png_const_bytep pa, png_const_bytep pb, png_uint_32 bit_width) if (p == 0) return 0; } +#else + /* From libpng-1.5.6 the overwrite should be fixed, so compare the trailing + * bits too: + */ + if (memcmp(pa, pb, (bit_width+7)>>3) == 0) + return 0; +#endif /* Return the index of the changed byte. */ { @@ -908,11 +938,13 @@ store_ensure_image(png_store *ps, png_structp pp, int nImages, png_size_t cbRow, ps->cb_row = cbRow; ps->image_h = cRows; - /* For error checking, the whole buffer is set to '1' - this matches what - * happens with the 'size' test images on write and also matches the unused - * bits in the test rows. + /* For error checking, the whole buffer is set to 10110010 (0xb2 - 178). + * This deliberately doesn't match the bits in the size test image which are + * outside the image; these are set to 0xff (all 1). To make the row + * comparison work in the 'size' test case the size rows are pre-initialized + * to the same value prior to calling 'standard_row'. */ - memset(ps->image, 0xff, cb); + memset(ps->image, 178, cb); /* Then put in the marks. */ while (--nImages >= 0) @@ -4360,7 +4392,7 @@ progressive_row(png_structp pp, png_bytep new_row, png_uint_32 y, int pass) if (dp->interlace_type == PNG_INTERLACE_ADAM7) deinterlace_row(row, new_row, dp->pixel_size, dp->w, pass); else - memcpy(row, new_row, dp->cbRow); + row_copy(row, new_row, dp->pixel_size * dp->w); } else png_progressive_combine_row(pp, row, new_row); @@ -4404,10 +4436,12 @@ sequential_row(standard_display *dp, png_structp pp, png_infop pi, /* The following aids (to some extent) error detection - we can * see where png_read_row wrote. Use opposite values in row and - * display to make this easier. + * display to make this easier. Don't use 0xff (which is used in + * the image write code to fill unused bits) or 0 (which is a + * likely value to overwrite unused bits with). */ - memset(row, 0xff, sizeof row); - memset(display, 0, sizeof display); + memset(row, 0xc5, sizeof row); + memset(display, 0x5c, sizeof display); png_read_row(pp, row, display); @@ -4440,39 +4474,51 @@ standard_row_validate(standard_display *dp, png_structp pp, int where; png_byte std[STANDARD_ROWMAX]; - memset(std, 0xff, sizeof std); + /* The row must be pre-initialized to the magic number here for the size + * tests to pass: + */ + memset(std, 178, sizeof std); standard_row(pp, std, dp->id, y); /* At the end both the 'row' and 'display' arrays should end up identical. * In earlier passes 'row' will be partially filled in, with only the pixels * that have been read so far, but 'display' will have those pixels * replicated to fill the unread pixels while reading an interlaced image. +#if PNG_LIBPNG_VER < 10506 * The side effect inside the libpng sequential reader is that the 'row' * array retains the correct values for unwritten pixels within the row * bytes, while the 'display' array gets bits off the end of the image (in * the last byte) trashed. Unfortunately in the progressive reader the * row bytes are always trashed, so we always do a pixel_cmp here even though * a memcmp of all cbRow bytes will succeed for the sequential reader. +#endif */ if (iImage >= 0 && (where = pixel_cmp(std, store_image_row(dp->ps, pp, iImage, y), dp->bit_width)) != 0) { char msg[64]; - sprintf(msg, "PNG image row %d changed at byte %d", y, where-1); + sprintf(msg, "PNG image row[%d][%d] changed from %.2x to %.2x", y, + where-1, std[where-1], + store_image_row(dp->ps, pp, iImage, y)[where-1]); png_error(pp, msg); } +#if PNG_LIBPNG_VER < 10506 /* In this case use pixel_cmp because we need to compare a partial * byte at the end of the row if the row is not an exact multiple - * of 8 bits wide. + * of 8 bits wide. (This is fixed in libpng-1.5.6 and pixel_cmp is + * changed to match!) */ +#endif if (iDisplay >= 0 && (where = pixel_cmp(std, store_image_row(dp->ps, pp, iDisplay, y), dp->bit_width)) != 0) { char msg[64]; - sprintf(msg, "display row %d changed at byte %d", y, where-1); + sprintf(msg, "display row[%d][%d] changed from %.2x to %.2x", y, + where-1, std[where-1], + store_image_row(dp->ps, pp, iDisplay, y)[where-1]); png_error(pp, msg); } }