diff --git a/crates/affinity/src/model.rs b/crates/affinity/src/model.rs index e629aa7b..298e32be 100644 --- a/crates/affinity/src/model.rs +++ b/crates/affinity/src/model.rs @@ -296,12 +296,23 @@ const MAX_TREE_DEPTH: usize = 128; /// Total layer nodes imported. const MAX_NODES: usize = 500_000; +/// Pixels decoded from pixel layers over a whole document (2 GB as RGBA). The archive's limits +/// count the bytes extracted from it, and a layer of one flat colour needs none (#1704). +const MAX_LAYER_PIXELS: u64 = 512 << 20; + /// Read a document: archive, `doc.dat` and the model. pub fn read(bytes: &[u8], limits: Limits) -> Result { + read_within(bytes, limits, MAX_LAYER_PIXELS) +} + +/// [`read`], its pixel layers holding at most `pixels` pixels together. +pub(crate) fn read_within(bytes: &[u8], limits: Limits, pixels: u64) -> Result { let mut archive = Archive::open(bytes, limits)?; let doc = archive.read("doc.dat")?; let s = stream::parse(&doc)?; - Reader::new(&s, &mut archive).document() + let mut reader = Reader::new(&s, &mut archive); + reader.pixels_left = pixels; + reader.document() } pub(crate) struct Reader<'s, 'a, 'b> { @@ -311,11 +322,13 @@ pub(crate) struct Reader<'s, 'a, 'b> { warnings: BTreeMap, nodes: usize, active: HashSet, + /// Pixels the pixel layers still to come may decode ([`MAX_LAYER_PIXELS`] in all). + pub(crate) pixels_left: u64, } impl<'s, 'a, 'b> Reader<'s, 'a, 'b> { fn new(s: &'s Stream, archive: &'b mut Archive<'a>) -> Self { - Self { s, archive, dpi: 72.0, warnings: BTreeMap::new(), nodes: 0, active: HashSet::new() } + Self { s, archive, dpi: 72.0, warnings: BTreeMap::new(), nodes: 0, active: HashSet::new(), pixels_left: MAX_LAYER_PIXELS } } pub(crate) fn warn(&mut self, what: impl Into) { diff --git a/crates/affinity/src/raster.rs b/crates/affinity/src/raster.rs index 21ecf2d9..98334717 100644 --- a/crates/affinity/src/raster.rs +++ b/crates/affinity/src/raster.rs @@ -105,10 +105,17 @@ fn decode(r: &mut Reader, bitmap: ObjId, crop: (u32, u32, u32, u32), world: Affi return Ok(None); } let (w, h) = (x1 - x0, y1 - y0); - if u64::from(w) * u64::from(h) > MAX_PIXELS { + let pixels = u64::from(w) * u64::from(h); + if pixels > MAX_PIXELS { r.warn("pixel layers larger than 64 megapixels"); return Ok(None); } + // Before anything is allocated: a layer's tiles can be flat colour, with nothing to extract. + let Some(left) = r.pixels_left.checked_sub(pixels) else { + r.warn("pixel layers beyond the pixels one document may hold"); + return Ok(None); + }; + r.pixels_left = left; let format = s.enumeration(bitmap, b"Frmt").map(|(f, _)| f); let (channels, bps) = match format { Some(0) => (4, 1), @@ -404,6 +411,41 @@ mod tests { } } + /// Pixel layers of one flat colour: nothing in the archive but `doc.dat` (state 2 is a tile + /// of 0xFF). Written by hand for the reader, like [`fixture`]. + fn flat_layers(count: u32, side: i32) -> Vec { + let layer = |i: u32| { + let mut bitmap = vec![(tag(b"Frmt"), F::Enum(0, 0)), (tag(b"BmpW"), F::I32(side)), (tag(b"BmpH"), F::I32(side))]; + for c in 1..=4 { + bitmap.push((tag(&super::tag(b"TWi", c)), F::I32(1))); + bitmap.push((tag(&super::tag(b"THi", c)), F::I32(1))); + bitmap.push((tag(&super::tag(b"Sta", c)), F::U8s(vec![2]))); + } + F::Def(10 + i, vec![tag(b"Rstr")], vec![(tag(b"Bitm"), F::Obj(tag(b"DyBm"), bitmap))]) + }; + let spread = F::Def(1, vec![tag(b"Sprd")], vec![(tag(b"Chld"), F::Shared((0..count).map(layer).collect()))]); + let doc = synth::stream(&[(tag(b"DocR"), F::Obj(tag(b"DocN"), vec![(tag(b"Chld"), F::Shared(vec![spread]))]))]); + synth::container(&[("doc.dat", &doc, Method::Zlib)], None) + } + + /// #1704: the pixels of all pixel layers together are bounded, also when the archive holds + /// no data for them: the layers past the allowance are left out with a note. + #[test] + fn pixel_layers_share_one_allowance_for_the_whole_document() { + let file = flat_layers(5, 16); + let layers = |pixels: u64| { + let doc = crate::model::read_within(&file, crate::Limits::default(), pixels).unwrap(); + let n = doc.spreads[0].nodes.iter().filter(|n| matches!(n.kind, Kind::Image(_))).count(); + (n, doc.warnings) + }; + assert_eq!(layers(5 * 256), (5, vec![])); + let (kept, warnings) = layers(3 * 256 + 255); + assert_eq!(kept, 3); + assert_eq!(warnings.len(), 1); + assert!(warnings[0].contains("pixel layers beyond the pixels one document may hold"), "{warnings:?}"); + assert_eq!(crate::read(&file, crate::Limits::default()).unwrap().spreads[0].nodes.len(), 5); + } + #[test] fn embedded_source_dimensions_and_pixel_budget_are_checked_before_decode() { let bytes = png(); diff --git a/crates/eps/src/import/interp.rs b/crates/eps/src/import/interp.rs index f74b5df0..b60aac39 100644 --- a/crates/eps/src/import/interp.rs +++ b/crates/eps/src/import/interp.rs @@ -29,6 +29,8 @@ pub(crate) const MAX_GSAVE: usize = 1000; const MAX_MEMORY: usize = 1 << 30; pub(crate) struct Interp<'a> { + /// The list of the arrays and dictionaries made since this started ([`super::obj::track`]). + made: u64, pub lex: Lexer<'a>, pub stack: Vec, pub dicts: Vec, @@ -101,6 +103,7 @@ const CATEGORIES: &str = "Category Generic Font CIDFont CMap FontSet Encoding Fo impl<'a> Interp<'a> { pub fn new(src: &'a [u8], g: GState, out: Out) -> Self { + let made = super::obj::track(); let system = new_dict(); { let mut d = system.borrow_mut(); @@ -140,6 +143,7 @@ impl<'a> Interp<'a> { system.borrow_mut().insert(Key::name(name), Obj::Dict(d.clone())); } Self { + made, lex: Lexer::new(src), stack: vec![], dicts: vec![system.clone(), global.clone(), user.clone()], @@ -205,6 +209,8 @@ impl<'a> Interp<'a> { self.dicts.clear(); self.stack.clear(); self.journal.clear(); + // What the program linked into itself, reachable or not. + super::obj::release_tracked(self.made); } // ---------- the operand stack ---------- diff --git a/crates/eps/src/import/obj.rs b/crates/eps/src/import/obj.rs index ae8f7a8e..f540500c 100644 --- a/crates/eps/src/import/obj.rs +++ b/crates/eps/src/import/obj.rs @@ -3,7 +3,7 @@ use std::cell::{Ref, RefCell}; use std::collections::HashMap; use std::hash::{Hash, Hasher}; -use std::rc::Rc; +use std::rc::{Rc, Weak}; use super::data::Stream; use super::graphics::GState; @@ -17,10 +17,95 @@ pub(crate) struct DictRef(Rc>); impl DictRef { pub fn new(d: Dict) -> Self { - Self(Rc::new(RefCell::new(d))) + let d = Rc::new(RefCell::new(d)); + note(Made::Dict(Rc::downgrade(&d))); + Self(d) } } +/// An array or a dictionary a program made. +enum Made { + Array(Weak>>), + Dict(Weak>), +} + +/// The arrays and dictionaries made on this thread since [`track`], for [`release_tracked`]. +struct Tracked { + owner: u64, + made: Vec, + /// The list is cleared of dropped objects when it reaches this length. + sweep_at: usize, +} + +const SWEEP_MIN: usize = 4096; + +#[cfg(test)] +thread_local! { + /// Tests: the arrays of the list last released on this thread that were still alive after it + /// (the ones the interpreter itself still held). + pub(crate) static ARRAYS_LEFT: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +thread_local! { + static TRACKED: RefCell> = const { RefCell::new(None) }; +} + +/// Start listing the arrays and dictionaries made on this thread (an interpreter starting) → the +/// list's owner, for [`release_tracked`]. A list already there is dropped: its objects are freed +/// as before this existed, unless they hold each other. +pub(crate) fn track() -> u64 { + static NEXT: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(1); + let owner = NEXT.fetch_add(1, std::sync::atomic::Ordering::Relaxed); + // (Nothing to list on when the thread is ending.) + let _ = TRACKED.try_with(|t| t.try_borrow_mut().map(|mut t| *t = Some(Tracked { owner, made: vec![], sweep_at: SWEEP_MIN }))); + owner +} + +fn note(m: Made) { + let _ = TRACKED.try_with(|t| { + let Ok(mut t) = t.try_borrow_mut() else { return }; + let Some(t) = t.as_mut() else { return }; + if t.made.len() >= t.sweep_at { + // Twice what is alive at most, so the list costs a program no more than its objects. + t.made.retain(|m| match m { + Made::Array(a) => a.strong_count() > 0, + Made::Dict(d) => d.strong_count() > 0, + }); + t.sweep_at = (t.made.len() * 2).max(SWEEP_MIN); + } + t.made.push(m); + }); +} + +/// Empty every array and dictionary still alive of the list `owner` started (the interpreter is +/// done with them), so those holding themselves or each other are freed too: reference counts +/// alone never free `/a 1 array def a 0 a put`, and a program can leave as much of that behind as +/// it may allocate, on every import (#1714). +pub(crate) fn release_tracked(owner: u64) { + let made = TRACKED.try_with(|t| { + let mut t = t.try_borrow_mut().ok()?; + t.take_if(|t| t.owner == owner).map(|t| t.made) + }); + let made = made.ok().flatten().unwrap_or_default(); + #[cfg(test)] + let arrays: Vec<_> = made.iter().filter_map(|m| if let Made::Array(a) = m { Some(a.clone()) } else { None }).collect(); + for m in made { + // What it holds is taken out first and dropped with nothing borrowed. + match m { + Made::Array(a) => { + let held = a.upgrade().and_then(|a| a.try_borrow_mut().ok().map(|mut values| std::mem::take(&mut *values))); + drop_held(held.unwrap_or_default().into_iter()); + } + Made::Dict(d) => { + let held = d.upgrade().and_then(|d| d.try_borrow_mut().ok().map(|mut d| std::mem::take(&mut *d))); + drop_held(held.unwrap_or_default().into_values()); + } + } + } + #[cfg(test)] + ARRAYS_LEFT.with(|n| n.set(arrays.iter().filter(|a| a.strong_count() > 0).count())); +} + impl std::ops::Deref for DictRef { type Target = Rc>; fn deref(&self) -> &Self::Target { @@ -70,6 +155,8 @@ fn drop_held(held: impl Iterator) { pub(crate) trait Held: Sized { /// Drop the values of storage nothing refers to any more. fn release(_values: Vec) {} + /// Storage of these values was made. + fn made(_storage: &Rc>>) {} } impl Held for u8 {} @@ -78,6 +165,9 @@ impl Held for Obj { fn release(values: Vec) { drop_held(values.into_iter()); } + fn made(storage: &Rc>>) { + note(Made::Array(Rc::downgrade(storage))); + } } /// A dictionary key. @@ -219,7 +309,9 @@ impl Drop for Shared { impl Shared { pub fn new(v: Vec) -> Self { - Self { len: v.len(), buf: Rc::new(RefCell::new(v)), at: 0 } + let (len, buf) = (v.len(), Rc::new(RefCell::new(v))); + T::made(&buf); + Self { len, buf, at: 0 } } pub fn len(&self) -> usize { diff --git a/crates/eps/src/import/tests.rs b/crates/eps/src/import/tests.rs index 63cf89b4..678ae993 100644 --- a/crates/eps/src/import/tests.rs +++ b/crates/eps/src/import/tests.rs @@ -19,6 +19,23 @@ pub(super) fn read(body: &str) -> crate::Imported { import(&eps(body)).unwrap() } +/// #1714: arrays and dictionaries a program links into themselves or each other are freed with +/// the interpreter, also the ones it no longer reaches and when it stops on an error. +#[test] +fn arrays_and_dictionaries_holding_themselves_are_freed() { + // The arrays still alive once a program's interpreter has released what it made: the ones + // the interpreter itself holds, the same for every program here. + let left = |program: &str| { + let _ = import(&eps(program)); + crate::import::obj::ARRAYS_LEFT.with(std::cell::Cell::get) + }; + let plain = left("/a 1 array def /b 1 array def a 0 b put"); + let cycles = "/a 1 array def a 0 a put /b 1 array def /c 1 array def b 0 c put c 0 b put /d 1 dict def /e 1 array def d /e e put e 0 d put 1 array dup 0 1 index put pop /a null def /b null def"; + for program in [cycles.to_string(), format!("{cycles} 1 0 div"), format!("{cycles} /p {{ p }} def")] { + assert_eq!(left(&program), plain, "{program}"); + } +} + /// The objects of the document's layer. pub(super) fn objects(d: &Document) -> Vec> { d.layers[0].children().unwrap().clone() diff --git a/crates/eps/src/ps.rs b/crates/eps/src/ps.rs index e00d813a..fb27425e 100644 --- a/crates/eps/src/ps.rs +++ b/crates/eps/src/ps.rs @@ -181,13 +181,26 @@ fn push85(s: &mut String, mut v: u32, n: usize) { } } -/// ASCII85 `text` (whitespace ignored, up to `~>`) decoded; `None` when it is malformed. +/// Most bytes [`ascii85_decode`] makes: what one filter or data section may hold. +pub(crate) const MAX_ASCII85: usize = 1 << 28; + +/// ASCII85 `text` (whitespace ignored, up to `~>`) decoded; `None` when it is malformed or longer +/// than [`MAX_ASCII85`]. pub(crate) fn ascii85_decode(text: &str) -> Option> { - let mut out = Vec::with_capacity(text.len() * 4 / 5); + ascii85_decode_within(text, MAX_ASCII85) +} + +/// [`ascii85_decode`] of at most `max` bytes: `z` is four bytes for one, so the length of the +/// text doesn't bound what it becomes (#1715). +pub(crate) fn ascii85_decode_within(text: &str, max: usize) -> Option> { + let mut out = Vec::with_capacity((text.len() / 5 * 4).min(max)); let mut group = [0u8; 5]; let mut n = 0; let mut chars = text.bytes().filter(|b| !b.is_ascii_whitespace()).peekable(); while let Some(b) = chars.next() { + if out.len() > max { + return None; + } match b { b'~' => { if chars.next() != Some(b'>') { @@ -201,7 +214,7 @@ pub(crate) fn ascii85_decode(text: &str) -> Option> { let v = value85(&padded)?; out.extend(v.to_be_bytes().iter().take(n - 1)); } - return Some(out); + return (out.len() <= max).then_some(out); } b'z' if n == 0 => out.extend([0; 4]), b'!'..=b'u' => { diff --git a/crates/eps/src/tests.rs b/crates/eps/src/tests.rs index b4e5fa1c..4cab4628 100644 --- a/crates/eps/src/tests.rs +++ b/crates/eps/src/tests.rs @@ -345,6 +345,11 @@ fn run_length_and_ascii85_round_trip() { assert_eq!(ps::ascii85(b"Man "), "9jqo^~>\n"); assert_eq!(ps::ascii85(&[0, 0, 0, 0]), "z~>\n"); assert!(ps::ascii85_decode("9jqo^").is_none(), "no end marker"); + // #1715: the result is bounded while it is made, not by the length of the text. + assert_eq!(ps::ascii85_decode_within("zz~>", 8), Some(vec![0; 8])); + assert!(ps::ascii85_decode_within("zzz~>", 8).is_none(), "three `z` are 12 bytes"); + assert!(ps::ascii85_decode_within("zz9jqo^~>", 8).is_none()); + assert!(ps::ascii85_decode_within(&format!("{}~>", "z".repeat(1 << 16)), 1 << 10).is_none()); assert!(ps::ascii85_decode("9jqo^v~>").is_none(), "not base 85"); assert_eq!(ps::num(1.0 / 3.0), "0.3333"); assert_eq!((ps::num(-0.00001), ps::num(f64::NAN), ps::num(2.5)), ("0".into(), "0".into(), "2.5".into())); diff --git a/crates/format/src/atomic.rs b/crates/format/src/atomic.rs index 29444959..1131d3e2 100644 --- a/crates/format/src/atomic.rs +++ b/crates/format/src/atomic.rs @@ -4,7 +4,10 @@ //! //! Where a folder refuses the temporary file or the rename (some network shares, a file another //! program holds open), the new content is first written and flushed elsewhere and only then copied -//! into the file itself; if that copy fails, the temporary file stays and the error names it. +//! into the file itself. That copy isn't atomic, so the file as it was is copied and flushed beside +//! the temporary file first (#1695): if the copy into the file fails or is cut short, both versions +//! are on disk, and the error names them. Where that copy of the old file can't be made, the file +//! is left as it was and the save fails. //! //! On macOS `File::sync_all` is a full flush (`F_FULLFSYNC`), which SMB shares refuse with //! `ENOTSUP` while a plain `fsync` works: [`sync_file`] falls back to the plain one there (#1620). @@ -78,8 +81,10 @@ fn write_atomic_stepping( /// The content of `tmp`, which is whole and flushed, copied into `target` itself, and `tmp` /// removed: for a folder that `refused` the atomic replacement. When `target` can't be opened for -/// writing it is untouched: `tmp` is removed and `refused` returned. When the copy fails `target` -/// is damaged: `tmp` stays and the error says where it is. +/// writing it is untouched: `tmp` is removed and `refused` returned. When what it holds can't be +/// copied aside first ([`keep_old`]) it is untouched too: `tmp` stays and the error names it. When +/// the copy into `target` fails it is damaged: `tmp` and the copy of the old content stay and the +/// error says where they are. fn write_over(tmp: &Path, target: &Path, refused: io::Error) -> io::Result<()> { let opened = File::open(tmp).and_then(|from| Ok((from, OpenOptions::new().write(true).create(true).truncate(false).open(target)?))); let Ok((mut from, mut to)) = opened else { @@ -87,18 +92,42 @@ fn write_over(tmp: &Path, target: &Path, refused: io::Error) -> io::Result<()> { let _ = std::fs::remove_file(tmp); return Err(refused); }; + // Nothing to keep of a file that is new or empty. + let old = to.metadata().map_or(true, |m| m.len() > 0).then(|| tmp.with_extension("old")); + if let Some(old) = &old + && let Err(e) = keep_old(target, old) + { + // Best effort: a partial copy is of no use. + let _ = std::fs::remove_file(old); + let why = + format!("{refused}, and the file can't be copied aside before it is overwritten ({e}); the new content is kept in {}", tmp.display()); + return Err(io::Error::new(refused.kind(), why)); + } let copied = to.set_len(0).and_then(|()| io::copy(&mut from, &mut to)).and_then(|_| sync_file(&to)); drop((from, to)); - match copied { - Ok(()) => { - // Best effort: the file is saved, so the copy is no longer needed. + match (copied, old) { + (Ok(()), old) => { + // Best effort: the file is saved, so the copies are no longer needed. let _ = std::fs::remove_file(tmp); + if let Some(old) = old { + let _ = std::fs::remove_file(old); + } Ok(()) } - Err(e) => Err(io::Error::new(e.kind(), format!("{e} (the new content is kept in {})", tmp.display()))), + (Err(e), Some(old)) => { + Err(io::Error::new(e.kind(), format!("{e} (the new content is kept in {}, the file as it was in {})", tmp.display(), old.display()))) + } + (Err(e), None) => Err(io::Error::new(e.kind(), format!("{e} (the new content is kept in {})", tmp.display()))), } } +/// A flushed copy of `target` as it is, at `old` (for its owner only). +fn keep_old(target: &Path, old: &Path) -> io::Result<()> { + let mut to = create_private(old)?; + io::copy(&mut File::open(target)?, &mut to)?; + sync_file(&to) +} + /// A new file for its owner only (Unix): the system's temporary folder is shared. fn create_private(path: &Path) -> io::Result { let mut options = OpenOptions::new(); @@ -185,8 +214,15 @@ fn plain_fsync(file: &File) -> io::Result<()> { fn full_sync(file: &File) -> io::Result<()> { #[cfg(test)] - if REFUSE_FULL_SYNC.with(std::cell::Cell::get) { - return Err(io::Error::from_raw_os_error(ENOTSUP)); + { + if REFUSE_FULL_SYNC.with(std::cell::Cell::get) { + return Err(io::Error::from_raw_os_error(ENOTSUP)); + } + match FLUSHES_LEFT.with(std::cell::Cell::get) { + Some(0) => return Err(io::Error::other("the connection was lost")), + Some(n) => FLUSHES_LEFT.with(|c| c.set(Some(n - 1))), + None => {} + } } file.sync_all() } @@ -203,6 +239,8 @@ thread_local! { static REFUSE_FULL_SYNC: std::cell::Cell = const { std::cell::Cell::new(false) }; /// Tests: the plain `fsync` fallback fails too, with this OS error. static PLAIN_FSYNC_FAILS: std::cell::Cell> = const { std::cell::Cell::new(None) }; + /// Tests: after this many more flushes on this thread, every flush fails. + static FLUSHES_LEFT: std::cell::Cell> = const { std::cell::Cell::new(None) }; /// Tests: how many plain `fsync` fallbacks were made on this thread. static PLAIN_FSYNCS: std::cell::Cell = const { std::cell::Cell::new(0) }; } @@ -363,8 +401,46 @@ mod tests { let _ = std::fs::remove_dir_all(&d); } - /// A refused rename and a copy into the file that fails (here: its flush): the file is damaged, - /// so the temporary file with the whole new content stays and the error names it. + /// #1695: a refused rename and a copy into the file that fails (here: its flush, after the copy + /// of the old content was flushed): the file is damaged, so the temporary file with the whole + /// new content and the copy of the old one stay, and the error names both. + #[test] + fn a_failed_copy_into_the_file_keeps_the_old_content_and_the_new() { + let d = dir("no-rename-copy-fails-late"); + let path = d.join("doc.vectorcraft"); + std::fs::write(&path, b"original").unwrap(); + let r = write_atomic_stepping( + &path, + |f| f.write_all(b"new"), + create_new, + |_, _| { + // One more flush works (the copy of the old content), the next doesn't. + FLUSHES_LEFT.with(|c| c.set(Some(1))); + denied() + }, + ); + FLUSHES_LEFT.with(|c| c.set(None)); + let message = r.unwrap_err().to_string(); + let mut left = leftovers(&d, "doc.vectorcraft"); + left.sort(); + let [old, tmp] = left.as_slice() else { panic!("the old content and the new: {left:?}") }; + assert!(old.ends_with(".old") && tmp.ends_with(".tmp"), "{left:?}"); + assert!(message.contains(tmp.as_str()) && message.contains(old.as_str()), "{message}"); + assert_eq!((std::fs::read(d.join(old)).unwrap(), std::fs::read(d.join(tmp)).unwrap()), (b"original".to_vec(), b"new".to_vec())); + // A new file has nothing to keep, and a save that works leaves neither copy. + std::fs::remove_file(d.join(old)).unwrap(); + std::fs::remove_file(d.join(tmp)).unwrap(); + let fresh = d.join("fresh.vectorcraft"); + write_atomic_stepping(&fresh, |f| f.write_all(b"new"), create_new, |_, _| denied()).unwrap(); + write_atomic_stepping(&fresh, |f| f.write_all(b"newer"), create_new, |_, _| denied()).unwrap(); + assert_eq!(std::fs::read(&fresh).unwrap(), b"newer"); + assert_eq!(leftovers(&d, "doc.vectorcraft"), ["fresh.vectorcraft"]); + let _ = std::fs::remove_dir_all(&d); + } + + /// A refused rename where the old content can't be copied aside (here: no flush works): the + /// file is left as it was, the temporary file with the whole new content stays and the error + /// names it. #[test] fn a_failed_copy_into_the_file_keeps_the_temporary_file() { let d = dir("no-rename-copy-fails"); @@ -388,6 +464,7 @@ mod tests { let [tmp] = left.as_slice() else { panic!("one temporary file: {left:?}") }; assert!(message.contains(tmp.as_str()), "{message}"); assert_eq!(std::fs::read(d.join(tmp)).unwrap(), b"new"); + assert_eq!(std::fs::read(&path).unwrap(), b"original", "not overwritten without a copy of it"); let _ = std::fs::remove_dir_all(&d); } diff --git a/crates/format/src/unpack.rs b/crates/format/src/unpack.rs index d38f1776..6e9af01d 100644 --- a/crates/format/src/unpack.rs +++ b/crates/format/src/unpack.rs @@ -122,6 +122,11 @@ impl<'a> Unpacked<'a> { /// data (entries sharing bytes, only in hand-made files, would copy them once each). Entries /// are read in the order of their offsets, each straight into its own buffer. pub(crate) fn entries(self, wanted: &[[u64; 2]]) -> Result>>, FormatError> { + self.entries_within(wanted, MAX_WORK) + } + + /// [`Self::entries`], unpacking at most `work` bytes of a streamed file to find them. + fn entries_within(self, wanted: &[[u64; 2]], work: u64) -> Result>>, FormatError> { let mut out: Vec>> = vec![None; wanted.len()]; let in_memory = |tail: &[u8], out: &mut Vec>>| { for (slot, &[offset, len]) in out.iter_mut().zip(wanted) { @@ -147,7 +152,7 @@ impl<'a> Unpacked<'a> { drop(self.json); let mut order: Vec = (0..wanted.len()).collect(); order.sort_by_key(|&i| wanted.get(i).map_or(0, |w| w[0])); - let mut s = Stream { r: Box::new(io::Cursor::new(read).chain(rest)), pos: 0, file, at }; + let mut s = Stream { r: Box::new(io::Cursor::new(read).chain(rest)), pos: 0, file, at, left: work }; for i in order { let Some(&[offset, len]) = wanted.get(i) else { continue }; if let Some(slot) = out.get_mut(i) { @@ -155,6 +160,7 @@ impl<'a> Unpacked<'a> { } } // To the end, so the stream's checksum is verified as before. + s.left = MAX_BLOBS; s.skip(MAX_BLOBS)?; } } @@ -188,12 +194,29 @@ struct Stream<'a> { file: &'a [u8], /// Where the data after the JSON starts in the unpacked stream. at: u64, + /// Bytes that may still be unpacked ([`MAX_WORK`] in all): the entries' own, the gaps before + /// them, and the stream from its start again for each entry behind the one before. + left: u64, } +/// Most bytes unpacked to read a streamed file's entries: the entries and the gaps between them +/// ([`MAX_BLOBS`] in a file a writer made, where they follow each other), and the JSON once more. +/// The entries' lengths alone don't bound it: one byte asked for at a far offset unpacks all +/// that lies before it, and each entry behind the last one starts the stream over (#1699). +const MAX_WORK: u64 = MAX_BLOBS + MAX_JSON; + impl Stream<'_> { + /// Count `n` bytes unpacked against the budget. + fn spend(&mut self, n: u64) -> Result<(), FormatError> { + self.left = self.left.checked_sub(n).ok_or(FormatError::TooBig)?; + Ok(()) + } + /// Unpack and drop up to `n` bytes → how many there were. fn skip(&mut self, n: u64) -> Result { - let skipped = io::copy(&mut (&mut self.r).take(n), &mut io::sink()).map_err(damaged)?; + // One byte past the budget at most, so going over it is seen without doing the work. + let skipped = io::copy(&mut (&mut self.r).take(n.min(self.left.saturating_add(1))), &mut io::sink()).map_err(damaged)?; + self.spend(skipped)?; self.pos += skipped; Ok(skipped) } @@ -204,7 +227,8 @@ impl Stream<'_> { // Entries out of order or overlapping (only hand-made files): unpack again from the // start. let mut dec = GzDecoder::new(self.file); - let skipped = io::copy(&mut (&mut dec).take(self.at), &mut io::sink()).map_err(damaged)?; + let skipped = io::copy(&mut (&mut dec).take(self.at.min(self.left.saturating_add(1))), &mut io::sink()).map_err(damaged)?; + self.spend(skipped)?; if skipped < self.at { return Ok(None); } @@ -219,7 +243,8 @@ impl Stream<'_> { let n = usize::try_from(len).map_err(|_| FormatError::TooBig)?; // The length is the file's word: reserve up to a bound, and let real data grow the rest. bytes.try_reserve_exact(n.min(RESERVE_MAX)).map_err(|_| FormatError::TooBig)?; - let got = (&mut self.r).take(len).read_to_end(&mut bytes).map_err(damaged)?; + let got = (&mut self.r).take(len.min(self.left.saturating_add(1))).read_to_end(&mut bytes).map_err(damaged)?; + self.spend(got as u64)?; self.pos += got as u64; Ok((got as u64 == len).then_some(bytes)) } @@ -237,6 +262,35 @@ mod tests { gz.finish().unwrap() } + /// #1699: reading a streamed file's entries unpacks a bounded number of bytes, whatever + /// their offsets: a far offset and entries behind one another are refused past the budget. + #[test] + fn a_streamed_files_entries_are_found_within_a_work_budget() { + let tail: Vec = (0..4096u32).map(|i| (i % 251) as u8).collect(); + let mut file = b"{}".to_vec(); + file.extend(crate::BLOB_MAGIC); + file.extend(&tail); + let file = gzip(&file); + let read = |wanted: &[[u64; 2]], work: u64| { + let u = Unpacked::new(&file).unwrap(); + assert!(matches!(u.tail, Tail::Stream { .. }), "read as a stream"); + u.entries_within(wanted, work) + }; + // Entries one after the other: the tail is unpacked once. + let parts = read(&[[0, 1000], [1000, 3096]], 4096).unwrap(); + assert_eq!(parts, vec![Some(tail[..1000].to_vec()), Some(tail[1000..].to_vec())]); + // One byte at the far end is the whole tail's work. + assert_eq!(read(&[[4095, 1]], 4096).unwrap(), vec![Some(vec![tail[4095]])]); + assert!(matches!(read(&[[4095, 1]], 4000), Err(FormatError::TooBig))); + // Entries over the same bytes start the stream again each time: counted, then refused. + let same = vec![[4000, 96]; 50]; + assert!(read(&same, 50 * (4096 + 64)).is_ok()); + assert!(matches!(read(&same, 3 * 4096), Err(FormatError::TooBig))); + // An offset past the data is still just missing, and the default budget reads all of it. + assert_eq!(read(&[[1 << 40, 1]], 1 << 20).unwrap(), vec![None]); + assert_eq!(Unpacked::new(&file).unwrap().entries(&[[0, 4096]]).unwrap(), vec![Some(tail.clone())]); + } + #[test] fn entries_sharing_bytes_obey_the_limits_in_memory_too() { // Each entry within MAX_BLOB, together past MAX_BLOBS: many references to the same bytes.