Bug#909632: pngmeta: segfault on bad png
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);
Reply to: