[libpng16] Fixed some bugs in ICC profile writing. The code should now accept

all potentially valid ICC profiles and reject obviously invalid ones.
It now uses png_error() to do so rather than casually writing a PNG
without the necessary color data.

ANNOUNCE

@@ -252,6 +252,10 @@ Version 1.6.0beta15 [March 2, 2012]
     still work-in-progress.
   Added tests for invalid palette index while reading and writing (work in
     progress, the latter isn't finished).
+  Fixed some bugs in ICC profile writing. The code should now accept
+    all potentially valid ICC profiles and reject obviously invalid ones.
+    It now uses png_error() to do so rather than casually writing a PNG
+    without the necessary color data.
 
 Send comments/corrections/commendations to png-mng-implement at lists.sf.net
 (subscription required; visit

CHANGES

@@ -4004,6 +4004,10 @@ Version 1.6.0beta15 [March 2, 2012]
     still work-in-progress.
   Added tests for invalid palette index while reading and writing (work in
     progress, the latter isn't finished).
+  Fixed some bugs in ICC profile writing. The code should now accept
+    all potentially valid ICC profiles and reject obviously invalid ones.
+    It now uses png_error() to do so rather than casually writing a PNG
+    without the necessary color data.
 
 Send comments/corrections/commendations to png-mng-implement at lists.sf.net
 (subscription required; visit

contrib/libtests/makepng.c

@@ -749,7 +749,10 @@ insert_iCCP(png_structp png_ptr, png_infop info_ptr, int nparams,
                      params[1], (unsigned long)fake_len);
                   exit(1);
                }
-               proflen = (png_uint_32)fake_len;
+               proflen = (png_uint_32)(fake_len & ~3U);
+               /* Always fix up the profile length. */
+               png_save_uint_32(profile, proflen);
+               break;
             }
          }
 
@@ -777,7 +780,7 @@ insert_iCCP(png_structp png_ptr, png_infop info_ptr, int nparams,
       {
          fprintf(stderr, "--insert iCCP %s: profile length field wrong:\n",
             params[1]);
-         fprintf(stderr, "  actual %lu, recorded value %lu (correted)\n",
+         fprintf(stderr, "  actual %lu, recorded value %lu (corrected)\n",
             (unsigned long)proflen, (unsigned long)prof_header);
          png_save_uint_32(profile, proflen);
       }
@@ -821,6 +824,20 @@ set_text(png_structp png_ptr, png_infop info_ptr, png_textp text,
          }
          break;
 
+      case '0': case '1': case '2': case '3': case '4':
+      case '5': case '6': case '7': case '8': case '9':
+         {
+            png_bytep data = NULL;
+            png_size_t fake_len = load_fake(param, &data);
+
+            if (fake_len > 0) /* else a simple parameter */
+            {
+               text->text_length = fake_len;
+               text->text = (png_charp)data;
+               break;
+            }
+         }
+
       default:
          text->text = param;
          break;
@@ -864,9 +881,9 @@ insert_iTXt(png_structp png_ptr, png_infop info_ptr, int nparams,
    check_param_count(nparams, 4);
    clear_text(&text, params[0]);
    text.compression = 2; /* iTXt + deflate */
-   text.lang = params[2];/* language tag */
+   text.lang = params[1];/* language tag */
-   text.lang_key = params[3]; /* translated keyword */
+   text.lang_key = params[2]; /* translated keyword */
-   set_text(png_ptr, info_ptr, &text, params[1]);
+   set_text(png_ptr, info_ptr, &text, params[3]);
 }
 
 static void

contrib/libtests/pngstest.c

@@ -1938,7 +1938,7 @@ static png_uint_16 gpc_error[16/*in*/][16/*out*/][4/*a*/] =
   { 0, 0, 0, 0 }, { 0, 0, 0, 0 }, { 0, 0, 0, 0 }, { 0, 0, 0, 0 },
   { 0, 0, 0, 0 }, { 0, 0, 0, 0 }, { 0, 0, 0, 0 }, { 0, 0, 0, 0 }
  }, { /* input: sRGB-rgb+alpha */
-  { 0, 4, 7, 0 }, { 0, 14, 7, 0 }, { 0, 19, 0, 0 }, { 0, 0, 0, 0 },
+  { 0, 4, 13, 0 }, { 0, 14, 13, 0 }, { 0, 19, 0, 0 }, { 0, 0, 0, 0 },
   { 0, 832, 764, 0 }, { 0, 832, 764, 0 }, { 0, 897, 788, 0 }, { 0, 897, 788, 0 },
   { 0, 0, 0, 0 }, { 0, 0, 0, 0 }, { 0, 0, 0, 0 }, { 0, 0, 0, 0 },
   { 0, 0, 0, 0 }, { 0, 0, 0, 0 }, { 0, 0, 0, 0 }, { 0, 0, 0, 0 }
@@ -2013,7 +2013,7 @@ static png_uint_16 gpc_error_via_linear[16][4/*out*/][4] =
  }, { /* input: sRGB-rgb */
   { 0, 0, 19, 0 }, { 0, 0, 19, 0 }, { 0, 0, 15, 0 }, { 0, 0, 15, 0 }
  }, { /* input: sRGB-rgb+alpha */
-  { 0, 9, 10, 0 }, { 0, 180, 10, 0 }, { 0, 14, 15, 0 }, { 0, 186, 15, 0 }
+  { 0, 12, 14, 0 }, { 0, 180, 14, 0 }, { 0, 14, 15, 0 }, { 0, 186, 15, 0 }
  }, { /* input: linear-gray */
   { 0, 0, 1, 0 }, { 0, 0, 1, 0 }, { 0, 0, 1, 0 }, { 0, 0, 1, 0 }
  }, { /* input: linear-gray+alpha */

pngpriv.h

@@ -898,7 +898,7 @@ PNG_INTERNAL_FUNCTION(void,png_write_sRGB,(png_structrp png_ptr,
 #ifdef PNG_WRITE_iCCP_SUPPORTED
 PNG_INTERNAL_FUNCTION(void,png_write_iCCP,(png_structrp png_ptr,
     png_const_charp name, int compression_type,
-    png_const_charp profile, int proflen),PNG_EMPTY);
+    png_const_bytep profile, png_uint_32 proflen),PNG_EMPTY);
    /* Note to maintainer: profile should be png_bytep */
 #endif
 

pngwrite.c

@@ -73,7 +73,7 @@ png_write_info_before_PLTE(png_structrp png_ptr, png_inforp info_ptr)
 #ifdef PNG_WRITE_iCCP_SUPPORTED
    if (info_ptr->valid & PNG_INFO_iCCP)
       png_write_iCCP(png_ptr, info_ptr->iccp_name, PNG_COMPRESSION_TYPE_BASE,
-          (png_charp)info_ptr->iccp_profile, (int)info_ptr->iccp_proflen);
+          info_ptr->iccp_profile, info_ptr->iccp_proflen);
 #endif
 #ifdef PNG_WRITE_sBIT_SUPPORTED
    if (info_ptr->valid & PNG_INFO_sBIT)

pngwutil.c

@@ -373,7 +373,7 @@ typedef struct
 } compression_state;
 
 /* Compress given text into storage in the png_ptr structure */
-static int /* PRIVATE */
+static png_size_t /* PRIVATE */
 png_text_compress(png_structrp png_ptr,
     png_const_charp text, png_size_t text_len, int compression,
     compression_state *comp)
@@ -390,7 +390,7 @@ png_text_compress(png_structrp png_ptr,
    if (compression == PNG_TEXT_COMPRESSION_NONE)
    {
       comp->input = (png_const_bytep)text;
-      return((int)text_len);
+      return text_len;
    }
 
    if (compression >= PNG_TEXT_COMPRESSION_LAST)
@@ -419,7 +419,7 @@ png_text_compress(png_structrp png_ptr,
    png_zlib_claim(png_ptr, PNG_ZLIB_FOR_TEXT);
 
    /* Set up the compression buffers */
-   /* TODO: the following cast hides a potential overflow problem. */
+   /* TODO: the following cast hides a ****CERTAIN**** overflow problem. */
    png_ptr->zstream.avail_in = (uInt)text_len;
 
    /* NOTE: assume zlib doesn't overwrite the input */
@@ -516,9 +516,8 @@ png_text_compress(png_structrp png_ptr,
                   old_ptr = comp->output_ptr;
 
                   /* This could be optimized to realloc() */
-                  comp->output_ptr = (png_bytepp)png_malloc(png_ptr,
+                  comp->output_ptr = png_voidcast(png_bytepp,png_malloc(png_ptr,
-                      (png_alloc_size_t)(comp->max_output_ptr *
+                      comp->max_output_ptr * png_sizeof(png_charp)));
-                      png_sizeof(png_charp)));
 
                   png_memcpy(comp->output_ptr, old_ptr,
                       old_max * png_sizeof(png_charp));
@@ -527,15 +526,13 @@ png_text_compress(png_structrp png_ptr,
                }
 
                else
-                  comp->output_ptr = (png_bytepp)png_malloc(png_ptr,
+                  comp->output_ptr = png_voidcast(png_bytepp,png_malloc(png_ptr,
-                      (png_alloc_size_t)(comp->max_output_ptr *
+                      comp->max_output_ptr * png_sizeof(png_charp)));
-                      png_sizeof(png_charp)));
             }
 
             /* Save the data */
-            comp->output_ptr[comp->num_output_ptr] =
+            comp->output_ptr[comp->num_output_ptr] = png_voidcast(png_bytep,
-                (png_bytep)png_malloc(png_ptr,
+                png_malloc(png_ptr, png_ptr->zbuf_size));
-                (png_alloc_size_t)png_ptr->zbuf_size);
 
             png_memcpy(comp->output_ptr[comp->num_output_ptr], png_ptr->zbuf,
                 png_ptr->zbuf_size);
@@ -559,12 +556,13 @@ png_text_compress(png_structrp png_ptr,
    } while (ret != Z_STREAM_END);
 
    /* Text length is number of buffers plus last buffer */
+   /* TODO: this ****WILL**** overflow */
    text_len = png_ptr->zbuf_size * comp->num_output_ptr;
 
    if (png_ptr->zstream.avail_out < png_ptr->zbuf_size)
-      text_len += png_ptr->zbuf_size - (png_size_t)png_ptr->zstream.avail_out;
+      text_len += png_ptr->zbuf_size - png_ptr->zstream.avail_out;
 
-   return((int)text_len);
+   return text_len;
 }
 
 /* Ship the compressed text out via chunk writes */
@@ -641,12 +639,13 @@ png_write_compressed_data_out(png_structrp png_ptr, compression_state *comp)
       }
 
       else
-         png_error(png_ptr,
+         png_warning(png_ptr, /* must be a warning or the data isn't freed */
              "Invalid zlib compression method or flags in non-IDAT chunk");
    }
 #endif /* PNG_WRITE_OPTIMIZE_CMF_SUPPORTED */
 
    /* Write saved output buffers, if any */
+   /* WARNING: SIDE EFFECT; the code below is what actually frees the data */
    for (i = 0; i < comp->num_output_ptr; i++)
    {
       png_write_chunk_data(png_ptr, comp->output_ptr[i],
@@ -1092,79 +1091,80 @@ png_write_sRGB(png_structrp png_ptr, int srgb_intent)
 /* Write an iCCP chunk */
 void /* PRIVATE */
 png_write_iCCP(png_structrp png_ptr, png_const_charp name, int compression_type,
-    png_const_charp profile, int profile_len)
+    png_const_bytep profile, png_uint_32 profile_len)
 {
-   png_size_t name_len;
+   png_size_t name_len, compressed_profile_len;
    png_charp new_name;
    compression_state comp;
-   int embedded_profile_len = 0;
+   png_uint_32 embedded_profile_len = 0;
 
    png_debug(1, "in png_write_iCCP");
 
+   if (compression_type != PNG_COMPRESSION_TYPE_BASE)
+      png_error(png_ptr, "Unknown compression type for iCCP chunk");
+
+   if (profile == NULL)
+      png_error(png_ptr, "No profile for iCCP chunk");
+
+   if (profile_len < 132)
+      png_error(png_ptr, "ICC profile too short");
+
+   if (profile_len & 3)
+      png_error(png_ptr, "ICC profile length invalid (not a multiple of 4)");
+
+   embedded_profile_len = png_get_uint_32(profile);
+
+   if (profile_len != embedded_profile_len)
+      png_error(png_ptr, "Profile length does not match profile");
+
+   if ((png_size_t)profile_len != profile_len)
+   {
+      /* This isn't an application error, technically, but all the same do
+       * not short-change the app by writing a PNG without the profile!
+       */
+      png_error(png_ptr, "Profile length exceeds system limits");
+   }
+
+   if ((name_len = png_check_keyword(png_ptr, name, &new_name)) == 0)
+      return;
+
    comp.num_output_ptr = 0;
    comp.max_output_ptr = 0;
    comp.output_ptr = NULL;
    comp.input = NULL;
    comp.input_len = 0;
 
-   if ((name_len = png_check_keyword(png_ptr, name, &new_name)) == 0)
+   compressed_profile_len = png_text_compress(png_ptr, (png_const_charp)profile,
-      return;
+       (png_size_t)/*ok*/profile_len, PNG_COMPRESSION_TYPE_BASE, &comp);
 
-   if (compression_type != PNG_COMPRESSION_TYPE_BASE)
+   /* 'name_len' is 1..79, so the following is safe: */
-      png_warning(png_ptr, "Unknown compression type in iCCP chunk");
+   if (compressed_profile_len > PNG_UINT_31_MAX - name_len - 2)
-
-   if (profile == NULL)
-      profile_len = 0;
-
-   if (profile_len > 3)
-      embedded_profile_len =
-          ((*( (png_const_bytep)profile    ))<<24) |
-          ((*( (png_const_bytep)profile + 1))<<16) |
-          ((*( (png_const_bytep)profile + 2))<< 8) |
-          ((*( (png_const_bytep)profile + 3))    );
-
-   if (embedded_profile_len < 0)
    {
-      png_warning(png_ptr,
-          "Embedded profile length in iCCP chunk is negative");
-
       png_free(png_ptr, new_name);
-      return;
+
+      /* TODO: there is no 'compression_free' function */
+      if (comp.output_ptr)
+      {
+         int i;
+         for (i = 0; i < comp.num_output_ptr; i++)
+            png_free(png_ptr, comp.output_ptr[i]);
+         png_free(png_ptr, comp.output_ptr);
+      }
+
+      png_error(png_ptr, "Compressed profile exceeds PNG size limits");
    }
 
-   if (profile_len < embedded_profile_len)
-   {
-      png_warning(png_ptr,
-          "Embedded profile length too large in iCCP chunk");
-
-      png_free(png_ptr, new_name);
-      return;
-   }
-
-   if (profile_len > embedded_profile_len)
-   {
-      png_warning(png_ptr,
-          "Truncating profile to actual length in iCCP chunk");
-
-      profile_len = embedded_profile_len;
-   }
-
-   if (profile_len)
-      profile_len = png_text_compress(png_ptr, profile,
-          (png_size_t)profile_len, PNG_COMPRESSION_TYPE_BASE, &comp);
-
    /* Make sure we include the NULL after the name and the compression type */
    png_write_chunk_header(png_ptr, png_iCCP,
-       (png_uint_32)(name_len + profile_len + 2));
+       (png_uint_32)/*ok*/(name_len + compressed_profile_len + 2));
 
    new_name[name_len + 1] = 0x00;
 
-   png_write_chunk_data(png_ptr, (png_bytep)new_name,
+   png_write_chunk_data(png_ptr, (png_bytep)new_name, name_len + 2);
-       (png_size_t)(name_len + 2));
 
-   if (profile_len)
+   if (compressed_profile_len)
    {
-      comp.input_len = profile_len;
+      comp.input_len = compressed_profile_len;
       png_write_compressed_data_out(png_ptr, &comp);
    }