[libpng16] Cleaned up and corrected ICC profile handling.

    contrib/libtests/makepng: corrected 'rgb' and 'gray' cases.  profile_error
    messages could be truncated; made a correct buffer size calculation and
    adjusted pngerror.c appropriately. png_icc_check_* checking improved;
    changed the functions to receive the correct color type of the PNG on read
    or write and check that it matches the color space of the profile (despite
    what the comments said before, there is danger in assuming the app will
    cope correctly with an RGB profile on a grayscale image and, since it
    violates the PNG spec, allowing it is certain to produce inconsistent
    app behavior and might even cause app crashes.) Check that profiles
    contain the tags needed to process the PNG (tags all required by the ICC
    spec). Removed unused PNG_STATIC from pngpriv.h.

8 files changed, 185 insertions, 45 deletions

diff --git a/ANNOUNCE b/ANNOUNCE
index 610f82a69..df496a3c9 100644
--- a/ANNOUNCE
+++ b/ANNOUNCE
@@ -1,5 +1,5 @@
 
-Libpng 1.6.0beta28 - August 18, 2012
+Libpng 1.6.0beta28 - August 25, 2012
 
 This is not intended to be a public release.  It will be replaced
 within a few weeks by a public version or by another test version.
@@ -442,7 +442,7 @@ Version 1.6.0beta27 [August 11, 2012]
   Work around gcc 3.x and Microsoft Visual Studio 2010 complaints. Both object
     to the split initialization of num_chunks.
 
-Version 1.6.0beta28 [August 18, 2012]
+Version 1.6.0beta28 [August 25, 2012]
   Unknown handling fixes and clean up. This adds more correct option
     control of the unknown handling, corrects the pre-existing bug where
     the per-chunk 'keep' setting is ignored and makes it possible to skip
@@ -469,6 +469,18 @@ Version 1.6.0beta28 [August 18, 2012]
     SAVE_UNKNOWN_CHUNKS is turned off *and* the application does not set
     a user callback an unknown chunk will not be read, leading to a read
     error, which was revealed by the "tunknown" test.
