Skip to content

Commit

Permalink
Fix handling of fdAT chunks shorter than 4 bytes.
Browse files Browse the repository at this point in the history
This has accidentally regressed in a recent commit and was caught during
PR review.
  • Loading branch information
anforowicz committed Nov 14, 2023
1 parent f4bc666 commit 2bca55c
Show file tree
Hide file tree
Showing 3 changed files with 133 additions and 7 deletions.
111 changes: 110 additions & 1 deletion src/decoder/stream.rs
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,8 @@ pub(crate) enum FormatErrorInner {
NoMoreImageData,
/// Bad text encoding
BadTextEncoding(TextDecodingError),
/// fdAT shorter than 4 bytes
FdatShorterThanFourBytes,
}

impl error::Error for DecodingError {
Expand Down Expand Up @@ -337,6 +339,7 @@ impl fmt::Display for FormatError {
}
}
}
FdatShorterThanFourBytes => write!(fmt, "fdAT chunk shorter than 4 bytes"),
}
}
}
Expand Down Expand Up @@ -651,7 +654,13 @@ impl StreamingDecoder {
ApngSequenceNumber => {
debug_assert_eq!(self.current_chunk.type_, chunk::fdAT);
let next_seq_no = val;
self.current_chunk.remaining -= 4;
if self.current_chunk.remaining < 4 {
return Err(DecodingError::Format(
FormatErrorInner::FdatShorterThanFourBytes.into(),
));
} else {
self.current_chunk.remaining -= 4;
}

if let Some(seq_no) = self.current_seq_no {
if next_seq_no != seq_no + 1 {
Expand Down Expand Up @@ -1363,7 +1372,11 @@ impl Default for ChunkState {
mod tests {
use super::ScaledFloat;
use super::SourceChromaticities;
use crate::test_utils::*;
use crate::{Decoder, DecodingError};
use byteorder::WriteBytesExt;
use std::fs::File;
use std::io::Write;

#[test]
fn image_gamma() -> Result<(), ()> {
Expand Down Expand Up @@ -1611,4 +1624,100 @@ mod tests {
// content (`b"test iccp contents"`) which would have a different CRC (797351983).
assert_eq!(4070462061, crc32fast::hash(&icc_profile));
}

/// Writes an acTL chunk.
/// See https://wiki.mozilla.org/APNG_Specification#.60acTL.60:_The_Animation_Control_Chunk
fn write_actl(w: &mut impl Write, animation: &crate::AnimationControl) {
let mut data = Vec::new();
data.write_u32::<byteorder::BigEndian>(animation.num_frames)
.unwrap();
data.write_u32::<byteorder::BigEndian>(animation.num_plays)
.unwrap();
write_chunk(w, b"acTL", &data);
}

/// Writes an fcTL chunk.
/// See https://wiki.mozilla.org/APNG_Specification#.60fcTL.60:_The_Frame_Control_Chunk
fn write_fctl(w: &mut impl Write, frame: &crate::FrameControl) {
let mut data = Vec::new();
data.write_u32::<byteorder::BigEndian>(frame.sequence_number)
.unwrap();
data.write_u32::<byteorder::BigEndian>(frame.width).unwrap();
data.write_u32::<byteorder::BigEndian>(frame.height)
.unwrap();
data.write_u32::<byteorder::BigEndian>(frame.x_offset)
.unwrap();
data.write_u32::<byteorder::BigEndian>(frame.y_offset)
.unwrap();
data.write_u16::<byteorder::BigEndian>(frame.delay_num)
.unwrap();
data.write_u16::<byteorder::BigEndian>(frame.delay_den)
.unwrap();
data.write_u8(frame.dispose_op as u8).unwrap();
data.write_u8(frame.blend_op as u8).unwrap();
write_chunk(w, b"fcTL", &data);
}

/// Writes PNG signature and chunks that can precede an fdAT chunk that is expected
/// to have
/// - `sequence_number` set to 0
/// - image data with rgba8 pixels in a `width` by `width` image
fn write_fdat_prefix(w: &mut impl Write, num_frames: u32, width: u32) {
write_png_sig(w);
write_rgba8_ihdr_with_width(w, width);
write_actl(
w,
&crate::AnimationControl {
num_frames,
num_plays: 0,
},
);

let mut fctl = crate::FrameControl {
width,
height: width,
..Default::default()
};
write_fctl(w, &fctl);
write_rgba8_idat_with_width(w, width);

fctl.sequence_number += 1;
write_fctl(w, &fctl);
}

#[test]
fn test_fdat_chunk_payload_length_0() {
let mut png = Vec::new();
write_fdat_prefix(&mut png, 2, 8);
write_chunk(&mut png, b"fdAT", &[]);

let decoder = Decoder::new(png.as_slice());
let mut reader = decoder.read_info().unwrap();
let mut buf = vec![0; reader.output_buffer_size()];
reader.next_frame(&mut buf).unwrap();

// 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);
}

#[test]
fn test_fdat_chunk_payload_length_3() {
let mut png = Vec::new();
write_fdat_prefix(&mut png, 2, 8);
write_chunk(&mut png, b"fdAT", &[1, 0, 0]);

let decoder = Decoder::new(png.as_slice());
let mut reader = decoder.read_info().unwrap();
let mut buf = vec![0; reader.output_buffer_size()];
reader.next_frame(&mut buf).unwrap();

// 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);
}
}
3 changes: 3 additions & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,5 +81,8 @@ pub use crate::decoder::{
pub use crate::encoder::{Encoder, EncodingError, StreamWriter, Writer};
pub use crate::filter::{AdaptiveFilterType, FilterType};

#[cfg(test)]
pub(crate) mod test_utils;

#[cfg(feature = "benchmarks")]
pub mod benchable_apis;
26 changes: 20 additions & 6 deletions src/test_utils.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
//! A set of test utilities.
//!
//! There is some overlap between this module and `src/encoder.rs` module, but:
//!
//! * This module (unlike `src/encoder.rs`) performs no validation of the data being written - this
//! allows building testcases that use arbitrary, potentially invalid PNGs as input.
//! * This module can be reused from `benches/decoder.rs` (a separate crate).
use byteorder::WriteBytesExt;
use std::io::Write;
Expand All @@ -20,19 +26,24 @@ use std::io::Write;
/// [this
/// discussion](https://github.com/image-rs/image-png/discussions/416#discussioncomment-7436871)
/// for more details).
#[allow(dead_code)] // Used from `benches/decoder.rs`
pub fn write_noncompressed_png(w: &mut impl Write, width: u32) {
write_png_sig(w);
write_ihdr(w, width);
write_noncompressed_idat(w, width);
write_rgba8_ihdr_with_width(w, width);
write_rgba8_idat_with_width(w, width);
write_iend(w);
}

fn write_png_sig(w: &mut impl Write) {
/// Writes PNG signature.
/// See http://www.libpng.org/pub/png/spec/1.2/PNG-Structure.html#PNG-file-signature
pub fn write_png_sig(w: &mut impl Write) {
const SIG: [u8; 8] = [137, 80, 78, 71, 13, 10, 26, 10];
w.write_all(&SIG).unwrap();
}

fn write_chunk(w: &mut impl Write, chunk_type: &[u8], data: &[u8]) {
/// Writes an arbitrary PNG chunk.
pub fn write_chunk(w: &mut impl Write, chunk_type: &[u8], data: &[u8]) {
assert_eq!(chunk_type.len(), 4);
let crc = {
let input = chunk_type
.iter()
Expand All @@ -48,7 +59,9 @@ fn write_chunk(w: &mut impl Write, chunk_type: &[u8], data: &[u8]) {
w.write_u32::<byteorder::BigEndian>(crc).unwrap();
}

fn write_ihdr(w: &mut impl Write, width: u32) {
/// Writes an IHDR chunk that indicates a non-interlaced RGBA8 that uses the same height and
/// `width`. See http://www.libpng.org/pub/png/spec/1.2/PNG-Chunks.html#C.IHDR
pub fn write_rgba8_ihdr_with_width(w: &mut impl Write, width: u32) {
let mut data = Vec::new();
data.write_u32::<byteorder::BigEndian>(width).unwrap();
data.write_u32::<byteorder::BigEndian>(width).unwrap(); // height
Expand All @@ -60,7 +73,8 @@ fn write_ihdr(w: &mut impl Write, width: u32) {
write_chunk(w, b"IHDR", &data);
}

fn write_noncompressed_idat(w: &mut impl Write, width: u32) {
/// Writes an IDAT chunk.
pub fn write_rgba8_idat_with_width(w: &mut impl Write, width: u32) {
// Generate arbitrary test pixels.
let image_pixels = {
let mut row = Vec::new();
Expand Down

0 comments on commit 2bca55c

Please sign in to comment.