From 864270e18f2cbeabcd235c900991d6bb40beb73a Mon Sep 17 00:00:00 2001 From: Glenn Randers-Pehrson Date: Fri, 10 Feb 2012 17:13:13 -0600 Subject: [PATCH] [libpng16] Fixed a memory overwrite bug in simplified read of RGB PNG with Fixed a memory overwrite bug in simplified read of RGB PNG with non-linear gamma Also bugs in the error checking in pngread.c and changed quite a lot of the checks in pngstest.c to be correct; either correctly written or not over-optimistic. The pngstest changes are insufficient to allow all possible RGB transforms to be passed; pngstest cmppixel needs to be rewritten to make it clearer which errors it allows and then changed to permit known inaccuracies. --- ANNOUNCE | 12 +- CHANGES | 10 +- contrib/libtests/pngstest.c | 245 ++++++++++++++++++++---------------- pngread.c | 44 ++++--- 4 files changed, 184 insertions(+), 127 deletions(-) diff --git a/ANNOUNCE b/ANNOUNCE index 7ee431041..7756606cb 100644 --- a/ANNOUNCE +++ b/ANNOUNCE @@ -1,5 +1,5 @@ -Libpng 1.6.0beta11 - February 4, 2012 +Libpng 1.6.0beta11 - February 10, 2012 This is not intended to be a public release. It will be replaced within a few weeks by a public version or by another test version. @@ -176,7 +176,7 @@ Version 1.6.0beta10 [February 3, 2012] Updated the prebuilt configure files to current condition. Revised INSTALL information about autogen.sh; it works in tar distributions. -Version 1.6.0beta11 [February 4, 2012] +Version 1.6.0beta11 [February 10, 2012] Fix character count in pngstest command in projects/owatcom/pngstest.tgt Revised test-pngstest.sh to report PASS/FAIL for each image. Updated documentation about the simplified API. @@ -188,6 +188,14 @@ Version 1.6.0beta11 [February 4, 2012] error of 14.99/255; 0.5/255^(1/2.2). pngstest now uses 15 for the permitted 8-bit error. This may still not be enough because of arithmetic error. + Removed some unused arrays (with #ifdef) from png_read_push_finish_row(). + Fixed a memory overwrite bug in simplified read of RGB PNG with + non-linear gamma Also bugs in the error checking in pngread.c and changed + quite a lot of the checks in pngstest.c to be correct; either correctly + written or not over-optimistic. The pngstest changes are insufficient to + allow all possible RGB transforms to be passed; pngstest cmppixel needs + to be rewritten to make it clearer which errors it allows and then changed + to permit known inaccuracies. Send comments/corrections/commendations to png-mng-implement at lists.sf.net (subscription required; visit diff --git a/CHANGES b/CHANGES index 4fb9111d9..3342c7395 100644 --- a/CHANGES +++ b/CHANGES @@ -3927,7 +3927,7 @@ Version 1.6.0beta10 [February 3, 2012] Updated the prebuilt configure files to current condition. Revised INSTALL information about autogen.sh; it works in tar distributions. -Version 1.6.0beta11 [February 4, 2012] +Version 1.6.0beta11 [February 10, 2012] Fix character count in pngstest command in projects/owatcom/pngstest.tgt Revised test-pngstest.sh to report PASS/FAIL for each image. Updated documentation about the simplified API. @@ -3939,6 +3939,14 @@ Version 1.6.0beta11 [February 4, 2012] error of 14.99/255; 0.5/255^(1/2.2). pngstest now uses 15 for the permitted 8-bit error. This may still not be enough because of arithmetic error. + Removed some unused arrays (with #ifdef) from png_read_push_finish_row(). + Fixed a memory overwrite bug in simplified read of RGB PNG with + non-linear gamma Also bugs in the error checking in pngread.c and changed + quite a lot of the checks in pngstest.c to be correct; either correctly + written or not over-optimistic. The pngstest changes are insufficient to + allow all possible RGB transforms to be passed; pngstest cmppixel needs + to be rewritten to make it clearer which errors it allows and then changed + to permit known inaccuracies. Send comments/corrections/commendations to png-mng-implement at lists.sf.net (subscription required; visit diff --git a/contrib/libtests/pngstest.c b/contrib/libtests/pngstest.c index bc42dfb9f..74e91dcbc 100644 --- a/contrib/libtests/pngstest.c +++ b/contrib/libtests/pngstest.c @@ -121,6 +121,19 @@ isRGB(int fixed_linear) return sRGB(fixed_linear / 65535.); } +static png_byte +unpremultiply(int component, int alpha) +{ + if (alpha == 0) + return 255; /* Arbitrary, but consistent with the libpng code */ + + else if (alpha >= 65535) + return isRGB(component); + + else + return sRGB((double)component / alpha); +} + static png_uint_16 ilineara(int fixed_srgb, int alpha) { @@ -414,7 +427,7 @@ static void format_default(format_list *pf, int redundant) if (redundant) { int i; - + /* set everything, including flags that are pointless */ for (i=0; ibits[i] = ~(png_uint_32)0; @@ -458,7 +471,6 @@ typedef struct ptrdiff_t stride; png_size_t bufsize; png_size_t allocsize; - /* png_color background; */ char tmpfile_name[32]; png_uint_16 colormap[256*4]; } @@ -671,7 +683,7 @@ typedef struct * The API returns false if an error is detected; this can only be if the alpha * value is less than the component in the linear case. */ -static int +static int get_pixel(Image *image, Pixel *pixel, png_const_bytep pp) { png_uint_32 format = image->image.format; @@ -848,14 +860,89 @@ get_pixel(Image *image, Pixel *pixel, png_const_bytep pp) static int error_to_linear = 811; /* by experiment */ static int error_to_linear_grayscale = 424; /* by experiment */ static int error_to_sRGB = 6; /* by experiment */ -static int error_to_sRGB_grayscale = 15; /* libpng error by calculation */ -static int error_in_compose = 0; -static int error_via_linear = 14; /* by experiment */ +static int error_to_sRGB_grayscale = 17; /* libpng error by calculation + + 2 by experiment */ +static int error_in_compose = 2; /* by experiment */ static int error_in_premultiply = 1; +/* The following is *just* the result of a round trip from 8-bit sRGB to linear + * then back to 8-bit sRGB when it is done by libpng. There are two problems: + * + * 1) libpng currently uses a 2.2 power law with no linear segment, this results + * in instability in the low values and even with 16-bit precision sRGB(1) ends + * up mapping to sRGB(0) as a result of rounding in the 16-bit representation. + * This gives an error of 1 in the handling of value 1 only. + * + * 2) libpng currently uses an intermediate 8-bit linear value in gamma + * correction of 8-bit values. This results in many more errors, the worse of + * which is mapping sRGB(14) to sRGB(0). + * + * The general 'error_via_linear' is more complex because of pre-multiplication, + * this compounds the 8-bit errors according to the alpha value of the pixel. + * As a result 256 values are pre-calculated for error_via_linear. + */ +static int error_in_libpng_gamma; +static int error_via_linear[256]; /* Indexed by 8-bit alpha */ + +static void +init_error_via_linear(void) +{ + int alpha; + + error_via_linear[0] = 255; /* transparent pixel */ + + for (alpha=1; alpha<=255; ++alpha) + { + /* 16-bit values less than 128.5 get rounded to 8-bit 0 and so the worst + * case error arises with 16-bit 128.5, work out what sRGB + * (non-associated) value generates 128.5; any value less than this is + * going to map to 0, so the worst error is floor(value). + * + * Note that errors are considerably higher (more than a factor of 2) + * because libpng uses a simple power law for sRGB data at present. + * + * Add .1 for arithmetic errors inside libpng. + */ + double v = floor(255*pow(.5/*(128.5 * 255 / 65535)*/ / alpha, 1/2.2)+.1); + + error_via_linear[alpha] = (int)v; + } + + /* This is actually 14.99, but, despite the closeness to 15, 14 seems to work + * ok in this case. + */ + error_in_libpng_gamma = 14; +} + +/* A precalculated floating point linear background value, for grayscale the + * green channel is used. + */ +typedef struct +{ + double r, g, b; +} +precalc_background; + +static void +init_background(precalc_background *ppb, const png_color *background) +{ + if (background == NULL) + { + double v = linear_from_sRGB(BUFFER_INIT8/255.); + ppb->r = ppb->g = ppb->b = v; + } + + else + { + ppb->r = linear_from_sRGB(background->red/255.); + ppb->g = linear_from_sRGB(background->green/255.); + ppb->b = linear_from_sRGB(background->blue/255.); + } +} + static const char * -cmppixel(Pixel *a, Pixel *b, const png_color *background, int via_linear, - int multiple_algorithms) +cmppixel(Pixel *a, Pixel *b, const precalc_background *background, + int via_linear, int multiple_algorithms) { int error_limit = 0; @@ -1120,63 +1207,25 @@ cmppixel(Pixel *a, Pixel *b, const png_color *background, int via_linear, * see where the parameter is set to non-zero below. */ if (!(a->format & PNG_FORMAT_FLAG_LINEAR) && via_linear) - error_limit = error_via_linear; - - if (b->format & PNG_FORMAT_FLAG_COLOR) - err = "8-bit color mismatch"; - - else - err = "8-bit gray mismatch"; - - /* If the original data had an alpha channel and was not pre-multiplied - * pre-multiplication may lose precision in non-opaque pixel values. If - * the output is linear the premultiplied 16-bit values will be used, but - * if 'via_linear' is set an intermediate 16-bit pre-multiplied form has - * been used and this must be taken into account here. - */ - if (via_linear && (a->format & PNG_FORMAT_FLAG_ALPHA) && - !(a->format & PNG_FORMAT_FLAG_LINEAR) && - a->a16 < 65535) { - if (a->a16 > 0) - { - /* First calculate the rounded 16-bit component values, (r,g,b) or y - * as appropriate, then back-calculate the 8-bit values for - * comparison below. - */ - if (a->format & PNG_FORMAT_FLAG_COLOR) - { - double r = closestinteger((65535. * a->r16) / a->a16)/65535; - double g = closestinteger((65535. * a->g16) / a->a16)/65535; - double blue = closestinteger((65535. * a->b16) / a->a16)/65535; - - a->r16 = u16d(r * a->a16); - a->g16 = u16d(g * a->a16); - a->b16 = u16d(blue * a->a16); - a->y16 = u16d(YfromRGBint(a->r16, a->g16, a->b16)); - - a->r8 = u8d(r * 255); - a->g8 = u8d(g * 255); - a->b8 = u8d(blue * 255); - a->y8 = u8d(255 * YfromRGB(r, g, blue)); - } - - else - { - double y = closestinteger((65535. * a->y16) / a->a16)/65535.; - - a->b16 = a->g16 = a->r16 = a->y16 = u16d(y * a->a16); - a->b8 = a->g8 = a->r8 = a->y8 = u8d(255 * y); - } - } + if (a->a8 > 0) + error_limit = error_via_linear[a->a8]; else { - a->r16 = a->g16 = a->b16 = a->y16 = 0; - a->r8 = a->g8 = a->b8 = a->y8 = 255; + /* The transform to linear will have set the component values to + * 65535, so they will come back as 255: + */ + error_limit = 0; + a->y8 = a->r8 = a->g8 = a->b8 = 255; } } + if (b->format & PNG_FORMAT_FLAG_COLOR) + err = "8-bit color mismatch"; + + else + err = "8-bit gray mismatch"; if (b->format & PNG_FORMAT_FLAG_ALPHA) { @@ -1225,25 +1274,14 @@ cmppixel(Pixel *a, Pixel *b, const png_color *background, int via_linear, { if (b->format & PNG_FORMAT_FLAG_COLOR) { - double r, g, blue; - - r = (255. * b->r16)/b->a16; - b->r8 = u8d(r); - - g = (255. * b->g16)/b->a16; - b->g8 = u8d(g); - - blue = (255. * b->b16)/b->a16; - b->b8 = u8d(blue); - - b->y8 = u8d(YfromRGB(r, g, blue)); + b->r8 = unpremultiply(b->r16, b->a16); + b->g8 = unpremultiply(b->g16, b->a16); + b->b8 = unpremultiply(b->b16, b->a16); + b->y8 = unpremultiply(b->y16, b->a16); } else - { - b->r8 = b->g8 = b->b8 = b->y8 = - u8d((255. * b->y16)/b->a16); - } + b->r8 = b->g8 = b->b8 = b->y8 = unpremultiply(b->y16, b->a16); } else @@ -1279,45 +1317,25 @@ cmppixel(Pixel *a, Pixel *b, const png_color *background, int via_linear, a->b8 = isRGB(a->b16); a->y8 = isRGB(a->y16); - /* There should be no libpng error in this (ideally) */ - error_limit = 0; - } - - else if (background == NULL) - { - double add = alpha * linear_from_sRGB(BUFFER_INIT8/255.); - double r, g, blue, y; - - r = a->r16 + add; - a->r16 = u16d(r); - a->r8 = sRGB(r/65535); - - g = a->g16 + add; - a->g16 = u16d(g); - a->g8 = sRGB(g/65535); - - blue = a->b16 + add; - a->b16 = u16d(blue); - a->b8 = sRGB(blue/65535); - - y = YfromRGB(r, g, blue); - a->y16 = u16d(y); - a->y8 = sRGB(y/65535); + /* There should be no libpng error in this (ideally), but the + * libpng gamma correction code has off-by-one errors. + */ + error_limit = error_in_libpng_gamma; } else { double r, g, blue, y; - r = a->r16 + alpha * linear_from_sRGB(background->red/255.); + r = a->r16 + alpha * background->r; a->r16 = u16d(r); a->r8 = sRGB(r/65535); - g = a->g16 + alpha * linear_from_sRGB(background->green/255.); + g = a->g16 + alpha * background->g; a->g16 = u16d(g); a->g8 = sRGB(g/65535); - blue = a->b16 + alpha * linear_from_sRGB(background->blue/255.); + blue = a->b16 + alpha * background->b; a->b16 = u16d(blue); a->b8 = sRGB(blue/65535); @@ -1334,7 +1352,11 @@ cmppixel(Pixel *a, Pixel *b, const png_color *background, int via_linear, if (via_linear) { a->r8 = a->g8 = a->b8 = a->y8 = isRGB(a->y16); - error_limit = 0; + + /* There should be no libpng error in this (ideally), but the + * libpng gamma correction code has off-by-one errors. + */ + error_limit = error_in_libpng_gamma; } else @@ -1342,8 +1364,7 @@ cmppixel(Pixel *a, Pixel *b, const png_color *background, int via_linear, /* When the output is gray the background comes from just the * green channel. */ - double y = a->y16 + alpha * linear_from_sRGB( - (background == NULL ? BUFFER_INIT8 : background->green)/255.); + double y = a->y16 + alpha * background->g; a->r16 = a->g16 = a->b16 = a->y16 = u16d(y); a->r8 = a->g8 = a->b8 = a->y8 = sRGB(y/65535); @@ -1557,6 +1578,7 @@ compare_two_images(Image *a, Image *b, int via_linear, int two_algorithms = ((formata ^ formatb) & PNG_FORMAT_FLAG_COLORMAP) != 0; int result = 1; unsigned int check_alpha = 0; /* must be zero or one */ + precalc_background pb; png_byte swap_mask[4]; png_uint_32 x, y; png_const_bytep ppa, ppb; @@ -1566,6 +1588,9 @@ compare_two_images(Image *a, Image *b, int via_linear, return logerror(a, a->file_name, ": width x height changed: ", b->file_name); + /* Set up the background */ + init_background(&pb, background); + /* Find the first row and inter-row space. */ if (!(formata & PNG_FORMAT_FLAG_COLORMAP) && (formata & PNG_FORMAT_FLAG_LINEAR)) @@ -1635,6 +1660,7 @@ compare_two_images(Image *a, Image *b, int via_linear, { /* Only 'b' has an alpha channel */ check_alpha = 1; + if (formatb & PNG_FORMAT_FLAG_AFIRST) { bstart = 1; @@ -1702,7 +1728,7 @@ compare_two_images(Image *a, Image *b, int via_linear, return badpixel(b, entry, 0, &pixel_b, "bad palette entry value"); - if (cmppixel(&pixel_a, &pixel_b, background, via_linear, + if (cmppixel(&pixel_a, &pixel_b, &pb, via_linear, 0/*multiple_algorithms*/) != NULL) break; @@ -1910,7 +1936,7 @@ compare_two_images(Image *a, Image *b, int via_linear, while (x < width) { png_const_bytep colormap_a = (png_const_bytep)a->colormap; - + switch (channels) { case 4: @@ -1948,7 +1974,7 @@ compare_two_images(Image *a, Image *b, int via_linear, while (x < width) { png_const_bytep colormap_b = (png_const_bytep)b->colormap; - + switch (channels) { case 4: @@ -2000,7 +2026,7 @@ compare_two_images(Image *a, Image *b, int via_linear, if (!get_pixel(b, &pixel_b, ppb)) return badpixel(b, x, y, &pixel_b, "bad pixel value"); - mismatch = cmppixel(&pixel_a, &pixel_b, background, via_linear, + mismatch = cmppixel(&pixel_a, &pixel_b, &pb, via_linear, two_algorithms); if (mismatch != NULL) @@ -2297,7 +2323,7 @@ testimage(Image *image, png_uint_32 opts, format_list *pf) Image output; newimage(&output); - + result = 1; /* Use the low bit of 'counter' to indicate whether or not to do alpha @@ -2419,6 +2445,7 @@ main(int argc, char **argv) int retval = 0; int c; + init_error_via_linear(); format_init(&formats); for (c=1; crow_number & 1) || png_ptr->width < 2) { @@ -1981,7 +1982,7 @@ make_rgb_colormap(png_image_read_control *display) #define PNG_RGB_INDEX(r,g,b) \ ((png_byte)(6 * (6 * PNG_DIV51(r) + PNG_DIV51(g)) + PNG_DIV51(b))) -static int +static int png_image_read_colormap(png_voidp argument) { png_image_read_control *display = @@ -2672,7 +2673,7 @@ png_image_read_colormap(png_voidp argument) output_encoding); } } - + else png_create_colormap_entry(display, i, colormap[i].red, colormap[i].green, colormap[i].blue, @@ -3070,7 +3071,7 @@ png_image_read_colormapped(png_voidp argument) { png_uint_32 y = image->height; png_bytep row = display->first_row; - + while (y-- > 0) { png_read_row(png_ptr, row, NULL); @@ -3230,14 +3231,14 @@ png_image_read_background(png_voidp argument) if ((png_ptr->transformations & PNG_COMPOSE) != 0) png_error(png_ptr, "unexpected compose"); - /* The palette code zaps PNG_GAMMA in place... */ - if ((png_ptr->color_type & PNG_COLOR_MASK_PALETTE) == 0 && - (png_ptr->transformations & PNG_GAMMA) == 0) - png_error(png_ptr, "lost gamma correction"); - if (png_get_channels(png_ptr, info_ptr) != 2) png_error(png_ptr, "lost/gained channels"); + /* Expect the 8-bit case to always remove the alpha channel */ + if ((image->format & PNG_FORMAT_FLAG_LINEAR) == 0 && + (image->format & PNG_FORMAT_FLAG_ALPHA) != 0) + png_error(png_ptr, "unexpected 8-bit transformation"); + switch (png_ptr->interlaced) { case PNG_INTERLACE_NONE: @@ -3293,7 +3294,7 @@ png_image_read_background(png_voidp argument) startx = 0; stepx = stepy = 1; } - + if (display->background == NULL) { for (; yfirst_row; ptrdiff_t step_row = display->row_bytes; - unsigned int outchannels = png_get_channels(png_ptr, info_ptr); int preserve_alpha = (image->format & PNG_FORMAT_FLAG_ALPHA) != 0; + unsigned int outchannels = 1+preserve_alpha; int swap_alpha = 0; if (preserve_alpha && (image->format & PNG_FORMAT_FLAG_AFIRST)) @@ -3399,9 +3400,11 @@ png_image_read_background(png_voidp argument) for (pass = 0; pass < passes; ++pass) { - unsigned int startx, stepx, stepy; /* all in pixels */ + unsigned int startx, stepx, stepy; png_uint_32 y; + /* The 'x' start and step are adjusted to output components here. + */ if (png_ptr->interlaced == PNG_INTERLACE_ADAM7) { /* The row may be empty for a short image: */ @@ -3454,7 +3457,7 @@ png_image_read_background(png_voidp argument) component = 0; outrow[swap_alpha] = (png_uint_16)component; - if (outchannels > 1) + if (preserve_alpha) outrow[1 ^ swap_alpha] = alpha; inrow += 2; /* components and alpha channel */ @@ -3489,7 +3492,7 @@ png_image_read_direct(png_voidp argument) * need 8 bits minimum, no palette and expanded tRNS. */ png_set_expand(png_ptr); - + /* Now check the format to see if it was modified. */ { png_uint_32 base_format = png_image_format(png_ptr) & @@ -3800,10 +3803,21 @@ png_image_read_direct(png_voidp argument) # endif # ifdef PNG_FORMAT_AFIRST_SUPPORTED - if (png_ptr->transformations & PNG_SWAP_ALPHA || + if (do_local_background == 2) + { + if (format & PNG_FORMAT_FLAG_AFIRST) + info_format |= PNG_FORMAT_FLAG_AFIRST; + } + + if ((png_ptr->transformations & PNG_SWAP_ALPHA) != 0 || ((png_ptr->transformations & PNG_ADD_ALPHA) != 0 && (png_ptr->flags & PNG_FLAG_FILLER_AFTER) == 0)) + { + if (do_local_background == 2) + png_error(png_ptr, "unexpected alpha swap transformation"); + info_format |= PNG_FORMAT_FLAG_AFIRST; + } # endif /* This is actually an internal error. */ @@ -3869,7 +3883,7 @@ png_image_read_direct(png_voidp argument) { png_uint_32 y = image->height; png_bytep row = display->first_row; - + while (y-- > 0) { png_read_row(png_ptr, row, NULL); -- GitLab