+  Cleaned up and corrected ICC profile handling.
+    contrib/libtests/makepng: corrected 'rgb' and 'gray' cases.  profile_error
+    messages could be truncated; made a correct buffer size calculation and
+    adjusted pngerror.c appropriately. png_icc_check_* checking improved;
+    changed the functions to receive the correct color type of the PNG on read
+    or write and check that it matches the color space of the profile (despite
+    what the comments said before, there is danger in assuming the app will
+    cope correctly with an RGB profile on a grayscale image and, since it
+    violates the PNG spec, allowing it is certain to produce inconsistent
+    app behavior and might even cause app crashes.) Check that profiles
+    contain the tags needed to process the PNG (tags all required by the ICC
+    spec). Removed unused PNG_STATIC from pngpriv.h.
 
 Send comments/corrections/commendations to png-mng-implement at lists.sf.net
 (subscription required; visit
diff --git a/CHANGES b/CHANGES
index f85d94bef..4fc380296 100644
--- a/CHANGES
+++ b/CHANGES
@@ -4193,7 +4193,7 @@ Version 1.6.0beta27 [August 11, 2012]
   Work around gcc 3.x and Microsoft Visual Studio 2010 complaints. Both object
     to the split initialization of num_chunks.
 
-Version 1.6.0beta28 [August 18, 2012]
+Version 1.6.0beta28 [August 25, 2012]
   Unknown handling fixes and clean up. This adds more correct option
     control of the unknown handling, corrects the pre-existing bug where
     the per-chunk 'keep' setting is ignored and makes it possible to skip
@@ -4220,6 +4220,18 @@ Version 1.6.0beta28 [August 18, 2012]
     SAVE_UNKNOWN_CHUNKS is turned off *and* the application does not set
     a user callback an unknown chunk will not be read, leading to a read
     error, which was revealed by the "tunknown" test.
+  Cleaned up and corrected ICC profile handling.
+    contrib/libtests/makepng: corrected 'rgb' and 'gray' cases.  profile_error
+    messages could be truncated; made a correct buffer size calculation and
+    adjusted pngerror.c appropriately. png_icc_check_* checking improved;
+    changed the functions to receive the correct color type of the PNG on read
+    or write and check that it matches the color space of the profile (despite
+    what the comments said before, there is danger in assuming the app will
+    cope correctly with an RGB profile on a grayscale image and, since it
+    violates the PNG spec, allowing it is certain to produce inconsistent
+    app behavior and might even cause app crashes.) Check that profiles
+    contain the tags needed to process the PNG (tags all required by the ICC
+    spec). Removed unused PNG_STATIC from pngpriv.h.
 
 Send comments/corrections/commendations to png-mng-implement at lists.sf.net
 (subscription required; visit
diff --git a/contrib/libtests/makepng.c b/contrib/libtests/makepng.c
index a0bfb4bae..5fdaa67d9 100644
--- a/contrib/libtests/makepng.c
+++ b/contrib/libtests/makepng.c
@@ -72,6 +72,7 @@
 #include <stdlib.h>
 #include <stdio.h>
 #include <string.h>
+#include <ctype.h>
 #include <math.h>
 #include <errno.h>
 
@@ -1125,7 +1126,7 @@ main(int argc, char **argv)
 
       if (strncmp(arg, "gray", 4) == 0)
       {
-         if (arg[5] == 0)
+         if (arg[4] == 0)
          {
             color_type = PNG_COLOR_TYPE_GRAY;
             continue;
@@ -1142,7 +1143,7 @@ main(int argc, char **argv)
 
       if (strncmp(arg, "rgb", 3) == 0)
       {
-         if (arg[4] == 0)
+         if (arg[3] == 0)
          {
             color_type = PNG_COLOR_TYPE_RGB;
             continue;
@@ -1157,7 +1158,7 @@ main(int argc, char **argv)
          }
       }
 
-      if (color_type == 8)
+      if (color_type == 8 && isdigit(arg[0]))
       {
          color_type = atoi(arg);
          if (color_type < 0 || color_type > 6 || color_type == 1 ||
@@ -1170,7 +1171,7 @@ main(int argc, char **argv)
          continue;
       }
 
-      if (bit_depth == 32)
+      if (bit_depth == 32 && isdigit(arg[0]))
       {
          bit_depth = atoi(arg);
          if (bit_depth <= 0 || bit_depth > 16 ||
diff --git a/png.c b/png.c
index d1789d6a0..861bde124 100644
--- a/png.c
+++ b/png.c
@@ -749,13 +749,13 @@ png_get_copyright(png_const_structrp png_ptr)
 #else
 #  ifdef __STDC__
    return PNG_STRING_NEWLINE \
-     "libpng version 1.6.0beta28 - August 22, 2012" PNG_STRING_NEWLINE \
+     "libpng version 1.6.0beta28 - August 25, 2012" PNG_STRING_NEWLINE \
      "Copyright (c) 1998-2012 Glenn Randers-Pehrson" PNG_STRING_NEWLINE \
      "Copyright (c) 1996-1997 Andreas Dilger" PNG_STRING_NEWLINE \
      "Copyright (c) 1995-1996 Guy Eric Schalnat, Group 42, Inc." \
      PNG_STRING_NEWLINE;
 #  else
-      return "libpng version 1.6.0beta28 - August 22, 2012\
+      return "libpng version 1.6.0beta28 - August 25, 2012\
       Copyright (c) 1998-2012 Glenn Randers-Pehrson\
       Copyright (c) 1996-1997 Andreas Dilger\
       Copyright (c) 1995-1996 Guy Eric Schalnat, Group 42, Inc.";
@@ -1693,24 +1693,25 @@ profile_error(png_const_structrp png_ptr, png_colorspacerp colorspace,
    png_const_charp name, png_alloc_size_t value, png_const_charp reason)
 {
    size_t pos;
-   char message[256];
+   char message[196]; /* see below for calculation */
 
    if (colorspace != NULL)
       colorspace->flags |= PNG_COLORSPACE_INVALID;
 
-   pos = png_safecat(message, (sizeof message), 0, "profile '");
-   pos = png_safecat(message, pos+79, pos, name);
-   pos = png_safecat(message, (sizeof message), pos, "': ");
+   pos = png_safecat(message, (sizeof message), 0, "profile '"); /* 9 chars */
+   pos = png_safecat(message, pos+79, pos, name); /* Truncate to 79 chars */
+   pos = png_safecat(message, (sizeof message), pos, "': "); /* +2 = 90 */
 #  ifdef PNG_WARNINGS_SUPPORTED
       {
-         char number[PNG_NUMBER_BUFFER_SIZE];
+         char number[PNG_NUMBER_BUFFER_SIZE]; /* +24 = 114*/
 
          pos = png_safecat(message, (sizeof message), pos,
             png_format_number(number, number+(sizeof number),
                PNG_NUMBER_FORMAT_x, value));
       }
-      pos = png_safecat(message, (sizeof message), pos, ": ");
+      pos = png_safecat(message, (sizeof message), pos, "h: "); /* +2 = 116 */
 #  endif
+   /* The 'reason' is an arbitrary message, allow +79 maximum 195 */
    pos = png_safecat(message, (sizeof message), pos, reason);
 
    if (colorspace != NULL)
@@ -1874,7 +1875,7 @@ png_icc_check_length(png_const_structrp png_ptr, png_colorspacerp colorspace,
 int /* PRIVATE */
 png_icc_check_header(png_const_structrp png_ptr, png_colorspacerp colorspace,
    png_const_charp name, png_uint_32 profile_length,
-   png_const_bytep profile/* first 132 bytes only */)
+   png_const_bytep profile/* first 132 bytes only */, int color_type)
 {
    png_uint_32 temp;
 
@@ -1930,16 +1931,37 @@ png_icc_check_header(png_const_structrp png_ptr, png_colorspacerp colorspace,
     * 6), or a greyscale colour space for greyscale images (PNG colour types 0
     * and 4)."
     *
-    * This code does not check the color type because png_set_iCCP may be called
-    * before png_set_IHDR on write and because, anyway, the PNG spec is
-    * fundamentally flawed: RGB profiles can be used quite meaningfully for
-    * grayscale images and both RGB and palette images might only have gray
-    * colors in them, so gray profiles may be appropriate.
+    * This checking code ensures the embedded profile (on either read or write)
+    * conforms to the specification requirements.  Notice that an ICC 'gray'
+    * color-space profile contains the information to transform the monochrome
+    * data to XYZ or L*a*b (according to which PCS the profile uses) and this
+    * should be used in preference to the standard libpng K channel replication
+    * into R, G and B channels.
+    *
+    * Previously it was suggested that an RGB profile on grayscale data could be
+    * handled.  However it it is clear that using an RGB profile in this context
+    * must be an error - there is no specification of what it means.  Thus it is
+    * almost certainly more correct to ignore the profile.
     */
    temp = png_get_uint_32(profile+16); /* data colour space field */
-   if (temp != 0x52474220 /* 'RGB ' */ && temp != 0x47524159 /* 'GRAY' */)
-      return profile_error(png_ptr, colorspace, name, temp,
-         "invalid color space");
+   switch (temp)
+   {
+      case 0x52474220: /* 'RGB ' */
+         if (!(color_type & PNG_COLOR_MASK_COLOR))
+            return profile_error(png_ptr, colorspace, name, temp,
+               "RGB color space not permitted on grayscale PNG");
+         break;
+
+      case 0x47524159: /* 'GRAY' */
+         if (color_type & PNG_COLOR_MASK_COLOR)
+            return profile_error(png_ptr, colorspace, name, temp,
+               "Gray color space not permitted on RGB PNG");
+         break;
+
+      default:
+         return profile_error(png_ptr, colorspace, name, temp,
+            "invalid color space");
+   }
 
    /* It is up to the application to check that the profile class matches the
     * application requirements; the spec provides no guidance, but it's pretty
@@ -1956,13 +1978,16 @@ png_icc_check_header(png_const_structrp png_ptr, png_colorspacerp colorspace,
    {
       if (temp == 0x6C696E6B /* 'link' */ || temp == 0x61627374 /* 'abst' */)
          return profile_error(png_ptr, colorspace, name, temp,
-            "invalid class");
+            "invalid ICC profile class");
 
       /* This can only be 0x6E6D636C: a 'nmcl' profile.  This is a device
-       * specific profile.
+       * specific profile.  The checks on the tags below will ensure that it can
+       * actually be used, but it certainly is not expected and is probably an
+       * error.
        */
       else
-         (void)profile_error(png_ptr, NULL, name, temp, "unexpected ICC class");
+         (void)profile_error(png_ptr, NULL, name, temp,
+            "unexpected ICC profile class");
    }
 
    return 1;
@@ -1976,6 +2001,16 @@ png_icc_check_tag_table(png_const_structrp png_ptr, png_colorspacerp colorspace,
    png_uint_32 tag_count = png_get_uint_32(profile+128);
    png_uint_32 itag;
    png_const_bytep tag = profile+132; /* The first tag */
+   int have_AToB0Tag = 0;   /* Whether the profile has an AToB0Tag */
+   int have_grayTRCTag = 0; /* Whether the profile has a grayTRCTag */
+   unsigned int matrix_TRC_tags = 0; /* Which matrix/TRC tags are present */
+#  define HAVE_redMatrixColumnTag   0x01
+#  define HAVE_greenMatrixColumnTag 0x02
+#  define HAVE_blueMatrixColumnTag  0x04
+#  define HAVE_redTRCTag            0x10
+#  define HAVE_greenTRCTag          0x20
+#  define HAVE_blueTRCTag           0x40
+#  define HAVE_all_tags             0x77
 
    for (itag=0; itag < tag_count; ++itag, tag += 12)
    {
@@ -1992,9 +2027,90 @@ png_icc_check_tag_table(png_const_structrp png_ptr, png_colorspacerp colorspace,
          tag_length > profile_length - tag_start)
          return profile_error(png_ptr, colorspace, name, tag_id,
             "tag data outside profile");
+
+      /* Check the tag_id for the specific profiles which must be present for
+       * the profile to be valid.
+       */
+      switch (tag_id)
+      {
+         case 0x41324230: /* 'A2B0' - AToB0Tag */
+            have_AToB0Tag = 1;
+            break;
+
+         case 0x6B545243: /* 'kTRC' - grayTRCTag */
+            have_grayTRCTag = 1;
+            break;
+
+         case 0x7258595A: /* 'rXYZ' - redMatrixColumnTag */
+            matrix_TRC_tags |= HAVE_redMatrixColumnTag;
+            break;
+
+         case 0x72545243: /* 'rTRC' - redTRCTag */
+            matrix_TRC_tags |= HAVE_redTRCTag;
+            break;
+
+         case 0x6758595A: /* 'gXYZ' - greenMatrixColumnTag */
+            matrix_TRC_tags |= HAVE_greenMatrixColumnTag;
+            break;
+
+         case 0x67545243: /* 'gTRC' - greenTRCTag */
+            matrix_TRC_tags |= HAVE_greenTRCTag;
+            break;
+
+         case 0x6258595A: /* 'bXYZ' - blueMatrixColumnTag */
+            matrix_TRC_tags |= HAVE_blueMatrixColumnTag;
+            break;
+
+         case 0x62545243: /* 'bTRC' - blueTRCTag */
+            matrix_TRC_tags |= HAVE_blueTRCTag;
+            break;
+
+         default:
+            break;
+      }
    }
 
-   return 1;
+   /* An AToB0Tag works in all valid profiles, but if it is absent then
+    * something matching the profile class and color space must be present.
+    */
+   if (!have_AToB0Tag)
+   {
+      png_uint_32 profile_class = png_get_uint_32(profile+12);
+
+      switch (profile_class)
+      {
+         case 0x73636E72: /* 'scnr' - an input profile */
+         case 0x6D6E7472: /* 'mntr' - a display device profile */
+         case 0x70727472: /* 'prtr' - an output device profile */
+            if (png_get_uint_32(profile+16) /* color space */ ==
+               0x47524159 /* gray */)
+            {
+               if (!have_grayTRCTag)
+                  return profile_error(png_ptr, colorspace, name, profile_class,
+                     "missing grayTRCTag for monochrome profile");
+            }
+
+            else
+            {
+               if (matrix_TRC_tags != HAVE_all_tags)
+                  return profile_error(png_ptr, colorspace, name, profile_class,
+                     "missing Matrix/TRC tags for RGB profile");
+            }
+
+            return 1;
+
+         case 0x73706163: /* 'spac' */
+            return profile_error(png_ptr, colorspace, name, profile_class,
+               "missing AToB0Tag for colorspace profile");
+
+         default: /* should have been checked before */
+            png_error(png_ptr, "invalid ICC class");
+            return 0; /* NOT REACHED */
+      }
+   }
+
+   else
+      return 1;
 }
 
 #ifdef PNG_sRGB_SUPPORTED
@@ -2192,14 +2308,15 @@ png_icc_set_gAMA_and_cHRM(png_const_structrp png_ptr,
 int /* PRIVATE */
 png_colorspace_set_ICC(png_const_structrp png_ptr, png_colorspacerp colorspace,
    png_const_charp name, png_uint_32 profile_length,
-   png_const_bytep profile, int preferred)
+   png_const_bytep profile, int preferred, int color_type)
 {
    if (colorspace->flags & PNG_COLORSPACE_INVALID)
       return 0;
 
    if (png_icc_check_length(png_ptr, colorspace, name, profile_length) &&
-      png_icc_check_header(png_ptr, colorspace, name, profile_length, profile)
-      && png_icc_check_tag_table(png_ptr, colorspace, name, profile_length,
+      png_icc_check_header(png_ptr, colorspace, name, profile_length, profile,
+         color_type) &&
+      png_icc_check_tag_table(png_ptr, colorspace, name, profile_length,
          profile))
    {
       png_icc_set_gAMA_and_cHRM(png_ptr, colorspace, name, profile, 0,
diff --git a/pngerror.c b/pngerror.c
index 0a0eeefbc..dbc6534ba 100644
--- a/pngerror.c
+++ b/pngerror.c
@@ -415,7 +415,7 @@ static PNG_CONST char png_digit[16] = {
    'A', 'B', 'C', 'D', 'E', 'F'
 };
 
-#define PNG_MAX_ERROR_TEXT 64
+#define PNG_MAX_ERROR_TEXT 196 /* Currently limited be profile_error in png.c */
 #if defined(PNG_WARNINGS_SUPPORTED) || defined(PNG_ERROR_TEXT_SUPPORTED)
 static void /* PRIVATE */
 png_format_buffer(png_const_structrp png_ptr, png_charp buffer, png_const_charp
diff --git a/pngpriv.h b/pngpriv.h
index 0970603b1..ffd4fd8ab 100644
--- a/pngpriv.h
+++ b/pngpriv.h
@@ -281,13 +281,6 @@ typedef const png_uint_16p * png_const_uint_16pp;
 #  define PNG_ZBUF_SIZE 65536L
 #endif
 
-/* PNG_STATIC is used to mark internal file scope functions if they need to be
- * accessed for implementation tests (see the code in tests/?*).
- */
-#ifndef PNG_STATIC
-#   define PNG_STATIC static
-#endif
-
 /* If warnings or errors are turned off the code is disabled or redirected here.
  * From 1.5.4 functions have been added to allow very limited formatting of
  * error and warning messages - this code will also be disabled here.
@@ -1449,8 +1442,8 @@ PNG_INTERNAL_FUNCTION(int,png_colorspace_set_sRGB,(png_const_structrp png_ptr,
 #ifdef PNG_iCCP_SUPPORTED
 PNG_INTERNAL_FUNCTION(int,png_colorspace_set_ICC,(png_const_structrp png_ptr,
    png_colorspacerp colorspace, png_const_charp name,
-   png_uint_32 profile_length, png_const_bytep profile, int preferred),
-   PNG_EMPTY);
+   png_uint_32 profile_length, png_const_bytep profile, int preferred,
+   int color_type), PNG_EMPTY);
    /* The 'name' is used for information only */
 
 /* Routines for checking parts of an ICC profile. */
@@ -1460,7 +1453,8 @@ PNG_INTERNAL_FUNCTION(int,png_icc_check_length,(png_const_structrp png_ptr,
 PNG_INTERNAL_FUNCTION(int,png_icc_check_header,(png_const_structrp png_ptr,
    png_colorspacerp colorspace, png_const_charp name,
    png_uint_32 profile_length,
-   png_const_bytep profile /* first 132 bytes only */), PNG_EMPTY);
+   png_const_bytep profile /* first 132 bytes only */, int color_type),
+   PNG_EMPTY);
 PNG_INTERNAL_FUNCTION(int,png_icc_check_tag_table,(png_const_structrp png_ptr,
    png_colorspacerp colorspace, png_const_charp name,
    png_uint_32 profile_length,
diff --git a/pngrutil.c b/pngrutil.c
index 975b3910f..b6dd2e500 100644
--- a/pngrutil.c
+++ b/pngrutil.c
@@ -1424,7 +1424,8 @@ png_handle_iCCP(png_structrp png_ptr, png_inforp info_ptr, png_uint_32 length)
                       * byte header.
                       */
                      if (png_icc_check_header(png_ptr, &png_ptr->colorspace,
-                        keyword, profile_length, profile_header))
+                        keyword, profile_length, profile_header,
+                        png_ptr->color_type))
                      {
                         /* Now read the tag table; a variable size buffer is
                          * needed at this point, allocate one for the whole
diff --git a/pngset.c b/pngset.c
index 17bfe726d..c7d2d5e47 100644
--- a/pngset.c
+++ b/pngset.c
@@ -643,11 +643,14 @@ png_set_iCCP(png_const_structrp png_ptr, png_inforp info_ptr,
 
    /* Set the colorspace first because this validates the profile; do not
     * override previously set app cHRM or gAMA here (because likely as not the
-    * application knows better than libpng what the correct values are.)
+    * application knows better than libpng what the correct values are.)  Pass
+    * the info_ptr color_type field to png_colorspace_set_ICC because in the
+    * write case it has not yet been stored in png_ptr.
     */
    {
       int result = png_colorspace_set_ICC(png_ptr, &info_ptr->colorspace, name,
-         proflen, profile, 0/* do *not* override the app cHRM or gAMA */);
+         proflen, profile, 0/* do *not* override the app cHRM or gAMA */,
+         info_ptr->color_type);
 
       png_colorspace_sync_info(png_ptr, info_ptr);