PNG and MNG/JNG image formats: home site

> libpng-1.0.38rc03, libpng-1.2.30rc03, and libpng-1.4.0beta22 are available from
> ftp://ftp.simplesystems.org/pub/png-group/src/
> and from
> http://libpng.sourceforge.net

> These fix potential memory leak of the pCAL "purpose" array and the
> sCAL "buffer" array.  Both are changed to "png_ptr->chunkdata".

Can you split the whitespace changes out into a separate diff?  It's
impossible to spot the real changes in there.

Thanks,
  Greg

On Tue, Jul 22, 2008 at 11:28 AM, Greg Roelofs <newt@...> wrote:
>
> Can you split the whitespace changes out into a separate diff?  It's
> impossible to spot the real changes in there.

OK, done.  The ignore-whitespace diff still amounts to 111k but that's less than
half the size of the full diff.

Glenn

>> Can you split the whitespace changes out into a separate diff?  It's
>> impossible to spot the real changes in there.

> OK, done.  The ignore-whitespace diff still amounts to 111k but that's less than
> half the size of the full diff.

Many thanks.

This is a little ugly (pngerror.c):

        warning_number[offset]=*(warning_message+offset+1);

What's wrong with

        warning_number[offset] = warning_message[offset+1];

?  It could also use a comment; I guess the warning syntax is supposed to
look like "#1234567890 foo bar", but that should be called out.

This is then incomplete:

@@ -281,9 +281,9 @@
             break;
      }
      if((offset > 1) && (offset < 15))
      {
-       warning_number[offset-1]='\0';
+       warning_number[offset + 1] = '\0';
        fprintf(stderr, "libpng warning no. %s: %s\n", warning_number,
           warning_message+offset);
      }
      else

The first part of the conditional should be eliminated; it was there only
to protect against buffer underruns.

It's hard to be certain, but this (pngread.c) looks like it probably should
be const:

+      png_bytep chunk_name = png_ptr->chunk_name;

Ditto the one in png_read_end().

The copyrights here are pointless:

@@ -2209,10 +2211,13 @@
 
 #if defined(PNG_READ_RGB_TO_GRAY_SUPPORTED)
 /* reduce RGB files to grayscale, with or without alpha
  * using the equation given in Poynton's ColorFAQ at
- * <http://www.inforamp.net/~poynton/>
+ * <http://www.inforamp.net/~poynton/>  (THIS LINK IS DEAD June 2008)
  * Copyright (c) 1998-01-04 Charles Poynton poynton at inforamp.net
+ * New links:
+ * <http://www.poynton.com/notes/colour_and_gamma/>
+ * Copyright (c) 2006-11-28 Charles Poynton poynton at poynton.com
  *
  *     Y = 0.212671 * R + 0.715160 * G + 0.072169 * B
  *
  *  We approximate this with

They're his sites; he can include the copyrights himself.  (The equation
cannot be copyrighted.)  All that's needed in libpng is the name and
(optionally) contact info, and that "only" for basic courtesy.

@@ -48,9 +47,17 @@
 
 png_uint_32 PNGAPI
 png_get_uint_31(png_structp png_ptr, png_bytep buf)
 {
+#ifdef PNG_READ_BIG_ENDIAN_SUPPORTED
    png_uint_32 i = png_get_uint_32(buf);
+#else
+   /* Avoid an extra function call by inlining the result. */
+   png_uint_32 i = ((png_uint_32)(*buf) << 24) +
+      ((png_uint_32)(*(buf + 1)) << 16) +
+      ((png_uint_32)(*(buf + 2)) << 8) +
+      (png_uint_32)(*(buf + 3));
+#endif

Don't we already have macros that do precisely this, including casting
buf in case it's not already a png_bytep?  I _know_ there's extremely
similar code in zlib.h, and I thought I had added something similar as
part of the alpha-blend macro, though I might be misremembering that.

