From 790fef342b83770450c56828a65c22641f04114d Mon Sep 17 00:00:00 2001 From: John Bowler Date: Wed, 16 Nov 2022 01:47:20 -0800 Subject: tools: Fix a memory leak in pngcp Signed-off-by: Cosmin Truta --- contrib/tools/pngcp.c | 72 +++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 58 insertions(+), 14 deletions(-) diff --git a/contrib/tools/pngcp.c b/contrib/tools/pngcp.c index 8b979f3d3..579e28d2b 100644 --- a/contrib/tools/pngcp.c +++ b/contrib/tools/pngcp.c @@ -1,6 +1,6 @@ /* pngcp.c * - * Copyright (c) 2016 John Cunningham Bowler + * Copyright (c) 2016,2022 John Cunningham Bowler * * This code is released under the libpng license. * For conditions of distribution and use, see the disclaimer @@ -12,6 +12,10 @@ * * For a more extensive example that uses the transforms see * contrib/libtests/pngimage.c in the libpng distribution. + * + * This code is not intended for installation in a release system; the command + * line options are not documented and most of the behavior is intended for + * testing libpng performance, both speed and compression. */ #include "pnglibconf.h" /* To find how libpng was configured. */ @@ -502,10 +506,10 @@ display_init(struct display *dp) } static void -display_clean_read(struct display *dp) +display_clean_read(struct display *dp, int freeinfo) { if (dp->read_pp != NULL) - png_destroy_read_struct(&dp->read_pp, NULL, NULL); + png_destroy_read_struct(&dp->read_pp, freeinfo ? &dp->ip : NULL, NULL); if (dp->fp != NULL) { @@ -516,7 +520,7 @@ display_clean_read(struct display *dp) } static void -display_clean_write(struct display *dp) +display_clean_write(struct display *dp, int freeinfo) { if (dp->fp != NULL) { @@ -526,14 +530,14 @@ display_clean_write(struct display *dp) } if (dp->write_pp != NULL) - png_destroy_write_struct(&dp->write_pp, dp->tsp > 0 ? NULL : &dp->ip); + png_destroy_write_struct(&dp->write_pp, freeinfo ? &dp->ip : NULL); } static void display_clean(struct display *dp) { - display_clean_read(dp); - display_clean_write(dp); + display_clean_read(dp, 1/*freeinfo*/); + display_clean_write(dp, 1/*freeinfo*/); dp->output_file = NULL; # if PNG_LIBPNG_VER < 10700 && defined PNG_TEXT_SUPPORTED @@ -1744,7 +1748,17 @@ read_function(png_structp pp, png_bytep data, size_t size) static void read_png(struct display *dp, const char *filename) { - display_clean_read(dp); /* safety */ + /* This is an assumption of the code; it may happen if a previous write fails + * and there is a bug in the cleanup handling below (look for setjmp). + * Passing freeinfo==1 to display_clean_read below avoids a second error + * on dp->ip != NULL below. + */ + if (dp->read_pp != NULL) + { + display_log(dp, APP_FAIL, "unexpected png_read_struct"); + display_clean_read(dp, 1/*freeinfo*/); /* recovery */ + } + display_start_read(dp, filename); dp->read_pp = png_create_read_struct(PNG_LIBPNG_VER_STRING, dp, @@ -1768,6 +1782,13 @@ read_png(struct display *dp, const char *filename) png_set_check_for_invalid_index(dp->read_pp, -1/*off completely*/); # endif /* IGNORE_INDEX */ + if (dp->ip != NULL) + { + /* UNEXPECTED: some problem in the display_clean function calls! */ + display_log(dp, APP_FAIL, "read_png: freeing old info struct"); + png_destroy_info_struct(dp->read_pp, &dp->ip); + } + /* The png_read_png API requires us to make the info struct, but it does the * call to png_read_info. */ @@ -1847,7 +1868,14 @@ read_png(struct display *dp, const char *filename) } #endif /* FIX_INDEX */ - display_clean_read(dp); + /* NOTE: dp->ip is where all the information about the PNG that was just read + * is stored. It can be used to write and write again a single PNG file, + * however be aware that prior to libpng 1.7 text chunks could only be + * written once; this is a bug which would require a significant code rewrite + * to fix, it has been there in several versions of libpng (it was introduced + * to fix another bug involving duplicate writes of the text chunks.) + */ + display_clean_read(dp, 0/*freeiinfo*/); dp->operation = "none"; } @@ -1974,7 +2002,21 @@ set_text_compression(struct display *dp) static void write_png(struct display *dp, const char *destname) { - display_clean_write(dp); /* safety */ + /* If this test fails png_write_png would fail *silently* below; this + * is not helpful, so catch the problem now and give up: + */ + if (dp->ip == NULL) + display_log(dp, INTERNAL_ERROR, "missing png_info"); + + /* This is an assumption of the code; it may happen if a previous + * write fails and there is a bug in the cleanup handling below. + */ + if (dp->write_pp != NULL) + { + display_log(dp, APP_FAIL, "unexpected png_write_struct"); + display_clean_write(dp, 0/*!freeinfo*/); + } + display_start_write(dp, destname); dp->write_pp = png_create_write_struct(PNG_LIBPNG_VER_STRING, dp, @@ -2072,10 +2114,6 @@ write_png(struct display *dp, const char *destname) destname == NULL ? "stdout" : destname, strerror(errno)); } - /* Clean it on the way out - if control returns to the caller then the - * written_file contains the required data. - */ - display_clean_write(dp); dp->operation = "none"; } @@ -2242,6 +2280,10 @@ cp_one_file(struct display *dp, const char *filename, const char *destname) /* Loop to find the best option. */ do { + /* Clean before each write_png; this just removes *dp->write_pp which + * cannot be reused. + */ + display_clean_write(dp, 0/*!freeinfo*/); write_png(dp, tmpname); /* And compare the sizes (the write function makes sure write_size @@ -2271,6 +2313,8 @@ cp_one_file(struct display *dp, const char *filename, const char *destname) /* Do this for the 'sizes' option so that it reports the correct size. */ dp->write_size = dp->best_size; } + + display_clean_write(dp, 1/*freeinfo*/); } static int -- cgit v1.2.1