From 66efa24241b2740868e44ba73130df1e3cf95248 Mon Sep 17 00:00:00 2001 From: John Bowler Date: Sun, 18 Mar 2012 21:10:29 -0500 Subject: [PATCH] [libpng16] Removed erroneous setting of DETECT_UNINITIALIZED and added more checks. The code now does a png_error if an attempt is made to do the row initialization twice; this is an application error and it has serious consequences because the transform data in png_struct is changed by each call. --- ANNOUNCE | 9 +++- CHANGES | 7 ++- pngread.c | 39 +++++++++------ pngrtran.c | 136 +++++++++++++++++++++++------------------------------ pngrutil.c | 11 ----- 5 files changed, 98 insertions(+), 104 deletions(-) diff --git a/ANNOUNCE b/ANNOUNCE index b678012f5..b4abf1ff8 100644 --- a/ANNOUNCE +++ b/ANNOUNCE @@ -1,5 +1,5 @@ -Libpng 1.6.0beta19 - March 18, 2012 +Libpng 1.6.0beta19 - March 19, 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. @@ -318,7 +318,7 @@ Version 1.6.0beta18 [March 16, 2012] read benign errors to warnings (regardless of the system default, unless this is disabled in which case the simplified API can't be built.) -Version 1.6.0beta19 [March 18, 2012] +Version 1.6.0beta19 [March 19, 2012] Work around for duplicate row start calls; added warning messages. This turns on PNG_FLAG_DETECT_UNINITIALIZED to detect app code that fails to call one of the 'start' routines (not enabled in libpng-1.5 @@ -328,6 +328,11 @@ Version 1.6.0beta19 [March 18, 2012] they were before changes to use png_inflate_claim. Somehow webkit is causing this to happen; this is probably a mis-feature in the zlib changes so this commit is only a work-round. + Removed erroneous setting of DETECT_UNINITIALIZED and added more + checks. The code now does a png_error if an attempt is made to do the + row initialization twice; this is an application error and it has + serious consequences because the transform data in png_struct is + changed by each call. Send comments/corrections/commendations to png-mng-implement at lists.sf.net (subscription required; visit diff --git a/CHANGES b/CHANGES index 9d498798d..72ae59f9c 100644 --- a/CHANGES +++ b/CHANGES @@ -4069,7 +4069,7 @@ Version 1.6.0beta18 [March 16, 2012] read benign errors to warnings (regardless of the system default, unless this is disabled in which case the simplified API can't be built.) -Version 1.6.0beta19 [March 18, 2012] +Version 1.6.0beta19 [March 19, 2012] Work around for duplicate row start calls; added warning messages. This turns on PNG_FLAG_DETECT_UNINITIALIZED to detect app code that fails to call one of the 'start' routines (not enabled in libpng-1.5 @@ -4079,6 +4079,11 @@ Version 1.6.0beta19 [March 18, 2012] they were before changes to use png_inflate_claim. Somehow webkit is causing this to happen; this is probably a mis-feature in the zlib changes so this commit is only a work-round. + Removed erroneous setting of DETECT_UNINITIALIZED and added more + checks. The code now does a png_error if an attempt is made to do the + row initialization twice; this is an application error and it has + serious consequences because the transform data in png_struct is + changed by each call. Send comments/corrections/commendations to png-mng-implement at lists.sf.net (subscription required; visit diff --git a/pngread.c b/pngread.c index 56281a273..db19b9db5 100644 --- a/pngread.c +++ b/pngread.c @@ -50,11 +50,6 @@ png_create_read_struct_2,(png_const_charp user_png_ver, png_voidp error_ptr, { png_ptr->mode = PNG_IS_READ_STRUCT; - /* Turn this on for all transforms in an attempt to detect failure to call - * the image reading start stuff. - */ - png_ptr->flags |= PNG_FLAG_DETECT_UNINITIALIZED; - /* Added in libpng-1.6.0; this can be used to detect a read structure if * required (it will be zero in a write structure.) */ @@ -254,16 +249,24 @@ png_read_update_info(png_structrp png_ptr, png_inforp info_ptr) { png_debug(1, "in png_read_update_info"); - if (png_ptr == NULL) - return; + if (png_ptr != NULL) + { + if ((png_ptr->flags & PNG_FLAG_ROW_INIT) == 0) + { + png_read_start_row(png_ptr); - png_read_start_row(png_ptr); +# ifdef PNG_READ_TRANSFORMS_SUPPORTED + png_read_transform_info(png_ptr, info_ptr); +# else + PNG_UNUSED(info_ptr) +# endif + } -#ifdef PNG_READ_TRANSFORMS_SUPPORTED - png_read_transform_info(png_ptr, info_ptr); -#else - PNG_UNUSED(info_ptr) -#endif + /* New in 1.6.0 this avoids the bug of doing the initializations twice */ + else + png_error(png_ptr, + "png_read_update_info/png_start_read_image: duplicate call"); + } } #ifdef PNG_SEQUENTIAL_READ_SUPPORTED @@ -278,7 +281,15 @@ png_start_read_image(png_structrp png_ptr) png_debug(1, "in png_start_read_image"); if (png_ptr != NULL) - png_read_start_row(png_ptr); + { + if ((png_ptr->flags & PNG_FLAG_ROW_INIT) == 0) + png_read_start_row(png_ptr); + + /* New in 1.6.0 this avoids the bug of doing the initializations twice */ + else + png_error(png_ptr, + "png_start_read_image/png_read_update_info: duplicate call"); + } } #endif /* PNG_SEQUENTIAL_READ_SUPPORTED */ diff --git a/pngrtran.c b/pngrtran.c index b9a744314..559779d8e 100644 --- a/pngrtran.c +++ b/pngrtran.c @@ -88,6 +88,33 @@ png_set_crc_action(png_structrp png_ptr, int crit_action, int ancil_action) } } +#ifdef PNG_READ_TRANSFORMS_SUPPORTED +/* Is it OK to set a transformation now? Only if png_start_read_image or + * png_read_update_info have not been called. It is not necessary for the IHDR + * to have been read in all cases, the parameter allows for this check too. + */ +static int +png_rtran_ok(png_structrp png_ptr, int need_IHDR) +{ + if (png_ptr != NULL) + { + if (png_ptr->flags & PNG_FLAG_ROW_INIT) + png_error(png_ptr, + "invalid after png_start_read_image or png_read_update_info"); + + if (need_IHDR && (png_ptr->mode & PNG_HAVE_IHDR) == 0) + png_error(png_ptr, "invalid before the PNG header has been read"); + + /* Turn on failure to initialize correctly for all transforms. */ + png_ptr->flags |= PNG_FLAG_DETECT_UNINITIALIZED; + + return 1; /* Ok */ + } + + return 0; /* no png_error possible! */ +} +#endif + #ifdef PNG_READ_BACKGROUND_SUPPORTED /* Handle alpha and tRNS via a background color */ void PNGFAPI @@ -97,7 +124,7 @@ png_set_background_fixed(png_structrp png_ptr, { png_debug(1, "in png_set_background_fixed"); - if (png_ptr == NULL || background_color == NULL) + if (!png_rtran_ok(png_ptr, 0) || background_color == NULL) return; if (background_gamma_code == PNG_BACKGROUND_GAMMA_UNKNOWN) @@ -141,7 +168,7 @@ png_set_scale_16(png_structrp png_ptr) { png_debug(1, "in png_set_scale_16"); - if (png_ptr == NULL) + if (!png_rtran_ok(png_ptr, 0)) return; png_ptr->transformations |= PNG_SCALE_16_TO_8; @@ -155,7 +182,7 @@ png_set_strip_16(png_structrp png_ptr) { png_debug(1, "in png_set_strip_16"); - if (png_ptr == NULL) + if (!png_rtran_ok(png_ptr, 0)) return; png_ptr->transformations |= PNG_16_TO_8; @@ -168,7 +195,7 @@ png_set_strip_alpha(png_structrp png_ptr) { png_debug(1, "in png_set_strip_alpha"); - if (png_ptr == NULL) + if (!png_rtran_ok(png_ptr, 0)) return; png_ptr->transformations |= PNG_STRIP_ALPHA; @@ -247,7 +274,7 @@ png_set_alpha_mode_fixed(png_structrp png_ptr, int mode, png_debug(1, "in png_set_alpha_mode"); - if (png_ptr == NULL) + if (!png_rtran_ok(png_ptr, 0)) return; output_gamma = translate_gamma_flags(png_ptr, output_gamma, 1/*screen*/); @@ -342,9 +369,6 @@ png_set_alpha_mode_fixed(png_structrp png_ptr, int mode, png_ptr->transformations |= PNG_COMPOSE; } - - /* New API, make sure apps call the correct initializers: */ - png_ptr->flags |= PNG_FLAG_DETECT_UNINITIALIZED; } # ifdef PNG_FLOATING_POINT_SUPPORTED @@ -383,7 +407,7 @@ png_set_quantize(png_structrp png_ptr, png_colorp palette, { png_debug(1, "in png_set_quantize"); - if (png_ptr == NULL) + if (!png_rtran_ok(png_ptr, 0)) return; png_ptr->transformations |= PNG_QUANTIZE; @@ -770,7 +794,7 @@ png_set_gamma_fixed(png_structrp png_ptr, png_fixed_point scrn_gamma, { png_debug(1, "in png_set_gamma_fixed"); - if (png_ptr == NULL) + if (!png_rtran_ok(png_ptr, 0)) return; /* New in libpng-1.5.4 - reserve particular negative values as flags. */ @@ -822,16 +846,10 @@ png_set_expand(png_structrp png_ptr) { png_debug(1, "in png_set_expand"); - if (png_ptr == NULL) + if (!png_rtran_ok(png_ptr, 0)) return; png_ptr->transformations |= (PNG_EXPAND | PNG_EXPAND_tRNS); - if (png_ptr->flags & PNG_FLAG_ROW_INIT) - { - /* TODO: should probably be an error */ - png_warning(png_ptr, "png_set_expand called after row initialization"); - png_ptr->flags &= ~PNG_FLAG_ROW_INIT; - } } /* GRR 19990627: the following three functions currently are identical @@ -858,17 +876,10 @@ png_set_palette_to_rgb(png_structrp png_ptr) { png_debug(1, "in png_set_palette_to_rgb"); - if (png_ptr == NULL) + if (!png_rtran_ok(png_ptr, 0)) return; png_ptr->transformations |= (PNG_EXPAND | PNG_EXPAND_tRNS); - if (png_ptr->flags & PNG_FLAG_ROW_INIT) - { - /* TODO: should probably be an error */ - png_warning(png_ptr, - "png_set_palette_to_rgb called after row initialization"); - png_ptr->flags &= ~PNG_FLAG_ROW_INIT; - } } /* Expand grayscale images of less than 8-bit depth to 8 bits. */ @@ -877,35 +888,22 @@ png_set_expand_gray_1_2_4_to_8(png_structrp png_ptr) { png_debug(1, "in png_set_expand_gray_1_2_4_to_8"); - if (png_ptr == NULL) + if (!png_rtran_ok(png_ptr, 0)) return; png_ptr->transformations |= PNG_EXPAND; - if (png_ptr->flags & PNG_FLAG_ROW_INIT) - { - /* TODO: should probably be an error */ - png_warning(png_ptr, - "png_set_expand_gray_1_2_4_to_8 called after row initialization"); - png_ptr->flags &= ~PNG_FLAG_ROW_INIT; - } } - - /* Expand tRNS chunks to alpha channels. */ void PNGAPI png_set_tRNS_to_alpha(png_structrp png_ptr) { png_debug(1, "in png_set_tRNS_to_alpha"); + if (!png_rtran_ok(png_ptr, 0)) + return; + png_ptr->transformations |= (PNG_EXPAND | PNG_EXPAND_tRNS); - if (png_ptr->flags & PNG_FLAG_ROW_INIT) - { - /* TODO: should probably be an error */ - png_warning(png_ptr, - "png_set_tRNS_to_alpha called after row initialization"); - png_ptr->flags &= ~PNG_FLAG_ROW_INIT; - } } #endif /* defined(PNG_READ_EXPAND_SUPPORTED) */ @@ -918,20 +916,10 @@ png_set_expand_16(png_structrp png_ptr) { png_debug(1, "in png_set_expand_16"); - if (png_ptr == NULL) + if (!png_rtran_ok(png_ptr, 0)) return; png_ptr->transformations |= (PNG_EXPAND_16 | PNG_EXPAND | PNG_EXPAND_tRNS); - if (png_ptr->flags & PNG_FLAG_ROW_INIT) - { - /* TODO: should probably be an error */ - png_warning(png_ptr, - "png_set_expand_16 called after row initialization"); - png_ptr->flags &= ~PNG_FLAG_ROW_INIT; - } - - /* New API, make sure apps call the correct initializers: */ - png_ptr->flags |= PNG_FLAG_DETECT_UNINITIALIZED; } #endif @@ -941,19 +929,12 @@ png_set_gray_to_rgb(png_structrp png_ptr) { png_debug(1, "in png_set_gray_to_rgb"); - if (png_ptr != NULL) - { - /* Because rgb must be 8 bits or more: */ - png_set_expand_gray_1_2_4_to_8(png_ptr); - png_ptr->transformations |= PNG_GRAY_TO_RGB; - if (png_ptr->flags & PNG_FLAG_ROW_INIT) - { - /* TODO: should probably be an error */ - png_warning(png_ptr, - "png_set_gray_to_rgb called after row initialization"); - png_ptr->flags &= ~PNG_FLAG_ROW_INIT; - } - } + if (!png_rtran_ok(png_ptr, 0)) + return; + + /* Because rgb must be 8 bits or more: */ + png_set_expand_gray_1_2_4_to_8(png_ptr); + png_ptr->transformations |= PNG_GRAY_TO_RGB; } #endif @@ -964,7 +945,9 @@ png_set_rgb_to_gray_fixed(png_structrp png_ptr, int error_action, { png_debug(1, "in png_set_rgb_to_gray"); - if (png_ptr == NULL) + /* Need the IHDR here because of the check on color_type below. */ + /* TODO: fix this */ + if (!png_rtran_ok(png_ptr, 1)) return; switch(error_action) @@ -990,10 +973,13 @@ png_set_rgb_to_gray_fixed(png_structrp png_ptr, int error_action, png_ptr->transformations |= PNG_EXPAND; #else { - png_warning(png_ptr, + /* Make this an error in 1.6 because otherwise the application may assume + * that it just worked and get a memory overwrite. + */ + png_error(png_ptr, "Cannot do RGB_TO_GRAY without EXPAND_SUPPORTED"); - png_ptr->transformations &= ~PNG_RGB_TO_GRAY; + /* png_ptr->transformations &= ~PNG_RGB_TO_GRAY; */ } #endif { @@ -1046,16 +1032,13 @@ void PNGAPI png_set_rgb_to_gray(png_structrp png_ptr, int error_action, double red, double green) { - if (png_ptr == NULL) - return; - png_set_rgb_to_gray_fixed(png_ptr, error_action, png_fixed(png_ptr, red, "rgb to gray red coefficient"), png_fixed(png_ptr, green, "rgb to gray green coefficient")); } #endif /* FLOATING POINT */ -#endif +#endif /* RGB_TO_GRAY */ #if defined(PNG_READ_USER_TRANSFORM_SUPPORTED) || \ defined(PNG_WRITE_USER_TRANSFORM_SUPPORTED) @@ -1065,7 +1048,7 @@ png_set_read_user_transform_fn(png_structrp png_ptr, png_user_transform_ptr { png_debug(1, "in png_set_read_user_transform_fn"); - if (png_ptr == NULL) + if (!png_rtran_ok(png_ptr, 0)) return; #ifdef PNG_READ_USER_TRANSFORM_SUPPORTED @@ -2122,8 +2105,9 @@ png_do_read_transformations(png_structrp png_ptr, png_row_infop row_info) /* The following is debugging; prior to 1.5.4 the code was never compiled in; * in 1.5.4 PNG_FLAG_DETECT_UNINITIALIZED was added and the macro - * PNG_WARN_UNINITIALIZED_ROW removed. In 1.5 the new flag is set only for - * selected new APIs to ensure that there is no API change. + * PNG_WARN_UNINITIALIZED_ROW removed. In 1.6 the new flag is set only for + * all transformations, however in practice the ROW_INIT always gets done on + * demand, if necessary. */ if ((png_ptr->flags & PNG_FLAG_DETECT_UNINITIALIZED) != 0 && !(png_ptr->flags & PNG_FLAG_ROW_INIT)) diff --git a/pngrutil.c b/pngrutil.c index 3a8924795..6d7eec45d 100644 --- a/pngrutil.c +++ b/pngrutil.c @@ -4105,17 +4105,6 @@ png_read_start_row(png_structrp png_ptr) png_debug(1, "in png_read_start_row"); - /* Because init_read_transformations, below, modifies values in png_struct - * it will not always work correctly if called twice. This error detects - * that condition but just warns, because it does tend to work most of the - * time. - */ - if (png_ptr->flags & PNG_FLAG_ROW_INIT) - { - png_warning(png_ptr, "unexpected duplicate call to png_read_start_row"); - png_ptr->zowner = 0; /* release previous claim */ - } - #ifdef PNG_READ_TRANSFORMS_SUPPORTED png_init_read_transformations(png_ptr); #endif