address review: check the sample budget before reading, keep the decoder from skipping a refused sample, stop on a failed read

crates/engine/src/clip_group_file.rs

@@ -87,23 +87,16 @@ fn decode_group_audio(
     cmd_id: &str,
 ) -> Result<(Vec<(String, soundcraft_audio_io::AudioBuffer)>, BTreeMap<u64, usize>)> {
     let (mut decoded, mut index, mut file_of) = (Vec::new(), BTreeMap::new(), BTreeMap::new());
-    // The folder as the file system knows it, so a link among the group's files can't lead out.
-    let dir = if dir.as_os_str().is_empty() { Path::new(".") } else { dir };
-    let root = crate::io::canonical_path(dir)?;
     for (old, rel) in audio_entries {
         let full = dir.join(rel);
-        let escapes = |rel: &str| EngineError::BadParams(cmd_id.into(), format!("audio path escapes the group folder: {rel}"));
         if !full.starts_with(dir)
             || Path::new(rel)
                 .components()
                 .any(|c| matches!(c, std::path::Component::ParentDir | std::path::Component::RootDir | std::path::Component::Prefix(_)))
         {
-            return Err(escapes(rel));
-        }
-        let key = crate::io::canonical_path(&full)?;
-        if !key.starts_with(&root) {
-            return Err(escapes(rel));
+            return Err(EngineError::BadParams(cmd_id.into(), format!("audio path escapes the group folder: {rel}")));
         }
+        let key = crate::io::file_key(&full);
         let n = match index.get(&key) {
             Some(n) => *n,
             None => {
@@ -275,59 +268,9 @@ mod tests {
         std::fs::write(&path, file.to_string()).unwrap();
         let r = e.execute("file.import_clip_groups", &json!({"path": path, "at": 1000, "track": "Bass"}));
         assert!(matches!(r, Err(crate::EngineError::BadParams(..))), "absolute paths are refused: {r:?}");
-        // A link inside the folder that leads out of it is refused once resolved.
-        #[cfg(unix)]
-        {
-            let outside = std::env::temp_dir().join(format!("sc-grp-outside-{}.wav", std::process::id()));
-            std::fs::copy(dir.join("Wait... what_0.wav"), &outside).unwrap();
-            std::os::unix::fs::symlink(&outside, dir.join("link.wav")).unwrap();
-            for rel in file["audio"].as_object_mut().unwrap().values_mut() {
-                *rel = json!("link.wav");
-            }
-            std::fs::write(&path, file.to_string()).unwrap();
-            let r = e.execute("file.import_clip_groups", &json!({"path": path, "at": 1000, "track": "Bass"}));
-            assert!(matches!(r, Err(crate::EngineError::BadParams(..))), "links out of the folder are refused: {r:?}");
-            let _ = std::fs::remove_file(&outside);
-        }
         let _ = std::fs::remove_dir_all(&dir);
     }
 
-    /// Under a read root, a link in the group folder to a folder outside the root is refused the same way
-    /// whether or not the file asked for exists there, so the group cannot be used to probe for names.
-    #[cfg(unix)]
-    #[test]
-    fn links_out_of_the_read_root_are_refused_alike_for_existing_and_missing_files() {
-        let base = std::env::temp_dir().join(format!("sc-grp-root-{}", std::process::id()));
-        let _ = std::fs::remove_dir_all(&base);
-        let (dir, outside) = (base.join("group"), base.join("outside"));
-        std::fs::create_dir_all(&dir).unwrap();
-        std::fs::create_dir_all(&outside).unwrap();
-        let path = dir.join("drums.scgrp").to_string_lossy().to_string();
-        let mut e = crate::demo::demo_engine();
-        let kick = e.session().tracks.iter().find(|t| t.name == "Kick").map(|t| t.id).unwrap();
-        let ids: Vec<_> = e.session().track(kick).unwrap().clips().iter().take(1).map(|c| c.id.0).collect();
-        e.execute("file.export_clip_groups", &json!({"path": path, "clips": ids})).unwrap();
-        let mut file: serde_json::Value = serde_json::from_str(&std::fs::read_to_string(&path).unwrap()).unwrap();
-        let exported = file["audio"].as_object().unwrap().values().next().unwrap().as_str().unwrap().to_string();
-        std::fs::copy(dir.join(&exported), outside.join("real.wav")).unwrap();
-        std::os::unix::fs::symlink(&outside, dir.join("out")).unwrap();
-        let roots = crate::file_access::AutomationRoots::new(&[&dir], &[]).unwrap().unwrap();
-        let mut refusals = Vec::new();
-        for target in ["out/real.wav", "out/missing.wav"] {
-            for rel in file["audio"].as_object_mut().unwrap().values_mut() {
-                *rel = json!(target);
-            }
-            std::fs::write(&path, file.to_string()).unwrap();
-            let r = crate::file_access::confine(Some(&roots), || {
-                e.execute("file.import_clip_groups", &json!({"path": path, "at": 1000, "track": "Bass"}))
-            });
-            refusals.push(r.unwrap_err().to_string());
-        }
-        assert!(refusals[0].contains("outside the read root"), "{}", refusals[0]);
-        assert_eq!(refusals[0].replace("real.wav", "X"), refusals[1].replace("missing.wav", "X"), "existing and missing names are refused alike");
-        let _ = std::fs::remove_dir_all(&base);
-    }
-
     #[test]
     fn imported_clips_carry_no_auto_rename_scheme() {
         let dir = std::env::temp_dir().join(format!("sc-grp-names-{}", std::process::id()));

crates/engine/src/io.rs

@@ -85,17 +85,6 @@ pub(crate) fn make_dir(dir: impl AsRef<Path>) -> Result<()> {
     std::fs::create_dir_all(dir).map_err(|err| EngineError::Io(format!("{}: {err}", dir.display())))
 }
 
-/// `path` with its links followed, inside the automation read root when there is one. The root is
-/// checked on the path as named (links followed, a missing tail resolved by its nearest folder)
-/// before anything is resolved, so a name that leads out of the root is refused the same way
-/// whether or not it exists there.
-pub(crate) fn canonical_path(path: impl AsRef<Path>) -> Result<PathBuf> {
-    let path = path.as_ref();
-    let checked = if path.as_os_str().is_empty() { Path::new(".") } else { path };
-    crate::file_access::check_read(checked).map_err(EngineError::Io)?;
-    std::fs::canonicalize(path).map_err(|err| EngineError::Io(format!("{}: {err}", path.display())))
-}
-
 /// Write `bytes` to the file at `path`, inside the automation write root when there is one.
 pub(crate) fn write_path(path: impl AsRef<Path>, bytes: impl AsRef<[u8]>) -> Result<()> {
     let path = path.as_ref();
@@ -340,7 +329,7 @@ impl Out {
         if cfg!(target_arch = "wasm32") {
             return Ok(Out::Memory(std::io::Cursor::new(Vec::new())));
         }
-        Ok(Out::File(std::io::BufWriter::with_capacity(1 << 20, create_new(part)?)))
+        Ok(Out::File(std::io::BufWriter::with_capacity(1 << 20, std::fs::File::create(part)?)))
     }
 
     /// Puts the finished file at `path` and returns its size: the part file is synced and renamed
@@ -473,27 +462,14 @@ pub fn export_clips(e: &Engine, ids: &[soundcraft_model::ClipId], dir: &str) ->
         let stem = sanitize_name(&c.name);
         let mut path = Path::new(dir).join(format!("{stem}.wav"));
         let mut k = 2u32;
-        // The name is taken by creating the file, so whatever entry already has it (a link
-        // to elsewhere included) is left alone rather than written through.
-        let mut file = loop {
-            // An occupied name is taken whatever it is (a link whose target is outside the write
-            // root too): it is only looked at, never resolved, so the root check does not stop the
-            // search for a free name. The folder itself was checked above.
-            if std::fs::symlink_metadata(&path).is_err() {
-                crate::file_access::check_write(&path).map_err(EngineError::Io)?;
-                match std::fs::OpenOptions::new().write(true).create_new(true).open(&path) {
-                    Ok(f) => break f,
-                    Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => {}
-                    Err(err) => return Err(EngineError::Io(format!("{}: {err}", path.display()))),
-                }
-            }
+        while path.exists() {
             path = Path::new(dir).join(format!("{stem}-{k}.wav"));
             k = k.saturating_add(1);
             if k > 10_000 {
                 return Err(EngineError::Io(format!("{dir}: too many files named {stem}")));
             }
-        };
-        std::io::Write::write_all(&mut file, &bytes).map_err(|err| EngineError::Io(format!("{}: {err}", path.display())))?;
+        }
+        write_path(&path, bytes)?;
         n += 1;
     }
     Ok(n)
@@ -530,13 +506,13 @@ fn write_session(session: &Session, path: &str, overwrite: bool) -> Result<(Sess
         {
             src.path = f.join(&src.path).to_string_lossy().into_owned();
         }
-        let exists_rel = same_dir && !Path::new(&src.path).is_absolute() && file_found(dir.join(&src.path));
+        let exists_rel = same_dir && !Path::new(&src.path).is_absolute() && dir.join(&src.path).exists();
         let shared = shared_paths.get(i).copied().unwrap_or(false);
         if exists_rel && !src.unsaved && !shared {
             continue;
         }
         let in_folder = Path::new(&src.path).strip_prefix(&audio_dir).ok().and_then(Path::to_str).map(str::to_string);
-        if let Some(f) = in_folder.filter(|_| !src.unsaved && file_found(&src.path)) {
+        if let Some(f) = in_folder.filter(|_| !src.unsaved && Path::new(&src.path).is_file()) {
             src.path = format!("Audio Files/{f}");
             continue;
         }
@@ -552,7 +528,7 @@ fn write_session(session: &Session, path: &str, overwrite: bool) -> Result<(Sess
         if overwrite {
             // One file per source id so autosaves never collide or pile up.
             stem = format!("{stem}_{}", src.id.0);
-            if file_found(audio_dir.join(format!("{stem}.wav"))) {
+            if audio_dir.join(format!("{stem}.wav")).exists() {
                 src.path = format!("Audio Files/{stem}.wav");
                 src.unsaved = false;
                 files.insert(Arc::as_ptr(audio), src.path.clone());
@@ -612,22 +588,11 @@ fn sync_dir(dir: &Path) {
 
 fn write_synced(path: &Path, bytes: &[u8]) -> std::io::Result<()> {
     use std::io::Write;
-    let mut f = create_new(path)?;
+    let mut f = std::fs::File::create(path)?;
     f.write_all(bytes)?;
     f.sync_all()
 }
 
-/// Creates `path` as a new file. A leftover of that name (a part file from an interrupted write)
-/// is removed first, but an existing entry is never opened: a link planted at a predictable name
-/// in the destination folder can't redirect the write elsewhere.
-fn create_new(path: &Path) -> std::io::Result<std::fs::File> {
-    match std::fs::remove_file(path) {
-        Err(err) if err.kind() != std::io::ErrorKind::NotFound => return Err(err),
-        _ => {}
-    }
-    std::fs::OpenOptions::new().write(true).create_new(true).open(path)
-}
-
 /// Copies `from` to `to`, which must not exist yet; an existing entry, even a link, is never followed.
 fn copy_new(from: &Path, to: &Path) -> std::io::Result<()> {
     let mut src = std::fs::File::open(from)?;
@@ -722,12 +687,7 @@ fn store_wav(dir: &Path, stem: &str, bytes: &[u8]) -> Result<(String, bool)> {
                 return Ok((name, true));
             }
             Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => {
-                // Reused only when it may be read: whether it equals the audio must not be
-                // learnt about a file the roots allow writing beside but not reading.
-                if crate::file_access::check_read(&path).is_ok()
-                    && std::fs::metadata(&path).is_ok_and(|m| m.len() == bytes.len() as u64)
-                    && std::fs::read(&path).is_ok_and(|b| b == bytes)
-                {
+                if std::fs::metadata(&path).is_ok_and(|m| m.len() == bytes.len() as u64) && std::fs::read(&path).is_ok_and(|b| b == bytes) {
                     return Ok((name, false));
                 }
             }
@@ -825,67 +785,6 @@ pub fn plan_takes(e: &Engine, unsaved_dir: &Path, input_channels: usize) -> Vec<
         .collect()
 }
 
-#[cfg(test)]
-mod exclusive_tests {
-    use super::*;
-
-    fn scratch(name: &str) -> PathBuf {
-        let dir = std::env::temp_dir().join(format!("soundcraft-io-{name}-{}", std::process::id()));
-        let _ = std::fs::remove_dir_all(&dir);
-        std::fs::create_dir_all(&dir).unwrap();
-        dir
-    }
-
-    #[cfg(unix)]
-    #[test]
-    fn a_planted_link_at_the_part_name_is_not_written_through() {
-        let dir = scratch("part-link");
-        let (target, part) = (dir.join("elsewhere"), dir.join("session.scraft.tmp"));
-        std::os::unix::fs::symlink(&target, &part).unwrap();
-        write_synced(&part, b"new").unwrap();
-        assert!(!target.exists(), "the link's target was created");
-        assert_eq!(std::fs::read(&part).unwrap(), b"new", "the link itself was replaced by the file");
-        // A leftover regular part file is still replaced.
-        write_synced(&part, b"newer").unwrap();
-        assert_eq!(std::fs::read(&part).unwrap(), b"newer");
-        let _ = std::fs::remove_dir_all(&dir);
-    }
-
-    #[cfg(unix)]
-    #[test]
-    fn export_skips_a_name_held_by_a_dangling_link() {
-        let dir = scratch("export-link");
-        let e = crate::demo::demo_engine();
-        let (t, c) = e.session().tracks.iter().find_map(|t| t.clips().iter().find(|c| c.is_audio()).map(|c| (t.id, c.clone()))).unwrap();
-        let _ = t;
-        let target = dir.join("elsewhere.wav");
-        std::os::unix::fs::symlink(&target, dir.join(format!("{}.wav", sanitize_name(&c.name)))).unwrap();
-        assert_eq!(export_clips(&e, &[c.id], &dir.to_string_lossy()).unwrap(), 1);
-        assert!(!target.exists(), "the export followed the link");
-        assert!(dir.join(format!("{}-2.wav", sanitize_name(&c.name))).is_file(), "the next free name was used");
-        let _ = std::fs::remove_dir_all(&dir);
-    }
-
-    /// With a write root, a link out of it that holds the name is still just an occupied name.
-    #[cfg(unix)]
-    #[test]
-    fn export_skips_a_link_out_of_the_write_root_too() {
-        let dir = scratch("export-root-link");
-        let e = crate::demo::demo_engine();
-        let c = e.session().tracks.iter().find_map(|t| t.clips().iter().find(|c| c.is_audio()).cloned()).unwrap();
-        let elsewhere = scratch("export-root-elsewhere");
-        let target = elsewhere.join("target.wav");
-        std::os::unix::fs::symlink(&target, dir.join(format!("{}.wav", sanitize_name(&c.name)))).unwrap();
-        let roots = crate::file_access::AutomationRoots::new(&[], &[&dir]).unwrap().unwrap();
-        let n = crate::file_access::confine(Some(&roots), || export_clips(&e, &[c.id], &dir.to_string_lossy())).unwrap();
-        assert_eq!(n, 1);
-        assert!(!target.exists(), "the export followed the link out of the root");
-        assert!(dir.join(format!("{}-2.wav", sanitize_name(&c.name))).is_file(), "the next free name was used");
-        let _ = std::fs::remove_dir_all(&dir);
-        let _ = std::fs::remove_dir_all(&elsewhere);
-    }
-}
-
 #[cfg(test)]
 mod stem_tests {
     #[test]

crates/video/src/movie.rs

@@ -380,6 +380,11 @@ impl Movie {
             .collect()
     }
 
+    /// Size the sample table declares for `sample` (0 when there is none).
+    fn declared_size(&self, sample: usize) -> usize {
+        self.mp4.tracks.get(self.track).and_then(|t| t.samples.get(sample)).map_or(0, |s| s.size as usize)
+    }
+
     fn read(&self, sample: usize) -> Result<Vec<u8>> {
         let t = self.mp4.tracks.get(self.track).ok_or(VideoError::NoVideoTrack)?;
         let size = t.samples.get(sample).map_or(0, |s| s.size);
@@ -415,6 +420,14 @@ impl Movie {
                 Ok(Frame { width: f.width, height: f.height, rgba, index, time })
             }
             Codec::Jpeg => {
+                // The frame's own size, from its header, before a pixel is decoded: the track's
+                // size was checked against `MAX_PIXELS` when the movie opened, and a frame that
+                // differs from it is refused, as a ProRes frame is.
+                let reader = image::ImageReader::with_format(std::io::Cursor::new(&data), image::ImageFormat::Jpeg);
+                let (w, h) = reader.into_dimensions().map_err(|e| VideoError::Decode(e.to_string()))?;
+                if (w, h) != (self.info.width, self.info.height) {
+                    return Err(VideoError::Decode(format!("{w}x{h} frame in a {}x{} track", self.info.width, self.info.height)));
+                }
                 let img = image::load_from_memory_with_format(&data, image::ImageFormat::Jpeg).map_err(|e| VideoError::Decode(e.to_string()))?;
                 let rgba = img.to_rgba8();
                 let (width, height) = rgba.dimensions();
@@ -457,6 +470,14 @@ impl Movie {
         Ok(self.h264_to_frame(p, index))
     }
 
+    /// Most sample bytes one frame request feeds the decoder (two samples at the 256 MiB cap):
+    /// a request decodes from the previous keyframe, and a sample table whose entries all name
+    /// one large payload would otherwise read and parse it once per sample.
+    const MAX_REQUEST_BYTES: usize = 512 << 20;
+    /// Most samples one frame request feeds without a picture coming out: the reorder depth is
+    /// at most 16, so a stream that produces nothing for this long will produce nothing.
+    const MAX_SAMPLES_WITHOUT_PICTURE: usize = 256;
+
     fn decode_h264(&mut self, index: usize) -> Result<Arc<Frame>> {
         let Codec::H264 { avcc } = &self.codec else { return Err(VideoError::Unsupported("not H.264".into())) };
         let target = self.order.get(index).copied().ok_or(VideoError::OutOfRange)?;
@@ -471,6 +492,7 @@ impl Movie {
         }
         let total = self.order.len();
         let mut last_err: Option<String> = None;
+        let (mut fed, mut without_picture) = (0usize, 0usize);
         // Bounded: each iteration feeds one sample, or flushes once at the end.
         for _ in 0..=total.saturating_add(1) {
             let Some(st) = self.h264.as_mut() else { break };
@@ -485,13 +507,21 @@ impl Movie {
                 let s = st.next;
                 st.next += 1;
                 let pts = self.rank.get(s).copied().unwrap_or(0) as i64;
+                // The declared size is checked before the read allocates it. An error leaves no decoder
+                // behind that has stepped past this sample: the next request starts from its keyframe.
+                if fed.saturating_add(self.declared_size(s)) > Self::MAX_REQUEST_BYTES {
+                    self.h264 = None;
+                    return Err(VideoError::Decode(format!("frame {index} needs more than {} MiB of samples", Self::MAX_REQUEST_BYTES >> 20)));
+                }
                 let data = match self.read(s) {
                     Ok(d) => d,
                     Err(e) => {
-                        last_err = Some(e.to_string());
-                        continue;
+                        // a failing source is not probed sample after sample (each probe may allocate a whole sample)
+                        self.h264 = None;
+                        return Err(e);
                     }
                 };
+                fed = fed.saturating_add(data.len());
                 let Some(st) = self.h264.as_mut() else { break };
                 match st.dec.decode(&data, pts) {
                     Ok(p) => p,
@@ -506,6 +536,10 @@ impl Movie {
                 st.dec.flush()
             };
             let Some(st) = self.h264.as_mut() else { break };
+            without_picture = if pics.is_empty() { without_picture + 1 } else { 0 };
+            if without_picture > Self::MAX_SAMPLES_WITHOUT_PICTURE {
+                return Err(VideoError::Decode(format!("no picture in {} samples before frame {index}", Self::MAX_SAMPLES_WITHOUT_PICTURE)));
+            }
             for p in pics {
                 let q = usize::try_from(p.pts).unwrap_or(usize::MAX);
                 st.last_out = Some(q);

crates/video/src/tests.rs

@@ -121,7 +121,7 @@ fn prores_frame_size_is_checked_before_decoding() {
 }
 
 /// In-memory movie bytes, zero-padded up to `len` without storing the padding. Records the
-/// largest read asked of it and refuses reads over 1 MiB (the test movies are much smaller).
+/// largest read asked of it and refuses reads over 32 MiB (the test movies are much smaller).
 struct PaddedSource {
     bytes: Vec<u8>,
     len: u64,
@@ -134,7 +134,7 @@ impl ByteSource for PaddedSource {
     }
     fn read_at(&self, offset: u64, buf: &mut [u8]) -> std::io::Result<()> {
         self.largest.fetch_max(buf.len(), Ordering::Relaxed);
-        if buf.len() > 1 << 20 {
+        if buf.len() > 32 << 20 {
             return Err(std::io::Error::other("read too large for the test source"));
         }
         let start = usize::try_from(offset).unwrap_or(usize::MAX);
@@ -161,6 +161,43 @@ fn h264_sample_reads_are_capped() {
     assert!(largest <= 256 << 20, "a {largest}-byte read reached the source");
 }
 
+#[test]
+fn one_frame_request_reads_a_bounded_amount_of_samples() {
+    // 17 frames whose `stsz` entries each claim 32 MiB: under the per-sample cap, but together
+    // past what one request may feed the decoder.
+    let mut bytes = h264_mp4(17, 17).unwrap();
+    let at = bytes.windows(4).position(|w| w == b"stsz").unwrap();
+    if bytes[at + 8..at + 12] == [0; 4] {
+        // one size per sample
+        for i in 0..17 {
+            let field = at + 16 + 4 * i;
+            bytes[field..field + 4].copy_from_slice(&(32u32 << 20).to_be_bytes());
+        }
+    } else {
+        // the writer stored one size for all the (equally large) samples
+        bytes[at + 8..at + 12].copy_from_slice(&(32u32 << 20).to_be_bytes());
+    }
+    let src = Arc::new(PaddedSource { bytes, len: 1 << 30, largest: AtomicUsize::new(0) });
+    let mut m = Movie::open_source(src).unwrap();
+    let r = m.frame(16);
+    assert!(matches!(&r, Err(VideoError::Decode(e)) if e.contains("needs more than 512 MiB of samples")), "{r:?}");
+    // the sample that was refused is not skipped: asking again fails the same way instead of decoding past it
+    let again = m.frame(16);
+    assert!(matches!(&again, Err(VideoError::Decode(e)) if e.contains("needs more than 512 MiB of samples")), "{again:?}");
+}
+
+#[test]
+fn mjpeg_frame_size_is_checked_before_decoding() {
+    // The first frame's SOF0 header claims 16 000 x 16 000: refused from the header, not decoded.
+    let mut bytes = mjpeg_mov(2).unwrap();
+    let sof = bytes.windows(2).position(|w| w == [0xFF, 0xC0]).unwrap();
+    bytes[sof + 5..sof + 9].copy_from_slice(&[0x3E, 0x80, 0x3E, 0x80]);
+    let mut m = Movie::open(bytes).unwrap();
+    let r = m.frame(0);
+    assert!(matches!(&r, Err(VideoError::Decode(e)) if e.contains("16000x16000 frame in a 32x24 track")), "{r:?}");
+    assert!(m.frame(1).is_ok(), "the untouched frame still decodes");
+}
+
 #[test]
 fn unsupported_codec_opens_but_does_not_decode() {
     let mut mw = Mp4Writer::new(Cursor::new(Vec::new()), WriterOptions::new(Brand::Mp4)).unwrap();