diff --git a/ANNOUNCE b/ANNOUNCE index 610f82a69..df496a3c9 100644 --- a/ANNOUNCE +++ b/ANNOUNCE @@ -1,5 +1,5 @@ -Libpng 1.6.0beta28 - August 18, 2012 +Libpng 1.6.0beta28 - August 25, 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. @@ -442,7 +442,7 @@ Version 1.6.0beta27 [August 11, 2012] Work around gcc 3.x and Microsoft Visual Studio 2010 complaints. Both object to the split initialization of num_chunks. -Version 1.6.0beta28 [August 18, 2012] +Version 1.6.0beta28 [August 25, 2012] Unknown handling fixes and clean up. This adds more correct option control of the unknown handling, corrects the pre-existing bug where the per-chunk 'keep' setting is ignored and makes it possible to skip @@ -469,6 +469,18 @@ Version 1.6.0beta28 [August 18, 2012] SAVE_UNKNOWN_CHUNKS is turned off *and* the application does not set a user callback an unknown chunk will not be read, leading to a read error, which was revealed by the "tunknown" test. + Cleaned up and corrected ICC profile handling. + contrib/libtests/makepng: corrected 'rgb' and 'gray' cases. profile_error + messages could be truncated; made a correct buffer size calculation and + adjusted pngerror.c appropriately. png_icc_check_* checking improved; + changed the functions to receive the correct color type of the PNG on read + or write and check that it matches the color space of the profile (despite + what the comments said before, there is danger in assuming the app will + cope correctly with an RGB profile on a grayscale image and, since it + violates the PNG spec, allowing it is certain to produce inconsistent + app behavior and might even cause app crashes.) Check that profiles + contain the tags needed to process the PNG (tags all required by the ICC + spec). Removed unused PNG_STATIC from pngpriv.h. Send comments/corrections/commendations to png-mng-implement at lists.sf.net (subscription required; visit diff --git a/CHANGES b/CHANGES index f85d94bef..4fc380296 100644 --- a/CHANGES +++ b/CHANGES @@ -4193,7 +4193,7 @@ Version 1.6.0beta27 [August 11, 2012] Work around gcc 3.x and Microsoft Visual Studio 2010 complaints. Both object to the split initialization of num_chunks. -Version 1.6.0beta28 [August 18, 2012] +Version 1.6.0beta28 [August 25, 2012] Unknown handling fixes and clean up. This adds more correct option control of the unknown handling, corrects the pre-existing bug where the per-chunk 'keep' setting is ignored and makes it possible to skip @@ -4220,6 +4220,18 @@ Version 1.6.0beta28 [August 18, 2012] SAVE_UNKNOWN_CHUNKS is turned off *and* the application does not set a user callback an unknown chunk will not be read, leading to a read error, which was revealed by the "tunknown" test. + Cleaned up and corrected ICC profile handling. + contrib/libtests/makepng: corrected 'rgb' and 'gray' cases. profile_error + messages could be truncated; made a correct buffer size calculation and + adjusted pngerror.c appropriately. png_icc_check_* checking improved; + changed the functions to receive the correct color type of the PNG on read + or write and check that it matches the color space of the profile (despite + what the comments said before, there is danger in assuming the app will + cope correctly with an RGB profile on a grayscale image and, since it + violates the PNG spec, allowing it is certain to produce inconsistent + app behavior and might even cause app crashes.) Check that profiles + contain the tags needed to process the PNG (tags all required by the ICC + spec). Removed unused PNG_STATIC from pngpriv.h. Send comments/corrections/commendations to png-mng-implement at lists.sf.net (subscription required; visit diff --git a/contrib/libtests/makepng.c b/contrib/libtests/makepng.c index a0bfb4bae..5fdaa67d9 100644 --- a/contrib/libtests/makepng.c +++ b/contrib/libtests/makepng.c @@ -72,6 +72,7 @@ #include #include #include +#include #include #include @@ -1125,7 +1126,7 @@ main(int argc, char **argv) if (strncmp(arg, "gray", 4) == 0) { - if (arg[5] == 0) + if (arg[4] == 0) { color_type = PNG_COLOR_TYPE_GRAY; continue; @@ -1142,7 +1143,7 @@ main(int argc, char **argv) if (strncmp(arg, "rgb", 3) == 0) { - if (arg[4] == 0) + if (arg[3] == 0) { color_type = PNG_COLOR_TYPE_RGB; continue; @@ -1157,7 +1158,7 @@ main(int argc, char **argv) } } - if (color_type == 8) + if (color_type == 8 && isdigit(arg[0])) { color_type = atoi(arg); if (color_type < 0 || color_type > 6 || color_type == 1 || @@ -1170,7 +1171,7 @@ main(int argc, char **argv) continue; } - if (bit_depth == 32) + if (bit_depth == 32 && isdigit(arg[0])) { bit_depth = atoi(arg); if (bit_depth <= 0 || bit_depth > 16 || diff --git a/png.c b/png.c index d1789d6a0..861bde124 100644 --- a/png.c +++ b/png.c @@ -749,13 +749,13 @@ png_get_copyright(png_const_structrp png_ptr) #else # ifdef __STDC__ return PNG_STRING_NEWLINE \ - "libpng version 1.6.0beta28 - August 22, 2012" PNG_STRING_NEWLINE \ + "libpng version 1.6.0beta28 - August 25, 2012" PNG_STRING_NEWLINE \ "Copyright (c) 1998-2012 Glenn Randers-Pehrson" PNG_STRING_NEWLINE \ "Copyright (c) 1996-1997 Andreas Dilger" PNG_STRING_NEWLINE \ "Copyright (c) 1995-1996 Guy Eric Schalnat, Group 42, Inc." \ PNG_STRING_NEWLINE; # else - return "libpng version 1.6.0beta28 - August 22, 2012\ + return "libpng version 1.6.0beta28 - August 25, 2012\ Copyright (c) 1998-2012 Glenn Randers-Pehrson\ Copyright (c) 1996-1997 Andreas Dilger\ Copyright (c) 1995-1996 Guy Eric Schalnat, Group 42, Inc."; @@ -1693,24 +1693,25 @@ profile_error(png_const_structrp png_ptr, png_colorspacerp colorspace, png_const_charp name, png_alloc_size_t value, png_const_charp reason) { size_t pos; - char message[256]; + char message[196]; /* see below for calculation */ if (colorspace != NULL) colorspace->flags |= PNG_COLORSPACE_INVALID; - pos = png_safecat(message, (sizeof message), 0, "profile '"); - pos = png_safecat(message, pos+79, pos, name); - pos = png_safecat(message, (sizeof message), pos, "': "); + pos = png_safecat(message, (sizeof message), 0, "profile '"); /* 9 chars */ + pos = png_safecat(message, pos+79, pos, name); /* Truncate to 79 chars */ + pos = png_safecat(message, (sizeof message), pos, "': "); /* +2 = 90 */ # ifdef PNG_WARNINGS_SUPPORTED { - char number[PNG_NUMBER_BUFFER_SIZE]; + char number[PNG_NUMBER_BUFFER_SIZE]; /* +24 = 114*/ pos = png_safecat(message, (sizeof message), pos, png_format_number(number, number+(sizeof number), PNG_NUMBER_FORMAT_x, value)); } - pos = png_safecat(message, (sizeof message), pos, ": "); + pos = png_safecat(message, (sizeof message), pos, "h: "); /* +2 = 116 */ # endif + /* The 'reason' is an arbitrary message, allow +79 maximum 195 */ pos = png_safecat(message, (sizeof message), pos, reason); if (colorspace != NULL) @@ -1874,7 +1875,7 @@ png_icc_check_length(png_const_structrp png_ptr, png_colorspacerp colorspace, int /* PRIVATE */ png_icc_check_header(png_const_structrp png_ptr, png_colorspacerp colorspace, png_const_charp name, png_uint_32 profile_length, - png_const_bytep profile/* first 132 bytes only */) + png_const_bytep profile/* first 132 bytes only */, int color_type) { png_uint_32 temp; @@ -1930,16 +1931,37 @@ png_icc_check_header(png_const_structrp png_ptr, png_colorspacerp colorspace, * 6), or a greyscale colour space for greyscale images (PNG colour types 0 * and 4)." * - * This code does not check the color type because png_set_iCCP may be called - * before png_set_IHDR on write and because, anyway, the PNG spec is - * fundamentally flawed: RGB profiles can be used quite meaningfully for - * grayscale images and both RGB and palette images might only have gray - * colors in them, so gray profiles may be appropriate. + * This checking code ensures the embedded profile (on either read or write) + * conforms to the specification requirements. Notice that an ICC 'gray' + * color-space profile contains the information to transform the monochrome + * data to XYZ or L*a*b (according to which PCS the profile uses) and this + * should be used in preference to the standard libpng K channel replication + * into R, G and B channels. + * + * Previously it was suggested that an RGB profile on grayscale data could be + * handled. However it it is clear that using an RGB profile in this context + * must be an error - there is no specification of what it means. Thus it is + * almost certainly more correct to ignore the profile. */ temp = png_get_uint_32(profile+16); /* data colour space field */ - if (temp != 0x52474220 /* 'RGB ' */ && temp != 0x47524159 /* 'GRAY' */) - return profile_error(png_ptr, colorspace, name, temp, - "invalid color space"); + switch (temp) + { + case 0x52474220: /* 'RGB ' */ + if (!(color_type & PNG_COLOR_MASK_COLOR)) + return profile_error(png_ptr, colorspace, name, temp, + "RGB color space not permitted on grayscale PNG"); + break; + + case 0x47524159: /* 'GRAY' */ + if (color_type & PNG_COLOR_MASK_COLOR) + return profile_error(png_ptr, colorspace, name, temp, + "Gray color space not permitted on RGB PNG"); + break; + + default: + return profile_error(png_ptr, colorspace, name, temp, + "invalid color space"); + } /* It is up to the application to check that the profile class matches the * application requirements; the spec provides no guidance, but it's pretty @@ -1956,13 +1978,16 @@ png_icc_check_header(png_const_structrp png_ptr, png_colorspacerp colorspace, { if (temp == 0x6C696E6B /* 'link' */ || temp == 0x61627374 /* 'abst' */) return profile_error(png_ptr, colorspace, name, temp, - "invalid class"); + "invalid ICC profile class"); /* This can only be 0x6E6D636C: a 'nmcl' profile. This is a device - * specific profile. + * specific profile. The checks on the tags below will ensure that it can + * actually be used, but it certainly is not expected and is probably an + * error. */ else - (void)profile_error(png_ptr, NULL, name, temp, "unexpected ICC class"); + (void)profile_error(png_ptr, NULL, name, temp, + "unexpected ICC profile class"); } return 1; @@ -1976,6 +2001,16 @@ png_icc_check_tag_table(png_const_structrp png_ptr, png_colorspacerp colorspace, png_uint_32 tag_count = png_get_uint_32(profile+128); png_uint_32 itag; png_const_bytep tag = profile+132; /* The first tag */ + int have_AToB0Tag = 0; /* Whether the profile has an AToB0Tag */ + int have_grayTRCTag = 0; /* Whether the profile has a grayTRCTag */ + unsigned int matrix_TRC_tags = 0; /* Which matrix/TRC tags are present */ +# define HAVE_redMatrixColumnTag 0x01 +# define HAVE_greenMatrixColumnTag 0x02 +# define HAVE_blueMatrixColumnTag 0x04 +# define HAVE_redTRCTag 0x10 +# define HAVE_greenTRCTag 0x20 +# define HAVE_blueTRCTag 0x40 +# define HAVE_all_tags 0x77 for (itag=0; itag < tag_count; ++itag, tag += 12) { @@ -1992,9 +2027,90 @@ png_icc_check_tag_table(png_const_structrp png_ptr, png_colorspacerp colorspace, tag_length > profile_length - tag_start) return profile_error(png_ptr, colorspace, name, tag_id, "tag data outside profile"); + + /* Check the tag_id for the specific profiles which must be present for + * the profile to be valid. + */ + switch (tag_id) + { + case 0x41324230: /* 'A2B0' - AToB0Tag */ + have_AToB0Tag = 1; + break; + + case 0x6B545243: /* 'kTRC' - grayTRCTag */ + have_grayTRCTag = 1; + break; + + case 0x7258595A: /* 'rXYZ' - redMatrixColumnTag */ + matrix_TRC_tags |= HAVE_redMatrixColumnTag; + break; + + case 0x72545243: /* 'rTRC' - redTRCTag */ + matrix_TRC_tags |= HAVE_redTRCTag; + break; + + case 0x6758595A: /* 'gXYZ' - greenMatrixColumnTag */ + matrix_TRC_tags |= HAVE_greenMatrixColumnTag; + break; + + case 0x67545243: /* 'gTRC' - greenTRCTag */ + matrix_TRC_tags |= HAVE_greenTRCTag; + break; + + case 0x6258595A: /* 'bXYZ' - blueMatrixColumnTag */ + matrix_TRC_tags |= HAVE_blueMatrixColumnTag; + break; + + case 0x62545243: /* 'bTRC' - blueTRCTag */ + matrix_TRC_tags |= HAVE_blueTRCTag; + break; + + default: + break; + } } - return 1; + /* An AToB0Tag works in all valid profiles, but if it is absent then + * something matching the profile class and color space must be present. + */ + if (!have_AToB0Tag) + { + png_uint_32 profile_class = png_get_uint_32(profile+12); + + switch (profile_class) + { + case 0x73636E72: /* 'scnr' - an input profile */ + case 0x6D6E7472: /* 'mntr' - a display device profile */ + case 0x70727472: /* 'prtr' - an output device profile */ + if (png_get_uint_32(profile+16) /* color space */ == + 0x47524159 /* gray */) + { + if (!have_grayTRCTag) + return profile_error(png_ptr, colorspace, name, profile_class, + "missing grayTRCTag for monochrome profile"); + } + + else + { + if (matrix_TRC_tags != HAVE_all_tags) + return profile_error(png_ptr, colorspace, name, profile_class, + "missing Matrix/TRC tags for RGB profile"); + } + + return 1; + + case 0x73706163: /* 'spac' */ + return profile_error(png_ptr, colorspace, name, profile_class, + "missing AToB0Tag for colorspace profile"); + + default: /* should have been checked before */ + png_error(png_ptr, "invalid ICC class"); + return 0; /* NOT REACHED */ + } + } + + else + return 1; } #ifdef PNG_sRGB_SUPPORTED @@ -2192,14 +2308,15 @@ png_icc_set_gAMA_and_cHRM(png_const_structrp png_ptr, int /* PRIVATE */ png_colorspace_set_ICC(png_const_structrp png_ptr, png_colorspacerp colorspace, png_const_charp name, png_uint_32 profile_length, - png_const_bytep profile, int preferred) + png_const_bytep profile, int preferred, int color_type) { if (colorspace->flags & PNG_COLORSPACE_INVALID) return 0; if (png_icc_check_length(png_ptr, colorspace, name, profile_length) && - png_icc_check_header(png_ptr, colorspace, name, profile_length, profile) - && png_icc_check_tag_table(png_ptr, colorspace, name, profile_length, + png_icc_check_header(png_ptr, colorspace, name, profile_length, profile, + color_type) && + png_icc_check_tag_table(png_ptr, colorspace, name, profile_length, profile)) { png_icc_set_gAMA_and_cHRM(png_ptr, colorspace, name, profile, 0, diff --git a/pngerror.c b/pngerror.c index 0a0eeefbc..dbc6534ba 100644 --- a/pngerror.c +++ b/pngerror.c @@ -415,7 +415,7 @@ static PNG_CONST char png_digit[16] = { 'A', 'B', 'C', 'D', 'E', 'F' }; -#define PNG_MAX_ERROR_TEXT 64 +#define PNG_MAX_ERROR_TEXT 196 /* Currently limited be profile_error in png.c */ #if defined(PNG_WARNINGS_SUPPORTED) || defined(PNG_ERROR_TEXT_SUPPORTED) static void /* PRIVATE */ png_format_buffer(png_const_structrp png_ptr, png_charp buffer, png_const_charp diff --git a/pngpriv.h b/pngpriv.h index 0970603b1..ffd4fd8ab 100644 --- a/pngpriv.h +++ b/pngpriv.h @@ -281,13 +281,6 @@ typedef const png_uint_16p * png_const_uint_16pp; # define PNG_ZBUF_SIZE 65536L #endif -/* PNG_STATIC is used to mark internal file scope functions if they need to be - * accessed for implementation tests (see the code in tests/?*). - */ -#ifndef PNG_STATIC -# define PNG_STATIC static -#endif - /* If warnings or errors are turned off the code is disabled or redirected here. * From 1.5.4 functions have been added to allow very limited formatting of * error and warning messages - this code will also be disabled here. @@ -1449,8 +1442,8 @@ PNG_INTERNAL_FUNCTION(int,png_colorspace_set_sRGB,(png_const_structrp png_ptr, #ifdef PNG_iCCP_SUPPORTED PNG_INTERNAL_FUNCTION(int,png_colorspace_set_ICC,(png_const_structrp png_ptr, png_colorspacerp colorspace, png_const_charp name, - png_uint_32 profile_length, png_const_bytep profile, int preferred), - PNG_EMPTY); + png_uint_32 profile_length, png_const_bytep profile, int preferred, + int color_type), PNG_EMPTY); /* The 'name' is used for information only */ /* Routines for checking parts of an ICC profile. */ @@ -1460,7 +1453,8 @@ PNG_INTERNAL_FUNCTION(int,png_icc_check_length,(png_const_structrp png_ptr, PNG_INTERNAL_FUNCTION(int,png_icc_check_header,(png_const_structrp png_ptr, png_colorspacerp colorspace, png_const_charp name, png_uint_32 profile_length, - png_const_bytep profile /* first 132 bytes only */), PNG_EMPTY); + png_const_bytep profile /* first 132 bytes only */, int color_type), + PNG_EMPTY); PNG_INTERNAL_FUNCTION(int,png_icc_check_tag_table,(png_const_structrp png_ptr, png_colorspacerp colorspace, png_const_charp name, png_uint_32 profile_length, diff --git a/pngrutil.c b/pngrutil.c index 975b3910f..b6dd2e500 100644 --- a/pngrutil.c +++ b/pngrutil.c @@ -1424,7 +1424,8 @@ png_handle_iCCP(png_structrp png_ptr, png_inforp info_ptr, png_uint_32 length) * byte header. */ if (png_icc_check_header(png_ptr, &png_ptr->colorspace, - keyword, profile_length, profile_header)) + keyword, profile_length, profile_header, + png_ptr->color_type)) { /* Now read the tag table; a variable size buffer is * needed at this point, allocate one for the whole diff --git a/pngset.c b/pngset.c index 17bfe726d..c7d2d5e47 100644 --- a/pngset.c +++ b/pngset.c @@ -643,11 +643,14 @@ png_set_iCCP(png_const_structrp png_ptr, png_inforp info_ptr, /* Set the colorspace first because this validates the profile; do not * override previously set app cHRM or gAMA here (because likely as not the - * application knows better than libpng what the correct values are.) + * application knows better than libpng what the correct values are.) Pass + * the info_ptr color_type field to png_colorspace_set_ICC because in the + * write case it has not yet been stored in png_ptr. */ { int result = png_colorspace_set_ICC(png_ptr, &info_ptr->colorspace, name, - proflen, profile, 0/* do *not* override the app cHRM or gAMA */); + proflen, profile, 0/* do *not* override the app cHRM or gAMA */, + info_ptr->color_type); png_colorspace_sync_info(png_ptr, info_ptr);