From bb2b7a6f870af76bdf71d50760412abb5e3a8680 Mon Sep 17 00:00:00 2001 From: midasmatterhorn <248507632+midasmatterhorn@users.noreply.github.com> Date: Thu, 15 Oct 2026 01:02:49 +0300 Subject: [PATCH 1/3] engine: atomic writes use an exclusive per-call temporary, so parallel writers cannot mix or follow a planted symlink (#1233) --- crates/engine/src/config.rs | 38 ++++++++++++++------ crates/engine/src/tests_settings.rs | 54 +++++++++++++++++++++++++++-- 2 files changed, 79 insertions(+), 13 deletions(-) diff --git a/crates/engine/src/config.rs b/crates/engine/src/config.rs index c333c973..68f403dd 100644 --- a/crates/engine/src/config.rs +++ b/crates/engine/src/config.rs @@ -164,10 +164,15 @@ impl ConfigStore for DirConfig { } } -/// The temporary file an atomic write of `path` goes through. +/// The temporary file an atomic write of `path` goes through. Unique per call (process id plus a +/// counter), so two writers saving the same destination never share a temporary — one cannot +/// truncate, rename or clean up another's — and there is no predictable name for a pre-existing +/// file or symlink to sit at. pub fn temp_path(path: &Path) -> PathBuf { + use std::sync::atomic::{AtomicU64, Ordering}; + static NEXT: AtomicU64 = AtomicU64::new(0); let mut name = path.file_name().map(|n| n.to_os_string()).unwrap_or_default(); - name.push(".tmp"); + name.push(format!(".{}-{}.tmp", std::process::id(), NEXT.fetch_add(1, Ordering::Relaxed))); path.with_file_name(name) } @@ -177,19 +182,30 @@ pub fn temp_path(path: &Path) -> PathBuf { pub fn atomic_write(path: &Path, data: &[u8]) -> std::io::Result<()> { atomic_write_with(path, data, |tmp, data| { use std::io::Write; - let mut f = std::fs::File::create(tmp)?; + // Exclusive creation: an entry already at the temporary's name (a leftover from a crashed + // writer, or something planted there) is never opened, truncated or written through. + let mut f = std::fs::OpenOptions::new().write(true).create_new(true).open(tmp)?; f.write_all(data)?; f.sync_all() }) } -/// [`atomic_write`] with an injectable temp-file writer (tests simulate a crash mid-write). A -/// failed write removes the temporary file and leaves `path` untouched. -pub fn atomic_write_with(path: &Path, data: &[u8], write: impl FnOnce(&Path, &[u8]) -> std::io::Result<()>) -> std::io::Result<()> { - let tmp = temp_path(path); - if let Err(e) = write(&tmp, data) { - let _ = std::fs::remove_file(&tmp); - return Err(e); +/// [`atomic_write`] with an injectable temp-file writer (tests simulate a crash mid-write). The +/// temporary belongs to this call alone: the writer must create it exclusively, and a failed +/// write removes only its own temporary and leaves `path` untouched. An `AlreadyExists` from the +/// writer means the name was taken (never opened by us): that entry is left alone and another +/// unique name is tried. +pub fn atomic_write_with(path: &Path, data: &[u8], mut write: impl FnMut(&Path, &[u8]) -> std::io::Result<()>) -> std::io::Result<()> { + for _ in 0..16 { + let tmp = temp_path(path); + match write(&tmp, data) { + Ok(()) => return std::fs::rename(&tmp, path), + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => continue, + Err(e) => { + let _ = std::fs::remove_file(&tmp); + return Err(e); + } + } } - std::fs::rename(&tmp, path) + Err(std::io::Error::new(std::io::ErrorKind::AlreadyExists, format!("{}: no free temporary name", path.display()))) } diff --git a/crates/engine/src/tests_settings.rs b/crates/engine/src/tests_settings.rs index 458c886f..4ce956ca 100644 --- a/crates/engine/src/tests_settings.rs +++ b/crates/engine/src/tests_settings.rs @@ -5,7 +5,7 @@ use std::sync::Arc; use serde_json::json; -use crate::config::{ConfigStore, DirConfig, MemoryConfig, atomic_write, atomic_write_with, temp_path}; +use crate::config::{ConfigStore, DirConfig, MemoryConfig, atomic_write, atomic_write_with}; use crate::prefs::{PREFS_FILE, Prefs}; use crate::shortcuts::{DEFAULT_PRESET, Keymaps, normalize}; use crate::{Session, autosave}; @@ -560,7 +560,9 @@ fn atomic_write_survives_a_crash_mid_write() { }); assert!(r.is_err()); assert_eq!(std::fs::read(&f).unwrap(), b"version 1"); - assert!(!temp_path(&f).exists(), "the partial temp file is cleaned up"); + let leftovers: Vec = + std::fs::read_dir(&d).unwrap().flatten().map(|e| e.file_name().to_string_lossy().to_string()).filter(|n| n != "p.ecproj").collect(); + assert!(leftovers.is_empty(), "the partial temporary is cleaned up: {leftovers:?}"); // A temp file left by a hard kill doesn't count as an auto-save. std::fs::write(d.join("p auto-save 1.ecproj.tmp"), b"junk").unwrap(); assert!(autosave::existing(&d, "p").is_empty()); @@ -569,6 +571,54 @@ fn atomic_write_survives_a_crash_mid_write() { let _ = std::fs::remove_dir_all(d); } +#[cfg(unix)] +#[test] +fn atomic_write_does_not_follow_a_planted_temporary_symlink() { + let d = tmp("atomic-link"); + let f = d.join("p.ecproj"); + let sentinel = d.join("sentinel.txt"); + std::fs::write(&sentinel, b"precious").unwrap(); + // Someone plants a symlink at the temporary name the write used to go through. + std::os::unix::fs::symlink(&sentinel, d.join("p.ecproj.tmp")).unwrap(); + atomic_write(&f, b"new").unwrap(); + assert_eq!(std::fs::read(&sentinel).unwrap(), b"precious", "a symlink at the temporary name is never followed"); + assert_eq!(std::fs::read(&f).unwrap(), b"new"); + let _ = std::fs::remove_dir_all(d); +} + +#[cfg(unix)] +#[test] +fn atomic_writes_in_parallel_never_publish_a_mixture() { + use std::sync::{Arc, Barrier}; + let d = tmp("atomic-race"); + let f = d.join("p.ecproj"); + let a = vec![b'A'; 64]; + let b = vec![b'B'; 64]; + // Writer A writes the first half of its temporary, then waits while writer B runs against + // the same destination, and only then finishes. + let (half, published) = (Arc::new(Barrier::new(2)), Arc::new(Barrier::new(2))); + let (f2, a2) = (f.clone(), a.clone()); + let (half2, published2) = (half.clone(), published.clone()); + let t = std::thread::spawn(move || { + atomic_write_with(&f2, &a2, |tmp, data| { + use std::io::Write; + let mut fh = std::fs::File::create(tmp)?; + fh.write_all(&data[..32])?; + half2.wait(); + published2.wait(); + fh.write_all(&data[32..])?; + fh.sync_all() + }) + }); + half.wait(); + atomic_write(&f, &b).unwrap(); + published.wait(); + let _ = t.join(); + let got = std::fs::read(&f).unwrap(); + assert!(got == a || got == b, "the destination holds one complete write, never a mixture: {}", String::from_utf8_lossy(&got)); + let _ = std::fs::remove_dir_all(d); +} + #[test] fn autosave_rotates_through_max_versions() { let d = tmp("rotate"); From 400379fd20594ca0b8e64a29b71ada8abf1af55d Mon Sep 17 00:00:00 2001 From: midasmatterhorn <248507632+midasmatterhorn@users.noreply.github.com> Date: Thu, 15 Oct 2026 16:05:32 +0300 Subject: [PATCH 2/3] pdf: a soft mask at an unsupported bit depth is ignored, not fatal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mask_image read /BitsPerComponent from the attacker-controlled /SMask dict with no validation and fed it straight into 1u32 << bpc — the main image path rejects anything but 1|2|4|8|16 before its own identical shift, the mask path did not. /BitsPerComponent 32 panics in debug and test builds and wraps the shift in release, mistranslating every sample into alpha. Mirror decode()'s depth check: an unsupported mask depth means no usable mask, so the image draws with the mask treated as absent. Regression test: smask_with_absurd_bpc_is_ignored_not_fatal — panics at crates/pdf/src/image.rs:184 on main, passes with the check. --- crates/pdf/src/image.rs | 5 +++++ crates/pdf/src/tests_import.rs | 25 +++++++++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/crates/pdf/src/image.rs b/crates/pdf/src/image.rs index 5e6c49a5..1b39f062 100644 --- a/crates/pdf/src/image.rs +++ b/crates/pdf/src/image.rs @@ -181,6 +181,11 @@ fn mask_image(file: &File, o: &Obj, soft: bool) -> Option<(Vec, usize, usize return Some((data.into_iter().step_by(jpeg_n.unwrap_or(1)).take(w * h).collect(), w, h)); } let bpc = if soft { file.get_num(d, "BitsPerComponent").unwrap_or(8.0) as usize } else { 1 }; + // Like `decode()`: only 1/2/4/8/16-bit depths are supported. Anything else (notably + // `bpc >= 32`, where `1u32 << bpc` below would overflow) means "no usable mask". + if !matches!(bpc, 1 | 2 | 4 | 8 | 16) { + return None; + } let max = ((1u32 << bpc) - 1).max(1) as f64; let dec = file.get(d, "Decode").map(|x| file.nums(x)).filter(|v| v.len() >= 2); let v = unpack(&data, w, h, 1, bpc); diff --git a/crates/pdf/src/tests_import.rs b/crates/pdf/src/tests_import.rs index add447e3..064b5614 100644 --- a/crates/pdf/src/tests_import.rs +++ b/crates/pdf/src/tests_import.rs @@ -263,6 +263,31 @@ fn hostile_structures_return_errors_or_draw_nothing() { assert!(!doc.skipped.is_empty()); } +#[test] +fn smask_with_absurd_bpc_is_ignored_not_fatal() { + // `/BitsPerComponent 32` in a soft mask: `1u32 << bpc` must neither panic (debug) + // nor mistranslate samples into alpha (release); the mask is treated as absent. + let bytes = page_pdf( + "q 2 0 0 2 0 0 cm /Im Do Q", + "/XObject << /Im 10 0 R >>", + vec![ + ( + 10, + "<< /Type /XObject /Subtype /Image /Width 2 /Height 2 /ColorSpace /DeviceGray /BitsPerComponent 8 /SMask 11 0 R >>".into(), + Some(vec![0x80; 4]), + ), + ( + 11, + "<< /Type /XObject /Subtype /Image /Width 2 /Height 2 /ColorSpace /DeviceGray /BitsPerComponent 32 >>".into(), + Some(vec![0xFF; 16]), + ), + ], + ); + let doc = parse(&bytes).unwrap(); + let img = render(&doc); + assert!(coverage(&img, 0, 0, 200, 100) > 0.0, "image still draws, opaque"); +} + #[test] fn truncated_and_corrupt_eps_never_panic() { let eps = b"%!PS-Adobe-3.0 EPSF-3.0\n%%BoundingBox: 0 0 100 100\n\ From 0fbf4e887eafbec649ec4dd91e10d98f642b06f5 Mon Sep 17 00:00:00 2001 From: bflatastic Date: Fri, 16 Oct 2026 18:39:10 +0700 Subject: [PATCH 3/3] pdf: leave the atomic-write test to its own PR Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/tests_settings.rs | 36 ----------------------------- 1 file changed, 36 deletions(-) diff --git a/crates/engine/src/tests_settings.rs b/crates/engine/src/tests_settings.rs index ee4c6ebe..65e39b46 100644 --- a/crates/engine/src/tests_settings.rs +++ b/crates/engine/src/tests_settings.rs @@ -629,42 +629,6 @@ fn atomic_write_survives_a_crash_mid_write() { let _ = std::fs::remove_dir_all(d); } -/// A writer that is paused mid-temporary while another writer fully publishes to the same -/// destination must not let either one publish a mixture: the bounded, exclusively-created -/// temporaries keep the two writes apart (#1233). Unlike -/// `concurrent_atomic_writes_each_publish_a_whole_file`, this pins the interleaving -/// deterministically instead of sampling it under real contention. -#[test] -fn atomic_writes_in_parallel_never_publish_a_mixture() { - use std::sync::{Arc, Barrier}; - let d = tmp("atomic-race"); - let f = d.join("p.ecproj"); - let a = vec![b'A'; 64]; - let b = vec![b'B'; 64]; - // Writer A writes the first half of its temporary, then waits while writer B fully publishes - // against the same destination, and only then finishes. - let (half, published) = (Arc::new(Barrier::new(2)), Arc::new(Barrier::new(2))); - let (f2, a2) = (f.clone(), a.clone()); - let (half2, published2) = (half.clone(), published.clone()); - let t = std::thread::spawn(move || { - atomic_write_with(&f2, &a2, |fh, data| { - use std::io::Write; - fh.write_all(&data[..32])?; - half2.wait(); - published2.wait(); - fh.write_all(&data[32..])?; - fh.sync_all() - }) - }); - half.wait(); - atomic_write(&f, &b).unwrap(); - published.wait(); - assert!(t.join().unwrap().is_ok(), "the paused writer still publishes"); - let got = std::fs::read(&f).unwrap(); - assert!(got == a || got == b, "the destination holds one complete write, never a mixture: {}", String::from_utf8_lossy(&got)); - let _ = std::fs::remove_dir_all(d); -} - #[test] fn autosave_rotates_through_max_versions() { let d = tmp("rotate");