Skip to content

Commit

Permalink
Avoid infinite loop when retrying after earlier fatal error.
Browse files Browse the repository at this point in the history
When `StreamingDecoder` reports an error, it leaves `state` set to
`None`.  Before this commit, calling `next_frame` in this state would
have led to an infinite loop:

* `ReadDecoder::decode_next` would loop forever (`!self.at_eof` is true
  after an error) and would fail to make progress, because
* When `StreamingDecoder::update` sees `state` saw set to `None` then
  before this commit it wouldn't enter the `next_state` loop and would
  immediately return no progress
  (`Ok((/* consumer bytes = */ 0, Decoded::Nothing))`).

After this commit, `StreamingDecoder::update` checks if the `state` is
`None` and treats this as an error.
  • Loading branch information
anforowicz authored and kornelski committed Oct 6, 2024
1 parent 1764d16 commit 1ec7613
Show file tree
Hide file tree
Showing 2 changed files with 32 additions and 7 deletions.
7 changes: 7 additions & 0 deletions src/common.rs
Original file line number Diff line number Diff line change
Expand Up @@ -791,6 +791,10 @@ pub(crate) enum ParameterErrorKind {
/// library will perform the checks necessary to ensure that data was accurate or error with a
/// format error otherwise.
PolledAfterEndOfImage,
/// Attempt to continue decoding after a fatal, non-resumable error was reported (e.g. after
/// [`DecodingError::Format`]). The only case when it is possible to resume after an error
/// is an `UnexpectedEof` scenario - see [`DecodingError::IoError`].
PolledAfterFatalError,
}

impl From<ParameterErrorKind> for ParameterError {
Expand All @@ -807,6 +811,9 @@ impl fmt::Display for ParameterError {
write!(fmt, "wrong data size, expected {} got {}", expected, actual)
}
PolledAfterEndOfImage => write!(fmt, "End of image has been reached"),
PolledAfterFatalError => {
write!(fmt, "A fatal decoding error has been encounted earlier")
}
}
}
}
32 changes: 25 additions & 7 deletions src/decoder/stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use super::zlib::ZlibStream;
use crate::chunk::{self, ChunkType, IDAT, IEND, IHDR};
use crate::common::{
AnimationControl, BitDepth, BlendOp, ColorType, DisposeOp, FrameControl, Info, ParameterError,
PixelDimensions, ScaledFloat, SourceChromaticities, Unit,
ParameterErrorKind, PixelDimensions, ScaledFloat, SourceChromaticities, Unit,
};
use crate::text_metadata::{ITXtChunk, TEXtChunk, TextDecodingError, ZTXtChunk};
use crate::traits::ReadBytesExt;
Expand Down Expand Up @@ -627,15 +627,24 @@ impl StreamingDecoder {
mut buf: &[u8],
image_data: &mut Vec<u8>,
) -> Result<(usize, Decoded), DecodingError> {
if self.state.is_none() {
return Err(DecodingError::Parameter(
ParameterErrorKind::PolledAfterFatalError.into(),
));
}

let len = buf.len();
while !buf.is_empty() && self.state.is_some() {
while !buf.is_empty() {
match self.next_state(buf, image_data) {
Ok((bytes, Decoded::Nothing)) => buf = &buf[bytes..],
Ok((bytes, result)) => {
buf = &buf[bytes..];
return Ok((len - buf.len(), result));
}
Err(err) => return Err(err),
Err(err) => {
debug_assert!(self.state.is_none());
return Err(err);
}
}
}
Ok((len - buf.len(), Decoded::Nothing))
Expand Down Expand Up @@ -1917,8 +1926,18 @@ mod tests {
// 0-length fdAT should result in an error.
let err = reader.next_frame(&mut buf).unwrap_err();
assert!(matches!(&err, DecodingError::Format(_)));
let msg = format!("{err}");
assert_eq!("fdAT chunk shorter than 4 bytes", msg);
assert_eq!("fdAT chunk shorter than 4 bytes", format!("{err}"));

// Calling `next_frame` again should return an error. Same error as above would be nice,
// but it is probably unnecessary and infeasible (`DecodingError` can't derive `Clone`
// because `std::io::Error` doesn't implement `Clone`).. But it definitely shouldn't enter
// an infinite loop.
let err2 = reader.next_frame(&mut buf).unwrap_err();
assert!(matches!(&err2, DecodingError::Parameter(_)));
assert_eq!(
"A fatal decoding error has been encounted earlier",
format!("{err2}")
);
}

#[test]
Expand All @@ -1935,8 +1954,7 @@ mod tests {
// 3-bytes-long fdAT should result in an error.
let err = reader.next_frame(&mut buf).unwrap_err();
assert!(matches!(&err, DecodingError::Format(_)));
let msg = format!("{err}");
assert_eq!("fdAT chunk shorter than 4 bytes", msg);
assert_eq!("fdAT chunk shorter than 4 bytes", format!("{err}"));
}

#[test]
Expand Down

0 comments on commit 1ec7613

Please sign in to comment.