diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 889698ec..00d5ad54 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -1,6 +1,10 @@ name: Release on: + # A nightly dry run on main: every release build except macOS, nothing signed or released + # (craft-common-actions release.yml `dry-run`). Finds release-only breakage before release day. + schedule: + - cron: "36 2 * * *" push: branches: [release] workflow_dispatch: @@ -30,6 +34,7 @@ jobs: # Intentionally tracked at @main (not SHA-pinned) so org-wide CI improvements, security fixes, and build optimizations propagate without PR churn across all craftapp repos. uses: storytold/craft-common-actions/.github/workflows/release.yml@main with: + dry-run: ${{ github.event_name == 'schedule' }} app-name: soundcraft app-title: SoundCraft version: ${{ inputs.version }} diff --git a/Cargo.lock b/Cargo.lock index 325d148f..7e331caa 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4344,7 +4344,7 @@ dependencies = [ [[package]] name = "soundcraft" -version = "0.19.1" +version = "0.20.0" dependencies = [ "chrono", "clap", @@ -4368,7 +4368,7 @@ dependencies = [ [[package]] name = "soundcraft-au-host" -version = "0.19.1" +version = "0.20.0" dependencies = [ "log", "serde", @@ -4378,7 +4378,7 @@ dependencies = [ [[package]] name = "soundcraft-automation" -version = "0.19.1" +version = "0.20.0" dependencies = [ "clap", "craft-audio", @@ -4391,7 +4391,7 @@ dependencies = [ [[package]] name = "soundcraft-clap-host" -version = "0.19.1" +version = "0.20.0" dependencies = [ "clap-sys", "libloading 0.9.0", @@ -4404,7 +4404,7 @@ dependencies = [ [[package]] name = "soundcraft-cli" -version = "0.19.1" +version = "0.20.0" dependencies = [ "clap", "clap_complete", @@ -4422,7 +4422,7 @@ dependencies = [ [[package]] name = "soundcraft-dsp" -version = "0.19.1" +version = "0.20.0" dependencies = [ "log", "proptest", @@ -4433,7 +4433,7 @@ dependencies = [ [[package]] name = "soundcraft-engine" -version = "0.19.1" +version = "0.20.0" dependencies = [ "craft-audio", "craft-docpath", @@ -4457,7 +4457,7 @@ dependencies = [ [[package]] name = "soundcraft-midi" -version = "0.19.1" +version = "0.20.0" dependencies = [ "proptest", "serde", @@ -4467,7 +4467,7 @@ dependencies = [ [[package]] name = "soundcraft-mix" -version = "0.19.1" +version = "0.20.0" dependencies = [ "craft-audio", "log", @@ -4483,7 +4483,7 @@ dependencies = [ [[package]] name = "soundcraft-model" -version = "0.19.1" +version = "0.20.0" dependencies = [ "craft-audio", "proptest", @@ -4496,7 +4496,7 @@ dependencies = [ [[package]] name = "soundcraft-playback" -version = "0.19.1" +version = "0.20.0" dependencies = [ "cpal", "craft-audio", @@ -4511,7 +4511,7 @@ dependencies = [ [[package]] name = "soundcraft-time" -version = "0.19.1" +version = "0.20.0" dependencies = [ "proptest", "serde", @@ -4521,7 +4521,7 @@ dependencies = [ [[package]] name = "soundcraft-ui-egui" -version = "0.19.1" +version = "0.20.0" dependencies = [ "craft-audio", "craft-ui", @@ -4551,7 +4551,7 @@ dependencies = [ [[package]] name = "soundcraft-video" -version = "0.19.1" +version = "0.20.0" dependencies = [ "craft-media", "image", @@ -4563,7 +4563,7 @@ dependencies = [ [[package]] name = "soundcraft-vst3-host" -version = "0.19.1" +version = "0.20.0" dependencies = [ "libloading 0.9.0", "log", @@ -4579,7 +4579,7 @@ dependencies = [ [[package]] name = "soundcraft-web" -version = "0.19.1" +version = "0.20.0" dependencies = [ "craft-audio", "eframe", @@ -6227,7 +6227,7 @@ checksum = "e450f9b2ed1dff33c94c12589a87338689467b9c4f5d8a5710bd09a847d2c8a7" [[package]] name = "xtask" -version = "0.19.1" +version = "0.20.0" dependencies = [ "serde_json", ] diff --git a/Cargo.toml b/Cargo.toml index 9f65b231..20abb0c3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -4,7 +4,7 @@ members = ["crates/*", "apps/*", "xtask"] default-members = ["crates/*", "apps/*", "xtask"] [workspace.package] -version = "0.19.1" +version = "0.20.0" edition = "2024" license = "MIT OR Apache-2.0" rust-version = "1.90" diff --git a/crates/engine/src/autosave.rs b/crates/engine/src/autosave.rs index f76701f1..1be701c7 100644 --- a/crates/engine/src/autosave.rs +++ b/crates/engine/src/autosave.rs @@ -454,6 +454,32 @@ pub fn create_folder(dir: &Path) -> std::io::Result<()> { builder.create(dir) } +/// A new folder directly in `parent`, named `{prefix}-` and 32 random hex digits. It is made +/// exclusively (it is created by this call or the call fails), so a folder or link another account +/// put there first is never used, and the name cannot be guessed ahead. On Unix it is owner-only +/// (`0o700`); on other systems it has the mode `parent` gives it. +pub fn create_unique_folder(parent: &Path, prefix: &str) -> std::io::Result { + use std::hash::{BuildHasher, Hasher}; + // One plain name: a prefix like `../x` would make a sibling of `parent`, not a child. + let mut parts = Path::new(prefix).components(); + if !matches!((parts.next(), parts.next()), (Some(std::path::Component::Normal(_)), None)) || prefix.contains(['/', '\\']) { + return Err(std::io::Error::new(std::io::ErrorKind::InvalidInput, "the folder prefix must be a single name")); + } + for _ in 0..16 { + let random = |_| std::collections::hash_map::RandomState::new().build_hasher().finish(); + let dir = parent.join(format!("{prefix}-{:016x}{:016x}", random(0), random(1))); + let mut builder = std::fs::DirBuilder::new(); + #[cfg(unix)] + std::os::unix::fs::DirBuilderExt::mode(&mut builder, 0o700); + match builder.create(&dir) { + Ok(()) => return Ok(dir), + Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => {} + Err(err) => return Err(err), + } + } + Err(std::io::Error::new(std::io::ErrorKind::AlreadyExists, "no unused folder name")) +} + fn write_synced(path: &Path, bytes: &[u8]) -> std::io::Result<()> { let mut f = std::fs::File::create(path)?; f.write_all(bytes)?; @@ -477,6 +503,25 @@ mod tests { use super::*; use crate::io::open_session; use craft_audio::AudioBuffer; + + #[test] + fn unique_folders_are_new_and_owner_only() { + let parent = std::env::temp_dir().join(format!("soundcraft-unique-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&parent); + std::fs::create_dir_all(&parent).unwrap(); + let (a, b) = (create_unique_folder(&parent, "T").unwrap(), create_unique_folder(&parent, "T").unwrap()); + assert_ne!(a, b); + assert!(a.is_dir() && a.parent() == Some(parent.as_path())); + assert_eq!(a.file_name().unwrap().len(), "T-".len() + 32); + #[cfg(unix)] + assert_eq!(std::os::unix::fs::PermissionsExt::mode(&std::fs::metadata(&a).unwrap().permissions()) & 0o777, 0o700); + assert!(create_unique_folder(&parent.join("missing"), "T").is_err(), "no parents are made"); + for bad in ["../outside", "a/b", "a\\b", "..", ".", "", "/abs"] { + assert!(create_unique_folder(&parent, bad).is_err(), "{bad:?} is not a single name"); + } + let _ = std::fs::remove_dir_all(&parent); + } + use serde_json::json; use soundcraft_model::{ChannelFormat, Clip, FileFormat, TrackKind}; diff --git a/crates/engine/src/clip_group_file.rs b/crates/engine/src/clip_group_file.rs index 23377428..05fb39eb 100644 --- a/crates/engine/src/clip_group_file.rs +++ b/crates/engine/src/clip_group_file.rs @@ -88,20 +88,27 @@ fn decode_group_audio( cmd_id: &str, ) -> Result<(Vec<(String, craft_audio::AudioBuffer)>, BTreeMap)> { 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(EngineError::BadParams(cmd_id.into(), format!("audio path escapes the group folder: {rel}"))); + return Err(escapes(rel)); + } + let key = crate::io::canonical_path(&full)?; + if !key.starts_with(&root) { + return Err(escapes(rel)); } - let key = crate::io::file_key(&full); let n = match index.get(&key) { Some(n) => *n, None => { - let bytes = crate::io::read_file(&full)?; + let bytes = crate::io::read_file_inside(&full, &root)?; let (_, buf) = craft_audio::decode(&bytes, Some("wav")).map_err(|err| EngineError::Io(format!("{rel}: {err}")))?; let buf = crate::io::at_rate(buf, rate, crate::io::MAX_IMPORT_SAMPLES) .map_err(|r| EngineError::Io(format!("{rel}: {}", r.message("too large to import"))))?; @@ -269,9 +276,59 @@ 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())); diff --git a/crates/engine/src/edit.rs b/crates/engine/src/edit.rs index 636c9c55..6faa03b1 100644 --- a/crates/engine/src/edit.rs +++ b/crates/engine/src/edit.rs @@ -136,6 +136,45 @@ fn split_at(s: &mut Session, track: TrackId, at: Samples, auto_name: impl Fn(&Cl count } +/// Split every clip on a track's active playlist at each of `points`, in one pass over the +/// playlist: [`separate_at_if`] once per point with `|_| false` (no piece takes an Auto Rename +/// name here), without rebuilding the playlist per point. Returns the number of splits. +/// `points` are sorted and deduplicated first, so the source offsets of stretched audio can differ +/// from per-point calls given in another order (each trim rounds its own adjustment). +pub fn separate_at_points(s: &mut Session, track: TrackId, points: &[Samples]) -> usize { + let mut pts = points.to_vec(); + pts.sort_unstable(); + pts.dedup(); + let tc_tempo = s.tempo.clone(); + let t = TimeCtx { tempo: &tc_tempo, sr: s.sample_rate }; + // The points strictly inside a clip. + let within = |c: &Clip| { + let (lo, hi) = (pts.partition_point(|p| *p <= c.start), pts.partition_point(|p| *p < c.end())); + lo..hi.max(lo) + }; + let n_split: usize = s.track(track).map_or(0, |tr| tr.clips().iter().map(|c| within(c).len()).sum()); + let mut ids: Vec = (0..n_split).map(|_| s.new_clip_id()).collect(); + let Some(pl) = s.track_mut(track).and_then(|t| t.playlist_mut()) else { return 0 }; + let mut out = Vec::with_capacity(pl.clips.len() + n_split); + for c in pl.clips.drain(..) { + let mut rest = c; + for p in &pts[within(&rest)] { + let Some(id) = ids.pop() else { break }; + match split_clip(&rest, *p, id, t) { + Some((l, r)) => { + out.push(l); + rest = r; + } + None => break, + } + } + out.push(rest); + } + pl.clips = out; + pl.sort(); + n_split +} + /// Gives `c` the next name of its Auto Rename scheme, when it has one, and counts the scheme up. fn take_auto_name(schemes: &mut std::collections::BTreeMap, c: &mut Clip) { if let Some(scheme) = c.auto_name.and_then(|k| schemes.get_mut(&k)) { diff --git a/crates/engine/src/io.rs b/crates/engine/src/io.rs index 02181010..90979672 100644 --- a/crates/engine/src/io.rs +++ b/crates/engine/src/io.rs @@ -66,6 +66,69 @@ fn read_file_within(path: &Path, session: bool) -> Result> { Ok(data) } +/// `O_NONBLOCK` of the platforms that have a FIFO to wait on (std has no name for it, and this crate has no unsafe +/// code or libc); 0 elsewhere, where a FIFO open still waits for a writer. +#[cfg(unix)] +const O_NONBLOCK: i32 = if cfg!(all( + any(target_os = "linux", target_os = "android"), + not(any( + target_arch = "mips", + target_arch = "mips32r6", + target_arch = "mips64", + target_arch = "mips64r6", + target_arch = "sparc", + target_arch = "sparc64" + )) +)) { + 0x800 +} else if cfg!(any( + target_os = "macos", + target_os = "ios", + target_os = "freebsd", + target_os = "netbsd", + target_os = "openbsd", + target_os = "dragonfly" +)) { + 0x4 +} else { + 0 +}; + +/// [`read_file`] for a file that must lie inside `root` (a canonical folder): the file is opened +/// once, and the bytes come from that handle only after the handle is shown to be the file the +/// path resolves to inside `root`. A file swapped in for the path after a check by name is +/// refused instead of read. Where the handle's identity can't be read (non-Unix targets) the +/// resolved path is checked against `root` alone. +pub(crate) fn read_file_inside(path: &Path, root: &Path) -> Result> { + use std::io::Read; + crate::file_access::check_read(path).map_err(EngineError::Io)?; + let err = |err: std::io::Error| EngineError::Io(format!("{}: {err}", path.display())); + // opened without waiting for a FIFO's writer (the type is checked on the opened handle, below) + let mut options = std::fs::OpenOptions::new(); + options.read(true); + #[cfg(unix)] + std::os::unix::fs::OpenOptionsExt::custom_flags(&mut options, O_NONBLOCK); + let mut file = options.open(path).map_err(err)?; + if !file.metadata().map_err(err)?.is_file() { + return Err(EngineError::Io(format!("{}: not a regular file", path.display()))); + } + let key = canonical_path(path)?; + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + let (a, b) = (file.metadata().map_err(err)?, std::fs::metadata(&key).map_err(err)?); + if (a.dev(), a.ino()) != (b.dev(), b.ino()) { + return Err(EngineError::Io(format!("{}: changed while it was opened", path.display()))); + } + } + if !key.starts_with(root) { + return Err(EngineError::Io(format!("{}: outside the folder it must stay in", path.display()))); + } + let mut bytes = Vec::new(); + file.read_to_end(&mut bytes).map_err(err)?; + Ok(bytes) +} + /// [`read_file`] as UTF-8 text. pub fn read_text(path: impl AsRef) -> Result { let path = path.as_ref(); @@ -193,6 +256,17 @@ pub(crate) fn make_dir(dir: impl AsRef) -> 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) -> Result { + 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, bytes: impl AsRef<[u8]>) -> Result<()> { let path = path.as_ref(); @@ -451,7 +525,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, std::fs::File::create(part)?))) + Ok(Out::File(std::io::BufWriter::with_capacity(1 << 20, create_new(part)?))) } /// Puts the finished file at `path` and returns its size: the part file is synced and renamed @@ -583,8 +657,28 @@ pub fn export_clips(e: &Engine, ids: &[soundcraft_model::ClipId], dir: &str) -> let audio = soundcraft_mix::render_clips(s, t, c.range()); let buf = AudioBuffer { sample_rate: s.sample_rate.hz(), channels: audio }; let bytes = craft_audio::encode(&buf, &EncodeOptions::default()).map_err(|err| EngineError::Io(err.to_string()))?; - let path = clip_file(dir, &sanitize_name(&c.name), &mut names, Path::exists)?; - write_file(&path.to_string_lossy(), &bytes)?; + let stem = sanitize_name(&c.name); + if cfg!(target_arch = "wasm32") { + let path = clip_file(dir, &stem, &mut names, Path::exists)?; + write_file(&path.to_string_lossy(), &bytes)?; + continue; + } + // 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. 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. + let (mut file, path) = loop { + let path = clip_file(dir, &stem, &mut names, |p| std::fs::symlink_metadata(p).is_ok())?; + 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, path), + // taken since it was looked at: `names` has it now, so the next name is tried + Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => {} + Err(err) => return Err(EngineError::Io(format!("{}: {err}", path.display()))), + } + }; + std::io::Write::write_all(&mut file, &bytes).map_err(|err| EngineError::Io(format!("{}: {err}", path.display())))?; } Ok(names.taken.len()) } @@ -649,13 +743,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() && dir.join(&src.path).exists(); + let exists_rel = same_dir && !Path::new(&src.path).is_absolute() && file_found(dir.join(&src.path)); 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 && Path::new(&src.path).is_file()) { + if let Some(f) = in_folder.filter(|_| !src.unsaved && file_found(&src.path)) { src.path = format!("Audio Files/{f}"); continue; } @@ -677,8 +771,11 @@ fn write_session(session: &Session, path: &str, overwrite: bool) -> Result<(Sess let mut stem = sanitize_name(&src.name); if overwrite { // One file per source id so autosaves never collide or pile up. + // (With only an automation write root, `file_found` cannot read an existing file, so each save + // writes a new numbered WAV: reuse never reads a file the roots do not allow, which keeps a save + // from revealing whether an unreadable file matches the audio.) stem = format!("{stem}_{}", src.id.0); - if audio_dir.join(format!("{stem}.wav")).exists() { + if file_found(audio_dir.join(format!("{stem}.wav"))) { src.path = format!("Audio Files/{stem}.wav"); src.unsaved = false; files.insert(Arc::as_ptr(audio), src.path.clone()); @@ -761,11 +858,22 @@ fn sync_dir(dir: &Path) { fn write_synced(path: &Path, bytes: &[u8]) -> std::io::Result<()> { use std::io::Write; - let mut f = std::fs::File::create(path)?; + let mut f = create_new(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 { + 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)?; @@ -845,7 +953,8 @@ fn same_path(a: &Path, b: &Path) -> bool { /// Store `bytes` as the first of `.wav`, `_01.wav`, … in `dir` that is free or already /// holds exactly these bytes, so saving never replaces audio; returns the name and whether it was -/// written. +/// written. Where the automation roots allow writing in `dir` but not reading (only a write root), +/// an existing file is never compared, so each save writes a new numbered WAV. fn store_wav(dir: &Path, stem: &str, bytes: &[u8]) -> Result<(String, bool)> { use std::io::Write; for k in 0..10_000u32 { @@ -860,7 +969,12 @@ 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 => { - 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) { + // 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) + { return Ok((name, false)); } } @@ -1059,6 +1173,89 @@ 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 a_file_read_within_a_root_must_resolve_inside_it() { + let dir = scratch("within"); + let (inside, outside) = (dir.join("group"), dir.join("elsewhere")); + std::fs::create_dir_all(&inside).unwrap(); + std::fs::create_dir_all(&outside).unwrap(); + std::fs::write(inside.join("a.wav"), b"a").unwrap(); + std::fs::write(outside.join("b.wav"), b"b").unwrap(); + std::os::unix::fs::symlink(outside.join("b.wav"), inside.join("link.wav")).unwrap(); + let root = canonical_path(&inside).unwrap(); + assert_eq!(read_file_inside(&inside.join("a.wav"), &root).unwrap(), b"a"); + assert!(read_file_inside(&inside.join("link.wav"), &root).is_err()); + // a FIFO with no writer is refused, not waited on (Linux: the coreutils tool makes it) + #[cfg(target_os = "linux")] + if std::process::Command::new("mkfifo").arg(inside.join("pipe.wav")).status().is_ok_and(|s| s.success()) { + let err = read_file_inside(&inside.join("pipe.wav"), &root).unwrap_err().to_string(); + assert!(err.contains("not a regular file"), "{err}"); + } + 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] diff --git a/crates/engine/src/io/strip.rs b/crates/engine/src/io/strip.rs index ec808241..981bc0db 100644 --- a/crates/engine/src/io/strip.rs +++ b/crates/engine/src/io/strip.rs @@ -117,9 +117,7 @@ pub fn strip_silence( } cut_points.push(r.end); let old: std::collections::HashSet<_> = s.track(t).map(|x| x.clips().iter().map(|c| c.id).collect()).unwrap_or_default(); - for p in &cut_points { - crate::edit::separate_at_if(s, t, *p, |_| false); - } + crate::edit::separate_at_points(s, t, &cut_points); let kept = strip_pieces(s, t, r, &keep, mode); crate::edit::auto_name_new_clips(s, t, &old, |c| !(named && c.is_audio() && c.start >= r.start && c.end() <= r.end)); Ok(kept) @@ -144,10 +142,24 @@ fn strip_pieces(s: &mut Session, t: TrackId, r: Range, keep: &[(Samples, Samples let audio: Vec = keep.iter().map(|(a, b)| Range::new(*a, *b)).collect(); let remove = if mode == StripMode::Extract { &audio } else { &gaps }; let inside = |c: &Clip, g: &Range| c.start >= g.start && c.end() <= g.end; + // The range of `rs` holding `c`: by binary search when `rs` is ascending and disjoint (the + // usual case, checked once by the caller), by scan otherwise. + let holder = |rs: &[Range], sorted: bool, c: &Clip| { + if sorted { + let i = rs.partition_point(|g| g.end < c.end()); + rs.get(i).filter(|g| inside(c, g)).map(|_| i) + } else { + rs.iter().position(|g| inside(c, g)) + } + }; + let ordered = |rs: &[Range]| rs.windows(2).all(|w| w[0].end <= w[1].start); let Some(pl) = s.track_mut(t).and_then(|x| x.playlist_mut()) else { return if mode == StripMode::Extract { 0 } else { keep.len() } }; - pl.clips.retain(|c| !remove.iter().any(|g| inside(c, g))); + let sorted = ordered(remove); + pl.clips.retain(|c| holder(remove, sorted, c).is_none()); if mode == StripMode::Extract { - return gaps.iter().filter(|g| pl.clips.iter().any(|c| inside(c, g))).count(); + let sorted = ordered(&gaps); + let held: std::collections::HashSet = pl.clips.iter().filter_map(|c| holder(&gaps, sorted, c)).collect(); + return held.len(); } keep.len() } @@ -294,6 +306,57 @@ mod strip_tests { (r, e.session().track(t).unwrap().clips().iter().map(|c| (c.start, c.end())).collect()) } + #[test] + fn cutting_at_many_points_in_one_pass_equals_cutting_at_each() { + let spans = |e: &Engine, t: TrackId| { + let mut v: Vec<_> = e.session().track(t).unwrap().clips().iter().map(|c| (c.start, c.end())).collect(); + v.sort_unstable(); + v + }; + let points = [30_000, 10, 5_000, 30_000, 120_000, 0, 20_000, 1_000_000]; + let (mut one, t) = tones(&[0, 20_000, 100_000], 40_000); + let (mut each, _) = tones(&[0, 20_000, 100_000], 40_000); + crate::edit::separate_at_points(one.session_mut(), t, &points); + for p in points { + crate::edit::separate_at_if(each.session_mut(), t, p, |_| false); + } + assert_eq!(spans(&one, t), spans(&each, t)); + assert!(spans(&one, t).len() > 3); + let ids: std::collections::HashSet<_> = one.session().track(t).unwrap().clips().iter().map(|c| c.id).collect(); + assert_eq!(ids.len(), spans(&one, t).len(), "new pieces have distinct ids"); + } + + #[test] + fn cutting_stretched_audio_at_ascending_points_matches_cutting_at_each() { + let pieces = |e: &Engine, t: TrackId| { + let mut v: Vec<_> = e + .session() + .track(t) + .unwrap() + .clips() + .iter() + .map(|c| (c.start, c.end(), if let soundcraft_model::ClipContent::Audio { offset, .. } = c.content { offset } else { -1 })) + .collect(); + v.sort_unstable(); + v + }; + let stretched = || { + let (mut e, t) = tones(&[0], 40_000); + for c in &mut e.session_mut().track_mut(t).unwrap().playlist_mut().unwrap().clips { + c.stretch = 2.0; + } + (e, t) + }; + let points = [1, 2, 3_001, 12_345]; + let ((mut one, t), (mut each, _)) = (stretched(), stretched()); + crate::edit::separate_at_points(one.session_mut(), t, &points); + for p in points { + crate::edit::separate_at_if(each.session_mut(), t, p, |_| false); + } + assert_eq!(pieces(&one, t), pieces(&each, t)); + assert_eq!(pieces(&one, t).len(), points.len() + 1); + } + #[test] fn strip_keeps_the_audio_and_removes_the_silence() { let (r, clips) = strip_mode("strip"); diff --git a/crates/ui-egui/src/app.rs b/crates/ui-egui/src/app.rs index 0808b026..28268906 100644 --- a/crates/ui-egui/src/app.rs +++ b/crates/ui-egui/src/app.rs @@ -46,6 +46,7 @@ impl SoundApp { services, native_menu_bar: false, autosave_dir: None, + fallback_take_dir: None, autosaver: None, autosaved_rev: 0, leftovers: Vec::new(), diff --git a/crates/ui-egui/src/app/playback.rs b/crates/ui-egui/src/app/playback.rs index ccb9e742..645a0d5f 100644 --- a/crates/ui-egui/src/app/playback.rs +++ b/crates/ui-egui/src/app/playback.rs @@ -155,15 +155,23 @@ impl SoundApp { /// autosave folder. Returns the takes it records. fn arm_recorder(&mut self) -> Result, String> { let name = soundcraft_engine::io::sanitize_name(&self.engine.session().name); - let unsaved_dir = self.autosave_dir.as_ref().map_or_else(std::env::temp_dir, |d| d.join(name)); + // Without a settings folder the takes go under the temporary folder, in a folder of this + // run's own that nobody else could have made first. + let unsaved_dir = match &self.autosave_dir { + Some(d) => d.join(name), + // A saved session records beside its file: `plan_takes` never uses this folder. + None if self.engine.path.is_some() => std::path::PathBuf::new(), + None => self.fallback_take_dir()?, + }; let Some(r) = &mut self.recorder else { return Err("no audio input device".into()) }; let plan = soundcraft_engine::io::plan_takes(&self.engine, &unsaved_dir, r.channels); if plan.is_empty() { return Err("no audio track is record-enabled".into()); } - // The takes are unsaved work: the folder they go into under the autosave root is - // owner-only, like the autosaves, before the recorder makes anything in it. - if self.engine.path.is_none() && self.autosave_dir.is_some() { + // The takes are unsaved work: on Unix the folder they go into is owner-only, like the + // autosaves, before the recorder makes anything in it (other systems get the folder's + // inherited access: no ACLs are set, so the temporary folder's own per-user access applies). + if self.engine.path.is_none() { for dir in plan.iter().filter_map(|(_, _, path)| path.parent()) { soundcraft_engine::autosave::create_folder(dir).map_err(|e| format!("{}: {e}", dir.display()))?; } @@ -176,6 +184,19 @@ impl SoundApp { Ok(takes) } + /// The folder for the takes of a session never saved when there is no autosave folder, made on + /// first use: exclusively and under a name nobody can guess, so another account cannot have + /// made it first, with open permissions or as a link. + fn fallback_take_dir(&mut self) -> Result { + if let Some(dir) = &self.fallback_take_dir { + return Ok(dir.clone()); + } + let dir = soundcraft_engine::autosave::create_unique_folder(&std::env::temp_dir(), "SoundCraft") + .map_err(|e| format!("temporary take folder: {e}"))?; + self.fallback_take_dir = Some(dir.clone()); + Ok(dir) + } + fn finish_recording(&mut self) { self.engine.transport.recording = false; if let Some(p) = &self.player { @@ -453,3 +474,18 @@ impl SoundApp { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn the_fallback_take_folder_is_made_once_under_the_temporary_folder() { + let mut app = SoundApp::new(soundcraft_engine::Engine::default(), None, crate::Services::default()); + let a = app.fallback_take_dir().unwrap(); + assert_eq!(a.parent(), Some(std::env::temp_dir().as_path())); + assert!(a.is_dir()); + assert_eq!(app.fallback_take_dir().unwrap(), a, "the same folder for the rest of the run"); + let _ = std::fs::remove_dir_all(&a); + } +} diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index a8dc9672..7d685d1a 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -291,6 +291,8 @@ pub struct SoundApp { pub native_menu_bar: bool, /// Folder for crash-recovery autosaves (native apps set it). pub autosave_dir: Option, + /// Where the takes of a session never saved go without an autosave folder: made once per run, under the temporary folder. + fallback_take_dir: Option, /// Writes the open session's autosaves in the background, once there is something to write. autosaver: Option, /// The document revision last handed to the autosaver (0 once that autosave is stale). diff --git a/crates/ui-egui/src/panels/midi_editor.rs b/crates/ui-egui/src/panels/midi_editor.rs index 2e48be01..77d81df6 100644 --- a/crates/ui-egui/src/panels/midi_editor.rs +++ b/crates/ui-egui/src/panels/midi_editor.rs @@ -265,20 +265,42 @@ fn draw_roll_background(painter: &egui::Painter, g: &Roll<'_>) { } } // Grid. + let step = grid_stride(grid_ticks, px_per_tick); let mut tick = 0; while tick <= len_ticks { let bar = tick % (TICKS_PER_QUARTER * 4) == 0; let x = g.x_of(tick); - if grid_ticks as f32 * px_per_tick > 4.0 || bar { - painter.line_segment( - [pos2(x, roll.min.y), pos2(x, roll.max.y)], - Stroke::new(1.0, if bar { Color32::from_rgb(80, 80, 86) } else { Color32::from_rgb(50, 50, 54) }), - ); - } - tick += grid_ticks; + painter.line_segment( + [pos2(x, roll.min.y), pos2(x, roll.max.y)], + Stroke::new(1.0, if bar { Color32::from_rgb(80, 80, 86) } else { Color32::from_rgb(50, 50, 54) }), + ); + tick = tick.saturating_add(step); } } +/// Closest lines the grid draws, in points. +const MIN_GRID_PX: f32 = 4.0; + +/// Ticks between grid lines: the grid when its lines are more than [`MIN_GRID_PX`] apart, else +/// as few common multiples of the grid and a bar as keep that spacing (so every line is a snap +/// position and falls on a bar). The lines drawn are then bounded by the roll's width, however long +/// the clip, instead of by its length. +fn grid_stride(grid_ticks: i64, px_per_tick: f32) -> i64 { + let bar_ticks = TICKS_PER_QUARTER * 4; + if grid_ticks as f32 * px_per_tick > MIN_GRID_PX { + return grid_ticks.max(1); + } + let grid = grid_ticks.max(1); + let (mut a, mut b) = (grid, bar_ticks); + while b != 0 { + (a, b) = (b, a % b); + } + let common = (grid / a).saturating_mul(bar_ticks); + // `as` saturates a huge or NaN quotient; a zero `px_per_tick` gives one common multiple. + let n = (MIN_GRID_PX / (common as f32 * px_per_tick)).ceil().max(1.0) as i64; + common.saturating_mul(n) +} + /// The notes, with a drag in progress applied. fn draw_notes(app: &SoundApp, painter: &egui::Painter, g: &Roll<'_>, base: Color32) { let Roll { sequence, grid_ticks, .. } = *g; @@ -478,6 +500,24 @@ fn velocity_lane(app: &mut SoundApp, ui: &mut Ui, g: &Roll<'_>, cid: ClipId, bas mod tests { use super::*; + #[test] + fn grid_lines_are_bounded_by_the_roll_width_not_the_clip_length() { + let sixteenth = TICKS_PER_QUARTER / 4; + // A 1000 pt roll showing a 4-bar clip: every sixteenth is 16 pt apart and drawn. + assert_eq!(grid_stride(sixteenth, 1000.0 / (16.0 * TICKS_PER_QUARTER as f32)), sixteenth); + // The same roll over a clip a billion ticks long: at most a line every 4 pt. + let (len, px_per_tick) = (1_000_000_000i64, 1000.0 / 1_000_000_000.0f32); + let step = grid_stride(sixteenth, px_per_tick); + assert_eq!(step % (TICKS_PER_QUARTER * 4), 0, "lines fall on bars"); + assert!((len / step) <= 251, "{} lines", len / step); + // a dotted quarter grid (1440 ticks): every line is also a grid position + let dotted = TICKS_PER_QUARTER * 3 / 2; + let step = grid_stride(dotted, px_per_tick); + assert_eq!((step % dotted, step % (TICKS_PER_QUARTER * 4)), (0, 0), "lines are snap positions on bars: {step}"); + assert!((len / step) <= 251, "{} lines", len / step); + assert!(grid_stride(0, 0.0) >= TICKS_PER_QUARTER * 4, "a zero grid or width still advances"); + } + #[test] fn switching_clip_drops_selection_and_gestures() { let mut st = MidiEditorState { selected: vec![0, 2], clip: Some(ClipId(1)), band: Some(pos2(1.0, 2.0)), ..Default::default() }; diff --git a/crates/ui-egui/src/panels/video_window.rs b/crates/ui-egui/src/panels/video_window.rs index 3e66e165..a6ca6751 100644 --- a/crates/ui-egui/src/panels/video_window.rs +++ b/crates/ui-egui/src/panels/video_window.rs @@ -115,6 +115,22 @@ fn first_movie_aspect(s: &soundcraft_model::Session) -> Option { s.videos.first().map(|v| v.aspect() as f32) } +/// Most thumbnails one clip draws in the Video Universe strip: 8 pt each, so more than the +/// widest strip holds. +const MAX_UNIVERSE_THUMBS: usize = 1 << 12; + +/// Left edges of the thumbnails of a clip starting at `clip_x` (each `tw` wide) that lie in +/// `shown_min..shown_max`. The index and position are worked out in `f64`: an `f32` this far from +/// the origin (a clip starting a billion points left) cannot step by a thumbnail's width. +fn thumb_xs(clip_x: f32, shown_min: f32, shown_max: f32, tw: f32) -> impl Iterator { + let (origin, tw) = (f64::from(clip_x), f64::from(tw)); + // Nothing of the clip is in view when the shown span is empty (or inverted). + let empty = shown_max <= shown_min; + let first = ((f64::from(shown_min) - origin) / tw).floor().max(0.0); + let count = if empty { 0 } else { ((f64::from(shown_max) - origin) / tw - first).ceil().max(0.0) as usize }; + (0..count.min(MAX_UNIVERSE_THUMBS)).map(move |i| (origin + (first + i as f64) * tw) as f32) +} + fn universe(app: &mut SoundApp, ctx: &egui::Context) { app.video.poll(ctx); let s = app.engine.session_arc(); @@ -145,8 +161,10 @@ fn universe(app: &mut SoundApp, ctx: &egui::Context) { } let tw = (cr.height() * v.aspect() as f32).max(8.0); let hpx = ((cr.height() * ppp / 16.0).ceil() * 16.0).clamp(16.0, 256.0) as u32; - let mut x = cr.min.x; - while x < cr.max.x { + // Only the thumbnails inside the strip, counted rather than stepped: a clip starting + // far left of it has an edge so large that adding a thumbnail's width wouldn't move it. + let shown = cr.intersect(r); + for x in thumb_xs(cr.min.x, shown.min.x, shown.max.x, tw) { let secs = movie_secs(&s, c.start, offset, strip_position(x, r.min.x, r.width(), end, c.start)).max(0.0); if let Some(tex) = app.video.thumb(ctx, key, &v.path, secs, hpx) { let tr = Rect::from_min_size(pos2(x, cr.min.y), vec2(tw, cr.height())); @@ -154,7 +172,6 @@ fn universe(app: &mut SoundApp, ctx: &egui::Context) { let uv = Rect::from_min_max(pos2(0.0, 0.0), pos2((vis.width() / tw).clamp(0.0, 1.0), 1.0)); painter.image(tex.id(), vis, uv, Color32::WHITE); } - x += tw; } } // The Edit window's visible range and the playhead. @@ -182,7 +199,21 @@ fn universe(app: &mut SoundApp, ctx: &egui::Context) { #[cfg(test)] mod tests { - use super::strip_position; + use super::{strip_position, thumb_xs}; + + #[test] + fn thumbnails_of_a_clip_far_left_of_the_strip_are_evenly_spaced() { + // 2^30 points left of the strip with 8 pt thumbnails: f32 steps of `first + i` here are 16 apart + let xs: Vec = thumb_xs(-1_073_741_824.0, 0.0, 80.0, 8.0).collect(); + assert_eq!(xs.len(), 10); + assert!(xs.windows(2).all(|w| w[1] - w[0] == 8.0), "{xs:?}"); + assert_eq!(xs[0], 0.0); + // a clip inside the strip starts at its own edge + assert_eq!(thumb_xs(10.0, 10.0, 34.0, 8.0).collect::>(), vec![10.0, 18.0, 26.0]); + // a clip ending left of the strip (the shown span is empty or inverted) draws none + assert_eq!(thumb_xs(-10.0, 0.0, -1.0, 8.0).count(), 0); + assert_eq!(thumb_xs(-10.0, 0.0, 0.0, 8.0).count(), 0); + } // A 1024 px strip over 1_024_000 samples: 1000 samples per point, exact in f32. const LEFT: f32 = 100.0; diff --git a/crates/ui-egui/src/video_track.rs b/crates/ui-egui/src/video_track.rs index eb6dfca1..6a2f7e77 100644 --- a/crates/ui-egui/src/video_track.rs +++ b/crates/ui-egui/src/video_track.rs @@ -90,6 +90,13 @@ impl Worker { } fn open_movie(path: &str) -> Result { + // The last check before the file system: the path came from a session file, so the read + // roots apply, and only a regular file is opened (a pipe would hold the video worker + // forever). + soundcraft_engine::file_access::check_read(path)?; + if !std::fs::metadata(path).map_err(|e| format!("{path}: {e}"))?.is_file() { + return Err(format!("{path}: not a regular file")); + } #[cfg(not(target_arch = "wasm32"))] { let f = std::fs::File::open(path).map_err(|e| format!("{path}: {e}"))?; @@ -491,3 +498,24 @@ pub fn draw_lane(app: &mut SoundApp, painter: &Painter, s: &Session, track: &Tra } } } + +#[cfg(all(test, not(target_arch = "wasm32")))] +mod tests { + use super::open_movie; + + #[test] + fn only_a_regular_movie_file_is_opened() { + let dir = std::env::temp_dir().join(format!("soundcraft-open-movie-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let (folder, missing, junk, movie) = (dir.to_string_lossy().to_string(), dir.join("none.mp4"), dir.join("junk.mp4"), dir.join("a.mp4")); + assert!(open_movie(&folder).err().unwrap_or_default().contains("not a regular file")); + assert!(open_movie(&missing.to_string_lossy()).is_err()); + std::fs::write(&junk, b"not a movie").unwrap(); + let err = open_movie(&junk.to_string_lossy()).err().unwrap_or_default(); + assert!(!err.is_empty() && !err.contains("not a regular file"), "{err}"); + std::fs::write(&movie, soundcraft_video::synth::h264_mp4(2, 2).unwrap()).unwrap(); + assert!(open_movie(&movie.to_string_lossy()).is_ok()); + let _ = std::fs::remove_dir_all(&dir); + } +} diff --git a/crates/video/src/movie.rs b/crates/video/src/movie.rs index bbed0230..c37a1b13 100644 --- a/crates/video/src/movie.rs +++ b/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> { 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,13 @@ 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: a frame may differ from + // the track's size (as before), but not exceed `MAX_PIXELS`. + 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 u64::from(w) * u64::from(h) > MAX_PIXELS { + return Err(VideoError::Decode("picture too large".into())); + } 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 +469,14 @@ impl Movie { Ok(self.h264_to_frame(p, index)) } + /// Most sample bytes one frame request feeds the decoder (eight 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 = 2 << 30; + /// 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> { 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 +491,7 @@ impl Movie { } let total = self.order.len(); let mut last_err: Option = 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 +506,29 @@ 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, + // an unreadable sample is skipped, as before, and counts as a sample without a picture: the + // request's byte and no-picture budgets bound the walk Err(e) => { last_err = Some(e.to_string()); + without_picture += 1; + 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 + ))); + } continue; } }; + 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 +543,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); diff --git a/crates/video/src/tests.rs b/crates/video/src/tests.rs index 741db023..4b680283 100644 --- a/crates/video/src/tests.rs +++ b/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, 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,80 @@ 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() { + // 65 frames whose `stsz` entries each claim 32 MiB: under the per-sample cap, but together + // past the 2 GiB one request may feed the decoder. + let mut bytes = h264_mp4(65, 65).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..65 { + 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: 3 << 30, largest: AtomicUsize::new(0) }); + let mut m = Movie::open_source(src).unwrap(); + let r = m.frame(64); + assert!(matches!(&r, Err(VideoError::Decode(e)) if e.contains("needs more than 2048 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(64); + assert!(matches!(&again, Err(VideoError::Decode(e)) if e.contains("needs more than 2048 MiB of samples")), "{again:?}"); +} + +/// A source that, once `failing` is set, cannot be read at all. +struct UnreadableSamples { + bytes: Vec, + failing: std::sync::atomic::AtomicBool, +} + +impl ByteSource for UnreadableSamples { + fn len(&self) -> u64 { + self.bytes.len() as u64 + } + fn read_at(&self, offset: u64, buf: &mut [u8]) -> std::io::Result<()> { + let start = usize::try_from(offset).unwrap_or(usize::MAX); + if self.failing.load(Ordering::Relaxed) || start.saturating_add(buf.len()) > self.bytes.len() { + return Err(std::io::Error::other("unreadable")); + } + buf.copy_from_slice(&self.bytes[start..start + buf.len()]); + Ok(()) + } +} + +#[test] +fn a_request_over_unreadable_samples_stops_at_the_no_picture_budget() { + // 400 frames of one GOP, none of whose samples can be read once the movie is open: the walk gives up after 256 skipped samples + let bytes = h264_mp4(400, 400).unwrap(); + let src = Arc::new(UnreadableSamples { bytes, failing: false.into() }); + let mut m = Movie::open_source(src.clone()).unwrap(); + src.failing.store(true, Ordering::Relaxed); + let r = m.frame(399); + assert!(matches!(&r, Err(VideoError::Decode(e)) if e.contains("no picture in 256 samples")), "{r:?}"); +} + +#[test] +fn mjpeg_frame_size_is_capped_from_its_header_but_may_differ_from_the_track() { + // A frame whose header differs from the track's size still decodes (within the dimension cap, as before)... + 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(&[0, 8, 0, 8]); + let mut m = Movie::open(bytes).unwrap(); + assert!(m.frame(0).is_ok()); + // ...but one claiming 16 000 x 16 000 is 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("picture too large")), "{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();