[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.
diff --git a/ANNOUNCE b/ANNOUNCE
index a90be28..1ed89a8 100644
--- a/ANNOUNCE
+++ b/ANNOUNCE
@@ -252,6 +252,10 @@
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
diff --git a/CHANGES b/CHANGES
index 6efb168..aa6bf63 100644
--- a/CHANGES
+++ b/CHANGES
@@ -4004,6 +4004,10 @@
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
diff --git a/contrib/libtests/makepng.c b/contrib/libtests/makepng.c
index e6a0436..e4b01f0 100644
--- a/contrib/libtests/makepng.c
+++ b/contrib/libtests/makepng.c
@@ -749,7 +749,10 @@
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 @@
{
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 @@
}
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 @@
check_param_count(nparams, 4);
clear_text(&text, params[0]);
text.compression = 2; /* iTXt + deflate */
- text.lang = params[2];/* language tag */
- text.lang_key = params[3]; /* translated keyword */
- set_text(png_ptr, info_ptr, &text, params[1]);
+ text.lang = params[1];/* language tag */
+ text.lang_key = params[2]; /* translated keyword */
+ set_text(png_ptr, info_ptr, &text, params[3]);
}
static void
diff --git a/contrib/libtests/pngstest.c b/contrib/libtests/pngstest.c
index fab845b..5bb2701 100644
--- a/contrib/libtests/pngstest.c
+++ b/contrib/libtests/pngstest.c
@@ -1938,7 +1938,7 @@
{ 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 @@
}, { /* 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 */
diff --git a/pngpriv.h b/pngpriv.h
index 9062367..56a545f 100644
--- a/pngpriv.h
+++ b/pngpriv.h
@@ -898,7 +898,7 @@
#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
diff --git a/pngwrite.c b/pngwrite.c
index c6f772a..1794d3e 100644
--- a/pngwrite.c
+++ b/pngwrite.c
@@ -73,7 +73,7 @@
#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)
diff --git a/pngwutil.c b/pngwutil.c
index 35cb31e..e3a0b21 100644
--- a/pngwutil.c
+++ b/pngwutil.c
@@ -373,7 +373,7 @@
} 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 @@
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_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 @@
old_ptr = comp->output_ptr;
/* This could be optimized to realloc() */
- comp->output_ptr = (png_bytepp)png_malloc(png_ptr,
- (png_alloc_size_t)(comp->max_output_ptr *
- png_sizeof(png_charp)));
+ comp->output_ptr = png_voidcast(png_bytepp,png_malloc(png_ptr,
+ comp->max_output_ptr * png_sizeof(png_charp)));
png_memcpy(comp->output_ptr, old_ptr,
old_max * png_sizeof(png_charp));
@@ -527,15 +526,13 @@
}
else
- comp->output_ptr = (png_bytepp)png_malloc(png_ptr,
- (png_alloc_size_t)(comp->max_output_ptr *
- png_sizeof(png_charp)));
+ comp->output_ptr = png_voidcast(png_bytepp,png_malloc(png_ptr,
+ comp->max_output_ptr * png_sizeof(png_charp)));
}
/* Save the data */
- comp->output_ptr[comp->num_output_ptr] =
- (png_bytep)png_malloc(png_ptr,
- (png_alloc_size_t)png_ptr->zbuf_size);
+ comp->output_ptr[comp->num_output_ptr] = png_voidcast(png_bytep,
+ png_malloc(png_ptr, png_ptr->zbuf_size));
png_memcpy(comp->output_ptr[comp->num_output_ptr], png_ptr->zbuf,
png_ptr->zbuf_size);
@@ -559,12 +556,13 @@
} 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 @@
}
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 @@
/* 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)
- return;
+ compressed_profile_len = png_text_compress(png_ptr, (png_const_charp)profile,
+ (png_size_t)/*ok*/profile_len, PNG_COMPRESSION_TYPE_BASE, &comp);
- if (compression_type != PNG_COMPRESSION_TYPE_BASE)
- png_warning(png_ptr, "Unknown compression type in iCCP chunk");
-
- 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)
+ /* 'name_len' is 1..79, so the following is safe: */
+ if (compressed_profile_len > PNG_UINT_31_MAX - name_len - 2)
{
- 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_size_t)(name_len + 2));
+ png_write_chunk_data(png_ptr, (png_bytep)new_name, 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);
}
