PNG re-work: improve error reporting for PNG read/write

diff --git a/Changes b/Changes
index 1fe7376596a513207eb7210490747cff25441ad3..6889d76726598b42ae662a3d4e06b49d90cbc3d8 100644 (file)
--- a/Changes
+++ b/Changes
@@ -1,5 +1,8 @@
 Imager release history.  Older releases can be found in Changes.old
 
+ - PNG rework
+   - improve error reporting
+
 Imager 0.90 - unreleased
 ===========
 

diff --git a/MANIFEST b/MANIFEST
index b33c65b6fd8b0af08727777b8a8de1bdbc0828ee..64f5975d0d23e68820c53e4149ce6519c0941f32 100644 (file)
--- a/MANIFEST
+++ b/MANIFEST
@@ -225,6 +225,7 @@ PNG/PNG.xs
 PNG/README
 PNG/t/00load.t
 PNG/t/10png.t                  Test png support
+PNG/testimg/badcrc.png
 PNG/testimg/palette.png
 PNG/testimg/palette_out.png
 pnm.c

diff --git a/MANIFEST.SKIP b/MANIFEST.SKIP
index 30c026a5e532674b8ea6a08952e00d3a339f28fd..ef04e5dd728e6ac45853fd372f593056e1dc0788 100644 (file)
--- a/MANIFEST.SKIP
+++ b/MANIFEST.SKIP
@@ -96,6 +96,9 @@ Makefile\.old$
 /meta\.tmp$
 ^imconfig\.h$
 
+# generated if we build them in their own directory
+^(PNG|TIFF|FT2|W32|GIF)/blib/
+
 # generated from .im files
 ^combine\.c$
 ^compose\.c$

diff --git a/PNG/Changes b/PNG/Changes
index eb9f02abe870fafc6438c085ce89ec0cf6547063..52bc4017ba8bbb6618accf1378cbc226b9345df0 100644 (file)
--- a/PNG/Changes
+++ b/PNG/Changes
@@ -1,3 +1,7 @@
+
+ - improve error reporting to actually report the error text from
+   libpng.
+
 Imager-File-PNG 0.84
 ====================
 
@@ -39,6 +43,10 @@ Imager-File-PNG 0.79
    plan to continue receiving mail at that address.
    https://rt.cpan.org/Ticket/Display.html?id=68591
 
+ - Makefile.PL updates to report library detection info back to the
+   main Imager Makefile.PL.
+   https://rt.cpan.org/Ticket/Display.html?id=9675
+
 Imager-File-PNG 0.78
 ====================
 

