diff --git a/apps/photocraft/src/services.rs b/apps/photocraft/src/services.rs index 94f259a1b4..a37bf4528b 100644 --- a/apps/photocraft/src/services.rs +++ b/apps/photocraft/src/services.rs @@ -156,7 +156,8 @@ pub fn native(automation: Option) -> })), pick_open: Some(Box::new(|| { let path = rfd::FileDialog::new().add_filter("All Formats", OPEN_EXTS).add_filter("PhotoCraft", &["pcraft"]).pick_file()?; - let bytes = std::fs::read(&path).ok()?; + // A read failure goes back to the app, which reports it like any other open failure. + let bytes = photocraft_format::read_file(&path).map_err(|e| e.to_string()); Some((path.to_string_lossy().to_string(), bytes)) })), pick_save: Some(Box::new(|suggested: &str| { diff --git a/crates/automation/src/files.rs b/crates/automation/src/files.rs index 16788982e9..09513c2317 100644 --- a/crates/automation/src/files.rs +++ b/crates/automation/src/files.rs @@ -30,7 +30,7 @@ pub fn open(path: &Path) -> Result { if path.is_dir() { return Ok(Opened { document: photocraft_format::load_path(path)?, warnings: Vec::new() }); } - let bytes = std::fs::read(path).map_err(|e| AutomationError::Io(format!("{}: {e}", path.display())))?; + let bytes = photocraft_format::read_file(path).map_err(|e| AutomationError::Io(format!("{}: {e}", path.display())))?; let name = path.file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_default(); open_bytes(&name, &bytes) } diff --git a/crates/automation/src/workspace.rs b/crates/automation/src/workspace.rs index 8ffadb4ee4..013919c1df 100644 --- a/crates/automation/src/workspace.rs +++ b/crates/automation/src/workspace.rs @@ -1,6 +1,6 @@ //! Capability-based filesystem policy for untrusted automation paths. -use std::io::{Read, Write}; +use std::io::Write; use std::path::{Path, PathBuf}; use std::sync::Arc; @@ -46,9 +46,8 @@ impl AuthorizedWorkspace { if !metadata.is_file() { return Err(AutomationError::BadRequest(format!("automation read path is not a regular file: `{path}`"))); } - let mut bytes = Vec::new(); - file.read_to_end(&mut bytes).map_err(|e| file_error("read", path, e))?; - Ok(bytes) + // Bounded reads, and a clear error for a file larger than memory (#375). + photocraft_format::read::read_all(&mut file, metadata.len()).map_err(|e| file_error("read", path, e)) } /// Create or replace one file below the configured write root, crash-safely: the bytes go to diff --git a/crates/engine/src/file_cmds.rs b/crates/engine/src/file_cmds.rs index cb96c18368..73496df73f 100644 --- a/crates/engine/src/file_cmds.rs +++ b/crates/engine/src/file_cmds.rs @@ -75,8 +75,10 @@ fn is_file(_path: &str) -> bool { } #[cfg(not(target_arch = "wasm32"))] +/// Every file the engine opens is read here: in bounded reads, with a clear error when it does +/// not fit in memory (see [`photocraft_format::read`]). pub(crate) fn read_file(path: &str) -> Result> { - std::fs::read(path).map_err(|e| EngineError::Other(format!("{path}: {e}"))) + photocraft_format::read_file(std::path::Path::new(path)).map_err(|e| EngineError::Other(format!("{path}: {e}"))) } #[cfg(target_arch = "wasm32")] pub(crate) fn read_file(path: &str) -> Result> { diff --git a/crates/engine/src/jobs.rs b/crates/engine/src/jobs.rs index 1ec15f012b..a5fa43685d 100644 --- a/crates/engine/src/jobs.rs +++ b/crates/engine/src/jobs.rs @@ -258,7 +258,8 @@ fn independent_of_document(id: &str) -> bool { #[cfg(not(target_arch = "wasm32"))] fn read_path(path: &str) -> Result> { - std::fs::read(path).map_err(|e| EngineError::Other(format!("{path}: {e}"))) + // Bounded reads, and a clear error for a file larger than memory (#375). + photocraft_format::read_file(std::path::Path::new(path)).map_err(|e| EngineError::Other(format!("{path}: {e}"))) } #[cfg(target_arch = "wasm32")] diff --git a/crates/engine/src/smart_cmds.rs b/crates/engine/src/smart_cmds.rs index fb76d25f27..3a06b1e8be 100644 --- a/crates/engine/src/smart_cmds.rs +++ b/crates/engine/src/smart_cmds.rs @@ -72,7 +72,7 @@ fn base_name(path: &str) -> String { } fn read_file(path: &str) -> Option> { - if path.is_empty() { None } else { std::fs::read(path).ok() } + if path.is_empty() { None } else { photocraft_format::read_file(std::path::Path::new(path)).ok() } } /// The source file of a smart object: embedded bytes, the PSD's embedded linked-layer data (PSD @@ -614,7 +614,7 @@ fn set_source(s: &mut Session, p: &Value, label: &str, keep_psd: bool, make: imp fn replace_contents(s: &mut Session, p: &Value) -> Result { let path = path_param("layer.smartObjects.replaceContents", p)?.to_string(); - let bytes = std::fs::read(&path).map_err(|e| other(format!("can't read {path}: {e}")))?; + let bytes = photocraft_format::read_file(std::path::Path::new(&path)).map_err(|e| other(format!("can't read {path}: {e}")))?; let name = base_name(&path); decode_source(&name, &bytes)?; // fail before touching the document set_source(s, p, "Replace Contents", false, |_, _| Ok(SmartSource::Embedded { file_name: name, bytes: Arc::new(bytes) })) diff --git a/crates/format/src/lib.rs b/crates/format/src/lib.rs index 4b54457e98..1e9de5ced4 100644 --- a/crates/format/src/lib.rs +++ b/crates/format/src/lib.rs @@ -25,6 +25,7 @@ pub mod autosave; mod convert; pub mod manifest; mod migrate; +pub mod read; mod store; pub mod zip; @@ -39,6 +40,7 @@ pub use autosave::RecoveryStore; pub use autosave::{Autosaver, RecoveryEntry, discard_recovery, list_recovery, recover}; pub use convert::MAX_GROUP_DEPTH; pub use manifest::{FORMAT_VERSION, Manifest}; +pub use read::read_file; pub use store::{PcraftWriter, SaveStats}; /// File extension of the native format. @@ -137,7 +139,7 @@ pub fn load_path_with(path: &Path, opts: &LoadOptions) -> Result { if path.is_dir() { store::load(&store::DirSource { root: path.to_path_buf() }, opts) } else { - let bytes = std::fs::read(path)?; + let bytes = read::read_file(path)?; load_from_bytes_with(&bytes, opts) } } diff --git a/crates/format/src/read.rs b/crates/format/src/read.rs new file mode 100644 index 0000000000..4ebfcbbfa6 --- /dev/null +++ b/crates/format/src/read.rs @@ -0,0 +1,122 @@ +//! Reading a whole file to open it: the read side of [`crate::atomic`]. +//! +//! What it adds over `std::fs::read` for multi-gigabyte PSB files: +//! +//! - **Bounded reads.** Each read asks for at most [`READ_CHUNK`] bytes. `std` asks Windows for up +//! to 4 GiB in one `ReadFile`, and some file systems (exFAT, network redirectors) reject a read +//! that large with `ERROR_INVALID_PARAMETER` ("The parameter is incorrect", #375). +//! - **A clear out-of-memory error.** The buffer is reserved up front with `try_reserve_exact`, +//! and a file larger than the memory available says how much it needed. +//! +//! On `wasm32` there is no file system and every call returns the `std` "unsupported" error. + +use std::io::{self, Read}; +use std::path::Path; + +/// Largest single read: big enough that a 5 GB file takes under a hundred reads. +pub const READ_CHUNK: usize = 64 << 20; + +/// Read all of `path`, in reads of at most [`READ_CHUNK`] bytes. +pub fn read_file(path: &Path) -> io::Result> { + read_file_with(path, READ_CHUNK) +} + +/// [`read_file`] with a chosen read size (tests use small ones to exercise the loop). +pub fn read_file_with(path: &Path, chunk: usize) -> io::Result> { + let mut f = std::fs::File::open(path)?; + let len = f.metadata().map(|m| m.len()).unwrap_or(0); + read_all_with(&mut f, len, chunk) +} + +/// Read `reader` to its end in reads of at most [`READ_CHUNK`] bytes, for an already open file +/// (a capability-scoped handle, say). `size_hint` (the file's length) sizes the buffer up front. +pub fn read_all(reader: &mut impl Read, size_hint: u64) -> io::Result> { + read_all_with(reader, size_hint, READ_CHUNK) +} + +fn read_all_with(reader: &mut impl Read, size_hint: u64, chunk: usize) -> io::Result> { + // The hint only sizes the buffer: a file that grows or shrinks while being read is still read + // to its end. + let mut buf = buffer_for(size_hint)?; + let chunk = u64::try_from(chunk.max(1)).unwrap_or(u64::MAX); + while reader.take(chunk).read_to_end(&mut buf)? > 0 {} + Ok(buf) +} + +/// An empty buffer with room for `len` bytes, or an `OutOfMemory` error saying how much was needed. +pub fn buffer_for(len: u64) -> io::Result> { + let mut buf = Vec::new(); + let reserved = usize::try_from(len).ok().and_then(|n| buf.try_reserve_exact(n).ok()); + match reserved { + Some(()) => Ok(buf), + None => Err(io::Error::new(io::ErrorKind::OutOfMemory, format!("not enough memory to read the whole file ({})", size_text(len)))), + } +} + +/// `len` bytes for people: "4.75 GB", "759 MB", "12 KB". +fn size_text(len: u64) -> String { + const UNITS: [&str; 5] = ["bytes", "KB", "MB", "GB", "TB"]; + let mut v = len as f64; + let mut unit = 0; + while v >= 1000.0 && unit + 1 < UNITS.len() { + v /= 1000.0; + unit += 1; + } + match UNITS.get(unit) { + Some(name) if unit > 0 => format!("{v:.2} {name}"), + _ => format!("{len} bytes"), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn temp(name: &str, bytes: &[u8]) -> std::path::PathBuf { + let dir = std::env::temp_dir().join(format!("photocraft-read-{}-{name}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let p = dir.join("f.bin"); + std::fs::write(&p, bytes).unwrap(); + p + } + + #[test] + fn reads_whole_file_across_many_chunks() { + let data: Vec = (0..100_003u32).map(|i| (i * 7 % 251) as u8).collect(); + let p = temp("chunks", &data); + for chunk in [1, 7, 4096, 100_003, 1 << 20] { + assert_eq!(read_file_with(&p, chunk).unwrap(), data, "chunk {chunk}"); + } + assert_eq!(read_file(&p).unwrap(), data); + assert_eq!(read_file_with(&p, 0).unwrap(), data, "a zero chunk still makes progress"); + // From an open reader, whatever the hint says. + for hint in [0, 10, data.len() as u64, 1 << 20] { + assert_eq!(read_all(&mut std::io::Cursor::new(&data), hint).unwrap(), data, "hint {hint}"); + } + } + + #[test] + fn empty_and_missing_files() { + assert!(read_file(&temp("empty", b"")).unwrap().is_empty()); + let missing = std::env::temp_dir().join("photocraft-read-does-not-exist.psb"); + assert_eq!(read_file(&missing).unwrap_err().kind(), io::ErrorKind::NotFound); + } + + #[test] + fn a_file_too_large_for_memory_is_a_clear_error() { + // More than any address space can hold. + let e = buffer_for(u64::MAX).unwrap_err(); + assert_eq!(e.kind(), io::ErrorKind::OutOfMemory); + assert!(e.to_string().contains("18446744.07 TB"), "{e}"); + let e = buffer_for(1 << 62).unwrap_err(); + assert_eq!(e.kind(), io::ErrorKind::OutOfMemory); + assert_eq!(buffer_for(1024).unwrap().capacity(), 1024); + } + + #[test] + fn sizes_read_like_the_os_shows_them() { + assert_eq!(size_text(512), "512 bytes"); + assert_eq!(size_text(4_750_000_000), "4.75 GB"); + assert_eq!(size_text(759_000_000), "759.00 MB"); + } +} diff --git a/crates/ui-egui/Cargo.toml b/crates/ui-egui/Cargo.toml index f055ef410a..6d0f2b1aa2 100644 --- a/crates/ui-egui/Cargo.toml +++ b/crates/ui-egui/Cargo.toml @@ -19,6 +19,8 @@ photocraft-plugins = { workspace = true } photocraft-algo = { workspace = true } photocraft-text = { workspace = true } photocraft-vector = { workspace = true } +# Reading files to open them (bounded reads, a clear error for files larger than memory). +photocraft-format = { workspace = true } egui = { workspace = true } egui_extras = { version = "0.36", default-features = false, features = ["svg"] } eframe = { workspace = true, features = ["default_fonts", "wgpu", "accesskit"] } @@ -36,7 +38,6 @@ egui_kittest = { workspace = true, features = ["wgpu", "snapshot", "eframe"] } photocraft-io = { workspace = true } # Only for examples/interactive_bench.rs (JPEG encode, .pcraft saves). photocraft-codecs = { workspace = true } -photocraft-format = { workspace = true } # Benchmark helpers for the `--json` benches `cargo xtask perf` runs (in-process RSS, percentiles). photocraft-testkit = { workspace = true } # Only for examples/perf_scenarios.rs (a 500-brush .abr for the #210 import row). diff --git a/crates/ui-egui/src/analysis_ui.rs b/crates/ui-egui/src/analysis_ui.rs index 6b9ab669ec..511af6169c 100644 --- a/crates/ui-egui/src/analysis_ui.rs +++ b/crates/ui-egui/src/analysis_ui.rs @@ -159,7 +159,10 @@ pub fn menu(app: &mut PhotocraftApp, id: &str, params: &Value) -> Option { - let (name, bytes) = app.services.pick_open.as_mut().and_then(|f| f())?; + let (name, bytes) = match app.pick_file_bytes()? { + Ok(picked) => picked, + Err(e) => return Some(Err(e)), + }; let r = photocraft_engine::notes_cmds::import_notes_from(&mut app.session, &name, &bytes).map_err(|e| e.to_string()); if r.is_ok() { app.ui.analysis.notes = true; diff --git a/crates/ui-egui/src/file_open.rs b/crates/ui-egui/src/file_open.rs index de6f92cf12..40acd925f7 100644 --- a/crates/ui-egui/src/file_open.rs +++ b/crates/ui-egui/src/file_open.rs @@ -54,7 +54,7 @@ impl PhotocraftApp { crate::jobs_ui::start_open(self, &display_name(path), Some(path.to_string()), photocraft_engine::jobs::OpenSource::Path(path.to_string()))?; return Ok(Vec::new()); } - let bytes = std::fs::read(path).map_err(|e| format!("{path}: {e}"))?; + let bytes = photocraft_format::read_file(std::path::Path::new(path)).map_err(|e| format!("{path}: {e}"))?; self.open_file(path, &bytes) } @@ -91,8 +91,13 @@ impl PhotocraftApp { for f in files { let path = f.path().to_string_lossy().to_string(); let name = f.path().file_name().map(|n| n.to_string_lossy().to_string()).unwrap_or_else(|| "dropped".into()); - let opened = - crate::read_dropped(&*f).and_then(|bytes| if f.path().is_absolute() { self.open_file(&path, &bytes) } else { self.open_bytes(&name, &bytes) }); + // A desktop drop opens like File › Open: on the background worker when jobs are on, and + // read in bounded reads either way (#375) rather than with egui's whole-file read. + let opened = if f.path().is_absolute() { + self.open_path(&path).map(|_| ()) + } else { + crate::read_dropped(&*f).and_then(|bytes| self.open_bytes(&name, &bytes)).map(|_| ()) + }; if let Err(e) = opened { self.open_failed(&name, &e); } diff --git a/crates/ui-egui/src/file_open/tests.rs b/crates/ui-egui/src/file_open/tests.rs index c970e8825c..6eab293c1d 100644 --- a/crates/ui-egui/src/file_open/tests.rs +++ b/crates/ui-egui/src/file_open/tests.rs @@ -29,7 +29,7 @@ fn app_with(pick_open: Option<(String, Vec)>, pick_save: Option) -> let warnings = if path.ends_with(".png") { vec!["Layers were flattened".to_string()] } else { Vec::new() }; Ok((b"out".to_vec(), warnings)) })), - pick_open: Some(Box::new(move || pick_open.take())), + pick_open: Some(Box::new(move || pick_open.take().map(|(name, bytes)| (name, Ok(bytes))))), pick_save: Some(Box::new(move |_s: &str| pick_save.clone())), write: Some(Box::new(move |p: &str, b: &[u8]| { w.borrow_mut().push((p.to_string(), b.to_vec())); @@ -140,6 +140,23 @@ fn open_failures_are_errors_and_leave_no_document() { assert!(app.ui.notices.last().is_some_and(|n| n.error)); } +#[test] +fn a_picked_file_that_cannot_be_read_is_reported_like_any_open_failure() { + let (mut app, _) = app_with(None, None); + let mut picked = Some(("C:/photos/big.psb".to_string(), Err("The parameter is incorrect. (os error 87)".to_string()))); + app.services.pick_open = Some(Box::new(move || picked.take())); + app.open_dialog_file(); + assert!(app.session.documents().is_empty()); + assert!(app.ui.status_error); + let notice = app.ui.notices.last().unwrap(); + assert!(notice.error && notice.title.contains("Couldn't open big.psb: The parameter is incorrect"), "{}", notice.title); + // Commands that read a picked file (scripts, notes, Place) get the same message as an error. + app.services.pick_open = Some(Box::new(|| Some(("C:/photos/big.psb".to_string(), Err("denied".to_string()))))); + assert_eq!(app.pick_file_bytes(), Some(Err("big.psb: denied".to_string()))); + app.services.pick_open = Some(Box::new(|| None)); + assert_eq!(app.pick_file_bytes(), None, "cancelled"); +} + #[test] fn open_paths_reports_each_failure_without_panicking() { let (mut app, _) = app_with(None, None); @@ -202,10 +219,15 @@ impl egui::DroppedFile for FakeDrop { #[test] fn dropped_files_open_with_path_and_recent() { let (mut app, _) = app_with(None, None); - let abs = std::env::temp_dir().join("dropped.psd"); + let dir = std::env::temp_dir().join(format!("photocraft-drop-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let abs = dir.join("dropped.psd"); + std::fs::write(&abs, b"x").unwrap(); let abs_s = abs.to_string_lossy().to_string(); app.open_dropped(vec![ - std::sync::Arc::new(FakeDrop(abs, Ok(b"x".to_vec()))), + // A desktop drop (absolute path) is read from disk like File › Open (#375), never through + // egui's whole-file `std::fs::read`. + std::sync::Arc::new(FakeDrop(abs, Err("egui's reader must not be used".into()))), // Unreadable and undecodable drops are errors; no path means no recent entry. std::sync::Arc::new(FakeDrop("gone.psd".into(), Err("permission denied".into()))), std::sync::Arc::new(FakeDrop("rel.psd".into(), Ok(b"x".to_vec()))), @@ -218,6 +240,7 @@ fn dropped_files_open_with_path_and_recent() { assert_eq!(app.ui.recent_files, vec![abs_s]); assert!(app.ui.status_error); assert_eq!(app.ui.notices.iter().filter(|n| n.error).count(), 2); + let _ = std::fs::remove_dir_all(&dir); } #[test] diff --git a/crates/ui-egui/src/file_ui.rs b/crates/ui-egui/src/file_ui.rs index 1eefef472a..d00c65f2bf 100644 --- a/crates/ui-egui/src/file_ui.rs +++ b/crates/ui-egui/src/file_ui.rs @@ -88,7 +88,10 @@ pub fn invoke(app: &mut PhotocraftApp, ctx: &egui::Context, id: &str, params: &V form(app, id, "Image Statistics", json!({"mode": "median", "input": dir, "align": false}), json!({"mode": modes})) } "file.scripts.browse" => { - let (name, bytes) = app.services.pick_open.as_mut().and_then(|f| f())?; + let (name, bytes) = match app.pick_file_bytes()? { + Ok(picked) => picked, + Err(e) => return Some(Err(e)), + }; let r = app.run(id, json!({"script": String::from_utf8_lossy(&bytes)})); if r.is_ok() { app.ui.status = format!("Ran script {name}"); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index b7db9354f3..aec5b20997 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -145,7 +145,9 @@ pub struct ExportSettings { /// Encode a document: (file bytes, warnings about anything approximated or dropped). pub type ExportFn = Box Result<(Vec, Vec), String>>; -pub type PickOpenFn = Box Option<(String, Vec)>>; +/// The picked file's name and its bytes, or why it could not be read (shown like any other open +/// failure); `None` when the dialog was cancelled. +pub type PickOpenFn = Box Option<(String, Result, String>)>>; pub type PickSaveFn = Box Option>; pub type WriteFn = Box Result<(), String>>; /// Read bytes through the desktop control session's authorized read root. @@ -689,14 +691,19 @@ impl PhotocraftApp { /// File › Open: the platform dialog returns the chosen file's path (native; the web delivers /// picks through the inbox instead). pub fn open_dialog_file(&mut self) { - let picked = self.services.pick_open.as_mut().and_then(|f| f()); - if let Some((path, bytes)) = picked - && let Err(e) = self.open_file(&path, &bytes) - { + let Some((path, bytes)) = self.services.pick_open.as_mut().and_then(|f| f()) else { return }; + if let Err(e) = bytes.and_then(|bytes| self.open_file(&path, &bytes)) { self.open_failed(&file_open::display_name(&path), &e); } } + /// Show the open dialog for a file a command reads (a script, notes, a placed image, presets): + /// `None` when cancelled, else its name and bytes or the read error. + pub(crate) fn pick_file_bytes(&mut self) -> Option), String>> { + let (name, bytes) = self.services.pick_open.as_mut().and_then(|f| f())?; + Some(bytes.map(|b| (name.clone(), b)).map_err(|e| format!("{}: {e}", file_open::display_name(&name)))) + } + /// Save the active document to `path` (or a path chosen in the save dialog); returns the path /// and the export warnings (also shown to the user). pub fn save_as(&mut self, path: Option) -> Result<(String, Vec), String> { diff --git a/crates/ui-egui/src/prefs_ui.rs b/crates/ui-egui/src/prefs_ui.rs index 5748490ed9..abc3be82d5 100644 --- a/crates/ui-egui/src/prefs_ui.rs +++ b/crates/ui-egui/src/prefs_ui.rs @@ -1338,7 +1338,7 @@ pub fn confirm(app: &mut PhotocraftApp, f: &Map) -> Result { let kinds: Vec<&str> = ["brushes", "customShapes"].into_iter().filter(|k| f.get(*k).and_then(Value::as_bool).unwrap_or(true)).collect(); if f.get("action").and_then(Value::as_str) == Some("import") { - let (name, bytes) = app.services.pick_open.as_mut().and_then(|p| p()).ok_or("cancelled")?; + let (name, bytes) = app.pick_file_bytes().ok_or_else(|| "cancelled".to_string())??; let text = String::from_utf8(bytes).map_err(|_| format!("{name} is not a preset file"))?; app.run("edit.presets.exportImportPresets", json!({"action": "import", "kinds": kinds, "data": text})) } else { diff --git a/crates/ui-egui/src/view_cmds.rs b/crates/ui-egui/src/view_cmds.rs index 3bf65ac291..8cc7d55fc5 100644 --- a/crates/ui-egui/src/view_cmds.rs +++ b/crates/ui-egui/src/view_cmds.rs @@ -800,7 +800,10 @@ fn front(app: &mut PhotocraftApp, id: &str, params: &Value) -> Option Some(save_a_copy(app)), "file.placeEmbedded" | "file.placeLinked" => { - let (name, bytes) = app.services.pick_open.as_mut().and_then(|f| f())?; + let (name, bytes) = match app.pick_file_bytes()? { + Ok(picked) => picked, + Err(e) => return Some(Err(e)), + }; let linked = (id == "file.placeLinked").then(|| name.clone()); let r = photocraft_engine::file_cmds::place_bytes(&mut app.session, &name, bytes, linked, &json!({})).map_err(|e| e.to_string()); app.sync_views();