From 43d7f66c9ef13da76f183dba4738b8f593f75bc1 Mon Sep 17 00:00:00 2001 From: "A. Molzer" <5550310+197g@users.noreply.github.com> Date: Thu, 10 Jul 2025 23:12:46 +0200 Subject: [PATCH] Refactor cycle and IFD tracking The previous strategy could not keep track of all cycles such as reflexive ones at the very start. Additionally, it would fail to be able to iterate after seeking since it was overly cautious and verified the next IFD position before allowing the read of a current IFD. The new implementation is furthermore proof against having multiple independent chains of IFD which may occur when iterating SubIFDs for thumbnails. (Note that we do not verify the tree-structure, only a freedom from cycles). --- src/decoder/cycles.rs | 169 ++++++++++++++++++++++++++++++++++++++++++ src/decoder/mod.rs | 49 +++++++----- tests/subsubifds.rs | 9 ++- 3 files changed, 208 insertions(+), 19 deletions(-) create mode 100644 src/decoder/cycles.rs diff --git a/src/decoder/cycles.rs b/src/decoder/cycles.rs new file mode 100644 index 0000000..b28880d --- /dev/null +++ b/src/decoder/cycles.rs @@ -0,0 +1,169 @@ +use crate::{tags::IfdPointer, TiffError, TiffFormatError}; +use std::collections::HashMap; + +/// The IFD structure of a TIFF file should be limited to a forest of entries, and a tree when we +/// only consider having a primary IFD and its children. There is up to one primary child of a node +/// that is the `next` entry in an IFD itself. Since we offer iteration over this chain we want to +/// detect when a new edge would introduce a cycle, such that callers can be guaranteed to +/// terminate iteration at some point even in malicious images. +/// +/// However, we are not necessarily visiting the IFDs in their topological order. +/// +/// Since we only consider one child, we run union find to assign each pointer to a known chain. +/// Then, when we insert a new edge we check if they belong to the same component. +#[derive(Default, Debug)] +pub struct IfdCycles { + /// The root of each component union. + component_union: HashMap, + links: HashMap, + chains: HashMap, +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] +struct ComponentId(u64); + +impl IfdCycles { + pub fn new() -> Self { + IfdCycles::default() + } + + pub fn insert_next( + &mut self, + from: IfdPointer, + to: Option, + ) -> Result { + let to_offset = to.map_or(0, |p| p.0); + + // For some reason we got + match self.links.get(&from) { + Some(existing) if *existing == to_offset => return Ok(false), + // We got here twice with two different reads of the same IFD. That is .. unusual and + // we interpret it as a cycle. + Some(_) => return Err(TiffError::FormatError(TiffFormatError::CycleInOffsets)), + None => self.links.insert(from, to_offset), + }; + + self.ensure_node(from); + + if let Some(to) = to { + self.ensure_node(to); + + let parent = self.nominal_component(from); + let child = self.nominal_component(to); + + if parent == child { + return Err(TiffError::FormatError(TiffFormatError::CycleInOffsets)); + } + + self.component_union.insert(child, parent); + } + + Ok(true) + } + + fn ensure_node(&mut self, ifd: IfdPointer) { + self.chains.entry(ifd).or_insert_with(|| { + let id = ComponentId(self.component_union.len() as u64); + self.component_union.insert(id, id); + id + }); + } + + fn nominal_component(&mut self, node: IfdPointer) -> ComponentId { + let id = self.chains[&node]; + + let nomimal = { + let mut iter = id; + + loop { + let parent = self.component_union[&iter]; + + if parent == iter { + break parent; + } + + iter = parent; + } + }; + + if nomimal != id { + // Compress. + let mut iter = id; + + loop { + let parent = self.component_union[&iter]; + + if parent == iter { + break; + } + + self.component_union.insert(iter, nomimal); + iter = parent; + } + } + + nomimal + } +} + +#[test] +fn cycles_are_detected() { + let mut cycles = IfdCycles::new(); + + cycles + .insert_next(IfdPointer(0x20), Some(IfdPointer(0x800))) + .expect("non-existing link is valid"); + + cycles + .insert_next(IfdPointer(0x800), Some(IfdPointer(0x20))) + .expect_err("cycle must be detected"); +} + +#[test] +fn reflective_cycle() { + let mut cycles = IfdCycles::new(); + + cycles + .insert_next(IfdPointer(0x20), Some(IfdPointer(0x20))) + .expect_err("self-referential cycle must be detected"); +} + +#[test] +fn late_cycle() { + let mut cycles = IfdCycles::new(); + + cycles + .insert_next(IfdPointer(0x20), Some(IfdPointer(0x40))) + .expect("non-existing link is valid"); + + cycles + .insert_next(IfdPointer(0x60), Some(IfdPointer(0x80))) + .expect("non-existing link is valid"); + cycles + .insert_next(IfdPointer(0x80), Some(IfdPointer(0x20))) + .expect("non-existing link is valid"); + + cycles + .insert_next(IfdPointer(0x40), Some(IfdPointer(0x60))) + .expect_err("non-existing link is valid"); +} + +#[test] +fn odd_cycle() { + let mut cycles = IfdCycles::new(); + + cycles + .insert_next(IfdPointer(0x20), Some(IfdPointer(0x40))) + .expect("non-existing link is valid"); + + cycles + .insert_next(IfdPointer(0x60), Some(IfdPointer(0x80))) + .expect("non-existing link is valid"); + cycles + .insert_next(IfdPointer(0x80), Some(IfdPointer(0x20))) + .expect("non-existing link is valid"); + + cycles + .insert_next(IfdPointer(0x40), Some(IfdPointer(0x80))) + .expect_err("non-existing link is valid"); +} diff --git a/src/decoder/mod.rs b/src/decoder/mod.rs index f6f4fa3..66f4b9a 100644 --- a/src/decoder/mod.rs +++ b/src/decoder/mod.rs @@ -1,4 +1,4 @@ -use std::collections::{BTreeMap, HashSet}; +use std::collections::BTreeMap; use std::io::{self, Read, Seek}; use crate::tags::{ @@ -14,6 +14,7 @@ use half::f16; use self::image::Image; use self::stream::{ByteOrder, EndianReader}; +mod cycles; pub mod ifd; mod image; mod stream; @@ -263,9 +264,12 @@ where reader: EndianReader, bigtiff: bool, limits: Limits, + current_ifd: Option, next_ifd: Option, + /// The IFDs we visited already in this chain of IFDs. ifd_offsets: Vec, - seen_ifds: HashSet, + /// Map from the ifd into the `ifd_offsets` ordered list. + seen_ifds: cycles::IfdCycles, image: Image, } @@ -495,9 +499,8 @@ impl Decoder { } .map(IfdPointer); - let mut seen_ifds = HashSet::new(); - seen_ifds.insert(*next_ifd.as_ref().unwrap()); - let ifd_offsets = vec![*next_ifd.as_ref().unwrap()]; + let current_ifd = *next_ifd.as_ref().unwrap(); + let ifd_offsets = vec![current_ifd]; let mut decoder = Decoder { reader, @@ -505,7 +508,8 @@ impl Decoder { limits: Default::default(), next_ifd, ifd_offsets, - seen_ifds, + current_ifd: None, + seen_ifds: cycles::IfdCycles::new(), image: Image { ifd: None, width: 0, @@ -542,6 +546,11 @@ impl Decoder { self.image().colortype() } + /// The offset of the directory representing the current image. + pub fn ifd_pointer(&mut self) -> Option { + self.current_ifd + } + fn image(&self) -> &Image { &self.image } @@ -552,6 +561,8 @@ impl Decoder { if ifd_index >= self.ifd_offsets.len() { // We possibly need to load in the next IFD if self.next_ifd.is_none() { + self.current_ifd = None; + return Err(TiffError::FormatError( TiffFormatError::ImageFileDirectoryNotFound, )); @@ -574,6 +585,8 @@ impl Decoder { // If the index is within the list of ifds then we can load the selected image/IFD if let Some(ifd_offset) = self.ifd_offsets.get(ifd_index) { let ifd = Self::read_ifd(&mut self.reader, self.bigtiff, *ifd_offset)?; + self.next_ifd = ifd.next(); + self.current_ifd = Some(*ifd_offset); self.image = Image::from_reader(&mut self.reader, ifd, &self.limits, self.bigtiff)?; Ok(()) @@ -591,20 +604,20 @@ impl Decoder { )); } - let ifd = Self::read_ifd( - &mut self.reader, - self.bigtiff, - self.next_ifd.take().unwrap(), - )?; + let ifd_offset = self.next_ifd.take().unwrap(); + let ifd = Self::read_ifd(&mut self.reader, self.bigtiff, ifd_offset)?; - if let Some(next) = ifd.next() { - if !self.seen_ifds.insert(next) { - return Err(TiffError::FormatError(TiffFormatError::CycleInOffsets)); - } - self.next_ifd = Some(next); - self.ifd_offsets.push(next); + // Ensure this walk does not get us into a cycle. + self.seen_ifds.insert_next(ifd_offset, ifd.next())?; + + // Extend the list of known IFD offsets in this chain, if needed. + if self.ifd_offsets.last().copied() == self.current_ifd { + self.ifd_offsets.push(ifd_offset); } + self.current_ifd = Some(ifd_offset); + self.next_ifd = ifd.next(); + Ok(ifd) } @@ -622,7 +635,7 @@ impl Decoder { self.next_ifd.is_some() } - /// Returns the byte_order + /// Returns the byte_order of the file. pub fn byte_order(&self) -> ByteOrder { self.reader.byte_order } diff --git a/tests/subsubifds.rs b/tests/subsubifds.rs index fc72e8c..7dfd2ca 100644 --- a/tests/subsubifds.rs +++ b/tests/subsubifds.rs @@ -1,4 +1,4 @@ -use tiff::{decoder::Decoder, tags::IfdPointer}; +use tiff::decoder::Decoder; // This file has the following IFD structure: // @@ -20,14 +20,21 @@ fn decode_seek_chain() { let file = File::open(TEST_IMAGE_SUBIFD).expect("Cannot open test image"); let mut decoder = Decoder::new(file).expect("Invalid format to create decoder"); + // Remember the offsets in the first iteration. + let offset0 = decoder.ifd_pointer().expect("First IFD pointer not found"); decoder.next_image().unwrap(); + let offset1 = decoder.ifd_pointer().expect("Second IFD pointer not found"); decoder.next_image().unwrap(); + let offset2 = decoder.ifd_pointer().expect("Third IFD pointer not found"); assert!(!decoder.more_images()); // Ensure we can seek, to do this again. decoder.seek_to_image(0) .expect("Failed to seek to first image"); + assert_eq!(Some(offset0), decoder.ifd_pointer()); decoder.next_image().unwrap(); + assert_eq!(Some(offset1), decoder.ifd_pointer()); decoder.next_image().unwrap(); + assert_eq!(Some(offset2), decoder.ifd_pointer()); assert!(!decoder.more_images()); }