Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Treat most auxiliary chunk errors as benign. #569

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

anforowicz
Copy link
Contributor

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.

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

  • 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...

Err(DecodingError::Format(
FormatErrorInner::DuplicateChunk { kind: chunk::pHYs }.into(),
))
if !self.have_idat && info.pixel_dims.is_none() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the if and else blocks are swapped here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants