Skip to content

Commit

Permalink
Treat UnexpectedEof under parse_chunk as a FormatError.
Browse files Browse the repository at this point in the history
Fuzzing found a broken PNG file where an `fcTL` chunk is too short, and
where `read_be` or `read_exact` in `parse_fctl` returns `UnexpectedEof`.
This file was saved under `fuzz/corpus/buf_independent/regressions`.

Before this commit, the input above would mislead the client/fuzzer code
into thinking that there is a recoverable error that may go away after
more PNG bytes are available.  (This was incorrect because `parse_chunk`
and `parse_fctl` are only called after gathering *all* the bytes of a
chunk into `ChunkState::raw_bytes`.)  But when the client tried
resuming, we got into an infinite loop because `StreamingDecoder` has
already given up and set `state` to `None`.

After this commit, the timeout is avoided by properly indicating a
non-recoverable error in such a situation.
  • Loading branch information
anforowicz authored and kornelski committed Sep 24, 2024
1 parent ab0e86c commit 3bc310d
Show file tree
Hide file tree
Showing 2 changed files with 22 additions and 2 deletions.
Binary file not shown.
24 changes: 22 additions & 2 deletions src/decoder/stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -250,6 +250,10 @@ pub(crate) enum FormatErrorInner {
UnexpectedRestartOfDataChunkSequence {
kind: ChunkType,
},
/// Failure to parse a chunk, because the chunk didn't contain enough bytes.
ChunkTooShort {
kind: ChunkType,
},
}

impl error::Error for DecodingError {
Expand Down Expand Up @@ -376,6 +380,9 @@ impl fmt::Display for FormatError {
UnexpectedRestartOfDataChunkSequence { kind } => {
write!(fmt, "Unexpected restart of {:?} chunk sequence", kind)
}
ChunkTooShort { kind } => {
write!(fmt, "Chunk is too short: {:?}", kind)
}
}
}
}
Expand Down Expand Up @@ -951,9 +958,22 @@ impl StreamingDecoder {
_ => Ok(Decoded::PartialChunk(type_str)),
};

if parse_result.is_err() {
let parse_result = parse_result.map_err(|e| {
self.state = None;
}
match e {
// `parse_chunk` is invoked after gathering **all** bytes of a chunk, so
// `UnexpectedEof` from something like `read_be` is permanent and indicates an
// invalid PNG that should be represented as a `FormatError`, rather than as a
// (potentially recoverable) `IoError` / `UnexpectedEof`.
DecodingError::IoError(e) if e.kind() == std::io::ErrorKind::UnexpectedEof => {
let fmt_err: FormatError =
FormatErrorInner::ChunkTooShort { kind: type_str }.into();
fmt_err.into()
}
e => e,
}
});

parse_result
}

Expand Down

0 comments on commit 3bc310d

Please sign in to comment.