Merge pull request #1863 from storytold/fix/audit-importers

Fix #1715, #1714, #1704, #1699, #1695: bounded ASCII85 and unpacking, EPS cycles, Affinity pixel allowance, save fallback

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<Document, Error> {
+    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<Document, Error> {
     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<String, usize>,
     nodes: usize,
     active: HashSet<ObjId>,
+    /// 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<String>) {

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<u8> {
+        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();

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<Obj>,
     pub dicts: Vec<DictRef>,
@@ -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 ----------

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<RefCell<Dict>>);
 
 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<RefCell<Vec<Obj>>>),
+    Dict(Weak<RefCell<Dict>>),
+}
+
+/// The arrays and dictionaries made on this thread since [`track`], for [`release_tracked`].
+struct Tracked {
+    owner: u64,
+    made: Vec<Made>,
+    /// 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<usize> = const { std::cell::Cell::new(0) };
+}
+
+thread_local! {
+    static TRACKED: RefCell<Option<Tracked>> = 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<RefCell<Dict>>;
     fn deref(&self) -> &Self::Target {
@@ -70,6 +155,8 @@ fn drop_held(held: impl Iterator<Item = Obj>) {
 pub(crate) trait Held: Sized {
     /// Drop the values of storage nothing refers to any more.
     fn release(_values: Vec<Self>) {}
+    /// Storage of these values was made.
+    fn made(_storage: &Rc<RefCell<Vec<Self>>>) {}
 }
 
 impl Held for u8 {}
@@ -78,6 +165,9 @@ impl Held for Obj {
     fn release(values: Vec<Self>) {
         drop_held(values.into_iter());
     }
+    fn made(storage: &Rc<RefCell<Vec<Self>>>) {
+        note(Made::Array(Rc::downgrade(storage)));
+    }
 }
 
 /// A dictionary key.
@@ -219,7 +309,9 @@ impl<T: Held> Drop for Shared<T> {
 
 impl<T: Clone + Held> Shared<T> {
     pub fn new(v: Vec<T>) -> 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 {

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<Arc<Node>> {
     d.layers[0].children().unwrap().clone()

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<Vec<u8>> {
-    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<Vec<u8>> {
+    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<Vec<u8>> {
                     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' => {

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()));

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<File> {
     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<bool> = const { std::cell::Cell::new(false) };
     /// Tests: the plain `fsync` fallback fails too, with this OS error.
     static PLAIN_FSYNC_FAILS: std::cell::Cell<Option<i32>> = const { std::cell::Cell::new(None) };
+    /// Tests: after this many more flushes on this thread, every flush fails.
+    static FLUSHES_LEFT: std::cell::Cell<Option<u32>> = const { std::cell::Cell::new(None) };
     /// Tests: how many plain `fsync` fallbacks were made on this thread.
     static PLAIN_FSYNCS: std::cell::Cell<u32> = 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);
     }
 

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<Vec<Option<Vec<u8>>>, 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<Vec<Option<Vec<u8>>>, FormatError> {
         let mut out: Vec<Option<Vec<u8>>> = vec![None; wanted.len()];
         let in_memory = |tail: &[u8], out: &mut Vec<Option<Vec<u8>>>| {
             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<usize> = (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<u64, FormatError> {
-        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<u8> = (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.