Corrupt auxillary chunks should be ignored, not treated as fatal errors

[fintelia]

The PNG spec says:

Unexpected values in fields of known chunks (for example, an unexpected compression method in the IHDR chunk) shall be checked for and treated as errors. However, it is recommended that unexpected field values be treated as fatal errors only in critical chunks. An unexpected value in an ancillary chunk can be handled by ignoring the whole chunk as though it were an unknown chunk type.

Right now, parse_chunk instead treats any error in any known chunk as fatal and sets the decoding state to None.

[anforowicz]

Good catch - thanks for filing the bug.

A few thoughts that I hope are helpful:

• I wonder if the PNG spec's notion of "auxiliary" can be directly applied, because acTL and fcTL chunks are kind of critical to APNG-aware decoders. OTOH, maybe an error parsing these chunks can be indeed treated as-if the chunk was missing.
• One way to fix this would be to tweak fn parse_chunk so that it turns Err(_) into Ok(Decoded::Nothing) for auxiliary chunks. The main risk is whether a fn parse_somechunk leaves the decoded in a bad state when returning an error (i.e. we'd have to "be careful" going forward that fn parse_fctl doesn't do that - we'd have to move self.inflated.reset() and self.ready_for_fdat_chunks = true toward the end of the function - next to self.info.as_mut().unwrap().frame_control = Some(fc)).
• I've been recently reading https://joshlf.com/files/talks/Safety%20in%20an%20Unsafe%20World.pdf and in theory we could make reporting an error for an auxiliary chunk a compilation error. One idea would be to make each chunk a separate type/struct and only allow constructing errors where TChunkType: CriticalChunkTrait or something like that. This seems like quite a big and complicated refactoring - I am not sure if this is worth it...

[kornelski]

APNG chunks started out as a non-standard hack, and it was long too late to fix their names when they got into the spec.

APNG chunks will need special case handling. They could simply be treated as mandatory, or mandatory after decoding past the first frame (iEND).

[fintelia]

For reference, this is how the spec recommends handling APNG errors:

APNG is designed to allow incremental display of frames before the entire datastream has been read. This implies that some errors may not be detected until partway through the animation. It is strongly recommended that when any error is encountered decoders should discard all subsequent frames, stop the animation, and revert to displaying the static image. A decoder which detects an error before the animation has started should display the static image. An error message may be displayed to the user if appropriate.

But really, the top priority should be making sure that adding support for new metadata chunks doesn't cause breaking changes for users due to previously readable PNGs now raising errors about the new chunk being corrupt.

[anforowicz]

But really, the top priority should be making sure that adding support for new metadata chunks doesn't cause breaking changes for users due to previously readable PNGs now raising errors about the new chunk being corrupt.

Agreed.

Is there also value in trying to ensure that already-supported auxiliary chunks can't trigger errors? In theory we could force functions that parse auxiliary chunks (e.g. parse_trns or parse_phys) to gracefully handle parsing errors by changing their signature to return Decoded instead of Result<Decoded, DecodingError> . But lax parsing leads to Hyrum's Law, so for now I assume that maybe this is not such a great idea...

[fintelia]

Existing chunks should probably also be switched. Hyrum's Law is unfortunate, but given the lax behavior of other common decoders, I think we're decades late in preventing it.

[kornelski]

Parsing of these chunks doesn't do anything during decoding. The decoder doesn't really need to be parsing them. The current implementation isn't even parsing them into another representation, merely checking if they're valid and then not reporting the failures.

The decoder could simply save the chunks' raw data, and then the Info could have parsers for them. This would let users of these chunks handle parsing errors when they read the data. The parsing would be lazy, and Info could be parsing the chunks into more user-friendly higher-level structs.