From 3d024874a5cafa7797cff8b03cb1235fea265752 Mon Sep 17 00:00:00 2001 From: John Bowler Date: Tue, 12 Jan 2016 09:36:10 -0800 Subject: [PATCH] Recently introduced palette sharing bug The internal read code change to stop sharing the palette was incompletely implemented. The result is that unless palette index checking is turned off and there are no read transformations the png_info palette gets deleted when the png_struct is deleted. This is normally harmless (png_info gets deleted first) but in the case of pngcp it results in use-after-free of the palette and, therefore, palette corruption and maybe on some operating systems and access violation. This also updated pngcp 'search' mode to check a restricted range of memLevels; there is an unrelated bug which means that lower zlib memLevels result in memory corruption under some circumstances, probably less often than 1:1000. Signed-off-by: John Bowler --- contrib/examples/pngcp.c | 13 +++++++---- pngrtran.c | 50 ++++++++++++++++++++++++++-------------- pngstruct.h | 5 ++-- 3 files changed, 44 insertions(+), 24 deletions(-) diff --git a/contrib/examples/pngcp.c b/contrib/examples/pngcp.c index 4540950f1..12fe7d28d 100644 --- a/contrib/examples/pngcp.c +++ b/contrib/examples/pngcp.c @@ -130,7 +130,7 @@ vl_strategy[] = vl_windowBits_text[] = { { "default", 15 }, - { "small", 9 }, + { "minimum", 8 }, RANGE(8, 15), { all, 0 } }, @@ -147,9 +147,12 @@ vl_level[] = }, vl_memLevel[] = { - { "default", 8 }, - { "least", 1 }, - RANGE(2, 9), /* exclude 1: there seems to be a zlib bug */ + { "9", 9 }, /* zlib maximum */ + { "1", 1 }, /* zlib minimum */ + { "default", 8 }, /* zlib default */ + { "2", 2 }, + { "3", 3 }, /* for explicit testing */ + RANGE(4, 9), /* exclude 3 and below: zlib bugs */ { all, 0 } }, #endif /* WRITE_CUSTOMIZE_*COMPRESSION */ @@ -1052,7 +1055,7 @@ getsearchopts(struct display *dp, const char *opt_str, int *value) } else if (opt == OPTIND(dp, memLevel)) - dp->value[opt] = 9; /* fixed */ + (void)advance_opt(dp, opt, 1/*search*/); else /* something else */ return 0; diff --git a/pngrtran.c b/pngrtran.c index 77f98d426..d6bb290fa 100644 --- a/pngrtran.c +++ b/pngrtran.c @@ -5998,6 +5998,8 @@ png_remove_PLTE_and_tRNS(png_structrp png_ptr) png_ptr->trans_alpha = NULL; png_ptr->num_trans = 0; # endif /* READ_tRNS */ + + png_ptr->palette_updated = 1U; } static void @@ -6164,7 +6166,11 @@ update_palette(png_structp png_ptr, png_cache_paramsp cp, * first unprocessed transform. The reset of the transform control loses the * gamma information as well, of course, as any information about the palette * and tRNS changes (such as the RANGE flags). + * + * The following ensures that png_read_update_info knows to update the + * palette in png_info (which is no longer shared). */ + png_ptr->palette_updated = 1U; } /* These structure and the save/restore routines that follow it exist to save @@ -6725,29 +6731,39 @@ png_read_transform_info(png_structrp png_ptr, png_inforp info_ptr) * but only if png_ptr has a new version, otherwise the invalid_info * settings from above can still invalidate the chunk. */ - if (png_ptr->palette != info_ptr->palette) + if (png_ptr->palette_updated) { - png_free_data(png_ptr, info_ptr, PNG_FREE_PLTE, 0); - info_ptr->palette = png_ptr->palette; - info_ptr->num_palette = png_ptr->num_palette; - if (info_ptr->palette != NULL && info_ptr->num_palette > 0) - info_ptr->valid |= PNG_INFO_PLTE; + if (png_ptr->num_palette > 0) + png_set_PLTE(png_ptr, info_ptr, png_ptr->palette, + png_ptr->num_palette); + + else + { + png_free_data(png_ptr, info_ptr, PNG_FREE_PLTE, 0); + info_ptr->valid &= PNG_BIC_MASK(PNG_INFO_PLTE); + } # ifdef PNG_READ_tRNS - /* ONLY do this if the palette was changed above because, in - * fact, the tRNS data is not shared (yes, this is inconsistent, - * perhaps fix it?) + /* If the output format is not a palette format the tRNS + * information was a single color which is now invalid + * (probably), otherwise the array of tRNS values must be + * updated. */ - if ((info_ptr->format & PNG_FORMAT_FLAG_COLORMAP) != 0 && - png_ptr->trans_alpha != info_ptr->trans_alpha) + if ((info_ptr->format & PNG_FORMAT_FLAG_COLORMAP) != 0) { - png_free_data(png_ptr, info_ptr, PNG_FREE_TRNS, 0); - /* NOTE: it is shared now! */ - info_ptr->trans_alpha = png_ptr->trans_alpha; - info_ptr->num_trans = png_ptr->num_trans; - if (info_ptr->trans_alpha != NULL && info_ptr->num_trans > 0) - info_ptr->valid |= PNG_INFO_tRNS; + if (png_ptr->num_trans > 0) + png_set_tRNS(png_ptr, info_ptr, png_ptr->trans_alpha, + png_ptr->num_trans, NULL/*trans color*/); + + else + { + png_free_data(png_ptr, info_ptr, PNG_FREE_tRNS, 0); + info_ptr->valid &= PNG_BIC_MASK(PNG_INFO_tRNS); + } } + + else + info_ptr->valid &= PNG_BIC_MASK(PNG_INFO_tRNS); # endif /* READ_tRNS */ } # endif /* READ_TRANSFORMS */ diff --git a/pngstruct.h b/pngstruct.h index ee77a4b92..bff8855cc 100644 --- a/pngstruct.h +++ b/pngstruct.h @@ -469,7 +469,7 @@ struct png_struct_def * default: PNG_DEFAULT_GAMMA_ACCURACY (665) */ #endif /* NYI */ - png_uint_16 gamma_threshold; + png_uint_16 gamma_threshold; /* Gamma threshold value as a fixed-point value in the range 0..1; the * threshold at or below which gamma correction is skipped. '0' forces * gamma correction even when there is none because the input and output @@ -479,7 +479,8 @@ struct png_struct_def */ #endif /* READ_GAMMA */ #ifdef PNG_READ_TRANSFORMS_SUPPORTED - unsigned int invalid_info; /* PNG_INFO_* for invalidated chunks */ + unsigned int invalid_info; /* PNG_INFO_* for invalidated chunks */ + unsigned int palette_updated:1; /* png_struct::palette changed */ #endif /* READ_TRANSFORMS */ #ifdef PNG_SEQUENTIAL_READ_SUPPORTED