Handling invalid, unknown ancillary chunks

[svgeesus]

Originally raised by DLiang Fun:

1) In regards to chunk types, the specification clearly states that each byte in the chunk type should be limited to ISO 646 letters (A-Z and a-z). Therefore, it appears reasonable to report errors if this condition is not satisfied.

2) The Error Handling section explicitly states that encountering an unknown ancillary chunk is never considered an error.

Now, here's the silly question that arises: If a chunk type contains non-ISO 646 letters but is still an unknown ancillary chunk, should we report an error?

While reading the specification, I do have the feeling that we should not report an error in such a case. However, the specification does not explicitly indicate that, in case of conflicts, we should follow the Error Handling philosophy first.

I have observed divergent implementations regarding this matter. Specifically, while libspng and lodepng choose to ignore the chunk in question, libpng halts the parsing process and reports a fatal error. Hence, I believe it would be beneficial to explicitly clarify this aspect in the specification.

[svgeesus]

This is a good question, and perhaps the specification could be clarified to make finding the answer more obvious.

However, a little digging clarified the issue and 13.1 Error handling is clear:

Encountering an unknown ancillary chunk is never an error. The chunk can simply be ignored.

and this is further clarified in 5.3 Chunk layout:

Encoders and decoders shall treat the chunk types as fixed binary values, not character strings.

Thus, for any arbitrary chunk type considered as a 32-bit unsigned integer in network byte order, the decoder:

1. Examines the ancillary bit, which is either set or not.
2. Compares the chunk type to the list of chunk types that it knows about

If the chunk is ancillary and the chunk is unknown, no further processing is required; the chunk can be ignored. If the chunk is critical and is not known, that is a fatal error.

Libpng is clearly incorrect in reporting a fatal error.

The restriction of registered chunk types to the upper and lowercase ASCII letters is a convenience when examining a PNG datastream, for example with a hex editor, but does not affect processing:

Each byte of a chunk type is restricted to the hexadecimal values 41 to 5A and 61 to 7A. These correspond to the uppercase and lowercase ISO 646 [ISO646] letters (A-Z and a-z) respectively for convenience in description and examination of PNG datastreams.

So in practice this just means that the PNG specification will not register chunk types containing numbers, control characters, etc.

[boofish]

Thanks for posting here, I am DLiang Fun who sends this email. Here is the png file that witnesses three different implementations (i.e., libpng, lodepng, libspng) handle it differently with regard to the error handling mentioned above.

The PNG file:

[annevk]

We should definitely add that to the test suite as it shows an interop issue.

[tnikkel]

Because we got multiple reports of this from in-the-wild files I wrote a patch for Firefox in https://bugzilla.mozilla.org/show_bug.cgi?id=1873422 that I plan to land shortly (in the 125 cycle). I can't access the poc link in the above comment but I expect the patch to change Firefox to display the image like Chrome.

[svgeesus]

A bit of testing reveals:

pngcheck gives an error and stops

chris@SuperNomad:/mnt/c/Users/chris/Documents/PNG$ pngcheck -vv invalid-unknown-ancillary.png
zlib warning:  different version (expected 1.2.13, using 1.3)

File: invalid-unknown-ancillary.png (263 bytes)
  chunk IHDR at offset 0x0000c, length 13
    32 x 32 image, 4-bit palette, non-interlaced
  chunk gAMA at offset 0x00025, length 4: 1.0000
  invalid chunk name "sIT" (73 01 49 54)
ERRORS DETECTED in invalid-unknown-ancillary.png

pngcrush gives an error, halts, and does not create an output file (even with -fix)

chris@SuperNomad:/mnt/c/Users/chris/Documents/PNG$ pngcrush -fix tmp.png
  Recompressing IDAT chunks in tmp.png to pngout.png
   Total length of data found in critical chunks            =       232
While converting tmp.png to pngout.png:
  pngcrush caught libpng error:
   s[01]IT: invalid chunk type

CPU time decode 0.000000, encode 0.000000, other 0.001100, total 0.001102 sec

Firefox Nightly 125 displays an error (as an image :) )

Chrome Canary 123 displays the image

TweakPNG gives an error, warns about a corrupted file, and displays only the chunks before the error

PNG format inspector displays the data without comment, but is basically a lightly-structured hex displayer anyway

PNG file chunk inspector gives a red error, but displays the invalid chunk and all other chunks correctly.

(I don't have access to a Mac currently so no Safari test)

pngcp from pngtools errors out because of libpng:

 pngcp invalid-unknown-ancillary.png foo.png
libpng error: s[01]IT: invalid chunk type
Could not set PNG jump valuefree(): invalid pointer
Aborted

[svgeesus]

@tnikkel great. I'm working on a WPT that uses this file (and a copy with the invalid chunk deleted, as the reference)

[svgeesus]

I see Mozilla Ignore "invalid chunk type" errors in pngs if we have enough data to display something when encountered.

[tnikkel]

Yeah, I could be mistaken but when I looked the invalid chunk type seemed to put libpng into a terminal error state, so the invalid chunk type is only ignored if it comes after we have image data.

[svgeesus]

wpt results for recovery from unknown and invalid ancillary chunk

On 29 Feb 2024: Chrome 124 and Edge 123 pass, Firefox 125 and Safari TP 189 fail.

[tnikkel]

The invalid chunk comes before the image data in that testcase, so that is what I would expect from that testcase.

[svgeesus]

@ProgramMax could you review https://github.com/web-platform-tests/wpt/pull/44936 please? It is the same as the other error-recovery test, but the invalid ancillary chunk is after IDAT.

[svgeesus]

It is the same as the other error-recovery test, but the invalid ancillary chunk is after IDAT.

WPT results show interop on error recovery for ancillary chunks after IDAT.