From 277e893e828fc500dfd26073014515948ba134d9 Mon Sep 17 00:00:00 2001 From: bflatastic Date: Wed, 14 Oct 2026 08:32:48 +0700 Subject: [PATCH 1/5] Fix #1715: ASCII85 decoding is bounded while it decodes A `z` is four bytes for one character, so the text's length didn't bound the result, and the filter's and the interpreter's budgets only saw it once it was whole. The decoder now stops at 256 MB, the most one filter or data section may hold. --- crates/eps/src/ps.rs | 19 ++++++++++++++++--- crates/eps/src/tests.rs | 5 +++++ 2 files changed, 21 insertions(+), 3 deletions(-) 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())); From 729e5ced7fa85b514444d9cdcf64f4b0c46a12f6 Mon Sep 17 00:00:00 2001 From: bflatastic Date: Wed, 14 Oct 2026 08:32:48 +0700 Subject: [PATCH 2/5] Fix #1714: the EPS interpreter frees arrays and dictionaries that hold themselves Composite objects are reference-counted, so `/a 1 array def a 0 a put` (or two dictionaries naming each other) outlived the import: emptying the standard dictionaries and the stacks doesn't break a cycle, least of all one the program no longer reaches. Up to the interpreter's whole memory budget could stay allocated per imported file. The interpreter now lists the arrays and dictionaries it makes (weakly, swept as they are dropped) and empties the ones still alive when it is done. --- crates/eps/src/import/interp.rs | 6 ++ crates/eps/src/import/obj.rs | 98 ++++++++++++++++++++++++++++++++- crates/eps/src/import/tests.rs | 17 ++++++ 3 files changed, 118 insertions(+), 3 deletions(-) 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() From 0ad92e4649c13af8c20b51570b4985728c1fd569 Mon Sep 17 00:00:00 2001 From: bflatastic Date: Wed, 14 Oct 2026 08:37:15 +0700 Subject: [PATCH 3/5] Fix #1704: an Affinity document's pixel layers share one pixel allowance Each pixel layer was held to 64 megapixels, and the archive's limits count the bytes extracted from it; a layer whose tiles are flat colour needs none, so a small file with many of them allocated 256 MB each without end. The layers of one document now decode 512 megapixels (2 GB) at most together; those past it are left out with a note, as an oversized one is. --- crates/affinity/src/model.rs | 17 ++++++++++++-- crates/affinity/src/raster.rs | 44 ++++++++++++++++++++++++++++++++++- 2 files changed, 58 insertions(+), 3 deletions(-) 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(); From 1d282159dd0fc1d7b18db104c8daafe5eeaba030 Mon Sep 17 00:00:00 2001 From: bflatastic Date: Wed, 14 Oct 2026 08:38:10 +0700 Subject: [PATCH 4/5] Fix #1699: opening a streamed native file unpacks a bounded number of bytes The limits on a compressed file's entries were on their lengths. An entry of one byte at a far offset unpacked everything before it, and each entry behind the previous one started the stream over and unpacked the JSON and the gap again: a small hand-made file could ask for hours of decompression. All of it is now counted against one budget (the entries a writer can make, and the JSON once more), past which the file is refused as too big. --- crates/format/src/unpack.rs | 62 ++++++++++++++++++++++++++++++++++--- 1 file changed, 58 insertions(+), 4 deletions(-) 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. From 2e6b925793e898f424fbebba11743191edc821aa Mon Sep 17 00:00:00 2001 From: bflatastic Date: Wed, 14 Oct 2026 08:39:44 +0700 Subject: [PATCH 5/5] Fix #1695: the copy-over fallback of a save keeps the old file until the new one is in Where a folder refuses the temporary file or the rename, the new content (whole and flushed elsewhere) is copied into the file itself: the file was truncated first, so a copy cut short by a lost connection or a crash left neither version in place, and only the new one on disk under a hidden name. The file as it was is now copied and flushed beside the temporary file before it is truncated; if it can't be, the file is left alone and the save fails, naming the temporary file. A failed copy names both. --- crates/format/src/atomic.rs | 99 ++++++++++++++++++++++++++++++++----- 1 file changed, 88 insertions(+), 11 deletions(-) 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); }