invalid quantization options now result in an error rather than defaults

diff --git a/Changes b/Changes
index eab9b6885ff8fd0aed6f2108f7d06a55a0de9115..5d31334754d70fefeece7d602c668b15d4460adc 100644 (file)
--- 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 f2250684add77babb60f403ae5c419a06f78f10a..82948f2bc14ac015319cba435e15e5042eea0110 100644 (file)
--- 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 0ba1282c51de2ac3cf18f734f2cb8a4f1723493a..2ff3c528421bdaf4dd768261435b1b95cebde9e4 100644 (file)
--- 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 e7722a21d46bdb64fc6b6daaa3fbf96413a4a99b..1442a20b086454dd21d76e96bd4eea9e8296d1bf 100644 (file)
--- 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 9d6e947d966028b8685d5c33eade3eeb9c80eeb0..9b76968595d5fa8e456091e9af59532c36ad60b8 100644 (file)
--- 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 a52aadd90f61b33ea5069c625121e9570f552fcc..de640c1f220f7fd695d70fb3f48f986754026c17 100644 (file)
--- 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 a5976b7a5e2ef50aecd74e55284558c4476e1aa4..00d8935b15db777560697c61c3eff9a074c931ce 100644 (file)
--- 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"
 }