From a3b721bb9af3baf668992e08d966b0f768d61e44 Mon Sep 17 00:00:00 2001 From: Tony Cook Date: Sat, 5 Jan 2019 14:09:01 +1100 Subject: [PATCH] invalid quantization options now result in an error rather than defaults --- Changes | 14 +++++- Imager.pm | 7 ++- Imager.xs | 93 +++++++++++++++++++++++++++++++------- imextpltypes.h | 7 ++- quant.c | 29 +++++++++--- t/100-base/010-introvert.t | 3 +- t/150-type/040-palette.t | 26 ++++++++++- 7 files changed, 149 insertions(+), 30 deletions(-) diff --git a/Changes b/Changes index eab9b688..5d313347 100644 --- a/Changes +++ b/Changes @@ -1,6 +1,11 @@ Imager release history. Older releases can be found in Changes.old -Coverity finally finished a build, fix a few problems: +General changes: + + - to_paletted() and make_palette() now fail (with an error in + errstr()) if invalid quantization parameters are supplied. + +Coverity finally finished a build[1], fix a few problems: High severity: @@ -91,6 +96,13 @@ High severity: - check the combine function pointer consistently rather than the combine code in one place in the fountain filter. + - error diffusion now validates a custom error diffusion map and reports + an error if it's bad. CID 185288. + +[1] The first two build submissions ended up at the end of a ~400 +build queue, and seemed to have been cancelled by Coverity. A build +submitted on NYE went through in minutes. + Imager 1.008 - 31 Dec 2018 ============ diff --git a/Imager.pm b/Imager.pm index f2250684..82948f2b 100644 --- a/Imager.pm +++ b/Imager.pm @@ -1042,7 +1042,12 @@ sub make_palette { ++$index; } - return i_img_make_palette($quant, map $_->{IMG}, @images); + my @cols = i_img_make_palette($quant, map $_->{IMG}, @images); + unless (@cols) { + Imager->_set_error(Imager->_error_as_msg); + return; + } + return @cols; } # convert a paletted (or any image) to an 8-bit/channel RGB image diff --git a/Imager.xs b/Imager.xs index 0ba1282c..2ff3c528 100644 --- a/Imager.xs +++ b/Imager.xs @@ -537,18 +537,29 @@ do_io_new_cb(pTHX_ SV *writecb, SV *readcb, SV *seekcb, SV *closecb) { } struct value_name { - char *name; + const char *name; int value; }; -static int lookup_name(const struct value_name *names, int count, char *name, int def_value) +static int +lookup_name(const struct value_name *names, int count, char *name, int def_value, int push_errors, const char *id, int *failed) { int i; + + if (push_errors) + *failed = 0; + for (i = 0; i < count; ++i) if (strEQ(names[i].name, name)) return names[i].value; + if (push_errors) { + i_push_errorf(0, "unknown value '%s' for %s", name, id); + *failed = 1; + } + return def_value; } + static struct value_name transp_names[] = { { "none", tr_none }, @@ -602,14 +613,14 @@ static struct value_name orddith_names[] = }; /* look through the hash for quantization options */ -static void -ip_handle_quant_opts(pTHX_ i_quantize *quant, HV *hv) +static int +ip_handle_quant_opts_low(pTHX_ i_quantize *quant, HV *hv, int push_errors) { - /*** POSSIBLY BROKEN: do I need to unref the SV from hv_fetch ***/ SV **sv; int i; STRLEN len; char *str; + int failed = 0; quant->mc_colors = mymalloc(quant->mc_size * sizeof(i_color)); @@ -617,7 +628,9 @@ ip_handle_quant_opts(pTHX_ i_quantize *quant, HV *hv) if (sv && *sv && (str = SvPV(*sv, len))) { quant->transp = lookup_name(transp_names, sizeof(transp_names)/sizeof(*transp_names), - str, tr_none); + str, tr_none, push_errors, "transp", &failed); + if (failed) + return 0; if (quant->transp != tr_none) { quant->tr_threshold = 127; sv = hv_fetch(hv, "tr_threshold", 12, 0); @@ -627,13 +640,18 @@ ip_handle_quant_opts(pTHX_ i_quantize *quant, HV *hv) if (quant->transp == tr_errdiff) { sv = hv_fetch(hv, "tr_errdiff", 10, 0); if (sv && *sv && (str = SvPV(*sv, len))) - quant->tr_errdiff = lookup_name(errdiff_names, sizeof(errdiff_names)/sizeof(*errdiff_names), str, ed_floyd); + quant->tr_errdiff = lookup_name(errdiff_names, sizeof(errdiff_names)/sizeof(*errdiff_names), str, ed_floyd, push_errors, "tr_errdiff", &failed); + if (failed) + return 0; } if (quant->transp == tr_ordered) { quant->tr_orddith = od_tiny; sv = hv_fetch(hv, "tr_orddith", 10, 0); - if (sv && *sv && (str = SvPV(*sv, len))) - quant->tr_orddith = lookup_name(orddith_names, sizeof(orddith_names)/sizeof(*orddith_names), str, od_random); + if (sv && *sv && (str = SvPV(*sv, len))) { + quant->tr_orddith = lookup_name(orddith_names, sizeof(orddith_names)/sizeof(*orddith_names), str, od_random, push_errors, "tr_orddith", &failed); + if (failed) + return 0; + } if (quant->tr_orddith == od_custom) { sv = hv_fetch(hv, "tr_map", 6, 0); @@ -658,7 +676,9 @@ ip_handle_quant_opts(pTHX_ i_quantize *quant, HV *hv) sv = hv_fetch(hv, "make_colors", 11, 0); if (sv && *sv && (str = SvPV(*sv, len))) { quant->make_colors = - lookup_name(make_color_names, sizeof(make_color_names)/sizeof(*make_color_names), str, mc_median_cut); + lookup_name(make_color_names, sizeof(make_color_names)/sizeof(*make_color_names), str, mc_median_cut, push_errors, "make_colors", &failed); + if (failed) + return 0; } sv = hv_fetch(hv, "colors", 6, 0); if (sv && *sv && SvROK(*sv) && SvTYPE(SvRV(*sv)) == SVt_PVAV) { @@ -675,6 +695,10 @@ ip_handle_quant_opts(pTHX_ i_quantize *quant, HV *hv) i_color *col = INT2PTR(i_color *, SvIV((SV*)SvRV(*sv1))); quant->mc_colors[i] = *col; } + else if (push_errors) { + i_push_errorf(0, "colors[%d] isn't an Imager::Color object", i); + return 0; + } } } sv = hv_fetch(hv, "max_colors", 10, 0); @@ -687,11 +711,15 @@ ip_handle_quant_opts(pTHX_ i_quantize *quant, HV *hv) quant->translate = pt_closest; sv = hv_fetch(hv, "translate", 9, 0); if (sv && *sv && (str = SvPV(*sv, len))) { - quant->translate = lookup_name(translate_names, sizeof(translate_names)/sizeof(*translate_names), str, pt_closest); + quant->translate = lookup_name(translate_names, sizeof(translate_names)/sizeof(*translate_names), str, pt_closest, push_errors, "translate", &failed); + if (failed) + return 0; } sv = hv_fetch(hv, "errdiff", 7, 0); if (sv && *sv && (str = SvPV(*sv, len))) { - quant->errdiff = lookup_name(errdiff_names, sizeof(errdiff_names)/sizeof(*errdiff_names), str, ed_floyd); + quant->errdiff = lookup_name(errdiff_names, sizeof(errdiff_names)/sizeof(*errdiff_names), str, ed_floyd, push_errors, "errdiff", &failed); + if (failed) + return 0; } if (quant->translate == pt_errdiff && quant->errdiff == ed_custom) { /* get the error diffusion map */ @@ -716,7 +744,12 @@ ip_handle_quant_opts(pTHX_ i_quantize *quant, HV *hv) for (i = 0; i < len; ++i) { SV **sv2 = av_fetch(av, i, 0); if (sv2 && *sv2) { - quant->ed_map[i] = SvIV(*sv2); + IV iv = SvIV(*sv2); + if (push_errors && iv < 0) { + i_push_errorf(0, "errdiff_map values must be non-negative, errdiff[%d] is negative", i); + return 0; + } + quant->ed_map[i] = iv; sum += quant->ed_map[i]; } } @@ -726,12 +759,18 @@ ip_handle_quant_opts(pTHX_ i_quantize *quant, HV *hv) myfree(quant->ed_map); quant->ed_map = 0; quant->errdiff = ed_floyd; + if (push_errors) { + i_push_error(0, "error diffusion map must contain some non-zero values"); + return 0; + } } } } sv = hv_fetch(hv, "perturb", 7, 0); if (sv && *sv) quant->perturb = SvIV(*sv); + + return 1; } static void @@ -741,6 +780,20 @@ ip_cleanup_quant_opts(pTHX_ i_quantize *quant) { myfree(quant->ed_map); } +static int +ip_handle_quant_opts2(pTHX_ i_quantize *quant, HV *hv) { + int result = ip_handle_quant_opts_low(aTHX_ quant, hv, 1); + if (!result) { + ip_cleanup_quant_opts(aTHX_ quant); + } + return result; +} + +static void +ip_handle_quant_opts(pTHX_ i_quantize *quant, HV *hv) { + (void)ip_handle_quant_opts_low(aTHX_ quant, hv, 0); +} + /* copies the color map from the hv into the colors member of the HV */ static void ip_copy_colors_back(pTHX_ HV *hv, i_quantize *quant) { @@ -786,7 +839,7 @@ S_get_poly_fill_mode(pTHX_ SV *sv) { else { return (i_poly_fill_mode_t)lookup_name (poly_fill_mode_names, ARRAY_COUNT(poly_fill_mode_names), - SvPV_nolen(sv), i_pfm_evenodd); + SvPV_nolen(sv), i_pfm_evenodd, 0, NULL, NULL); } } @@ -1056,7 +1109,8 @@ static im_pl_ext_funcs im_perl_funcs = IMAGER_PL_API_LEVEL, ip_handle_quant_opts, ip_cleanup_quant_opts, - ip_copy_colors_back + ip_copy_colors_back, + ip_handle_quant_opts2 }; #define PERL_PL_SET_GLOBAL_CALLBACKS \ @@ -3148,7 +3202,10 @@ i_img_to_pal(src, quant_hv) memset(&quant, 0, sizeof(quant)); quant.version = 1; quant.mc_size = 256; - ip_handle_quant_opts(aTHX_ &quant, quant_hv); + i_clear_error(); + if (!ip_handle_quant_opts2(aTHX_ &quant, quant_hv)) { + XSRETURN_EMPTY; + } RETVAL = i_img_to_pal(src, &quant); if (RETVAL) { ip_copy_colors_back(aTHX_ quant_hv, &quant); @@ -3185,7 +3242,9 @@ i_img_make_palette(HV *quant_hv, ...) memset(&quant, 0, sizeof(quant)); quant.version = 1; quant.mc_size = 256; - ip_handle_quant_opts(aTHX_ &quant, quant_hv); + if (!ip_handle_quant_opts2(aTHX_ &quant, quant_hv)) { + XSRETURN_EMPTY; + } i_quant_makemap(&quant, imgs, count); EXTEND(SP, quant.mc_count); for (i = 0; i < quant.mc_count; ++i) { diff --git a/imextpltypes.h b/imextpltypes.h index e7722a21..1442a20b 100644 --- a/imextpltypes.h +++ b/imextpltypes.h @@ -11,7 +11,7 @@ interfacing with perl - these functions aren't part of the core Imager API. */ -#define IMAGER_PL_API_LEVEL 1 +#define IMAGER_PL_API_LEVEL 2 typedef struct { int version; @@ -22,7 +22,10 @@ typedef struct { void (*f_ip_cleanup_quant_opts)(pTHX_ i_quantize *quant); void (*f_ip_copy_colors_back)(pTHX_ HV *hv, i_quantize *quant); - /* IMAGER_PL_API_LEVEL 2 functions will go here */ + /* IMAGER_PL_API_LEVEL 2 */ + int (*f_ip_handle_quant_opts2)(pTHX_ i_quantize *quant, HV *hv); + + /* IMAGER_PL_API_LEVEL 3 functions will go here */ } im_pl_ext_funcs; #define PERL_PL_FUNCTION_TABLE_NAME "Imager::__ext_pl_func_table" diff --git a/quant.c b/quant.c index 9d6e947d..9b769685 100644 --- a/quant.c +++ b/quant.c @@ -93,7 +93,7 @@ i_quant_makemap(i_quantize *quant, i_img **imgs, int count) { } static void translate_closest(i_quantize *, i_img *, i_palidx *); -static void translate_errdiff(i_quantize *, i_img *, i_palidx *); +static int translate_errdiff(i_quantize *, i_img *, i_palidx *); static void translate_addi(i_quantize *, i_img *, i_palidx *); /* @@ -143,7 +143,10 @@ i_quant_translate(i_quantize *quant, i_img *img) { break; case pt_errdiff: - translate_errdiff(quant, img, result); + if (!translate_errdiff(quant, img, result)) { + myfree(result); + return NULL; + } break; case pt_perturb: @@ -1354,8 +1357,7 @@ typedef struct errdiff_tag { } errdiff_t; /* perform an error diffusion dither */ -static -void +static int translate_errdiff(i_quantize *quant, i_img *img, i_palidx *out) { int *map; int mapw, maph, mapo; @@ -1383,14 +1385,25 @@ translate_errdiff(i_quantize *quant, i_img *img, i_palidx *out) { mapo = maps[index].orig; } + difftotal = 0; + for (i = 0; i < maph * mapw; ++i) { + if (map[i] < 0) { + i_push_errorf(0, "errdiff_map values must be non-negative, errdiff[%d] is negative", i); + return 0; + } + difftotal += map[i]; + } + + if (!difftotal) { + i_push_error(0, "error diffusion map must contain some non-zero values"); + return 0; + } + errw = img->xsize+mapw; err = mymalloc(sizeof(*err) * maph * errw); /*errp = err+mapo;*/ memset(err, 0, sizeof(*err) * maph * errw); - difftotal = 0; - for (i = 0; i < maph * mapw; ++i) - difftotal += map[i]; /*printf("map:\n"); for (dy = 0; dy < maph; ++dy) { for (dx = 0; dx < mapw; ++dx) { @@ -1444,6 +1457,8 @@ translate_errdiff(i_quantize *quant, i_img *img, i_palidx *out) { } CF_CLEANUP; myfree(err); + + return 1; } /* Prescan finds the boxes in the image that have the highest number of colors and that result is used as the initial value for the vectores */ diff --git a/t/100-base/010-introvert.t b/t/100-base/010-introvert.t index a52aadd9..de640c1f 100644 --- a/t/100-base/010-introvert.t +++ b/t/100-base/010-introvert.t @@ -113,7 +113,8 @@ is(Imager::i_img_type($im_pal), 0, "pal img shouldn't be paletted now"); make_colors => 'none', ); my $im_pal2 = Imager::i_img_to_pal($im_pal, \%quant); - ok($im_pal2, "got an image from quantizing"); + ok($im_pal2, "got an image from quantizing") + or diag _get_error(); is(@{$quant{colors}}, 4, "quant has the right number of colours"); is(Imager::i_colorcount($im_pal2), 4, "and so does the image"); my @colors = Imager::i_getcolors($im_pal2, 0, 4); diff --git a/t/150-type/040-palette.t b/t/150-type/040-palette.t index a5976b7a..00d8935b 100644 --- a/t/150-type/040-palette.t +++ b/t/150-type/040-palette.t @@ -1,7 +1,7 @@ #!perl -w # some of this is tested in t01introvert.t too use strict; -use Test::More tests => 226; +use Test::More; BEGIN { use_ok("Imager", ':handy'); } use Imager::Test qw(image_bounds_checks test_image is_color3 isnt_image is_color4 is_fcolor3); @@ -438,6 +438,11 @@ cmp_ok(Imager->errstr, '=~', qr/Channels must be positive and <= 4/, is_color4($map[33], 33, 33, 33, 255, "check map[2]"); is_color4($map[255], 255, 255, 255, 255, "check map[15]"); } + { + my @map = Imager->make_palette({ make_colors => "xxx" }, $im); + is(@map, 0, "fail with bad make_colors"); + is(Imager->errstr, "unknown value 'xxx' for make_colors"); + } } my $psamp_outside_error = "Image position outside of image"; @@ -627,8 +632,27 @@ my $psamp_outside_error = "Image position outside of image"; "check error message"); } +{ + # check error handling with zero error diffusion matrix + my $im = test_image; + my $new = $im->to_paletted + ( + make_colors => "webmap", + translate => "errdiff", + errdiff => "custom", + errdiff_width => 2, + errdiff_height => 2, + errdiff_map => [ 0, 0, 0, 0 ], + ); + ok(!$new, "can't errdiff with an all zero map"); + is($im->errstr, "error diffusion map must contain some non-zero values", + "check error message"); +} + Imager->close_log; +done_testing(); + unless ($ENV{IMAGER_KEEP_FILES}) { unlink "testout/t023palette.log" } -- 2.39.5