From 69fa2ea9f848097058549d3762f05f43afae0460 Mon Sep 17 00:00:00 2001 From: jcl-bit <336218001+jcl-bit@users.noreply.github.com> Date: Thu, 8 Oct 2026 14:07:00 -0700 Subject: [PATCH] Platform: one save-staging helper for automation and the desktop app (#356) Follow-up to #319, from review. create_staging, staging_suffixes and STAGING_ATTEMPTS were copied in pdfcraft-automation and pdfcraft-ui-egui, which are both L7 and can't depend on each other. They now live once in the new pdfcraft-platform crate (L7, std only), which layers.rs already reserved with both sideways edges. A pure move: the body is unchanged apart from a StagingName parameter that keeps each caller's existing file name (.{name}.{suffix}.pdfcraft-tmp for automation, .{name}.pdfcraft-{suffix}.tmp for the desktop save). The doc comment on staging_suffixes now says the suffix is SipHash keyed from the OS random source: enough that a name can't be planted in advance, but not a CSPRNG. New tests in the platform crate pin both name forms, the 128-byte stem cut, the 16-name limit and refusal when no suffix is left. The callers' planted-file, link, folder and failed-rename tests stay where they are. Co-authored-by: Claude Opus 5.5 Co-authored-by: Brandon Thomas --- Cargo.lock | 6 ++ Cargo.toml | 1 + crates/automation/Cargo.toml | 1 + crates/automation/src/lib.rs | 45 +----------- crates/platform/Cargo.toml | 11 +++ crates/platform/README.md | 7 ++ crates/platform/src/lib.rs | 6 ++ crates/platform/src/staging.rs | 129 +++++++++++++++++++++++++++++++++ crates/ui-egui/Cargo.toml | 1 + crates/ui-egui/src/editing.rs | 50 +------------ 10 files changed, 170 insertions(+), 87 deletions(-) create mode 100644 crates/platform/Cargo.toml create mode 100644 crates/platform/README.md create mode 100644 crates/platform/src/lib.rs create mode 100644 crates/platform/src/staging.rs diff --git a/Cargo.lock b/Cargo.lock index e229f2c6..6c6e75c2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3488,6 +3488,7 @@ dependencies = [ "image", "pdfcraft-engine", "pdfcraft-organize", + "pdfcraft-platform", "pdfcraft-render", "pdfcraft-xfa", "png 0.18.1", @@ -3703,6 +3704,10 @@ dependencies = [ "thiserror 2.0.21", ] +[[package]] +name = "pdfcraft-platform" +version = "0.4.0" + [[package]] name = "pdfcraft-preflight" version = "0.4.0" @@ -3792,6 +3797,7 @@ dependencies = [ "pdfcraft-engine", "pdfcraft-fonts", "pdfcraft-model", + "pdfcraft-platform", "pdfcraft-render", "pdfcraft-xfa", "png 0.18.1", diff --git a/Cargo.toml b/Cargo.toml index 8a1c3b65..37f8fe30 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -40,6 +40,7 @@ pdfcraft-forms = { path = "crates/forms", version = "0.4.0" } pdfcraft-xfa = { path = "crates/xfa", version = "0.4.0" } pdfcraft-render = { path = "crates/render", version = "0.4.0" } pdfcraft-engine = { path = "crates/engine", version = "0.4.0" } +pdfcraft-platform = { path = "crates/platform", version = "0.4.0" } pdfcraft-ui-egui = { path = "crates/ui-egui", version = "0.4.0" } pdfcraft-automation = { path = "crates/automation", version = "0.4.0", default-features = false } diff --git a/crates/automation/Cargo.toml b/crates/automation/Cargo.toml index 556e9da0..7383970b 100644 --- a/crates/automation/Cargo.toml +++ b/crates/automation/Cargo.toml @@ -12,6 +12,7 @@ workspace = true pdfcraft-engine = { workspace = true } pdfcraft-render = { workspace = true } pdfcraft-organize = { workspace = true } +pdfcraft-platform = { workspace = true } serde_json = { workspace = true } png = "0.18" base64 = { version = "0.22", optional = true } diff --git a/crates/automation/src/lib.rs b/crates/automation/src/lib.rs index 35dbadf7..e1219951 100644 --- a/crates/automation/src/lib.rs +++ b/crates/automation/src/lib.rs @@ -29,6 +29,7 @@ use std::path::{Path, PathBuf}; use std::sync::Arc; use pdfcraft_engine::{DocId, Document, Edit, Session, commands}; +use pdfcraft_platform::staging::{StagingName, create_staging, staging_suffixes}; use pdfcraft_render::{PageRenderer, PageText, RenderConfig, RenderRequest, RequestKind}; use serde_json::{Value, json}; @@ -1750,7 +1751,8 @@ fn write_atomic_with(path: &Path, bytes: &[u8], suffixes: impl IntoIterator) -> std::io::Result<(PathBuf, std::fs::File)> { - // At most 128 bytes of the target's name, cut between characters, so the staging name fits - // the 255-byte (Linux, macOS) and 255-unit (Windows) limits however long that name is. - let mut stem = String::new(); - for c in name.chars() { - if stem.len() + c.len_utf8() > 128 { - break; - } - stem.push(c); - } - for suffix in suffixes.into_iter().take(STAGING_ATTEMPTS) { - let tmp = dir.join(format!(".{stem}.{suffix:016x}.pdfcraft-tmp")); - match std::fs::OpenOptions::new().write(true).create_new(true).open(&tmp) { - Ok(file) => return Ok((tmp, file)), - Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => {} - // Windows reports a folder at the name as "access denied"; it is taken all the same. - Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied && tmp.symlink_metadata().is_ok() => {} - Err(e) => return Err(e), - } - } - Err(std::io::Error::new(std::io::ErrorKind::AlreadyExists, "every temporary file name tried is taken")) -} - -/// Unpredictable staging-name suffixes, so a name can't be planted in advance. `RandomState` is -/// keyed from the operating system's random source. -fn staging_suffixes() -> impl Iterator { - use std::hash::BuildHasher; - let state = std::hash::RandomState::new(); - (0u64..).map(move |i| state.hash_one(i)) -} - #[cfg(test)] mod tests { use super::*; + use pdfcraft_platform::staging::STAGING_ATTEMPTS; #[test] fn dot_dot_resolves_by_name_and_never_climbs_above_the_start() { diff --git a/crates/platform/Cargo.toml b/crates/platform/Cargo.toml new file mode 100644 index 00000000..54e7030a --- /dev/null +++ b/crates/platform/Cargo.toml @@ -0,0 +1,11 @@ +[package] +name = "pdfcraft-platform" +description = "Operating-system services shared by the PdfCraft desktop app and automation tools" +version.workspace = true +edition.workspace = true +license.workspace = true + +[lints] +workspace = true + +[dependencies] diff --git a/crates/platform/README.md b/crates/platform/README.md new file mode 100644 index 00000000..6b23752b --- /dev/null +++ b/crates/platform/README.md @@ -0,0 +1,7 @@ +# pdfcraft-platform + +Layer L7 holds operating-system services that both the desktop app (`pdfcraft-ui-egui`) and the automation tools (`pdfcraft-automation`) need. Those two crates sit on the same layer and can't depend on each other, so a helper they share lives here instead of being copied into each (`xtask/src/layers.rs` allows both edges). It uses only the standard library and has no UI dependencies. + +`staging` creates the temporary file an atomic save writes before renaming it over the target. The file is opened with `create_new`, so anything already at the name (a file, a hard link, a symbolic link even when dangling, a folder) is skipped and never written through. On Windows a folder at the name fails with "access denied" rather than "already exists", and is skipped only when something is actually there. Names carry an unpredictable 64-bit suffix from `staging_suffixes` (the standard library's keyed hash, currently SipHash-1-3, keyed from the operating system's random source), which stops a name being planted in advance but is not a CSPRNG; fixed suffixes are only for tests. At most 16 names are tried, and then the save is refused. The target's name is cut to 128 bytes on a character boundary so the staging name fits every system's limit. Each caller keeps its own name form (`.{name}.{suffix}.pdfcraft-tmp` for automation, `.{name}.pdfcraft-{suffix}.tmp` for the desktop save). Writing, closing the file before the rename, renaming and removing the staging file on failure stay with the caller. + +Synthetic tests pin both name forms, the 128-byte cut, the attempt limit and refusal when no name is left. The planted-file, link, folder and failed-rename tests stay with each caller, where they exercise the whole save. diff --git a/crates/platform/src/lib.rs b/crates/platform/src/lib.rs new file mode 100644 index 00000000..799eb566 --- /dev/null +++ b/crates/platform/src/lib.rs @@ -0,0 +1,6 @@ +//! pdfcraft-platform — L7 operating-system services shared by the desktop app (`ui-egui`) and the +//! automation tools (`automation`), which sit on the same layer and can't depend on each other. + +#![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] + +pub mod staging; diff --git a/crates/platform/src/staging.rs b/crates/platform/src/staging.rs new file mode 100644 index 00000000..64172611 --- /dev/null +++ b/crates/platform/src/staging.rs @@ -0,0 +1,129 @@ +//! Staging files for atomic saves: a new, unpredictably named file beside the target, written and +//! then renamed over it. Writing, closing, renaming and cleaning up stay with the caller. + +use std::path::{Path, PathBuf}; + +/// How many staging names [`create_staging`] tries. A random 64-bit name is only taken if someone +/// put a file there on purpose, so running out means refusing, not trying harder. +pub const STAGING_ATTEMPTS: usize = 16; + +/// Where the suffix goes in a staging name. Each caller keeps the form it has always used. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub enum StagingName { + /// `.{name}.{suffix}.pdfcraft-tmp`, used by the automation tools. + SuffixThenTag, + /// `.{name}.pdfcraft-{suffix}.tmp`, used by the desktop app's save. + TagThenSuffix, +} + +/// Create a new, empty staging file in `dir` for the file `name`, one name per suffix. It is +/// opened with `create_new`, which fails if anything already has the name (a file, a hard link, +/// a symbolic link even when dangling, a folder), on Windows as everywhere else; such a name is +/// skipped, never opened, so a file planted at the staging path can't receive or redirect the +/// write. Pass [`staging_suffixes`]: fixed suffixes are for tests, and a predictable name can be +/// taken in advance to refuse every save. +pub fn create_staging( + dir: &Path, + name: &str, + form: StagingName, + suffixes: impl IntoIterator, +) -> std::io::Result<(PathBuf, std::fs::File)> { + // At most 128 bytes of the target's name, cut between characters, so the staging name fits + // the 255-byte (Linux, macOS) and 255-unit (Windows) limits however long that name is. + let mut stem = String::new(); + for c in name.chars() { + if stem.len() + c.len_utf8() > 128 { + break; + } + stem.push(c); + } + for suffix in suffixes.into_iter().take(STAGING_ATTEMPTS) { + let tmp = dir.join(match form { + StagingName::SuffixThenTag => format!(".{stem}.{suffix:016x}.pdfcraft-tmp"), + StagingName::TagThenSuffix => format!(".{stem}.pdfcraft-{suffix:016x}.tmp"), + }); + match std::fs::OpenOptions::new().write(true).create_new(true).open(&tmp) { + Ok(file) => return Ok((tmp, file)), + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => {} + // Windows reports a folder at the name as "access denied"; it is taken all the same. + Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied && tmp.symlink_metadata().is_ok() => {} + Err(e) => return Err(e), + } + } + Err(std::io::Error::new(std::io::ErrorKind::AlreadyExists, "every temporary file name tried is taken")) +} + +/// Unpredictable staging-name suffixes, so a name can't be planted in advance. Each is the +/// standard library's keyed hash of a counter (`RandomState::hash_one`, currently SipHash-1-3), +/// keyed from the operating system's random source. That is all a staging name needs, but it is +/// not a CSPRNG: don't use these where a value must stay secret or unguessable after it has been +/// seen, such as a token or a key. +pub fn staging_suffixes() -> impl Iterator { + use std::hash::BuildHasher; + let state = std::hash::RandomState::new(); + (0u64..).map(move |i| state.hash_one(i)) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// A fresh, empty folder for one test. + fn test_dir(test: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!("pdfcraft-platform-staging-{}-{test}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + dir + } + + fn file_name(p: &Path) -> String { + p.file_name().unwrap().to_string_lossy().into_owned() + } + + /// The callers' tests plant files at these exact names; if a name changed, those tests would + /// plant where nothing writes and pass without testing anything. + #[test] + fn each_form_keeps_its_exact_name() { + let dir = test_dir("forms"); + let (tools, _) = create_staging(&dir, "out.pdf", StagingName::SuffixThenTag, [0xab]).unwrap(); + assert_eq!(file_name(&tools), ".out.pdf.00000000000000ab.pdfcraft-tmp"); + let (desktop, _) = create_staging(&dir, "out.pdf", StagingName::TagThenSuffix, [0xab]).unwrap(); + assert_eq!(file_name(&desktop), ".out.pdf.pdfcraft-00000000000000ab.tmp"); + assert_eq!(std::fs::read(&tools).unwrap(), b"", "a new, empty file"); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn a_long_name_is_cut_to_128_bytes_between_characters() { + let dir = test_dir("long"); + // One byte, then four-byte characters: the 128-byte limit falls inside the 32nd one. + let name = format!("a{}.pdf", "\u{1F600}".repeat(40)); + let (tmp, _) = create_staging(&dir, &name, StagingName::SuffixThenTag, [1]).unwrap(); + assert_eq!(file_name(&tmp), format!(".a{}.0000000000000001.pdfcraft-tmp", "\u{1F600}".repeat(31))); + // One-byte characters: exactly 128 of them are kept. + let (tmp, _) = create_staging(&dir, &"x".repeat(200), StagingName::SuffixThenTag, [2]).unwrap(); + assert_eq!(file_name(&tmp), format!(".{}.0000000000000002.pdfcraft-tmp", "x".repeat(128))); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn running_out_of_suffixes_refuses_and_creates_nothing() { + let dir = test_dir("empty"); + let e = create_staging(&dir, "out.pdf", StagingName::TagThenSuffix, []).unwrap_err(); + assert_eq!(e.kind(), std::io::ErrorKind::AlreadyExists); + assert_eq!(std::fs::read_dir(&dir).unwrap().count(), 0); + let _ = std::fs::remove_dir_all(&dir); + } + + #[test] + fn only_the_first_sixteen_names_are_tried() { + let dir = test_dir("cap"); + let name = |s: u64| dir.join(format!(".out.pdf.{s:016x}.pdfcraft-tmp")); + for s in 1..=STAGING_ATTEMPTS as u64 { + std::fs::write(name(s), "PLANTED").unwrap(); + } + assert!(create_staging(&dir, "out.pdf", StagingName::SuffixThenTag, 1..).is_err()); + assert!(!name(STAGING_ATTEMPTS as u64 + 1).exists(), "a 17th name was tried"); + let _ = std::fs::remove_dir_all(&dir); + } +} diff --git a/crates/ui-egui/Cargo.toml b/crates/ui-egui/Cargo.toml index 8f817457..6a0b9fca 100644 --- a/crates/ui-egui/Cargo.toml +++ b/crates/ui-egui/Cargo.toml @@ -12,6 +12,7 @@ workspace = true pdfcraft-fonts = { workspace = true } pdfcraft-engine = { workspace = true } pdfcraft-render = { workspace = true } +pdfcraft-platform = { workspace = true } egui = { workspace = true } egui_extras = { workspace = true } eframe = { workspace = true, features = ["default_fonts", "wgpu", "accesskit", "persistence", "links"] } diff --git a/crates/ui-egui/src/editing.rs b/crates/ui-egui/src/editing.rs index e8a9b8ee..4025a24b 100644 --- a/crates/ui-egui/src/editing.rs +++ b/crates/ui-egui/src/editing.rs @@ -2,6 +2,7 @@ //! "save changes?" prompt when closing a tab or quitting with unsaved edits. use pdfcraft_engine::Edit; +use pdfcraft_platform::staging::{StagingName, create_staging, staging_suffixes}; use crate::PdfCraftApp; @@ -527,7 +528,7 @@ fn write_atomically_with(path: &str, bytes: &[u8], suffixes: impl IntoIterator, -) -> std::io::Result<(std::path::PathBuf, std::fs::File)> { - // At most 128 bytes of the target's name, cut between characters, so the staging name fits - // the 255-byte (Linux, macOS) and 255-unit (Windows) limits however long that name is. - let mut stem = String::new(); - for c in name.chars() { - if stem.len() + c.len_utf8() > 128 { - break; - } - stem.push(c); - } - for suffix in suffixes.into_iter().take(STAGING_ATTEMPTS) { - let tmp = dir.join(format!(".{stem}.pdfcraft-{suffix:016x}.tmp")); - match std::fs::OpenOptions::new().write(true).create_new(true).open(&tmp) { - Ok(file) => return Ok((tmp, file)), - Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => {} - // Windows reports a folder at the name as "access denied"; it is taken all the same. - Err(e) if e.kind() == std::io::ErrorKind::PermissionDenied && tmp.symlink_metadata().is_ok() => {} - Err(e) => return Err(e), - } - } - Err(std::io::Error::new(std::io::ErrorKind::AlreadyExists, "every temporary file name tried is taken")) -} - -/// Unpredictable staging-name suffixes, so a name can't be planted in advance. `RandomState` is -/// keyed from the operating system's random source. -fn staging_suffixes() -> impl Iterator { - use std::hash::BuildHasher; - let state = std::hash::RandomState::new(); - (0u64..).map(move |i| state.hash_one(i)) -} - #[cfg(not(target_arch = "wasm32"))] fn short_name(path: &str) -> String { std::path::Path::new(path).file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_else(|| path.to_string()) @@ -668,7 +625,8 @@ fn bookmark_at<'a>(items: &'a [pdfcraft_render::OutlineItem], path: &[usize]) -> #[cfg(test)] mod tests { - use super::{STAGING_ATTEMPTS, staging_suffixes, write_atomically, write_atomically_with}; + use super::{write_atomically, write_atomically_with}; + use pdfcraft_platform::staging::{STAGING_ATTEMPTS, staging_suffixes}; use std::path::{Path, PathBuf}; /// A fresh, empty folder for one staging test.