From 30b8911dd07188f94543c302bf920982550fb00f Mon Sep 17 00:00:00 2001 From: Tomcatt Date: Mon, 2 Nov 2026 00:44:39 +0900 Subject: [PATCH] fix(update): refuse a folder in a file's place; per-process staging names A shipped file whose destination is a folder is now caught before anything is staged, instead of failing its rename after earlier files were replaced. The unpack folder and staged .update files carry the process ID, so two updates running at once can't remove or overwrite each other's files. Adds a test for each. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_019oRr8q5tawLko7mLqEXf8g --- crates/update/src/self_update.rs | 65 +++++++++++++++++++++++++++++++- 1 file changed, 63 insertions(+), 2 deletions(-) diff --git a/crates/update/src/self_update.rs b/crates/update/src/self_update.rs index c71995cb..37a9f847 100644 --- a/crates/update/src/self_update.rs +++ b/crates/update/src/self_update.rs @@ -474,7 +474,8 @@ fn replace_appimage(new: &Path, path: &Path) -> Result<(), String> { /// unwritable folder) changes nothing; the executable is renamed last. #[cfg(unix)] fn replace_standalone(app: &App, paths: &Paths, tarball: &Path, exe: &Path) -> Result<(), String> { - let staging = paths.cache.join(format!("{}.standalone", app.id)); + // Per-process names, so two updates running at once can't remove each other's files. + let staging = paths.cache.join(format!("{}.standalone.{}", app.id, std::process::id())); let result = install::tree::unpack(app, tarball, &staging).and_then(|tree| replace_from_tree(app, &tree, exe)); let _ = std::fs::remove_dir_all(&staging); result @@ -504,6 +505,10 @@ fn replace_from_tree(app: &App, tree: &Path, exe: &Path) -> Result<(), String> { } } moves.push((new_exe, exe.to_path_buf())); + // A folder where a file goes would fail its rename after earlier files were replaced. + if let Some((_, to)) = moves.iter().find(|(_, to)| to.symlink_metadata().is_ok_and(|m| m.is_dir())) { + return Err(format!("couldn't replace {} (it's a folder)", to.display())); + } // Copy everything next to its destination first, so a full disk or an unwritable folder // stops the update before any installed file changes. Renames within a folder don't fail // for those reasons, and the executable is renamed last. @@ -539,7 +544,7 @@ fn stage(from: &Path, to: &Path, executable: bool) -> Result { let dir = to.parent().ok_or_else(|| format!("{} isn't in a folder", to.display()))?; std::fs::create_dir_all(dir).map_err(|e| format!("couldn't create {} ({e})", dir.display()))?; let name = to.file_name().map(|n| n.to_string_lossy().into_owned()).unwrap_or_default(); - let staged = dir.join(format!(".{name}.update")); + let staged = dir.join(format!(".{name}.{}.update", std::process::id())); let done = std::fs::copy(from, &staged).map_err(|e| format!("couldn't write next to {} ({e})", to.display())).and_then(|_| { let mode = std::fs::metadata(from).map(|m| m.permissions().mode()).unwrap_or(0o644); let mode = if executable { 0o755 } else { mode & 0o755 }; @@ -2083,6 +2088,62 @@ mod tests { assert!(leftovers.is_empty(), "staged files are removed: {leftovers:?}"); } + #[cfg(unix)] + #[test] + fn a_folder_where_a_shipped_file_goes_stops_the_update_before_anything_changes() { + let dir = tempfile::tempdir().unwrap(); + let prefix = dir.path().join("local"); + std::fs::create_dir_all(prefix.join("bin")).unwrap(); + std::fs::create_dir_all(prefix.join("share/pdfcraft/b.txt")).unwrap(); + let exe = prefix.join("bin/pdfcraft"); + std::fs::write(&exe, b"old app").unwrap(); + std::fs::write(prefix.join("share/pdfcraft/a.txt"), b"old a").unwrap(); + std::fs::write(prefix.join("share/pdfcraft/c.txt"), b"old c").unwrap(); + let tarball = dir.path().join("new.tar.gz"); + release_tarball( + &tarball, + &[ + ("bin/pdfcraft", b"new app"), + ("share/pdfcraft/a.txt", b"new a"), + ("share/pdfcraft/b.txt", b"new b"), + ("share/pdfcraft/c.txt", b"new c"), + ], + ); + let paths = Paths::under(&dir.path().join("data")); + let result = replace_standalone(&crate::suite::PDFCRAFT, &paths, &tarball, &exe); + assert!(result.is_err()); + let read = |p: &str| std::fs::read(prefix.join(p)).unwrap(); + assert_eq!(read("bin/pdfcraft"), b"old app"); + assert_eq!(read("share/pdfcraft/a.txt"), b"old a", "no file is replaced when one can't be"); + assert_eq!(read("share/pdfcraft/c.txt"), b"old c", "no file is replaced when one can't be"); + assert!(prefix.join("share/pdfcraft/b.txt").is_dir()); + let leftovers: Vec<_> = walk(&prefix).into_iter().filter(|p| p.to_string_lossy().contains(".update")).collect(); + assert!(leftovers.is_empty(), "staged files are removed: {leftovers:?}"); + } + + #[cfg(unix)] + #[test] + fn a_standalone_update_leaves_another_updates_staged_files_alone() { + let dir = tempfile::tempdir().unwrap(); + let prefix = dir.path().join("local"); + std::fs::create_dir_all(prefix.join("bin")).unwrap(); + let exe = prefix.join("bin/pdfcraft"); + std::fs::write(&exe, b"old app").unwrap(); + // What a concurrent update would have staged under the shared names. + let other_file = prefix.join("bin/.pdfcraft.update"); + std::fs::write(&other_file, b"other update").unwrap(); + let paths = Paths::under(&dir.path().join("data")); + let other_dir = paths.cache.join("pdfcraft.standalone"); + std::fs::create_dir_all(&other_dir).unwrap(); + std::fs::write(other_dir.join("marker"), b"other update").unwrap(); + let tarball = dir.path().join("new.tar.gz"); + release_tarball(&tarball, &[("bin/pdfcraft", b"new app")]); + replace_standalone(&crate::suite::PDFCRAFT, &paths, &tarball, &exe).unwrap(); + assert_eq!(std::fs::read(&exe).unwrap(), b"new app"); + assert_eq!(std::fs::read(&other_file).unwrap(), b"other update"); + assert_eq!(std::fs::read(other_dir.join("marker")).unwrap(), b"other update"); + } + #[cfg(unix)] #[test] fn a_lone_standalone_binary_gets_only_what_sits_beside_it() {