Skip to content

Commit

Permalink
Treat most auxiliary chunk errors as benign. (#569)
Browse files Browse the repository at this point in the history
  • Loading branch information
@anforowicz
anforowicz authored Feb 9, 2025
1 parent 819d91e commit 466bdae
Show file tree
Hide file tree
Showing 3 changed files with 113 additions and 65 deletions.
172 changes: 110 additions & 62 deletions src/decoder/stream.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -975,7 +975,7 @@ impl StreamingDecoder {

fn parse_chunk(&mut self, type_str: ChunkType) -> Result<Decoded, DecodingError> {
self.state = Some(State::new_u32(U32ValueKind::Crc(type_str)));
let parse_result = match type_str {
let mut parse_result = match type_str {
IHDR => self.parse_ihdr(),
chunk::sBIT => self.parse_sbit(),
chunk::PLTE => self.parse_plte(),
Expand All @@ -998,8 +998,7 @@ impl StreamingDecoder {
_ => Ok(Decoded::PartialChunk(type_str)),
};

parse_result.map_err(|e| {
self.state = None;
parse_result = parse_result.map_err(|e| {
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
Expand All @@ -1012,7 +1011,37 @@ impl StreamingDecoder {
}
e => e,
}
})
});

// Ignore benign errors in some auxiliary chunks. `LimitsExceeded`, `Parameter`
// and other error kinds are *not* treated as benign. We only ignore errors in *some*
// auxiliary chunks (i.e. we don't use `chunk::is_critical`), because for chunks like
// `fcTL` or `fdAT` the fallback to the static/non-animated image has to be implemented
// *on top* of the `StreamingDecoder` API.
//
// TODO: Consider supporting a strict mode where even benign errors are reported up.
// See https://github.com/image-rs/image-png/pull/569#issuecomment-2642062285
if matches!(parse_result.as_ref(), Err(DecodingError::Format(_)))
&& matches!(
type_str,
chunk::cHRM
| chunk::gAMA
| chunk::iCCP
| chunk::pHYs
| chunk::sBIT
| chunk::sRGB
| chunk::tRNS
)
{
parse_result = Ok(Decoded::Nothing);
}

// Clear the parsing state to enforce that parsing can't continue after an error.
if parse_result.is_err() {
self.state = None;
}

parse_result
}

fn parse_fctl(&mut self) -> Result<Decoded, DecodingError> {
Expand Down Expand Up @@ -1113,74 +1142,69 @@ impl StreamingDecoder {
}

fn parse_sbit(&mut self) -> Result<Decoded, DecodingError> {
let mut parse = || {
let info = self.info.as_mut().unwrap();
if info.palette.is_some() {
return Err(DecodingError::Format(
FormatErrorInner::AfterPlte { kind: chunk::sBIT }.into(),
));
}
let info = self.info.as_mut().unwrap();
if info.palette.is_some() {
return Err(DecodingError::Format(
FormatErrorInner::AfterPlte { kind: chunk::sBIT }.into(),
));
}

if self.have_idat {
return Err(DecodingError::Format(
FormatErrorInner::AfterIdat { kind: chunk::sBIT }.into(),
));
}
if self.have_idat {
return Err(DecodingError::Format(
FormatErrorInner::AfterIdat { kind: chunk::sBIT }.into(),
));
}

if info.sbit.is_some() {
return Err(DecodingError::Format(
FormatErrorInner::DuplicateChunk { kind: chunk::sBIT }.into(),
));
}
if info.sbit.is_some() {
return Err(DecodingError::Format(
FormatErrorInner::DuplicateChunk { kind: chunk::sBIT }.into(),
));
}

let (color_type, bit_depth) = { (info.color_type, info.bit_depth) };
// The sample depth for color type 3 is fixed at eight bits.
let sample_depth = if color_type == ColorType::Indexed {
BitDepth::Eight
} else {
bit_depth
};
self.limits
.reserve_bytes(self.current_chunk.raw_bytes.len())?;
let vec = self.current_chunk.raw_bytes.clone();
let len = vec.len();
let (color_type, bit_depth) = { (info.color_type, info.bit_depth) };
// The sample depth for color type 3 is fixed at eight bits.
let sample_depth = if color_type == ColorType::Indexed {
BitDepth::Eight
} else {
bit_depth
};
self.limits
.reserve_bytes(self.current_chunk.raw_bytes.len())?;
let vec = self.current_chunk.raw_bytes.clone();
let len = vec.len();

// expected lenth of the chunk
let expected = match color_type {
ColorType::Grayscale => 1,
ColorType::Rgb | ColorType::Indexed => 3,
ColorType::GrayscaleAlpha => 2,
ColorType::Rgba => 4,
};
// expected lenth of the chunk
let expected = match color_type {
ColorType::Grayscale => 1,
ColorType::Rgb | ColorType::Indexed => 3,
ColorType::GrayscaleAlpha => 2,
ColorType::Rgba => 4,
};

// Check if the sbit chunk size is valid.
if expected != len {
return Err(DecodingError::Format(
FormatErrorInner::InvalidSbitChunkSize {
color_type,
expected,
len,
}
.into(),
));
}

// Check if the sbit chunk size is valid.
if expected != len {
for sbit in &vec {
if *sbit < 1 || *sbit > sample_depth as u8 {
return Err(DecodingError::Format(
FormatErrorInner::InvalidSbitChunkSize {
color_type,
expected,
len,
FormatErrorInner::InvalidSbit {
sample_depth,
sbit: *sbit,
}
.into(),
));
}

for sbit in &vec {
if *sbit < 1 || *sbit > sample_depth as u8 {
return Err(DecodingError::Format(
FormatErrorInner::InvalidSbit {
sample_depth,
sbit: *sbit,
}
.into(),
));
}
}
info.sbit = Some(Cow::Owned(vec));
Ok(Decoded::Nothing)
};

parse().ok();
}
info.sbit = Some(Cow::Owned(vec));
Ok(Decoded::Nothing)
}

Expand Down Expand Up @@ -2963,4 +2987,28 @@ mod tests {
&err,
);
}

#[test]
fn test_incorrect_trns_chunk_is_ignored() {
let png = {
let mut png = Vec::new();
write_png_sig(&mut png);
write_rgba8_ihdr_with_width(&mut png, 8);
write_chunk(&mut png, b"tRNS", &[12, 34, 56]);
write_chunk(
&mut png,
b"IDAT",
&generate_rgba8_with_width_and_height(8, 8),
);
write_iend(&mut png);
png
};
let decoder = Decoder::new(png.as_slice());
let mut reader = decoder.read_info().unwrap();
let mut buf = vec![0; reader.output_buffer_size()];
assert!(reader.info().trns.is_none());
reader.next_frame(&mut buf).unwrap();
assert_eq!(3093270825, crc32fast::hash(&buf));
assert!(reader.info().trns.is_none());
}
}
2 changes: 1 addition & 1 deletion src/filter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ fn filter_paeth_stbi(a: u8, b: u8, c: u8) -> u8 {
let hi = a.max(b);
let t0 = if hi as i16 <= thresh { lo } else { c };
let t1 = if thresh <= lo as i16 { hi } else { t0 };
return t1;
t1
}

#[cfg(any(test, all(feature = "unstable", target_arch = "x86_64")))]
Expand Down
4 changes: 2 additions & 2 deletions src/text_metadata.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
//! chunks. There are three kinds of text chunks.
//! - `tEXt`: This has a `keyword` and `text` field, and is ISO 8859-1 encoded.
//! - `zTXt`: This is semantically the same as `tEXt`, i.e. it has the same fields and
//! encoding, but the `text` field is compressed before being written into the PNG file.
//! encoding, but the `text` field is compressed before being written into the PNG file.
//! - `iTXt`: This chunk allows for its `text` field to be any valid UTF-8, and supports
//! compression of the text field as well.
//! compression of the text field as well.
//!
//! The `ISO 8859-1` encoding technically doesn't allow any control characters
//! to be used, but in practice these values are encountered anyway. This can
Expand Down

0 comments on commit 466bdae

Please sign in to comment.