-
Notifications
You must be signed in to change notification settings - Fork 144
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
Restore using NoMoreImageData
errors.
#500
Conversation
image-rs#496 has incorrectly started to return `UnexpectedEof` in a situation when when there is no `Read`-level EOF. In particular, this may happen when an `IDAT` of `fdAT` chunk has been fully decoded, decompressed, and flushed, but still doesn't contain the expected number of image pixels. In such a situation the old `NoMoreImageData` error was appropriate. In a sense, this commit is a partial revert and/or refactoring of the earlier commit image-rs@b5b0674 This commit fixes a timeout found locally by the `buf_independent` fuzzer. The repro case has been saved under `fuzz/corpus/buf_independent/regressions`.
return Err(DecodingError::IoError(ErrorKind::UnexpectedEof.into())); | ||
return Err(DecodingError::Format( | ||
FormatErrorInner::NoMoreImageData.into(), | ||
)); |
There was a problem hiding this comment.
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.
self.subframe.consumed_and_flushed = true; | ||
} | ||
None => { | ||
return Err(DecodingError::IoError(ErrorKind::UnexpectedEof.into())); | ||
} |
There was a problem hiding this comment.
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 forIDAT
when creating theReader
and when callingnext_frame
(andnext_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 additionalimage_data
+ it seems okay to rely on the other check above to returnNoMoreImageData
error) - I think that it is okay to not restore distinguishing between
NoMoreImageData
andUnexpectedEndOfChunk
(the difference was not exposed through the public API [well, except throughDebug
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 byNoMoreImageData
)
- I think that treating this as
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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-casingNone
seems wrong, becauseNone
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 covernext_row
; and even if we addednext_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.
This is a follow-up to the feedback at image-rs#500 (comment)
This is a follow-up to the feedback at #500 (comment)
#496 has incorrectly started to return
UnexpectedEof
in a situation when when there is noRead
-level EOF. In particular, this may happen when anIDAT
offdAT
chunk has been fully decoded, decompressed, and flushed, but still doesn't contain the expected number of image pixels. In such a situation the oldNoMoreImageData
error was appropriate. In a sense, this commit is a partial revert and/or refactoring of the earlier commit b5b0674This commit fixes a timeout found locally by the
buf_independent
fuzzer. The repro case has been saved underfuzz/corpus/buf_independent/regressions
.