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.

Sign up for GitHub

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

Jump to bottom

Restore using NoMoreImageData errors. #500

Merged
merged 1 commit into from
Sep 20, 2024
Merged

Restore using NoMoreImageData errors. #500

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file added ...orpus/buf_independent/regressions/minimized-from-110a360793767757e44c9e4b95c495666e39e672
View file
Open in desktop
Binary file not shown.
9 changes: 4 additions & 5 deletions src/decoder/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -702,7 +702,9 @@ impl<R: Read> Reader<R> {
// Read image data until we have at least one full row (but possibly more than one).
while self.data_stream.len() - self.current_start < rowlen {
if self.subframe.consumed_and_flushed {
return Err(DecodingError::IoError(ErrorKind::UnexpectedEof.into()));
return Err(DecodingError::Format(
FormatErrorInner::NoMoreImageData.into(),
));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change above is needed to avoid a timeout when cargo fuzz run buf_independent fuzz/corpus/buf_independent/regressions/minimized-from-110a360793767757e44c9e4b95c495666e39e672 is run.

}

// Clear the current buffer before appending more data.
Expand All @@ -716,12 +718,9 @@ impl<R: Read> Reader<R> {

match self.decoder.decode_next(&mut self.data_stream)? {
Some(Decoded::ImageData) => {}
Some(Decoded::ImageDataFlushed) => {
Some(Decoded::ImageDataFlushed) | None /* after IEND chunk */ => {
self.subframe.consumed_and_flushed = true;
}
None => {
return Err(DecodingError::IoError(ErrorKind::UnexpectedEof.into()));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change above is not associated with a regression test:

  • I haven't been able to hit this branch when fuzzing
  • I think that ImageDataFlushed will always come first - this makes me question if this branch can be hit in practice.
  • Maybe we could hit this branch when we get into this loop outside of IDAT processing. This seems tricky, given that we search for IDAT when creating the Reader and when calling next_frame (and next_row doesn't progress beyond the current frame).

Nevertheless, I think this change is still desirable:

  • This change means that this commit restores all NoMoreImageData cases touched by b5b0674
  • I think the simplification is desirable and okay:
    • I think that treating this as consumed_and_flushed makes sense (it seems desirable to check the loop condition one last time in case there is some additional image_data + it seems okay to rely on the other check above to return NoMoreImageData error)
    • I think that it is okay to not restore distinguishing between NoMoreImageData and UnexpectedEndOfChunk (the difference was not exposed through the public API [well, except through Debug implementation]; and in both cases / at a high-level we want more rows/pixels but can't get them - this seems to be covered well by NoMoreImageData)

Copy link
Contributor

Choose a reason for hiding this comment

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

In general, if a specific case is impossible we should panic! rather than returning an error or silently continuing. That way fuzzing can detect if they actually can be hit, which usually signals a bug elsewhere in the code.

This crate used to have tons of error handling code for cases that weren't even reachable in the first place, which I've tried to clean up over time.

Copy link
Contributor Author

@anforowicz anforowicz Sep 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(edited - my initial markdown attempt didn't work... :-) )

Ack. You're right that my attempt to avoid risk here leads to suboptimal code. Let me try to think through the options available.

  • Option 1: None => unreachable!(). Special-casing None seems wrong, because None is not the only unreachable option here - e.g. Some(Decoded::Header) is another unreachable option.
  • Option 2: Removing | None. This simplifies the code. And fuzzing indeed increases our confidence that things still work fine without this branch (and without leading to a timeout / without making this an infinite loop).
  • Option 3: Exhaustively list all the possible options and panic for ones we think are unreachable. I don't like this option because:
    • This adds noise to the code, making it harder to read and understand this loop
    • This also seems a bit risky - fuzzing doesn't cover all valid ways of using the public API (it only covers next_frame today and doesn't cover next_row; and even if we added next_row coverage, then there are various permutations of public API calls that we may need to fuzz).

I am leaning toward option 2. For now, let me try option #2 locally and double-check that fuzzing doesn't complain.

_ => (),
}
}
Expand Down
6 changes: 6 additions & 0 deletions src/decoder/stream.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,8 @@ pub(crate) enum FormatErrorInner {
CorruptFlateStream {
err: fdeflate::DecompressionError,
},
/// The image data chunk was too short for the expected pixel count.
NoMoreImageData,
/// Bad text encoding
BadTextEncoding(TextDecodingError),
/// fdAT shorter than 4 bytes
Expand Down Expand Up @@ -333,6 +335,10 @@ impl fmt::Display for FormatError {
UnknownInterlaceMethod(nr) => write!(fmt, "Unknown interlace method {}.", nr),
BadSubFrameBounds {} => write!(fmt, "Sub frame is out-of-bounds."),
InvalidSignature => write!(fmt, "Invalid PNG signature."),
NoMoreImageData => write!(
fmt,
"IDAT or fDAT chunk does not have enough data for image."
),
CorruptFlateStream { err } => {
write!(fmt, "Corrupt deflate stream. ")?;
write!(fmt, "{:?}", err)
Expand Down
Loading