Treat most auxiliary chunk errors as benign.

[anforowicz]

This commit is based on what png_handle_... functions do in libpng/pngrutil.c:

• Chunks appearing before IHDR are a fatal error. png crate already detects this in a single, centeralized location and reports ChunkBeforeIhdr error.
• Chunks appearing after IDAT chunk, or duplicated chunks are treated as a benign error and ignored.

Note that there are still some places where png crate is more strict. In some places this is unavoidable - e.g. the SrgbRenderingIntent enum can't represent values greater than 3 and the Unit enum can't represent values greater than 1. There are also some places where png crate is more permissive - e.g. in general parse_... functions don't complain if the chunk is longer than necessary.

[anforowicz]

FWIW, I mostly just went through parse_... functions in stream.rs and for each fatal error I tried to see if the same/similar scenario is instead treated as a benign error in libpng:

• I haven't yet changed or looked at the handling of APNG-related chunks. Maybe we'll have to do that later, but I would like to spend more time thinking about it. Ignoring errors in acTL chunks seems reasonable, but maybe in this scenario subsequent fcTL chunks be ignored as well. I also note that the APNG-spec-prescribed behavior (reverting to the static image) can't be implemented within the png crate - it has to be implemented on-top-of the png crate.
• I also haven't looked at text-chunk parsing, because I have run out of time today... :-(
• I see that png_handle_PLTE handles a duplicate chunk as a fatal / non-benign error, so I left this unchanged.
• I see that png_handle_tRNS is stricter than parse_trns wrt the length of the chunk. And in general, png crate is more permissive here. I think this is okay. And changing this risks breaking some users/clients of the png crate.
• For parse_phys we can't avoid parsing the Unit enum and reporting errors if this fails...
• Same for parse_srgb - can't avoid a fatal error if we can't parse SrgbRenderingIntent enum
• For parse_sbit (and parse_plte and a few similar functions) we still call self.limits.reserve_bytes(...)? - this fatal error seems desirable and we should probably keep it
• I notice that libpng does extra validation in png_handle_cHRM:
  • Ordering wrt PLTE chunk (skipping cHRM / treating this as a benign error)
  • png_colorspace_endpoints_match checks for inconsistent chromacities (whatever that means :-)
  • I mostly kept unchanged the lax chunk-length validation in png crate. But libpng treats too-short as a benign error, so I changed this aspect.

The list above doesn't describe all the changes in this PR, but it should hopefully help provide the high-level context for the changes.

[anforowicz]

Not sure if I can/should say in the commit descriptions that it fixes #525. Probably not yet, because we may still need to do something for fcTL, acTL, fdAT, tEXt, zTXt, iTXt chunks in the future.

[anforowicz]

• For parse_phys we can't avoid parsing the Unit enum and reporting errors if this fails...
• Same for parse_srgb - can't avoid a fatal error if we can't parse SrgbRenderingIntent enum

I think I was incorrect above. The errors above are avoidable - we can ignore the whole chunk if enum parsing fails. There may also be other options - e.g. extending the enum with an "unknown" value (this would be a breaking change), or marking it with the non_exhaustive attribute. It's probably okay to do that later though...

[fintelia]

I think the if and else blocks are swapped here?

[anforowicz]

Ooops. Thanks for catching this. Let me fix this change when rebasing on top of the tip-of-tree.

I'll write a note to myself to try to improve unit test coverage for auxiliary chunks. (Sadly I am not sure if I'll have time for it this week, but I'll keep it in my to-do list.)

[anforowicz]

Force push above is primarily to rebase and resolve merge conflicts that arose from #568, but also to:

• fix/avoid accidental logic changes (one caught by @fintelia's review in parse_phys - thanks!; one caught in parse_trns in another self-review).
• slightly tweak the commit descriptions (to avoid the incorrect statements where it used to say that hard errors from enum-parsing are unavoidable)

Hmmm... let me figure out which button to press to flush my "pending" review replies/comments...

[anforowicz]

Ooops. Thanks for catching this. Let me fix this change when rebasing on top of the tip-of-tree.

I'll write a note to myself to try to improve unit test coverage for auxiliary chunks. (Sadly I am not sure if I'll have time for it this week, but I'll keep it in my to-do list.)

[anforowicz]

Preserving the old conditions requires saying && instead of ||: info.palette.is_some() && !self.have_idat . I'll fix this when rebasing... :-/

[anforowicz]

@fintelia, can you PTAL again?

[fintelia]

Sorry, been meaning to get a chance to do another review pass.

One thing I've been wanting to consider is whether it would be cleaner to have all the chunk parsing functions continue to report errors, but have parse_chunk suppress any errors coming from auxiliary chunks. That would allow there to be an opt-in "strict" mode where auxiliary continued to get propagated to the caller

[anforowicz]

Sorry, been meaning to get a chance to do another review pass.

No worries, thanks for getting back to this.

One thing I've been wanting to consider is whether it would be cleaner to have all the chunk parsing functions continue to report errors, but have parse_chunk suppress any errors coming from auxiliary chunks. That would allow there to be an opt-in "strict" mode where auxiliary continued to get propagated to the caller

I see. This way Rust png could be used for something like pngcheck.

I'll try to tweak this PR to reintroduce the errors (and once that's done, I'll give a heads up to ask for another review pass).

[anforowicz]

Ok, I think this is ready for another review please.

• Suppressing errors in fn parse_chunk means that enum conversion failures (e.g. in pHYs and sRGB) are now treated as benign errors (unlike in my previous attempt). This is better / more desirable.
• I think we still want to propagate some auxiliary chunk errors. One example: if tRNS hits LimitsExceeded, then I think the client wants to hear about it (rather than ignore the chunk and silently decode slightly different pixels).
• Currently all FormatErrors are treated as suppressable/potentially-benign. This is probably ok for now, but in the future maybe we want to have some FormatErrors that are fatal even if present in benign chunks? This is hypothetical, so probably not worth spending much time on it. But I was wondering whether we may want to tweak the public API to explicitly surface the "benign" bit (it seems nice to be able to report it if somebody wanted to implement pngcheck-like utility on top of the png crate).

[anforowicz]

Ugh... right after I pressed the "comment" / "send" button I realized that there are some fuzzing failures... let me take a look...

[anforowicz]

Ugh... right after I pressed the "comment" / "send" button I realized that there are some fuzzing failures... let me take a look...

Ok, this is now fixed. I missed that fn parse_chunk clears self.state before the new benign-error-suppressing code. I've added a regression test for this mishap (which I probably should have done earlier, because the test also covers that benign errors are indeed ignored).

[fintelia]

Thanks!