collective
Browse / github.com/storytold/lightcraft / patch
Never crash: CR2 slice widths that overflow are an error; XMP attribute unescape in one pass
Never crash: CR2 slice widths that overflow are an error; XMP attribute unescape in one pass ## Problem - A corrupt Canon CR2 whose slice-width tag (0xc640) holds huge values made `widths.iter().sum()` wrap. The wrapped width can still divide the frame, and the slice copy then indexes outside the buffer: the app panics on opening the file instead of reporting it as corrupt. - `unescape_attr` in the XMP reader searched the rest of the attribute value for `;` at every `&`, so a large value full of `&` took quadratic time (a sidecar that stalls import). ## Change - `crates/raw/src/vendor/cr2.rs`: checked sum of the slice widths (`RawError::Corrupt` on overflow) and slice copies through
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- emmertex (imported from pull request #3007, open)
- Base
387bed199fca44d76af5aa1c24dd7cc3de4c8db2- Size
- +61 −9 in 3 files
- Review
- pending
- Shared
- 2026-10-28
- Id
90408115a4ae2031f300ddf2
How it went
No other operator has reported using it yet.
Diff
raw · crates/meta/src/xmp.rs crates/raw/src/vendor/cr2.rs crates/raw/tests/it/vendor_robust.rs
@@ -234,7 +234,18 @@ pub(crate) fn unescape_attr(s: &str) -> String { while let Some(i) = rest.find('&') { out.push_str(&rest[..i]); rest = &rest[i..];- match rest.find(';').and_then(|j| entity(&rest[1..j]).map(|c| (c, j))) {+ // the longest reference is `` plus leading zeros: skip the zeros (each byte once, since a+ // miss resumes after the `&`), then search a short window, not the rest of the value per `&`+ let b = rest.as_bytes();+ let digits = match (b.get(1), b.get(2)) {+ (Some(b'#'), Some(b'x' | b'X')) => 3,+ (Some(b'#'), _) => 2,+ _ => 1,+ };+ let zeros = b.iter().skip(digits).take_while(|&&c| c == b'0').count();+ let skip = digits + zeros.saturating_sub(1);+ let end = b.iter().skip(skip).take(12).position(|&c| c == b';').map(|j| j + skip);+ match end.and_then(|j| entity(&rest[1..j]).map(|c| (c, j))) { Some((c, j)) => { out.push(c); rest = &rest[j + 1..];@@ -741,6 +752,24 @@ pub fn write_xmp_lc(meta: &Metadata, lc: &[(&str, &str)]) -> String { mod tests { use super::*; + /// References resolve, anything else (unknown, unterminated, or a `;` too far away to end one) stays as+ /// written, and a value that is nothing but `&` is handled in one pass.+ #[test]+ fn attribute_references_resolve_and_hostile_ones_stay_verbatim() {+ assert_eq!(unescape_attr("a&bAB<>"'"), "a&bAB<>\"'");+ assert_eq!(unescape_attr("&bogus; & &#xZZ; tail&"), "&bogus; & &#xZZ; tail&");+ let far = format!("&{};", "x".repeat(40));+ assert_eq!(unescape_attr(&far), far);+ let many = "&".repeat(1 << 20);+ assert_eq!(unescape_attr(&many), many);+ // multi-byte text around a reference is never cut inside a character+ assert_eq!(unescape_attr("é&日&é;"), "é&日&é;");+ // leading zeros are valid in numeric references, however many there are+ assert_eq!(unescape_attr("AB"), "AB");+ let zeros = format!("&#{};", "0".repeat(1 << 20));+ assert_eq!(unescape_attr(&zeros), zeros);+ }+ fn full() -> Metadata { Metadata { // write_xmp doesn't emit regions yet (read-only interchange; see docs/xmp-interop.md), so the@@ -226,16 +226,19 @@ pub(crate) fn decode(bytes: &[u8], mode: Mode) -> Result<RawImage> { let widths: Vec<usize> = match &slices { Some(s) => { let n = s[0].min(64) as usize;- let mut v = vec![s[1] as usize; n];- v.push(s[2] as usize);+ // `as usize` would truncate a 64-bit width on 32-bit targets (wasm32, x86) and hide the overflow+ let width = |w: u64| usize::try_from(w).map_err(|_| RawError::Corrupt("CR2 slice widths overflow".into()));+ let mut v = vec![width(s[1])?; n];+ v.push(width(s[2])?); v } None => { let w = (fw as u64).checked_mul(nc as u64).ok_or_else(|| RawError::Corrupt("bad CR2 frame size".into()))?;- vec![w as usize]+ vec![usize::try_from(w).map_err(|_| RawError::Corrupt("bad CR2 frame size".into()))?] } };- let width: usize = widths.iter().sum();+ // slice widths come from the file: a sum that overflows is a corrupt file, not a wrapped width+ let width = widths.iter().try_fold(0usize, |acc, &w| acc.checked_add(w)).ok_or_else(|| RawError::Corrupt("CR2 slice widths overflow".into()))?; if width == 0 || total % width != 0 { return Err(RawError::Corrupt(format!("CR2 slices ({width}) do not divide the frame ({total} samples)"))); }@@ -247,7 +250,11 @@ pub(crate) fn decode(bytes: &[u8], mode: Mode) -> Result<RawImage> { let mut x0 = 0; for &sw in &widths { for y in 0..height {- data[y * width + x0..y * width + x0 + sw].copy_from_slice(&frame.data[i..i + sw]);+ // sums to `width`, so these are in range; `get` keeps a surprise an error, not a panic+ let (Some(dst), Some(src)) = (data.get_mut(y * width + x0..y * width + x0 + sw), frame.data.get(i..i + sw)) else {+ return Err(RawError::Corrupt("CR2 slices outside the frame".into()));+ };+ dst.copy_from_slice(src); i += sw; } x0 += sw;@@ -145,16 +145,21 @@ fn pef(compression: u16) -> Vec<u8> { /// A Canon CR2: three placeholder IFDs and IFD3 with a 2-component lossless JPEG frame. fn cr2() -> Vec<u8> {+ cr2_with_slices(Value::Short(vec![1, 16, 16]), false)+}++/// The same CR2 with its slice-width tag (0xc640) replaced, optionally in a BigTIFF (for 64-bit widths).+fn cr2_with_slices(slices: Value, bigtiff: bool) -> Vec<u8> { let img: Vec<u16> = (0..(W * H) as usize).map(|i| 1024 + (i * 7919 % 12000) as u16).collect(); let enc = lightcraft_raw::ljpeg::encode(&img, W as usize / 2, H as usize, 2, 14, 1, 0); let mut ifd0 = IfdBuilder::new(); ifd0.set(t::MAKE, Value::Ascii("Canon".into())); let mut ifd3 = IfdBuilder::new(); ifd3.set(t::COMPRESSION, Value::Short(vec![6]));- ifd3.set(0xc640, Value::Short(vec![1, 16, 16]));+ ifd3.set(0xc640, slices); ifd3.set_image(ImageData::Strips { rows_per_strip: H, strips: vec![enc] }); let blank = || IfdBuilder::new().with(1, Value::Short(vec![0]));- TiffWriter::new(ByteOrder::Little, false).write(&[ifd0, blank(), blank(), ifd3]).unwrap()+ TiffWriter::new(ByteOrder::Little, bigtiff).write(&[ifd0, blank(), blank(), ifd3]).unwrap() } /// A Fujifilm RAF: header, records, raw block (LE TIFF whose IFD0 points to a 0xf000 raw IFD).@@ -241,6 +246,8 @@ fn samples() -> Vec<Vec<u8>> { rw2(), cr2(), raf(),+ cr2_with_slices(Value::Long8(vec![64, u64::MAX, u64::MAX]), true),+ cr2_with_slices(Value::Long8(vec![1, 1 << 40, 16]), true), ] .into_iter() .chain(rw2_formats())@@ -283,7 +290,7 @@ proptest! { #![proptest_config(ProptestConfig { cases: 2000, .. ProptestConfig::default() })] #[test]- fn mutated_vendor_files_never_panic(kind in 0usize..18, flips in proptest::collection::vec((any::<usize>(), any::<u8>()), 1..16), cut in any::<usize>()) {+ fn mutated_vendor_files_never_panic(kind in 0usize..20, flips in proptest::collection::vec((any::<usize>(), any::<u8>()), 1..16), cut in any::<usize>()) { let mut data = samples().swap_remove(kind); let n = data.len(); for (i, v) in flips {@@ -302,3 +309,12 @@ fn rw2_samples_decode_unmutated() { assert_eq!((r.width, r.height), (W as usize, H as usize)); } }++/// Slice widths come from the file: widths whose sum overflows (or exceeds the frame) are an error, not a+/// wrapped width that indexes outside the frame.+#[test]+fn cr2_slice_widths_that_overflow_are_an_error() {+ for slices in [vec![64, u64::MAX, u64::MAX], vec![1, 1 << 40, 16], vec![2, u64::MAX, 1]] {+ assert!(lightcraft_raw::decode(&cr2_with_slices(Value::Long8(slices.clone()), true)).is_err(), "{slices:?}");+ }+}