+png_uint_32 /* PRIVATE */
+png_read_chunk_header(png_structp png_ptr)
+{
+   png_byte buf[8];
+   png_uint_32 length;
+
+   /* read the length and the chunk name */
+   png_read_data(png_ptr, buf, 8);
+   length = png_get_uint_31(png_ptr, buf);
+
+   /* put the chunk name into png_ptr->chunk_name */
+   png_memcpy(png_ptr->chunk_name, buf + 4, 4);

Wouldn't it make sense to do some validation on chunk_name at this point?
If it's garbage (via corruption or whatever), you potentially save a fair
bit of I/O and computation by avoiding reading to the end of the file (and
computing the CRC) due to a ridiculously large chunk length.

You could signal it with UINT32_MAX (or whatever it's called), which is
an invalid chunk length.  There are only three callers so far, so the
changes would be pretty minimal.

Does this even compile??

@@ -514,8 +552,9 @@
       {
          if (png_ptr->flags & PNG_FLAG_CRC_ANCILLARY_NOWARN)
          {
             png_chunk_error(png_ptr, "CRC error");
+%15+%            png_chunk_benign_error(png_ptr, "CRC error");
          }
          else
          {
             png_chunk_warning(png_ptr, "CRC error");

OK, I'm a little more than 1/3 of the way through it...

Greg

On Thu, Jul 24, 2008 at 12:17 PM, Greg Roelofs <newt@...> wrote:

> This is then incomplete:
>
> @@ -281,9 +281,9 @@
>             break;
>      }
>      if((offset > 1) && (offset < 15))
>      {
> -       warning_number[offset-1]='\0';
> +       warning_number[offset + 1] = '\0';
>        fprintf(stderr, "libpng warning no. %s: %s\n", warning_number,
>           warning_message+offset);
>      }
>      else
>
> The first part of the conditional should be eliminated; it was there only
> to protect against buffer underruns.

Yikes, should be [offset - 1] not [offset + 1]

> +%15+%            png_chunk_benign_error(png_ptr, "CRC error");

Yikes^2

Glenn

On Thu, Jul 24, 2008 at 12:17 PM, Greg Roelofs <newt@...> wrote:

> @@ -48,9 +47,17 @@
>
>  png_uint_32 PNGAPI
>  png_get_uint_31(png_structp png_ptr, png_bytep buf)
>  {
> +#ifdef PNG_READ_BIG_ENDIAN_SUPPORTED
>    png_uint_32 i = png_get_uint_32(buf);
> +#else
> +   /* Avoid an extra function call by inlining the result. */
> +   png_uint_32 i = ((png_uint_32)(*buf) << 24) +
> +      ((png_uint_32)(*(buf + 1)) << 16) +
> +      ((png_uint_32)(*(buf + 2)) << 8) +
> +      (png_uint_32)(*(buf + 3));
> +#endif
>
> Don't we already have macros that do precisely this, including casting
> buf in case it's not already a png_bytep?

The macros are for when BIG_ENDIAN_SUPPORTED is true, i.e., hardly
ever.  This change also supports the others (note "#else").

Glenn

On Thu, Jul 24, 2008 at 12:17 PM, Greg Roelofs <newt@...> wrote:

>
> Wouldn't it make sense to do some validation on chunk_name at this point?

Sure.

> If it's garbage (via corruption or whatever), you potentially save a fair
> bit of I/O and computation by avoiding reading to the end of the file (and
> computing the CRC) due to a ridiculously large chunk length.
>
> You could signal it with UINT32_MAX (or whatever it's called), which is
> an invalid chunk length.

Why not signal it with png_error()?

>There are only three callers so far, so the
> changes would be pretty minimal.

With png_error() there would be no changes except inside
png_read_chunk_header().

Glenn

>> This is then incomplete:
>>
>> @@ -281,9 +281,9 @@
>>             break;
>>      }
>>      if((offset > 1) && (offset < 15))
>>      {
>> -       warning_number[offset-1]='\0';
>> +       warning_number[offset + 1] = '\0';
>>        fprintf(stderr, "libpng warning no. %s: %s\n", warning_number,
>>           warning_message+offset);
>>      }
>>      else
>>
>> The first part of the conditional should be eliminated; it was there only
>> to protect against buffer underruns.

> Yikes, should be [offset - 1] not [offset + 1]

No.  Look again...

Greg

>> @@ -48,9 +47,17 @@
>>
>>  png_uint_32 PNGAPI
>>  png_get_uint_31(png_structp png_ptr, png_bytep buf)
>>  {
>> +#ifdef PNG_READ_BIG_ENDIAN_SUPPORTED
>>    png_uint_32 i = png_get_uint_32(buf);
>> +#else
>> +   /* Avoid an extra function call by inlining the result. */
>> +   png_uint_32 i = ((png_uint_32)(*buf) << 24) +
>> +      ((png_uint_32)(*(buf + 1)) << 16) +
>> +      ((png_uint_32)(*(buf + 2)) << 8) +
>> +      (png_uint_32)(*(buf + 3));
>> +#endif

>> Don't we already have macros that do precisely this, including casting
>> buf in case it's not already a png_bytep?

> The macros are for when BIG_ENDIAN_SUPPORTED is true, i.e., hardly
> ever.  This change also supports the others (note "#else").

Wait, png_get_uint_32() is both a macro AND a function?  Argh, that's
terrible.  It should always be a macro, and if it needs to invoke a
function in some (ifdef'd) cases, then it should simply be a wrapper
for said function (e.g., png_get_uint_32_fn() or something).  But no
part of this bit of code actually requires functions...

Even worse, the macro is terribly, horribly wrong:

#  define png_get_uint_32(buf) ( *((png_uint_32p) (buf)))

Trying to cast an unaligned byte pointer to a doubleword pointer and
dereference it?  Helloooo, bus error!  That's one of the biggest no-nos
in the business.  You _always_ have to reconstruct the int from bytes
if you want the code to be even vaguely portable, which brings us back
to the four-liner above.  Add casts and stuff it into png.h unconditionally,
and we should be set.

(And oddly enough, I don't find a similar macro anywhere in the zlib
headers, though there is one in funzip.c and various bits of code I've
written.)

Greg

>> If it's garbage (via corruption or whatever), you potentially save a fair
>> bit of I/O and computation by avoiding reading to the end of the file (and
>> computing the CRC) due to a ridiculously large chunk length.
>>
>> You could signal it with UINT32_MAX (or whatever it's called), which is
>> an invalid chunk length.

> Why not signal it with png_error()?

Indeed, that would be even better.

> With png_error() there would be no changes except inside
> png_read_chunk_header().

Yup.

Greg

On Thu, Jul 24, 2008 at 6:01 PM, Greg Roelofs <newt@...> wrote:
>>> @@ -48,9 +47,17 @@
>>>
>>>  png_uint_32 PNGAPI
>>>  png_get_uint_31(png_structp png_ptr, png_bytep buf)
>>>  {
>>> +#ifdef PNG_READ_BIG_ENDIAN_SUPPORTED
>>>    png_uint_32 i = png_get_uint_32(buf);
>>> +#else
>>> +   /* Avoid an extra function call by inlining the result. */
>>> +   png_uint_32 i = ((png_uint_32)(*buf) << 24) +
>>> +      ((png_uint_32)(*(buf + 1)) << 16) +
>>> +      ((png_uint_32)(*(buf + 2)) << 8) +
>>> +      (png_uint_32)(*(buf + 3));
>>> +#endif
>
>>> Don't we already have macros that do precisely this, including casting
>>> buf in case it's not already a png_bytep?
>
>> The macros are for when BIG_ENDIAN_SUPPORTED is true, i.e., hardly
>> ever.  This change also supports the others (note "#else").
>
> Wait, png_get_uint_32() is both a macro AND a function?  Argh, that's
> terrible.  It should always be a macro, and if it needs to invoke a
> function in some (ifdef'd) cases, then it should simply be a wrapper
> for said function (e.g., png_get_uint_32_fn() or something).  But no
> part of this bit of code actually requires functions...
>
> Even worse, the macro is terribly, horribly wrong:
>
> #  define png_get_uint_32(buf) ( *((png_uint_32p) (buf)))
>
> Trying to cast an unaligned byte pointer to a doubleword pointer and
> dereference it?  Helloooo, bus error!  That's one of the biggest no-nos
> in the business.  You _always_ have to reconstruct the int from bytes
> if you want the code to be even vaguely portable, which brings us back
> to the four-liner above.  Add casts and stuff it into png.h unconditionally,
> and we should be set.
>
> (And oddly enough, I don't find a similar macro anywhere in the zlib
> headers, though there is one in funzip.c and various bits of code I've
> written.)

You are talking about code that was contributed to libpng-0.95 and has
quietly lurked in libpng ever since with no one complaining.  I guess
most people are on little-endian pc's and won't see a problem if there
is one.

Glenn

On Thu, Jul 24, 2008 at 5:26 PM, Greg Roelofs <newt@...> wrote:
>>> This is then incomplete:
>>>
>>> @@ -281,9 +281,9 @@
>>>             break;
>>>      }
>>>      if((offset > 1) && (offset < 15))
>>>      {
>>> -       warning_number[offset-1]='\0';
>>> +       warning_number[offset + 1] = '\0';
>>>        fprintf(stderr, "libpng warning no. %s: %s\n", warning_number,
>>>           warning_message+offset);
>>>      }
>>>      else
>>>
>>> The first part of the conditional should be eliminated; it was there only
>>> to protect against buffer underruns.
>
>> Yikes, should be [offset - 1] not [offset + 1]
>
> No.  Look again...

I looked again and I don't see what you are seeing.  "offset" points to
the first blank after the number in warning_message, and to the next
char after that first blank in warning_number.  The code is trying to
insert a string-terminating NULL at the appropriate place after the
number.

This is code that never gets used, because we have never rewritten
the error and warning text to include #nnnn at the beginning.  We
got a start in the abandoned 1.1.x version.

we should probably be defining PNG_NO_ERROR_NUMBERS by
default in pngconf.h, if making that change doesn't introduce
any ABI incompatibilty.

Glenn

On Thu, 24 Jul 2008, Greg Roelofs wrote:
>
> Even worse, the macro is terribly, horribly wrong:
>
> #  define png_get_uint_32(buf) ( *((png_uint_32p) (buf)))
>
> Trying to cast an unaligned byte pointer to a doubleword pointer and
> dereference it?  Helloooo, bus error!  That's one of the biggest no-nos

Bus error or perhaps just really slow.

I use this macro in GraphicsMagick.  The macro allows importing the 
32-bit value in either endian.  Testing showed that the direct cast 
approach was (oddly) not really any faster.

#define ImportLongQuantum(endian,quantum,p) \
{ \
   if (LSBEndian != endian) \
     { \
       quantum=(*p++ << 24); \
       quantum|=(*p++ << 16); \
       quantum|=(*p++ << 8); \
       quantum|=(*p++); \
     } \
   else \
     { \
       quantum=(*p++); \
       quantum|=(*p++ << 8); \
       quantum|=(*p++ << 16); \
       quantum|=(*p++ << 24); \
     } \
}

======================================
Bob Friesenhahn
bfriesen@..., http://www.simplesystems.org/users/bfriesen/
GraphicsMagick Maintainer,    http://www.GraphicsMagick.org/

> On Thu, Jul 24, 2008 at 5:26 PM, Greg Roelofs <newt@...> wrote:
> >>> This is then incomplete:
> >>>
> >>> @@ -281,9 +281,9 @@
> >>>             break;
> >>>      }
> >>>      if((offset > 1) && (offset < 15))
> >>>      {
> >>> -       warning_number[offset-1]='\0';
> >>> +       warning_number[offset + 1] = '\0';
> >>>        fprintf(stderr, "libpng warning no. %s: %s\n", warning_number,
> >>>           warning_message+offset);
> >>>      }
> >>>      else
> >>>
> >>> The first part of the conditional should be eliminated; it was there only
> >>> to protect against buffer underruns.
> >
> >> Yikes, should be [offset - 1] not [offset + 1]
> >
> > No.  Look again...

> I looked again and I don't see what you are seeing.  "offset" points to
> the first blank after the number in warning_message, and to the next
> char after that first blank in warning_number.  The code is trying to
> insert a string-terminating NULL at the appropriate place after the
> number.

The only thing that matters is its relationship to warning_number, and
the last entry in that was _at_ offset.  Ergo, offset+1 for termination
is correct.

If the loop runs out, the code above doesn't get used anyway (offset == 15),
so you can ignore that case.

> This is code that never gets used, because we have never rewritten
> the error and warning text to include #nnnn at the beginning.  We
> got a start in the abandoned 1.1.x version.

That would probably explain why it was so hard to understand...  But
then, why did you notice the bug and fix it?  Or did someone else do
so?

> we should probably be defining PNG_NO_ERROR_NUMBERS by
> default in pngconf.h, if making that change doesn't introduce
> any ABI incompatibilty.

Seems harmless enough on the face of it, but I haven't thought about
it deeply enough to be sure.  I know libpng's unexpected/unannounced
API/ABI changes have annoyed lots of people over the years, so it's
probably best to be very sure about it before making changes.

Greg

On Thu, Jul 24, 2008 at 7:12 PM, Greg Roelofs <newt@...> wrote:

> That would probably explain why it was so hard to understand...  But
> then, why did you notice the bug and fix it?  Or did someone else do
> so?

I made a typo while trying to add whitespace around the "-" sign.

>> we should probably be defining PNG_NO_ERROR_NUMBERS by
>> default in pngconf.h, if making that change doesn't introduce
>> any ABI incompatibilty.
>
> Seems harmless enough on the face of it, but I haven't thought about
> it deeply enough to be sure.  I know libpng's unexpected/unannounced
> API/ABI changes have annoyed lots of people over the years, so it's
> probably best to be very sure about it before making changes.

The change would remove png_set_strip_error_numbers(), unless we
leave an empty stub function that does nothing but satisfy the loaders.

But there'd be no problem turning it off in 1.4.0.

Glenn

At 09:17 AM 7/24/2008 -0700, Greg Roelofs wrote:
>+png_uint_32 /* PRIVATE */
>+png_read_chunk_header(png_structp png_ptr)
>+{
>+   png_byte buf[8];
>+   png_uint_32 length;
>+
>+   /* read the length and the chunk name */
>+   png_read_data(png_ptr, buf, 8);
>+   length = png_get_uint_31(png_ptr, buf);
>+
>+   /* put the chunk name into png_ptr->chunk_name */
>+   png_memcpy(png_ptr->chunk_name, buf + 4, 4);
>
>Wouldn't it make sense to do some validation on chunk_name at this point?
>If it's garbage (via corruption or whatever), you potentially save a fair
>bit of I/O and computation by avoiding reading to the end of the file (and
>computing the CRC) due to a ridiculously large chunk length.

I have made this change, but looking at the original code I could not
find a path that would read to the end of an invalidly-named chunk.
So the potential IO savings is probably not there, even though the
new code is a bit cleaner (once the missing png_check_chunk_name() is
added to the progressive reader).

As I mentioned in an earlier message we now reject chunk names with
a lowercase third byte.  This is in contravention to our PNG-1.2 spec
which says a future version of PNG might define a valid meaning
for that.  We can deal with that when it happens.  I seriously
doubt that it will happen anytime soon.  ISO-PNG seems to allow
conformant decoders to reject a chunk name with a lowercase third byte.

GLenn

At 08:32 AM 7/26/2008 -0400, Glenn Randers-Pehrson wrote:
>ISO-PNG seems to allow
>conformant decoders to reject a chunk name with a lowercase third byte.

Hmm, 15.2.3c and g require decoders to treat it as an unknown chunk.

So, should I revert the change that rejects them?  This means that if
we are supporting saving unknown chunks, we will have to read any
all-lowercase "junk" chunk, hunt for the CRC, and only bail out if it's
invalid or missing.

Glenn

>>ISO-PNG seems to allow
>>conformant decoders to reject a chunk name with a lowercase third byte.

> Hmm, 15.2.3c and g require decoders to treat it as an unknown chunk.

I haven't read those sections, but the two statements aren't necessarily
contradictory.  Indeed, the second statement is almost content-free--if
you don't know what it is or how to interpret it, it's unknown by definition.

Since the chunk-naming rules are "metarules"--i.e., they affect the inter-
pretation of unknown chunks--it seems to me that rejection is correct.  If
you don't even know how to interpret the description of the unknown chunk,
much less the actual meaning of the chunk itself, you have no way of knowing
whether it represents a new dimension that's equivalent in importance to
critical/ancillary.  (At least in principle...)

> So, should I revert the change that rejects them?  This means that if
> we are supporting saving unknown chunks, we will have to read any
> all-lowercase "junk" chunk, hunt for the CRC, and only bail out if it's
> invalid or missing.

I guess I'd argue against reverting it, at least until someone comes up
with a more convincing argument about it.

Greg

Greg Roelofs <newt@...> wrote:

> > Hmm, 15.2.3c and g require decoders to treat it as an unknown chunk.
> 
> I haven't read those sections, but...

If you read those sections, you'll see it's an open-and-shut case.

15.2.3 Conformance of PNG decoders

c. Unknown chunk types are handled as described in 5.4 Chunk naming
conventions.  An unknown chunk type is not treated as an error unless it
is a critical chunk.

g. A chunk type in which the reserved bit is set is treated as an
unknown chunk type.

AMC

On Thu, Jul 24, 2008 at 12:17 PM, Greg Roelofs <newt@...> wrote:

>
> OK, I'm a little more than 1/3 of the way through it...
>

Eagerly awaiting the other 2/3.  If no more bugs turn up I'll release
1.2.30 on Tuesday 5 August.  Otherwise I'll do rc07 with another
week's delay.

Glenn