From d8ceb3a6af2b5bd86fdcb947eb8a4f5b1c372125 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Sat, 24 Oct 2026 05:17:19 -0400 Subject: [PATCH] security/meta,merge,raw,tiff,denoise: bound memory sized by attacker-shaped inputs XMP flattening, sparse-focus EXIF decode (plus the maker-note and TIFF value budgets), lossy-DNG chunk staging, panorama stitch frames, the reference ONNX executor's tensors and the denoise product reader all now reject or cap what a malformed file can make them allocate: each result is checked before the memory behind it is reserved, every regression carrying a test that fails without its bound. --- Cargo.lock | 1 + crates/denoise-core/Cargo.toml | 1 + crates/denoise-core/src/product.rs | 37 ++++++- crates/denoise-core/tests/product_heap.rs | 50 +++++++++ crates/denoise/src/reference.rs | 76 +++++++++++++- crates/merge/src/lib.rs | 2 + crates/merge/src/pano/camera.rs | 10 +- crates/merge/src/pano/mod.rs | 8 ++ crates/merge/tests/it/pano.rs | 12 +++ crates/meta/src/exif.rs | 12 ++- crates/meta/src/focus_read.rs | 10 +- crates/meta/src/lib.rs | 2 +- crates/meta/src/xmp.rs | 69 +++++++++++-- crates/raw/src/tiffraw.rs | 118 ++++++++++++++++------ crates/tiff/src/makernote.rs | 35 ++++++- crates/tiff/src/reader.rs | 19 ++++ 16 files changed, 406 insertions(+), 56 deletions(-) create mode 100644 crates/denoise-core/tests/product_heap.rs diff --git a/Cargo.lock b/Cargo.lock index 6e932cba8..9d7e4ec18 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3221,6 +3221,7 @@ dependencies = [ name = "lightcraft-denoise-core" version = "0.14.0" dependencies = [ + "dhat", "lightcraft-raster", "miniz_oxide 0.8.9", "proptest", diff --git a/crates/denoise-core/Cargo.toml b/crates/denoise-core/Cargo.toml index b65f90ae4..d0a0b10c1 100644 --- a/crates/denoise-core/Cargo.toml +++ b/crates/denoise-core/Cargo.toml @@ -24,3 +24,4 @@ web-time = { workspace = true } [dev-dependencies] proptest = { workspace = true } +dhat = { workspace = true } diff --git a/crates/denoise-core/src/product.rs b/crates/denoise-core/src/product.rs index 5a607c8f4..c33b1d4c2 100644 --- a/crates/denoise-core/src/product.rs +++ b/crates/denoise-core/src/product.rs @@ -9,7 +9,7 @@ //! compressed length and CRC-32, then the strips' bytes. `key` says what the product was made from (the photo's //! content, the model, this code's version, the settings it depends on); a file with another key is stale. -use std::io::{Read, Write}; +use std::io::{Read, Seek, Write}; use std::path::Path; use lightcraft_raster::Rgb32f; @@ -216,6 +216,10 @@ fn read_start(r: &mut impl Read) -> Result { r.read_exact(&mut e)?; let l = u32::from_le_bytes([e[0], e[1], e[2], e[3]]) as usize; let c = u32::from_le_bytes([e[4], e[5], e[6], e[7]]); + // a strip never compresses to nothing (it has at least one row, so at least one deflated pixel) + if l == 0 { + return Err(ProductError::Format("a strip is empty".into())); + } // a strip never compresses to much more than its raw size if l > 12 * h.strip * h.width + 4096 { return Err(ProductError::Format("a strip is larger than it can be".into())); @@ -305,7 +309,6 @@ pub fn read_window(path: &Path, key: &str, window: Option, factor: usize } // the picture rows and columns the blocks cover let (x_end, y_end) = if whole { (w.x + w.width, w.y + w.height) } else { (w.x + ow * factor, w.y + oh * factor) }; - let mut out = Rgb32f { width: ow, height: oh, data: allocate(ow * oh, [0.0; 3])? }; let lut = f16_table(); let value = |raw: &[u8], c: usize, n: usize, i: usize| -> f32 { let (lo, hi) = (raw.get(2 * c * n + i).copied().unwrap_or(0), raw.get((2 * c + 1) * n + i).copied().unwrap_or(0)); @@ -318,6 +321,21 @@ pub fn read_window(path: &Path, key: &str, window: Option, factor: usize let strips: Vec = (0..table.len()).filter(|&s| wanted(s)).collect(); // skip the strips before the first one wanted let skipped: usize = table.iter().take(strips.first().copied().unwrap_or(0)).map(|e| e.0).sum(); + // The output is reserved only after the wanted strips' compressed bytes are present and match their checksums: + // a malformed cache whose strips are empty, damaged or short are rejected here, before the (large) result is + // allocated, not after. Reading them twice is cheap next to decoding them, and the fold below re-seeks here. + let strips_at = f.stream_position()?; + f.seek_relative(i64::try_from(skipped).map_err(|_| ProductError::Format("the file is too large".into()))?)?; + for &s in &strips { + let Some(&(len, expected)) = table.get(s) else { return Err(ProductError::Format("a strip is missing".into())) }; + let mut packed = allocate(len, 0u8)?; + f.read_exact(&mut packed)?; + if crc(&packed) != expected { + return Err(ProductError::Format(format!("strip {s} is damaged"))); + } + } + let mut out = Rgb32f { width: ow, height: oh, data: allocate(ow * oh, [0.0; 3])? }; + f.seek(std::io::SeekFrom::Start(strips_at))?; f.seek_relative(i64::try_from(skipped).map_err(|_| ProductError::Format("the file is too large".into()))?)?; for group in strips.chunks(GROUP) { let mut packed = Vec::with_capacity(group.len()); @@ -422,6 +440,21 @@ mod tests { let _ = std::fs::remove_file(p); } + #[test] + fn an_empty_strip_entry_is_rejected_by_the_header() { + let p = temp("empty-strip"); + let header = serde_json::to_vec(&Header { width: 16, height: 1, strip: STRIP_ROWS, key: "k".into() }).unwrap(); + let mut bytes = Vec::from(MAGIC.as_slice()); + bytes.extend_from_slice(&[VERSION, 0, 0, 0]); + bytes.extend_from_slice(&(header.len() as u32).to_le_bytes()); + bytes.extend_from_slice(&header); + // one strip with a zero-length entry: it has a row, so its deflated bytes are never empty + bytes.extend_from_slice(&[0; 8]); + std::fs::write(&p, bytes).unwrap(); + assert!(matches!(read_header_at(&p), Err(ProductError::Format(why)) if why.contains("empty"))); + let _ = std::fs::remove_file(p); + } + fn temp(name: &str) -> std::path::PathBuf { std::env::temp_dir().join(format!("lc-denoise-{name}-{}.lcdn", std::process::id())) } diff --git a/crates/denoise-core/tests/product_heap.rs b/crates/denoise-core/tests/product_heap.rs new file mode 100644 index 000000000..40c170861 --- /dev/null +++ b/crates/denoise-core/tests/product_heap.rs @@ -0,0 +1,50 @@ +//! Peak heap of a `read_window` that must reject a product's strips (`crates/denoise-core/src/product.rs`), measured +//! with dhat's testing mode. The result is allocated only after the wanted strips' checksums are verified, so a cache +//! whose compressed bytes are damaged or absent fails without first reserving the (multi-megabyte) output. One test +//! per binary (dhat's profiler is global). + +use lightcraft_denoise_core::product::read_window; + +#[global_allocator] +static ALLOC: dhat::Alloc = dhat::Alloc; + +fn temp(name: &str) -> std::path::PathBuf { + std::env::temp_dir().join(format!("lc-denoise-{name}-{}.lcdn", std::process::id())) +} + +/// A 2000 × 1024 product of key `k` (a 24 MB result): its 8 strips each claim 8 bytes that do not match their +/// checksum, and the file is exactly the length its header and table declare. A 24 MB peak means the output was +/// reserved; the cap here bounds the header, the table and the strips' own bytes. +fn damaged_product() -> Vec { + let header = br#"{"width":2000,"height":1024,"strip":128,"key":"k"}"#; + const STRIP_LEN: usize = 8; + let mut bytes = Vec::from(b"LCDN".as_slice()); + bytes.extend_from_slice(&[1, 0, 0, 0]); + bytes.extend_from_slice(&(header.len() as u32).to_le_bytes()); + bytes.extend_from_slice(header); + let expected_len = 12 + header.len() + 8 * 8 + 8 * STRIP_LEN; + for _ in 0..8 { + // 8 claimed bytes whose recorded checksum does not match theirs (0xAA bytes do not have this CRC) + bytes.extend_from_slice(&(STRIP_LEN as u32).to_le_bytes()); + bytes.extend_from_slice(&[0xAA, 0xBB, 0xCC, 0xDD]); + } + for _ in 0..8 { + bytes.extend_from_slice(&[0xAAu8; STRIP_LEN]); + } + assert_eq!(bytes.len(), expected_len, "the crafted file lies about its length"); + bytes +} + +#[test] +fn a_damaged_product_is_rejected_before_its_output_is_allocated() { + let bytes = damaged_product(); + let p = temp("damaged-product-heap"); + std::fs::write(&p, &bytes).unwrap(); + let _profiler = dhat::Profiler::builder().testing().build(); + let why = read_window(&p, "k", None, 1, None).err().unwrap().to_string(); + let peak = dhat::HeapStats::get().max_bytes; + let _ = std::fs::remove_file(p); + assert!(why.contains("damaged"), "{why}"); + eprintln!("peak heap {peak} bytes"); + assert!(peak < 4 << 20, "peak heap {peak} bytes: the output was reserved before the strips were checked"); +} diff --git a/crates/denoise/src/reference.rs b/crates/denoise/src/reference.rs index ef58b5b9c..488e9a7b7 100644 --- a/crates/denoise/src/reference.rs +++ b/crates/denoise/src/reference.rs @@ -4,6 +4,12 @@ use crate::net::{Net, NetError, Op, TensorId}; use crate::run::{Error, TileRunner}; +/// The tensors' size cap: the same one the real runner's buffers share (`crates/denoise/src/cpu.rs`). A hostile `Net` +/// only ever has to satisfy the final shape: a `ConvT2` doubles the sides and a `MaxPool` halves them, so a small +/// accepted graph can zoom its intermediates far out and back. Every tensor is counted here, and a run whose layers +/// would retain more than one real tile's whole working set is refused before the memory behind it is reserved. +const MAX_VALUES: usize = 128 * 1024 * 1024; + /// A tensor in `[channels][side][side]` order. struct T { channels: usize, @@ -11,12 +17,36 @@ struct T { data: Vec, } +/// A tensor's plain float count, in bounds. +fn count(channels: usize, side: usize) -> Result { + channels + .checked_mul(side) + .and_then(|n| n.checked_mul(side)) + .filter(|&n| n > 0 && n <= MAX_VALUES) + .ok_or_else(|| NetError::Invalid("a tensor has no size or is too large".into())) +} + +/// A zeroed tensor, keeping the memory the run keeps alive in bounds (`tensors` retains every layer's result). +fn tensor(channels: usize, side: usize, total: &mut usize) -> Result, NetError> { + let n = count(channels, side)?; + *total = (*total).saturating_add(n); + if *total > MAX_VALUES { + return Err(NetError::Invalid("the tile needs too much working memory".into())); + } + let mut data = Vec::new(); + data.try_reserve_exact(n).map_err(|_| NetError::Invalid("not enough memory for a reference tile".into()))?; + data.resize(n, 0.0); + Ok(data) +} + /// Run `net` on `input` (`in_channels` planes of `tile × tile`). pub fn run(net: &Net, tile: usize, input: &[f32]) -> Result, NetError> { if !net.fits_tile(tile) || input.len() != net.in_channels * tile * tile { return Err(NetError::Invalid("the tile does not fit the network".into())); } - let mut tensors: Vec = vec![T { channels: net.in_channels, side: tile, data: input.to_vec() }]; + let input = input.to_vec(); + let mut total = input.len(); + let mut tensors = vec![T { channels: net.in_channels, side: tile, data: input }]; let get = |tensors: &[T], id: TensorId| -> Result { if id < tensors.len() { Ok(id) } else { Err(NetError::Invalid("a layer reads a tensor that does not exist yet".into())) } }; @@ -27,7 +57,7 @@ pub fn run(net: &Net, tile: usize, input: &[f32]) -> Result, NetError> let b = c.src2.map(|s| get(&tensors, s)).transpose()?; let side = tensors[a].side; let pad = (c.k - 1) / 2; - let mut data = vec![0f32; c.cout * side * side]; + let mut data = tensor(c.cout, side, &mut total)?; // the input channels in order: the first source's, then the second's let planes: Vec<&[f32]> = std::iter::once(&tensors[a]).chain(b.map(|b| &tensors[b])).flat_map(|t| t.data.chunks(side * side)).collect(); @@ -56,7 +86,7 @@ pub fn run(net: &Net, tile: usize, input: &[f32]) -> Result, NetError> let a = get(&tensors, c.src)?; let side = tensors[a].side; let out_side = 2 * side; - let mut data = vec![0f32; c.cout * out_side * out_side]; + let mut data = tensor(c.cout, out_side, &mut total)?; for co in 0..c.cout { for y in 0..side { for x in 0..side { @@ -77,7 +107,7 @@ pub fn run(net: &Net, tile: usize, input: &[f32]) -> Result, NetError> Op::MaxPool2 { src } => { let a = get(&tensors, *src)?; let (channels, side) = (tensors[a].channels, tensors[a].side / 2); - let mut data = vec![0f32; channels * side * side]; + let mut data = tensor(channels, side, &mut total)?; for ch in 0..channels { for y in 0..side { for x in 0..side { @@ -92,7 +122,7 @@ pub fn run(net: &Net, tile: usize, input: &[f32]) -> Result, NetError> let a = get(&tensors, *src)?; let (channels, side) = (tensors[a].channels / 4, tensors[a].side); let out_side = 2 * side; - let mut data = vec![0f32; channels * out_side * out_side]; + let mut data = tensor(channels, out_side, &mut total)?; for ch in 0..channels { for y in 0..side { for x in 0..side { @@ -131,3 +161,39 @@ impl TileRunner for ReferenceRunner { run(&self.net, self.tile, input).map_err(|e| Error::Runtime(e.to_string())) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::net::ConvT2; + + fn convt(src: TensorId, cin: usize, cout: usize) -> Op { + Op::ConvT2(ConvT2 { src, cin, cout, weight: vec![0.1; cin * cout * 4], bias: vec![0.0; cout], leaky: None }) + } + + /// A graph that fits the final-shape check while zooming its intermediates out and back: 256 `ConvT2`s double + /// the sides (256 tiers), 255 `MaxPool`s halve them back to the layer above the input. + fn hostile() -> Net { + let mut ops = Vec::new(); + let mut last = 0usize; + for _ in 0..256 { + ops.push(convt(last, 1, 1)); + last += 1; + } + for _ in 0..255 { + ops.push(Op::MaxPool2 { src: last }); + last += 1; + } + Net::new(1, ops).unwrap() + } + + /// The zoomed-out layers' memory was never reserved: either one tensor or the retained whole (the two bounds + /// share the cap) says why the run stops. + #[test] + fn a_graph_that_zooms_its_intermediates_out_and_back_is_refused_before_their_memory_is_reserved() { + assert!(matches!( + run(&hostile(), 1, &[1.0]), + Err(NetError::Invalid(why)) if why.contains("too large") || why.contains("too much working memory") + )); + } +} diff --git a/crates/merge/src/lib.rs b/crates/merge/src/lib.rs index b5ccd4bb8..ca7f91821 100644 --- a/crates/merge/src/lib.rs +++ b/crates/merge/src/lib.rs @@ -34,6 +34,8 @@ pub use pano::{PanoOptions, PanoResult, Projection, stitch}; pub enum MergeError { #[error("{0} photo(s) selected; at least {1} are needed")] TooFew(usize, usize), + #[error("{0} photo(s) selected; a stitch of more than {1} is refused: matching every pair would take far too long")] + TooMany(usize, usize), #[error("{0}")] Mismatch(String), #[error("could not read a photo: {0}")] diff --git a/crates/merge/src/pano/camera.rs b/crates/merge/src/pano/camera.rs index 7625170c3..4589f88db 100644 --- a/crates/merge/src/pano/camera.rs +++ b/crates/merge/src/pano/camera.rs @@ -61,10 +61,12 @@ pub struct PairMatch { /// Match every pair of images. `feats[k]` are in work coordinates scaled by `scales[k]`. pub fn match_pairs(feats: &[Features], scales: &[f64]) -> Vec { let n = feats.len(); - let pairs: Vec<(usize, usize)> = (0..n).flat_map(|i| (i + 1..n).map(move |j| (i, j))).collect(); - let mut out: Vec = pairs - .par_iter() - .filter_map(|&(i, j)| { + // the pairs are generated lazily: materialising all n(n−1)/2 up front is quadratic memory, including + // when every comparison comes up empty (no usable features) + let mut out: Vec = (0..n) + .into_par_iter() + .flat_map_iter(|i| ((i + 1)..n).map(move |j| (i, j))) + .filter_map(|(i, j)| { let m = features::match_features(&feats[i], &feats[j], 0.8); if m.len() < 12 { return None; diff --git a/crates/merge/src/pano/mod.rs b/crates/merge/src/pano/mod.rs index 5ce439681..ba7e1c63f 100644 --- a/crates/merge/src/pano/mod.rs +++ b/crates/merge/src/pano/mod.rs @@ -53,6 +53,11 @@ impl Default for PanoOptions { /// The largest panorama, in megapixels (Lightroom's limit too); a bigger one is scaled down. pub const MAX_MEGAPIXELS: f64 = 512.0; +/// Most frames one stitch accepts: pair matching compares every pair of frames (n(n−1)/2 runs), so an +/// unbounded frame count is quadratic CPU as well as quadratic memory while the frames load. A panorama +/// with more than [`MAX_FRAMES`] shots is refused with an error, not attempted. +pub const MAX_FRAMES: usize = 256; + /// The longest edge of a panorama, in pixels (Lightroom's limit too); a longer one is scaled down. pub const MAX_EDGE: f64 = 65_000.0; @@ -224,6 +229,9 @@ pub fn stitch(mut frames: Vec, opts: &PanoOptions, progress: &Progress) - if n < 2 { return Err(MergeError::TooFew(n, 2)); } + if n > MAX_FRAMES { + return Err(MergeError::TooMany(n, MAX_FRAMES)); + } check(progress, 0.01, "Finding features")?; let (cams, rms_before, rms) = register(&frames, progress)?; let used: Vec = (0..n).filter(|&i| cams[i].is_some()).collect(); diff --git a/crates/merge/tests/it/pano.rs b/crates/merge/tests/it/pano.rs index fdb736524..aeecde64c 100644 --- a/crates/merge/tests/it/pano.rs +++ b/crates/merge/tests/it/pano.rs @@ -246,6 +246,18 @@ fn unrelated_photos_do_not_stitch() { assert!(stitch(frames, &PanoOptions::default(), &no_progress).is_err()); } +/// Pair matching compares every pair of frames before anything is known about them: an unbounded +/// photo count is quadratic CPU and memory (issue #1519). More than `MAX_FRAMES` is refused at once, +/// before any frame is worked on. +#[test] +fn a_stitch_of_more_than_the_frame_cap_is_refused_up_front() { + let frames: Vec = (0..lightcraft_merge::pano::MAX_FRAMES + 1) + .map(|i| Frame::from_linear_rec2020(Rgb32f::from_fn(8, 8, |_, _| [(i % 255) as f32 / 255.0, 0.0, 0.0]), None)) + .collect(); + let err = stitch(frames, &PanoOptions::default(), &no_progress).unwrap_err(); + assert!(err.to_string().contains("refused"), "the frame cap errors, it does not begin the quadratic match: {err}"); +} + /// Issue #1455: a panorama came out at 40 MP whatever its photos' resolution. It now keeps their /// own resolution unless a size is asked for (up to Lightroom's 512 MP); a size scales it down, /// and a NaN one (from an agent) is no size at all, not a 0.1 MP thumbnail. diff --git a/crates/meta/src/exif.rs b/crates/meta/src/exif.rs index f24bb5f2f..adcba0f80 100644 --- a/crates/meta/src/exif.rs +++ b/crates/meta/src/exif.rs @@ -1,7 +1,7 @@ //! Exif / TIFF tags → [`Metadata`] (CIPA DC-008 Exif 2.32, TIFF 6.0, DNG 1.7 tag semantics). use crate::{DateTime, Flash, Gps, Metadata, Orientation}; -use lightcraft_tiff::{Ifd, Tiff, TiffError, tags as t}; +use lightcraft_tiff::{Ifd, ParseOptions, Tiff, TiffError, tags as t}; /// Strip an `Exif\0\0` (or `Exif\0\xff`) prefix as found in JPEG APP1 and some WebP `EXIF` chunks. pub fn strip_exif_header(b: &[u8]) -> &[u8] { @@ -16,8 +16,16 @@ pub fn read_exif(bytes: &[u8]) -> Metadata { /// Like [`read_exif`] but reports a structurally invalid TIFF header / IFD0. pub fn try_read_exif(bytes: &[u8]) -> Result { + try_read_exif_with(bytes, &ParseOptions::default()) +} + +/// [`try_read_exif`] with the caller's parser options (its value allocation budget in particular). +/// A decoder working from a sparse buffer — only selected ranges of the logical file are actually +/// loaded, the rest is zeros — must not size its allocations by the logical file length, which +/// unrelated bytes of the file can inflate at no cost to the attacker. +pub fn try_read_exif_with(bytes: &[u8], opts: &ParseOptions) -> Result { let data = strip_exif_header(bytes); - let tiff = Tiff::parse(data)?; + let tiff = Tiff::parse_with(data, opts)?; let mut m = from_tiff(&tiff); // Panasonic writes no LensModel, only the name in its maker note (and in a RW2 that note is not in the raw's own Exif) if m.lens_model.is_none() { diff --git a/crates/meta/src/focus_read.rs b/crates/meta/src/focus_read.rs index 5a5d349d1..65adbee75 100644 --- a/crates/meta/src/focus_read.rs +++ b/crates/meta/src/focus_read.rs @@ -171,7 +171,15 @@ fn tiff(r: &mut Reader, head: Vec) -> io::Result> { loaded.insert(0, head.len() as u64); for _ in 0..MAX_ROUNDS { match plan(&buf, &loaded) { - Step::Ready => return Ok(crate::try_read_exif(&buf).ok().and_then(|m| m.focus)), + Step::Ready => { + // The decoder's allocation budget follows the bytes actually fetched, not the logical + // file length: only the loaded ranges are data, and an unrelated entry that claims a + // huge value must be cut by what leaving it cost, not by what the buffer claims to hold. + // The same 8 × / + 1 MiB shape the general TIFF reader uses, over `reader.bytes` instead. + let opts = + lightcraft_tiff::ParseOptions { value_budget: Some(r.bytes.saturating_mul(8).saturating_add(1 << 20)), ..Default::default() }; + return Ok(crate::try_read_exif_with(&buf, &opts).ok().and_then(|m| m.focus)); + } Step::Nothing => return Ok(None), Step::Need(ranges) => { for (a, b) in ranges { diff --git a/crates/meta/src/lib.rs b/crates/meta/src/lib.rs index baebbfe0f..46c7a1043 100644 --- a/crates/meta/src/lib.rs +++ b/crates/meta/src/lib.rs @@ -27,7 +27,7 @@ mod xmp_merge; pub use container::{Embedded, embedded, jpeg_segments, png_chunks, webp_chunks}; pub use datetime::DateTime; -pub use exif::{from_tiff, read_exif, strip_exif_header, try_read_exif, write_exif}; +pub use exif::{from_tiff, read_exif, strip_exif_header, try_read_exif, try_read_exif_with, write_exif}; pub use focus::FocusPoint; pub use gpx::{GpxError, Match, TrackPoint, Tracklog, parse_gpx}; pub use iptc::parse_iptc; diff --git a/crates/meta/src/xmp.rs b/crates/meta/src/xmp.rs index 2dbf59482..d2f6f8ee2 100644 --- a/crates/meta/src/xmp.rs +++ b/crates/meta/src/xmp.rs @@ -154,6 +154,11 @@ const MAX_NODES: usize = 200_000; // Expanding a long namespace URI on many short names must not amplify a small packet without bound. pub(crate) const MAX_OPAQUE_NAME_BYTES: usize = 8 * 1024 * 1024; +/// Most text a converter's flattened property keys may add up to. Keys repeat their whole ancestor +/// path (`parent/field`), so a document with a long nested name and a wide child fan-out otherwise +/// retains its prefix once per property: node limits alone cannot bound that duplication. +const MAX_KEY_BYTES: usize = 1 << 20; + /// Names canonicalised by namespace URI (see [`NAMESPACES`]); conflicting foreign prefixes are opaque. pub(crate) fn canonical_name(qname: &str, scopes: &[Vec<(String, String)>], opaque_budget: &mut usize) -> Result { canonical(qname, scopes, opaque_budget) @@ -332,18 +337,29 @@ fn is_rdf_meta_attr(k: &str) -> bool { matches!(k, "rdf:about" | "rdf:ID" | "rdf:nodeID" | "xml:lang" | "rdf:parseType" | "rdf:resource" | "rdf:datatype") || k.starts_with("xmlns") } -fn collect_description(d: &Node, prefix: &str, props: &mut BTreeMap>) { +fn collect_description(d: &Node, prefix: &str, props: &mut BTreeMap>, budget: &mut usize) { for (k, v) in &d.attrs { - if !is_rdf_meta_attr(k) { + if !is_rdf_meta_attr(k) && take_key_bytes(&format!("{prefix}{k}"), budget) { props.entry(format!("{prefix}{k}")).or_default().push(v.clone()); } } for c in &d.children { - collect_property(c, prefix, props); + collect_property(c, prefix, props, budget); } } -fn collect_property(p: &Node, prefix: &str, props: &mut BTreeMap>) { +/// Charge one flattened key's length to the key budget; `false` when the budget is spent (the +/// property is then left out, like other elements a limit has cut). +fn take_key_bytes(key: &str, budget: &mut usize) -> bool { + let n = key.len(); + if *budget < n { + return false; + } + *budget -= n; + true +} + +fn collect_property(p: &Node, prefix: &str, props: &mut BTreeMap>, budget: &mut usize) { let key = format!("{prefix}{}", p.name); if let Some(arr) = p.children.iter().find(|c| matches!(c.name.as_str(), "rdf:Seq" | "rdf:Bag" | "rdf:Alt")) { let mut items: Vec<(bool, String)> = arr @@ -357,29 +373,36 @@ fn collect_property(p: &Node, prefix: &str, props: &mut BTreeMap = p.attrs.iter().filter(|(k, _)| !is_rdf_meta_attr(k)).collect(); if !field_attrs.is_empty() && p.children.is_empty() && p.text.trim().is_empty() { for (k, v) in field_attrs { - props.entry(format!("{struct_prefix}{k}")).or_default().push(v.clone()); + let full = format!("{struct_prefix}{k}"); + if take_key_bytes(&full, budget) { + props.entry(full).or_default().push(v.clone()); + } } return; } let v = p.attr("rdf:resource").map(str::to_string).unwrap_or_else(|| p.text.trim().to_string()); - props.entry(key).or_default().push(v); + if take_key_bytes(&key, budget) { + props.entry(key).or_default().push(v); + } } fn find_descriptions<'a>(n: &'a Node, out: &mut Vec<&'a Node>) { @@ -498,7 +521,8 @@ pub fn parse_xmp(s: &str) -> Result { let mut props = BTreeMap::new(); let mut values = BTreeMap::new(); for d in descs { - collect_description(d, "", &mut props); + let mut key_budget = MAX_KEY_BYTES; + collect_description(d, "", &mut props, &mut key_budget); if let XmpValue::Struct(m) = node_struct(d) { values.extend(m); } @@ -1171,4 +1195,29 @@ mod tests { assert_eq!(entity("#x41"), Some('A')); assert_eq!(entity("#9"), Some('\t')); } + + /// Flattened keys repeat their whole ancestor path, so a long nested name with a wide child + /// fan-out would otherwise retain its prefix once per leaf: the cumulative key budget stops it + /// (a 65 KB document with 3000 leaves must not retain ~2.4 MB of key text). + #[test] + fn flattened_keys_stay_within_the_cumulative_budget() { + let level = "n".repeat(200); + let mut doc = String::from( + "", + ); + for _ in 0..4 { + doc.push_str(&format!("")); + } + doc.push_str(&"".repeat(3000)); + doc.push_str(""); + for _ in 0..4 { + doc.push_str(&format!("")); + } + doc.push_str(""); + let d = parse_xmp(&doc).unwrap(); + let total: usize = d.properties.keys().map(String::len).sum(); + assert!(total <= MAX_KEY_BYTES, "flattened-key text {total} is over the {MAX_KEY_BYTES} budget"); + assert!(d.properties.len() < 3000, "keys stop being added once the budget is spent"); + assert!(!d.properties.is_empty(), "properties parsed before the budget are kept"); + } } diff --git a/crates/raw/src/tiffraw.rs b/crates/raw/src/tiffraw.rs index 8cd6a13e2..34bd7e53b 100644 --- a/crates/raw/src/tiffraw.rs +++ b/crates/raw/src/tiffraw.rs @@ -105,48 +105,72 @@ pub fn read_image(data: &[u8], info: &ImageInfo, order: ByteOrder, packing: Pack let chunks = info.chunks(data.len() as u64); let planar = info.planar == 2 && cpp > 1; let ccpp = if planar { 1 } else { cpp }; - let decoded: Vec> = - chunks.par_iter().map(|c| decode_chunk(data, info, c, order, packing, ccpp, bits, float).map(|p| (*c, p))).collect(); + // The chunks are decoded and copied into the output in groups whose combined nominal size stays within + // a few times the output buffer's: everything decoded at once would retain the whole image twice, and + // a hostile tile table's chosen padding could stretch that much further (issue #1517). One group + // still decodes in parallel; the grid order groups make, the copies stay disjoint. + let group_samples = (total / 4).max(1 << 20); let mut out_u16 = if float { Vec::new() } else { vec![0u16; total] }; let mut out_f32 = if float { vec![0f32; total] } else { Vec::new() }; - for r in decoded { - // a chunk that cannot be decoded is not a partial image: the file stays preview-only - let (c, px) = r?; - let (cw, ch) = (c.width as usize, c.height as usize); - let (x0, y0) = (c.x as usize, c.y as usize); - if x0 >= w || y0 >= h { - continue; + let mut decode_from = 0usize; + while decode_from < chunks.len() { + let mut to = decode_from; + let mut taken = 0usize; + for (k, c) in chunks[decode_from..].iter().enumerate() { + let n = chunk_samples(c, ccpp); + if taken > 0 && taken + n > group_samples { + break; + } + taken = taken.saturating_add(n); + to = decode_from + k + 1; } - let copy_w = cw.min(w - x0); - let copy_h = ch.min(h - y0); - for y in 0..copy_h { - for x in 0..copy_w { - for s in 0..ccpp { - let src = (y * cw + x) * ccpp + s; - let dst_s = if planar { c.plane as usize } else { s }; - if dst_s >= cpp { - continue; - } - let dst = ((y0 + y) * w + x0 + x) * cpp + dst_s; - match &px { - ChunkPx::U16(v) => { - if let (Some(o), Some(&s)) = (out_u16.get_mut(dst), v.get(src)) { - *o = s; - } + let decoded: Vec> = + chunks[decode_from..to].par_iter().map(|c| decode_chunk(data, info, c, order, packing, ccpp, bits, float).map(|p| (*c, p))).collect(); + for r in decoded { + // a chunk that cannot be decoded is not a partial image: the file stays preview-only + let (c, px) = r?; + let (cw, ch) = (c.width as usize, c.height as usize); + let (x0, y0) = (c.x as usize, c.y as usize); + if x0 >= w || y0 >= h { + continue; + } + let copy_w = cw.min(w - x0); + let copy_h = ch.min(h - y0); + for y in 0..copy_h { + for x in 0..copy_w { + for s in 0..ccpp { + let src = (y * cw + x) * ccpp + s; + let dst_s = if planar { c.plane as usize } else { s }; + if dst_s >= cpp { + continue; } - ChunkPx::F32(v) => { - if let (Some(o), Some(&s)) = (out_f32.get_mut(dst), v.get(src)) { - *o = s; + let dst = ((y0 + y) * w + x0 + x) * cpp + dst_s; + match &px { + ChunkPx::U16(v) => { + if let (Some(o), Some(&s)) = (out_u16.get_mut(dst), v.get(src)) { + *o = s; + } + } + ChunkPx::F32(v) => { + if let (Some(o), Some(&s)) = (out_f32.get_mut(dst), v.get(src)) { + *o = s; + } } } } } } } + decode_from = to; } Ok(if float { RawData::F32(out_f32) } else { RawData::U16(out_u16) }) } +/// A chunk's nominal decoded size in samples (per plane, as the decoder builds it). +fn chunk_samples(c: &Chunk, ccpp: usize) -> usize { + (c.width as usize).saturating_mul(c.height as usize).saturating_mul(ccpp) +} + #[allow(clippy::too_many_arguments)] fn decode_chunk(data: &[u8], info: &ImageInfo, c: &Chunk, order: ByteOrder, packing: Packing, cpp: usize, bits: u32, float: bool) -> Result { let src = chunk_bytes(data, c).ok_or_else(|| RawError::Corrupt("chunk offset past end of file".into()))?; @@ -180,8 +204,12 @@ fn decode_chunk(data: &[u8], info: &ImageInfo, c: &Chunk, order: ByteOrder, pack comp::LOSSY_JPEG => { // lossy DNG: each chunk is a baseline (DCT) JPEG of 8-bit samples let (px, jw, jh, jc) = lossy_jpeg(src, cpp, MAX_JPEG_SAMPLES)?; - let mut v = vec![0u16; n]; - for y in 0..ch.min(jh) { + // the buffer holds the rows the decoded JPEG covers (the caller zero-fills the tile's + // remainder and reads only rows it fills): a tile-many-larger decoded JPEG does not + // get the nominal tile's full buffer (issue #1517) + let rows = ch.min(jh); + let mut v = vec![0u16; rows.max(1).saturating_mul(cw).saturating_mul(cpp)]; + for y in 0..rows { for x in 0..cw.min(jw) { for k in 0..cpp { v[(y * cw + x) * cpp + k] = px[(y * jw + x) * jc + k.min(jc - 1)] as u16; @@ -518,6 +546,36 @@ mod lossy_tests { assert!(lossy_jpeg(&j, 3, MAX_JPEG_SAMPLES).is_ok() || matches!(lossy_jpeg(&j, 3, MAX_JPEG_SAMPLES), Err(RawError::Corrupt(_)))); } + /// A lossy tile whose JPEG decodes far less than the nominal tile is fine: the rows the JPEG + /// covers hold its pixels, the rest of the output stays zero (and the tile's intermediate buffer + /// is sized by the JPEG, not by the nominal tile). + #[test] + fn a_small_lossy_jpeg_in_a_large_nominal_tile_decodes_with_zeros_below() { + let px: Vec = (0..8 * 8).flat_map(|_| [30u8, 90, 150]).collect(); + let mut j = Vec::new(); + jpeg_encoder::Encoder::new(&mut j, 100).encode(&px, 8, 8, jpeg_encoder::ColorType::Rgb).unwrap(); + let info = ImageInfo { + width: 16, + height: 16, + bits_per_sample: vec![8, 8, 8], + samples_per_pixel: 3, + compression: comp::LOSSY_JPEG, + photometric: 34892, + planar: 1, + predictor: 1, + sample_format: 1, + new_subfile_type: 0, + layout: Layout::Tiles { tile_width: 16, tile_height: 16 }, + offsets: vec![0], + byte_counts: vec![j.len() as u64], + }; + let RawData::U16(v) = read_image(&j, &info, ByteOrder::Little, Packing::Msb).unwrap() else { panic!("integer samples") }; + assert_eq!(v.len(), 16 * 16 * 3, "the output is the full nominal image"); + let px = |x: usize, y: usize| &v[(y * 16 + x) * 3..(y * 16 + x) * 3 + 3]; + assert!((px(3, 3)[0] as i32 - 30).abs() <= 3 && (px(3, 3)[2] as i32 - 150).abs() <= 3, "covered rows: {:?}", px(3, 3)); + assert!(px(3, 12).iter().all(|&s| s == 0), "rows below the JPEG stay zero: {:?}", px(3, 12)); + } + /// A deflated tile is refused when its packed bytes are too few for deflate to have produced /// the nominal chunk, or when the stream stops short of the rows inside the image; a fully /// covered flat tile still decodes. diff --git a/crates/tiff/src/makernote.rs b/crates/tiff/src/makernote.rs index 7ed5e1997..288b366f6 100644 --- a/crates/tiff/src/makernote.rs +++ b/crates/tiff/src/makernote.rs @@ -55,7 +55,11 @@ pub fn parse_makernote(data: &[u8], offset: u64, len: u64, order: ByteOrder, mak Some(b"MM") => Some(ByteOrder::Big), _ => None, }; - let opts = ParseOptions { max_ifds: 64, max_depth: 3, ..Default::default() }; + // A fixed value budget: a maker note is at most tens of kilobytes of values in any honest file, and + // computing the budget from the buffer's length multiplies the file length (a hostile entry in a + // sparse header-read buffer, where the buffer holds the file's logical size but only a few + // kilobytes are loaded, would decode allocations no actual data backs). + let opts = ParseOptions { max_ifds: 64, max_depth: 3, value_budget: Some(1 << 20), ..Default::default() }; let try_parse = |kind: MakerNoteKind, ifd_at: u64, base: u64, ord: ByteOrder| -> Option { let (ifd, _) = parse_ifd_at(data, ifd_at, ord, base, false, &opts).ok()?; (!ifd.entries.is_empty()).then_some(MakerNote { kind, order: ord, base, ifd }) @@ -192,6 +196,35 @@ mod tests { assert_eq!(m.ifd.u64s(2).unwrap(), vec![9, 0]); } + /// A maker note's value budget is fixed, not computed from the length of the buffer it sits in: + /// an entry claiming more than the note budget (here 2 MB) is dropped, a fitting one is read. In a + /// sparse header read the buffer holds the file's logical size while only kilobytes were loaded, + /// so a length-scaled budget would let an unrelated entry decode megabytes of zeros (see the focus + /// reader, try_read_exif_with). + #[test] + fn a_note_entry_past_the_fixed_value_budget_is_dropped() { + let order = ByteOrder::Little; + let mut note = b"SONY DSC \0\0\0".to_vec(); + // the Sony IFD directly after the 12-byte header, one fitting BYTE entry and a decoy + // BYTE[2 MB] entry pointing into the file's own (zero-filled) body + order.put_u16(&mut note, 2); + order.put_u16(&mut note, 0x201b); + order.put_u16(&mut note, 1); + order.put_u32(&mut note, 1); + note.extend([3, 0, 0, 0]); + order.put_u16(&mut note, 0x9900); + order.put_u16(&mut note, 1); + order.put_u32(&mut note, 2 << 20); + order.put_u32(&mut note, 200); + order.put_u32(&mut note, 0); + let mut bytes = note.clone(); + bytes.resize(200 + (2 << 20), 0); // the decoy's value range is inside the real bytes + let m = parse_makernote(&bytes, 0, note.len() as u64, order, "SONY").unwrap(); + assert_eq!(m.kind, MakerNoteKind::Sony); + assert!(m.ifd.get(0x201b).is_some(), "the fitting entry is read"); + assert!(m.ifd.get(0x9900).is_none(), "the decoy is dropped by the value budget"); + } + #[test] fn garbage_note_is_none() { let (bytes, off, len) = with_note(ByteOrder::Little, vec![0xff; 40]); diff --git a/crates/tiff/src/reader.rs b/crates/tiff/src/reader.rs index bd728ca9c..70c66c5a7 100644 --- a/crates/tiff/src/reader.rs +++ b/crates/tiff/src/reader.rs @@ -338,6 +338,25 @@ pub fn decode(o: ByteOrder, typ: FieldType, b: &[u8]) -> Value { #[cfg(test)] mod tests { use super::*; + use crate::Value; + use crate::tags as t; + use crate::writer::{IfdBuilder, TiffWriter}; + + /// A value budget the caller sets bounds what is decoded: an entry claiming more value bytes than + /// the budget allows is dropped, entries that fit are read (the sparse header-read path, where the + /// buffer holds the file's logical size but only a few kilobytes were ever loaded, sizes its + /// budget from what was fetched, not from whatever length the file claims). + #[test] + fn the_value_budget_drops_over_budget_entries_and_keeps_the_rest() { + let mut ifd0 = IfdBuilder::new(); + ifd0.set(t::MAKE, Value::Ascii("Keep".into())); + ifd0.set(0x9000, Value::Byte(vec![0; 2 << 20])); + let file = TiffWriter { order: ByteOrder::Little, bigtiff: false }.write(&[ifd0]).unwrap(); + let opts = ParseOptions { value_budget: Some(1 << 20), ..Default::default() }; + let tiff = Tiff::parse_with(&file, &opts).unwrap(); + assert_eq!(tiff.ifds[0].string(t::MAKE), Some("Keep".to_string()), "a fitting entry is read"); + assert!(tiff.ifds[0].get(0x9000).is_none(), "the over-budget entry is dropped"); + } /// A BigTIFF whose first IFD, at 80, declares `declared` entries but holds one, followed by a next-IFD pointer /// `next`; a valid one-entry IFD sits at 16.