From 466bdae43afdec0a4cacfd18bf432daf1d86575d Mon Sep 17 00:00:00 2001 From: Lukasz Anforowicz Date: Sun, 9 Feb 2025 15:58:22 -0800 Subject: [PATCH] Treat most auxiliary chunk errors as benign. (#569) --- src/decoder/stream.rs | 172 +++++++++++++++++++++++++++--------------- src/filter.rs | 2 +- src/text_metadata.rs | 4 +- 3 files changed, 113 insertions(+), 65 deletions(-) diff --git a/src/decoder/stream.rs b/src/decoder/stream.rs index 0e65ef23..fb572ee2 100644 --- a/src/decoder/stream.rs +++ b/src/decoder/stream.rs @@ -975,7 +975,7 @@ impl StreamingDecoder { fn parse_chunk(&mut self, type_str: ChunkType) -> Result { 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(), @@ -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 @@ -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 { @@ -1113,74 +1142,69 @@ impl StreamingDecoder { } fn parse_sbit(&mut self) -> Result { - 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) } @@ -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()); + } } diff --git a/src/filter.rs b/src/filter.rs index 38b619b1..c33cc205 100644 --- a/src/filter.rs +++ b/src/filter.rs @@ -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")))] diff --git a/src/text_metadata.rs b/src/text_metadata.rs index 3bb491fe..a5ace94a 100644 --- a/src/text_metadata.rs +++ b/src/text_metadata.rs @@ -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