Skip to content

Commit

Permalink
Idempotent handling of missing fdAT chunk in a truncated PNG file.
Browse files Browse the repository at this point in the history
Fuzzing found a broken PNG file where the sequence of chunks looks more
or less like this: `IHDR`, `acTL`, `IDAT`, `fcTL`, `fctL`, EOF.  This
commit preserves this repro file under
`fuzz/corpus/buf_independent/regressions`.

Before this commit such fuzzing input would lead to problems when
resuming a call to `next_frame` after an interittent EOF.  There were a
few loosely related problems:

1. `fn read_until_image_data` would unnecessarily set `self.subframe` in
   the `Decoded::FrameControl` branch.  This was unnecessary because
   `self.subframe` is also set after breaking out of the loop.
2. `fn read_until_image_data` is called to position the input stream at
   the beginning of the next image frame - it breaks out of its loop and
   returns after encountering the start of the next `IDAT` or `fdAT`
   chunk.  This means that there is a discrepancy:
    - Whether we are in the middle of an `IDAT`/`fdAT` data stream can
      be determined by checking `self.subframe.consumed_and_flushed`.
    - But before this commit `fn next_frame` would decide whether to
      call `read_until_image_data` based on `self.next_frame`, rather
      than based on `self.subframe.consumed_and_flushed`.
3. After calling `fn finish_decoding` from `fn next_frame` the
   `self.subframe.consumed_and_flushed` wouldn't be reset.

Problem2 above meant that after an intermittent `UnexpectedEof` in
`read_until_image_data` retrying a call to `next_frame` would *not*
retry `read_until_image_data` (because `self.next_frame` would be
updated by the previous `read_until_image_data`, and now `fn next_frame`
would incorrectly treat this as not needing to call
`read_until_image_data`, even though we still haven't reached an `IDAT`
nor `fdAT` chunk).  This was fixed by changing how `fn next_frame`
decides whether to call `read_until_image_data` (as a side-effect this
allowed deleting `subframe_idx`).

Fixing problem2 was insufficient, because problem1 meant that
`consumed_and_flushed` was prematurely set (before actually encountering
an `IDAT` or `fdAT` frame).

And we also needed to fix problem3 to ensure that the next call to
`next_frame` knows that it is time to advance to the next `IDAT` /
`fdAT` chunks.
  • Loading branch information
anforowicz authored and kornelski committed Sep 24, 2024
1 parent 3bc310d commit 59043bd
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 8 deletions.
Binary file not shown.
12 changes: 4 additions & 8 deletions src/decoder/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -415,7 +415,6 @@ impl<R: Read> Reader<R> {
Some(Decoded::ChunkBegin(_, chunk::IDAT))
| Some(Decoded::ChunkBegin(_, chunk::fdAT)) => break,
Some(Decoded::FrameControl(_)) => {
self.subframe = SubframeInfo::new(self.info());
// The next frame is the one to which this chunk applies.
self.next_frame = SubframeIdx::Some(self.fctl_read);
// TODO: what about overflow here? That would imply there are more fctl chunks
Expand Down Expand Up @@ -474,17 +473,13 @@ impl<R: Read> Reader<R> {
/// Output lines will be written in row-major, packed matrix with width and height of the read
/// frame (or subframe), all samples are in big endian byte order where this matters.
pub fn next_frame(&mut self, buf: &mut [u8]) -> Result<OutputInfo, DecodingError> {
let subframe_idx = match self.decoder.info().unwrap().frame_control() {
None => SubframeIdx::Initial,
Some(_) => SubframeIdx::Some(self.fctl_read - 1),
};

if self.next_frame == SubframeIdx::End {
return Err(DecodingError::Parameter(
ParameterErrorKind::PolledAfterEndOfImage.into(),
));
} else if self.next_frame != subframe_idx {
// Advance until we've read the info / fcTL for this frame.
} else if self.subframe.consumed_and_flushed {
// Advance until the next `fdAT`
// (along the way we should encounter the fcTL for this frame).
self.read_until_image_data()?;
}

Expand Down Expand Up @@ -539,6 +534,7 @@ impl<R: Read> Reader<R> {
// Advance over the rest of data for this (sub-)frame.
if !self.subframe.consumed_and_flushed {
self.decoder.finish_decoding()?;
self.subframe.consumed_and_flushed = true;
}

// Advance our state to expect the next frame.
Expand Down

0 comments on commit 59043bd

Please sign in to comment.