* generic/tkImgPNG.c (WriteIDAT): [Bug 2984787]: Use the correct

	flushing semantics when handling the last data from the image. Without
	this, many PNG readers (notably including Firefox) refuse to show the
	image and instead complain about errors.
	(ReadIDAT): Added sanity checks to ensure that when we've got bad data
	of the sorts of forms we were previously generating, we detect it and
	error out rather than silently failing.
	(WriteExtraChunks): New function to write in some basic metadata.

3 files changed, 119 insertions, 13 deletions

diff --git a/ChangeLog b/ChangeLog
index 9366bba..ee48e71 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2010-04-12  Donal K. Fellows  <dkf@users.sf.net>
+
+	* generic/tkImgPNG.c (WriteIDAT): [Bug 2984787]: Use the correct
+	flushing semantics when handling the last data from the image. Without
+	this, many PNG readers (notably including Firefox) refuse to show the
+	image and instead complain about errors.
+	(ReadIDAT): Added sanity checks to ensure that when we've got bad data
+	of the sorts of forms we were previously generating, we detect it and
+	error out rather than silently failing.
+	(WriteExtraChunks): New function to write in some basic metadata.
+
 2010-04-09  Jan Nijtmans  <nijtmans@users.sf.net>
 
 	* doc/photo.n:    Follow-up to [Bug 2983824]: update doc.
@@ -15,10 +26,10 @@
 
 2010-04-06  Jan Nijtmans  <nijtmans@users.sf.net>
 
-	* win/tcl.m4          Sync with Tcl version
-	* unix/tcl.m4
-	* win/configure       (regenerate with autoconf-2.59)
-	* unix/configure      [Bug 2982540]: configure and install* script
+	* win/tcl.m4:         Sync with Tcl version
+	* unix/tcl.m4:
+	* win/configure:      (regenerate with autoconf-2.59)
+	* unix/configure:     [Bug 2982540]: configure and install* script
 			      files should always have LF
 
 2010-03-29  Jan Nijtmans  <nijtmans@users.sf.net>
diff --git a/generic/tkImgPNG.c b/generic/tkImgPNG.c
index 0468ca9..c138b78 100644
--- a/generic/tkImgPNG.c
+++ b/generic/tkImgPNG.c
@@ -9,7 +9,7 @@
  * See the file "license.terms" for information on usage and redistribution of
  * this file, and for a DISCLAIMER OF ALL WARRANTIES.
  *
- * RCS: @(#) $Id: tkImgPNG.c,v 1.5 2010/02/05 22:45:03 nijtmans Exp $
+ * RCS: @(#) $Id: tkImgPNG.c,v 1.6 2010/04/12 08:40:15 dkf Exp $
  */
 
 #include "tkInt.h"
@@ -256,6 +256,8 @@ static inline int	WriteChunk(Tcl_Interp *interp, PNGImage *pngPtr,
 static int		WriteData(Tcl_Interp *interp, PNGImage *pngPtr,
 			    const unsigned char *srcPtr, int srcSz,
 			    unsigned long *crcPtr);
+static int		WriteExtraChunks(Tcl_Interp *interp,
+			    PNGImage *pngPtr);
 static int		WriteIHDR(Tcl_Interp *interp, PNGImage *pngPtr,
 			    Tk_PhotoImageBlock *blockPtr);
 static int		WriteIDAT(Tcl_Interp *interp, PNGImage *pngPtr,
@@ -2127,6 +2129,22 @@ ReadIDAT(
 	 */
     }
 
+    /*
+     * Ensure that when we've got to the end of the compressed data, we've
+     * also got to the end of the compressed stream. This sanity check is
+     * enforced by most PNG readers.
+     */
+
+    if (!Tcl_ZlibStreamEof(pngPtr->stream)) {
+	Tcl_AppendResult(interp, "unfinalized data stream in PNG data", NULL);
+	return TCL_ERROR;
+    }
+    if (chunkSz != 0) {
+	Tcl_AppendResult(interp,
+		"compressed data after stream finalize in PNG data", NULL);
+	return TCL_ERROR;
+    }
+
     if (CheckCRC(interp, pngPtr, crc) == TCL_ERROR) {
 	return TCL_ERROR;
     }
@@ -3099,11 +3117,13 @@ WriteIDAT(
 
 	/*
 	 * Compress the line of pixels into the destination. If this is the
-	 * last line, flush at the same time.
+	 * last line, finalize the compressor at the same time. Note that this
+	 * can't be just a flush; that leads to a file that some PNG readers
+	 * choke on. [Bug 2984787]
 	 */
 
 	if (rowNum + 1 == blockPtr->height) {
-	    flush = TCL_ZLIB_FLUSH;
+	    flush = TCL_ZLIB_FINALIZE;
 	}
 	if (Tcl_ZlibStreamPut(pngPtr->stream, pngPtr->thisLineObj,
 		flush) != TCL_OK) {
@@ -3138,6 +3158,64 @@ WriteIDAT(
 /*
  *----------------------------------------------------------------------
  *
+ * WriteExtraChunks --
+ *
+ *	Writes an sBIT and a tEXt chunks to the PNG image, describing a bunch
+ *	of not very important metadata that many readers seem to need anyway.
+ *
+ * Results:
+ *	TCL_OK, or TCL_ERROR if the write fails.
+ *
+ * Side effects:
+ *	None
+ *
+ *----------------------------------------------------------------------
+ */
+
+static int
+WriteExtraChunks(
+    Tcl_Interp *interp,
+    PNGImage *pngPtr)
+{
+    static const unsigned char sBIT_contents[] = {
+	8, 8, 8, 8
+    };
+    Tcl_DString buf;
+
+    /*
+     * Each byte of each channel is always significant; we always write RGBA
+     * images with 8 bits per channel as that is what the photo image's basic
+     * data model is.
+     */
+
+    if (WriteChunk(interp, pngPtr, CHUNK_sBIT, sBIT_contents, 4) != TCL_OK) {
+	return TCL_ERROR;
+    }
+
+    /*
+     * Say that it is Tk that made the PNG. Note that we *need* the NUL at the
+     * end of "Software" to be transferred; do *not* change the length
+     * parameter to -1 there!
+     */
+
+    Tcl_DStringInit(&buf);
+    Tcl_DStringAppend(&buf, "Software", 9);
+    Tcl_DStringAppend(&buf, "Tk Toolkit v", -1);
+    Tcl_DStringAppend(&buf, TK_PATCH_LEVEL, -1);
+    if (WriteChunk(interp, pngPtr, CHUNK_tEXt,
+	    (unsigned char *) Tcl_DStringValue(&buf),
+	    Tcl_DStringLength(&buf)) != TCL_OK) {
+	Tcl_DStringFree(&buf);
+	return TCL_ERROR;
+    }
+    Tcl_DStringFree(&buf);
+
+    return TCL_OK;
+}
+
+/*
+ *----------------------------------------------------------------------
+ *
  * EncodePNG --
  *
  *	This function handles the entirety of writing a PNG file (or data)
@@ -3235,6 +3313,15 @@ EncodePNG(
     }
 
     /*
+     * Write out the extra chunks containing metadata that is of interest to
+     * other programs more than us.
+     */
+
+    if (WriteExtraChunks(interp, pngPtr) == TCL_ERROR) {
+	return TCL_ERROR;
+    }
+
+    /*
      * Write out the image pixels in the IDAT (data) chunk.
      */
 
diff --git a/tests/imgPNG.test b/tests/imgPNG.test
index 7fb2622..1cfaad0 100644
--- a/tests/imgPNG.test
+++ b/tests/imgPNG.test
@@ -8,7 +8,7 @@
 # Copyright (c) 2008 Donal K. Fellows
 # All rights reserved.
 #
-# RCS: @(#) $Id: imgPNG.test,v 1.2 2009/01/13 01:46:06 patthoyts Exp $
+# RCS: @(#) $Id: imgPNG.test,v 1.3 2010/04/12 08:40:17 dkf Exp $
 
 package require tcltest 2.2
 namespace import ::tcltest::*
@@ -19,16 +19,20 @@ imageInit
 namespace eval png {
     variable encoded
     # Key names are from the names of the source images, which come from
-    # http://www.schaik.com/pngsuite/pngsuite.html
+    #    http://www.schaik.com/pngsuite/pngsuite.html
+    # The exception is "BadX", which is used to test handling badly compressed
+    # images.
     array set encoded {
 	basn0g08 "iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAAAAABWESUoAAAABGdBTUEAAYagMeiWXwAAAEFJREFUeJxjZGAkABQIyLMMBQWMDwgp+PcfP2B5MBwUMMoRkGdkonlcDAYFjI/wyv7/z/iH5nExGBQwyuCVZWQEAFDl/nE14thZAAAAAElFTkSuQmCC"
 	basn2c08 "iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAIAAAD8GO2jAAAABGdBTUEAAYagMeiWXwAAAEhJREFUeJzt1cEJADAMAkCF7JH9t3ITO0Qr9KH4zuErtA0EO4AKFPgcoO3kfUx4QIECD0qHH8KEBxQo8KB0OCOpQIG7cHejwAGCsfleD0DPSwAAAABJRU5ErkJggg=="
 	basn3p08 "iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAMAAABEpIrGAAAABGdBTUEAAYagMeiWXwAAAwBQTFRFIkQA9f/td/93y///EQoAOncAIiL//xH/EQAAIiIA/6xVZv9m/2Zm/wH/IhIA3P//zP+ZRET/AFVVIgAAy8v/REQAVf9Vy8sAMxoA/+zc7f//5P/L/9zcRP9EZmb/MwAARCIA7e3/ZmYA/6RE//+q7e0AAMvL/v///f/+//8BM/8zVSoAAQH/iIj/AKqqAQEARAAAiIgA/+TLulsAIv8iZjIA//+Zqqr/VQAAqqoAy2MAEf8R1P+qdzoA/0RE3GsAZgAAAf8BiEIA7P/ca9wA/9y6ADMzAO0A7XMA//+ImUoAEf//dwAA/4MB/7q6/nsA//7/AMsA/5mZIv//iAAA//93AIiI/9z/GjMAAACqM///AJkAmQAAAAABMmYA/7r/RP///6r/AHcAAP7+qgAASpkA//9m/yIiAACZi/8RVf///wEB/4j/AFUAABER///+//3+pP9EZv///2b/ADMA//9V/3d3AACI/0T/ABEAd///AGZm///tAAEA//XtERH///9E/yL//+3tEREAiP//AAB3k/8iANzcMzP//gD+urr/mf//MzMAY8sAuroArP9V///c//8ze/4A7QDtVVX/qv//3Nz/VVUAAABm3NwA3ADcg/8Bd3f//v7////L/1VVd3cA/v4AywDLAAD+AQIAAQAAEiIA//8iAEREm/8z/9SqAABVmZn/mZkAugC6KlUA/8vLtP9m/5sz//+6qgCqQogAU6oA/6qqAADtALq6//8RAP4AAABEAJmZmQCZ/8yZugAAiACIANwA/5MiAADc/v/+qlMAdwB3AgEAywAAAAAz/+3/ALoA/zMz7f/t/8SIvP93AKoAZgBmACIi3AAA/8v/3P/c/4sRAADLAAEBVQBVAIgAAAAiAf//y//L7QAA/4iIRABEW7oA/7x3/5n/AGYAuv+6AHd3c+0A/gAAMwAzAAC6/3f/AEQAqv+q//7+AAARIgAixP+IAO3tmf+Z/1X/ACIA/7RmEQARChEA/xER3P+6uv//iP+IAQAB/zP/uY7TYgAAAbFJREFUeJwNwQcACAQQAMBHqIxIZCs7Mwlla1hlZ+8VitCw9yoqNGiYDatsyt6jjIadlVkysve+u5jC9xTmV/qyl6bcJR7kAQZzg568xXmuE2lIyUNM5So7OMAFIhvp+YgGvEtFNnOKeJonSEvwP9NZzhHiOfLzBXPoxKP8yD6iPMXITjP+oTdfsp14lTJMJjGtOMFQfiFe4wWK8BP7qUd31hBNqMos2tKYFbRnJdGGjTzPz2yjEA1ZSKymKCM5ylaWcJrZxCZK8jgfU4vc/MW3xE7K8RUvsZb3Wc/XxCEqk4v/qMQlFvMZcZIafMOnLKM13zGceJNqPMU4KnCQAqQgbrKHpXSgFK/Qn6REO9YxjWE8Sx2SMJD4jfl8wgzy0YgPuEeUJQcD6EoWWpCaHsQkHuY9RpGON/icK0RyrvE680jG22TlHaIbx6jLnySkF+M5QxzmD6pwkTsMoSAdidqsojipuMyHzOQ4sYgfyElpzjKGErQkqvMyC7jFv9xmBM2JuTzDRDLxN4l4jF1EZjIwmhfZzSOMpT4xiH70IQG/k5En2UKcowudycsG8jCBmtwHgRv+EIeWyOAAAAAASUVORK5CYII="
 	basn6a08 "iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAYAAABzenr0AAAABGdBTUEAAYagMeiWXwAAAG9JREFUeJzt1jEKgDAMRuEnZGhPofc/VQSPIcTdxUV4HVLoUCj8H00o2YoBMF57fpz/ujODHXUFRwPKBqj5DVigB041HiJ9gFyCVOMbsEIPXNwuAHkgiJL/4qABNqB7QAeUPBAE2QAZUDZAfwEb8ABSIBqcFg+4TAAAAABJRU5ErkJggg=="
+
+	BadX "iVBORw0KGgoAAAANSUhEUgAAAAUAAAAFCAYAAACNbyblAAAABHNCSVQICAgIfAhkiAAAABN0RVh0U29mdHdhcmUAVGsgOC42YjEuMcrtT1oAAAAcSURBVHicYmBgYPjPgAr+ozP+o0uj68BUiWEmAAAA//8SozfjAAAAAElFTkSuQmCC"
     }
 
 # $encoded(basn0g08), $encoded(basn2c08), $encoded(basn3p08), $encoded(basn6a08)
-test png-1.1 {reading basic images; grayscale} -setup {
+test imgPNG-1.1 {reading basic images; grayscale} -setup {
     catch {rename foo ""}
 } -body {
     image create photo foo -data $encoded(basn0g08)
@@ -36,7 +40,7 @@ test png-1.1 {reading basic images; grayscale} -setup {
 } -cleanup {
     rename foo ""
 } -result {32 32}
-test png-1.2 {reading basic images; color} -setup {
+test imgPNG-1.2 {reading basic images; color} -setup {
     catch {rename foo ""}
 } -body {
     image create photo foo -data $encoded(basn2c08)
@@ -44,7 +48,7 @@ test png-1.2 {reading basic images; color} -setup {
 } -cleanup {
     rename foo ""
 } -result {32 32}
-test png-1.3 {reading basic images; color with palette} -setup {
+test imgPNG-1.3 {reading basic images; color with palette} -setup {
     catch {rename foo ""}
 } -body {
     image create photo foo -data $encoded(basn3p08)
@@ -52,7 +56,7 @@ test png-1.3 {reading basic images; color with palette} -setup {
 } -cleanup {
     rename foo ""
 } -result {32 32}
-test png-1.4 {reading basic images; alpha} -setup {
+test imgPNG-1.4 {reading basic images; alpha} -setup {
     catch {rename foo ""}
 } -body {
     image create photo foo -data $encoded(basn6a08)
@@ -60,6 +64,10 @@ test png-1.4 {reading basic images; alpha} -setup {
 } -cleanup {
     rename foo ""
 } -result {32 32}
+
+test imgPNG-2.1 {reading a bad image} -body {
+    image create photo -data $encoded(BadX)
+} -returnCodes error -result {unfinalized data stream in PNG data}
 
 }
 namespace delete png