Bug#909632: pngmeta: segfault on bad png

[Kevin Ryde <user42_kevin@yahoo.com.au>]

tag 909632 + patch
thanks

Actually, pngmeta segfaults on good pngs too.  I believe

- The time displaying bit has dodgy pointers to pointers which stores to
  uninitialized if png contains a time chunk.  The normal gcc warning
  for uninitialized apparently doesn't appear due to other warnings.

- The final png_destroy_read_struct() should pass pointers to pointers
  the same as the earlier such calls in the program.

- The Debian change removing "png_skip_till_end" means no end_info, so
  tIME after IDAT not printed eg. /usr/share/pixmaps/debian-logo.png

I get some joy from the diff below.  Whole png_read_png() is the easiest
way to pick up time and text after IDAT.  It'd be more efficient not to
hold the image data in memory, but can start by working.

--- pngmeta.c.orig	2018-10-19 17:38:53.000000000 +1100
+++ pngmeta.c	2018-10-23 16:47:56.000000000 +1100
@@ -45,6 +45,7 @@
 
 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>
 #include <strings.h>
 #include <ctype.h>
 
@@ -481,7 +482,10 @@
 
   /* Initialise data input */
   png_init_io(png_ptr, in_fp);
-  png_read_info(png_ptr, info_ptr);
+
+  /* Debian change: whole png_read_png() picks up text and time after IDAT.
+     (A bit easier than png_read_info(), skip IDAT, png_read_end().) */
+  png_read_png(png_ptr, info_ptr, 0, NULL);
   
   print_init(out_fp, output_type, pngfile, uri, quiet);
 
@@ -522,10 +526,10 @@
     sprintf(value, "%d", bit_depth);
     print_kv(out_fp, output_type, "image-colors", value);
     
-    sprintf(value, "%ld", width);
+    sprintf(value, "%ld", (long) width);
     print_kv(out_fp, output_type, "image-width", value);
     
-    sprintf(value, "%ld", height);
+    sprintf(value, "%ld", (long) height);
     print_kv(out_fp, output_type, "image-height", value);
     
     sprintf(value, "%s, %sinterlaced",
@@ -553,26 +557,26 @@
 
   { 
 #if PNG_LIBPNG_VER_MAJOR >= 1 && PNG_LIBPNG_VER_MINOR >= 4
-  int valid = png_get_valid(png_ptr, info_ptr, 0);
-  int end_valid = png_get_valid(png_ptr, end_info, 0);
-  png_timep *mod_time, *end_mod_time;
-
-  png_get_tIME(png_ptr, info_ptr, mod_time);
-  png_get_tIME(png_ptr, end_info, end_mod_time);
+  png_timep mod_time;
+  int valid = png_get_tIME(png_ptr, info_ptr, &mod_time);
 #else
   int valid = info_ptr->valid;
-  int end_valid = end_info->valid;
-  png_timep *mod_time = &info_ptr->mod_time;
-  png_timep *end_mod_time = &end_info->mod_time;
+  png_timep mod_time = &info_ptr->mod_time;
 #endif
 
   /* Print modification time (tIME chunk) if present */
-  if (valid & PNG_INFO_tIME)
-    print_kv(out_fp, output_type, "Modification Time",
-             png_convert_to_rfc1123(png_ptr, &mod_time));
-  else if (end_valid & PNG_INFO_tIME)
-    print_kv(out_fp, output_type, "Modification Time",
-             png_convert_to_rfc1123(png_ptr, &end_mod_time));
+  if (valid & PNG_INFO_tIME) {
+    const char *str = png_convert_to_rfc1123(png_ptr, mod_time);
+    if (str) {
+      print_kv(out_fp, output_type, "Modification Time", str);
+    } else {
+      /* png_convert_to_rfc1123() returns NULL for a bad tIME, such as month
+         not 1..12 (and hence no month name string).  Think an error here is
+         ok, and leave tolerating badness to a fix or dump program. */
+      fprintf(STDERR, "%s: invalid tIME in %s\n", progname, pngfile);
+      exit(1);
+    }
+  }
   }
 
   print_end_image(out_fp, output_type);
@@ -585,7 +589,7 @@
   print_finish(out_fp, output_type);
   
   /* Cleanup */
-  png_destroy_read_struct(png_ptr, info_ptr, end_info);
+  png_destroy_read_struct(&png_ptr, &info_ptr, &end_info);
 
   fclose(in_fp);