diff --git a/PNG/impng.c b/PNG/impng.c
index 5fd41512616014621b2cdec4a1cfc46346254ec2..d831eb3a625c0a5ab072b0c41d7fcd0ae5597c14 100644 (file)
--- a/PNG/impng.c
+++ b/PNG/impng.c
@@ -34,13 +34,13 @@ static int CC2C[PNG_COLOR_MASK_PALETTE|PNG_COLOR_MASK_COLOR|PNG_COLOR_MASK_ALPHA
 static void
 wiol_read_data(png_structp png_ptr, png_bytep data, png_size_t length) {
   io_glue *ig = png_get_io_ptr(png_ptr);
-  int rc = i_io_read(ig, data, length);
+  ssize_t rc = i_io_read(ig, data, length);
   if (rc != length) png_error(png_ptr, "Read overflow error on an iolayer source.");
 }
 
 static void
 wiol_write_data(png_structp png_ptr, png_bytep data, png_size_t length) {
-  int rc;
+  ssize_t rc;
   io_glue *ig = png_get_io_ptr(png_ptr);
   rc = i_io_write(ig, data, length);
   if (rc != length) png_error(png_ptr, "Write error on an iolayer source.");
@@ -63,11 +63,34 @@ check_if_png(char *file_name, FILE **fp) {
 }
 */
 
+static void
+error_handler(png_structp png_ptr, png_const_charp msg) {
+  mm_log((1, "PNG error: '%s'\n", msg));
+
+  i_push_error(0, msg);
+  longjmp(png_jmpbuf(png_ptr), 1);
+}
+
+/*
+
+For writing a warning might have information about an error, so send
+it to the error stack.
+
+*/
+static void
+write_warn_handler(png_structp png_ptr, png_const_charp msg) {
+  mm_log((1, "PNG write warning '%s'\n", msg));
+
+  i_push_error(0, msg);
+}
+
+#define PNG_DIM_MAX 0x7fffffffL
+
 undef_int
 i_writepng_wiol(i_img *im, io_glue *ig) {
   png_structp png_ptr;
   png_infop info_ptr = NULL;
-  int width,height,y;
+  i_img_dim width,height,y;
   volatile int cspace,channels;
   double xres, yres;
   int aspect_only, have_res;
@@ -84,6 +107,20 @@ i_writepng_wiol(i_img *im, io_glue *ig) {
   height = im->ysize;
   width  = im->xsize;
 
+  /* if we ever have 64-bit i_img_dim
+   * the libpng docs state that png_set_user_limits() can be used to
+   * override the PNG_USER_*_MAX limits, but as implemented they
+   * don't.  We check against the theoretical limit of PNG here, and
+   * try to override the limits below, in case the libpng
+   * implementation ever matches the documentation.
+   *
+   * https://sourceforge.net/tracker/?func=detail&atid=105624&aid=3314943&group_id=5624
+  */
+  if (width > PNG_DIM_MAX || height > PNG_DIM_MAX) {
+    i_push_error(0, "Image too large for PNG");
+    return 0;
+  }
+
   channels=im->channels;
 
   if (channels > 2) { cspace = PNG_COLOR_TYPE_RGB; channels-=3; }
@@ -101,7 +138,8 @@ i_writepng_wiol(i_img *im, io_glue *ig) {
    * in case we are using dynamically linked libraries.  REQUIRED.
    */
   
-  png_ptr = png_create_write_struct(PNG_LIBPNG_VER_STRING,NULL,NULL,NULL);
+  png_ptr = png_create_write_struct(PNG_LIBPNG_VER_STRING, NULL, 
+                                   error_handler, write_warn_handler);
   
   if (png_ptr == NULL) return 0;
 
@@ -133,6 +171,12 @@ i_writepng_wiol(i_img *im, io_glue *ig) {
    * currently be PNG_COMPRESSION_TYPE_BASE and PNG_FILTER_TYPE_BASE. REQUIRED
    */
 
+  /* by default, libpng (not PNG) limits the image size to a maximum
+   * 1000000 pixels in each direction, but Imager doesn't.
+   * Configure libpng to avoid that limit.
+   */
+  png_set_user_limits(png_ptr, width, height);
+
   png_set_IHDR(png_ptr, info_ptr, width, height, 8, cspace,
               PNG_INTERLACE_NONE, PNG_COMPRESSION_TYPE_BASE, PNG_FILTER_TYPE_BASE);
 
@@ -183,9 +227,18 @@ i_writepng_wiol(i_img *im, io_glue *ig) {
   return(1);
 }
 
+static void 
+get_png_tags(i_img *im, png_structp png_ptr, png_infop info_ptr);
+
+typedef struct {
+  char *warnings;
+} i_png_read_state, *i_png_read_statep;
 
+static void
+read_warn_handler(png_structp, png_const_charp);
 
-static void get_png_tags(i_img *im, png_structp png_ptr, png_infop info_ptr);
+static void
+cleanup_read_state(i_png_read_statep);
 
 i_img*
 i_readpng_wiol(io_glue *ig) {
@@ -197,17 +250,26 @@ i_readpng_wiol(io_glue *ig) {
   int number_passes,y;
   int channels,pass;
   unsigned int sig_read;
+  i_png_read_state rs;
 
+  rs.warnings = NULL;
   sig_read  = 0;
 
   mm_log((1,"i_readpng_wiol(ig %p)\n", ig));
+  i_clear_error();
 
-  png_ptr = png_create_read_struct(PNG_LIBPNG_VER_STRING,NULL,NULL,NULL);
+  png_ptr = png_create_read_struct(PNG_LIBPNG_VER_STRING, &rs, 
+                                  error_handler, read_warn_handler);
+  if (!png_ptr) {
+    i_push_error(0, "Cannot create PNG read structure");
+    return NULL;
+  }
   png_set_read_fn(png_ptr, (png_voidp) (ig), wiol_read_data);
   
   info_ptr = png_create_info_struct(png_ptr);
   if (info_ptr == NULL) {
     png_destroy_read_struct(&png_ptr, (png_infopp)NULL, (png_infopp)NULL);
+    i_push_error(0, "Cannot create PNG info structure");
     return NULL;
   }
   
@@ -215,6 +277,7 @@ i_readpng_wiol(io_glue *ig) {
     if (im) i_img_destroy(im);
     mm_log((1,"i_readpng_wiol: error.\n"));
     png_destroy_read_struct(&png_ptr, &info_ptr, (png_infopp)NULL);
+    cleanup_read_state(&rs);
     return NULL;
   }
 
@@ -270,13 +333,19 @@ i_readpng_wiol(io_glue *ig) {
   get_png_tags(im, png_ptr, info_ptr);
 
   png_destroy_read_struct(&png_ptr, &info_ptr, (png_infopp)NULL);
+
+  if (rs.warnings) {
+    i_tags_set(&im->tags, "png_warnings", rs.warnings, -1);
+  }
+  cleanup_read_state(&rs);
   
   mm_log((1,"(0x%08X) <- i_readpng_scalar\n", im));  
   
   return im;
 }
 
-static void get_png_tags(i_img *im, png_structp png_ptr, png_infop info_ptr) {
+static void
+get_png_tags(i_img *im, png_structp png_ptr, png_infop info_ptr) {
   png_uint_32 xres, yres;
   int unit_type;
 
@@ -294,3 +363,35 @@ static void get_png_tags(i_img *im, png_structp png_ptr, png_infop info_ptr) {
     }
   }
 }
+
+static void
+read_warn_handler(png_structp png_ptr, png_const_charp msg) {
+  i_png_read_statep rs = (i_png_read_statep)png_get_error_ptr(png_ptr);
+  char *workp;
+  size_t new_size;
+
+  mm_log((1, "PNG read warning '%s'\n", msg));
+
+  /* in case this is part of an error report */
+  i_push_error(0, msg);
+  
+  /* and save in the warnings so if we do manage to succeed, we 
+   * can save it as a tag
+   */
+  new_size = (rs->warnings ? strlen(rs->warnings) : 0)
+    + 1 /* NUL */
+    + strlen(msg) /* new text */
+    + 1; /* newline */
+  workp = myrealloc(rs->warnings, new_size);
+  if (!rs->warnings)
+    *workp = '\0';
+  strcat(workp, msg);
+  strcat(workp, "\n");
+  rs->warnings = workp;
+}
+
+static void
+cleanup_read_state(i_png_read_statep rs) {
+  if (rs->warnings)
+    myfree(rs->warnings);
+}

diff --git a/PNG/t/10png.t b/PNG/t/10png.t
index 4077a99e68a487315694ef8725ef239af5f8e486..759a266a3de543bf6adfd37b5fc3d07039d1df2e 100644 (file)
--- a/PNG/t/10png.t
+++ b/PNG/t/10png.t
@@ -4,6 +4,8 @@ use Imager qw(:all);
 use Test::More;
 use Imager::Test qw(test_image_raw test_image);
 
+my $debug_writes = 1;
+
 -d "testout" or mkdir "testout";
 
 init_log("testout/t102png.log",1);
@@ -11,7 +13,7 @@ init_log("testout/t102png.log",1);
 $Imager::formats{"png"}
   or plan skip_all => "No png support";
 
-plan tests => 35;
+plan tests => 39;
 
 my $green  = i_color_new(0,   255, 0,   255);
 my $blue   = i_color_new(0,   0,   255, 255);
@@ -165,3 +167,36 @@ EOS
   ok(grep($_ eq 'png', Imager->write_types), "check png in write types");
 }
 
+{ # read error reporting
+  my $im = Imager->new;
+  ok(!$im->read(file => "testimg/badcrc.png", type => "png"),
+     "read png with bad CRC chunk should fail");
+  is($im->errstr, "IHDR: CRC error", "check error message");
+}
+
+{ # write error reporting
+  my $im = test_image();
+  ok(!$im->write(type => "png", callback => limited_write(1), buffered => 0),
+     "write limited to 1 byte should fail");
+  is($im->errstr, "Write error on an iolayer source.: limit reached",
+     "check error message");
+}
+
+sub limited_write {
+  my ($limit) = @_;
+
+  return
+     sub {
+       my ($data) = @_;
+       $limit -= length $data;
+       if ($limit >= 0) {
+         print "# write of ", length $data, " bytes successful ($limit left)\n" if $debug_writes;
+         return 1;
+       }
+       else {
+         print "# write of ", length $data, " bytes failed\n";
+         Imager::i_push_error(0, "limit reached");
+         return;
+       }
+     };
+}

diff --git a/PNG/testimg/badcrc.png b/PNG/testimg/badcrc.png
new file mode 100644 (file)
index 0000000..36f6d6b
Binary files /dev/null and b/PNG/testimg/badcrc.png differ