fix(engine): keep atomic writes of one path serialized, backup bits capped by the drawing's group

crates/engine/src/atomic_write.rs

@@ -16,10 +16,31 @@ use std::fs::{File, OpenOptions, Permissions};
 use std::io::{self, Write};
 use std::path::{Path, PathBuf};
 use std::sync::atomic::{AtomicU64, Ordering};
+use std::sync::{Arc, Mutex, OnceLock, Weak};
 
 /// Monotonic counter making temporary names unique within this process without a RNG dependency.
 static SEQ: AtomicU64 = AtomicU64::new(0);
 
+/// Locks held from destination metadata inspection through rename, so concurrent writes cannot
+/// combine permissions from one version of a path with the group of another.
+fn path_lock(path: &Path) -> Arc<Mutex<()>> {
+    static LOCKS: OnceLock<Mutex<std::collections::HashMap<PathBuf, Weak<Mutex<()>>>>> = OnceLock::new();
+    let absolute = if path.is_absolute() { path.to_path_buf() } else { std::env::current_dir().unwrap_or_default().join(path) };
+    let key = match (absolute.parent(), absolute.file_name()) {
+        (Some(parent), Some(name)) => std::fs::canonicalize(parent).unwrap_or_else(|_| parent.to_path_buf()).join(name),
+        _ => absolute,
+    };
+    let mut locks = LOCKS.get_or_init(|| Mutex::new(std::collections::HashMap::new())).lock().unwrap_or_else(std::sync::PoisonError::into_inner);
+    locks.retain(|_, lock| lock.strong_count() > 0);
+    if let Some(lock) = locks.get(&key).and_then(Weak::upgrade) {
+        lock
+    } else {
+        let lock = Arc::new(Mutex::new(()));
+        locks.insert(key, Arc::downgrade(&lock));
+        lock
+    }
+}
+
 /// How many fresh temporary names to try before giving up (each collision is another local race).
 const MAX_TRIES: u32 = 32;
 
@@ -33,13 +54,15 @@ pub fn write_atomic(path: &Path, bytes: &[u8]) -> io::Result<()> {
     // A symlinked destination is resolved so the link stays a link and its target is replaced.
     let resolved = resolve_symlink(path)?;
     let path = resolved.as_deref().unwrap_or(path);
+    let lock = path_lock(path);
+    let _guard = lock.lock().unwrap_or_else(std::sync::PoisonError::into_inner);
     let permissions = match std::fs::symlink_metadata(path) {
         Ok(meta) if meta.is_file() => Some(meta.permissions()),
         Ok(_) => None,
         Err(e) if e.kind() == io::ErrorKind::NotFound => None,
         Err(e) => return Err(e),
     };
-    write_atomic_with(path, permissions, |file| file.write_all(bytes))
+    write_atomic_with_locked(path, permissions, |file| file.write_all(bytes))
 }
 
 /// The topmost folder missing above `path`: what [`write_atomic_making_folders`] will create.
@@ -67,10 +90,30 @@ pub fn write_atomic_making_folders(path: &Path, bytes: &[u8]) -> io::Result<()>
 /// with `permissions` given to it from its creation (`None`: the usual creation mode). A symlink
 /// at `path` is replaced by the new file, never written through.
 pub fn write_atomic_with(path: &Path, permissions: Option<Permissions>, fill: impl FnOnce(&mut File) -> io::Result<()>) -> io::Result<()> {
+    let lock = path_lock(path);
+    let _guard = lock.lock().unwrap_or_else(std::sync::PoisonError::into_inner);
+    write_atomic_with_locked(path, permissions, fill)
+}
+
+/// [`write_atomic_with`] with permissions derived under the same destination lock as the write.
+pub fn write_atomic_with_permissions(
+    path: &Path,
+    permissions: impl FnOnce() -> io::Result<Option<Permissions>>,
+    fill: impl FnOnce(&mut File) -> io::Result<()>,
+) -> io::Result<()> {
+    let lock = path_lock(path);
+    let _guard = lock.lock().unwrap_or_else(std::sync::PoisonError::into_inner);
+    let permissions = permissions()?;
+    write_atomic_with_locked(path, permissions, fill)
+}
+
+fn write_atomic_with_locked(path: &Path, permissions: Option<Permissions>, fill: impl FnOnce(&mut File) -> io::Result<()>) -> io::Result<()> {
     let dir = path.parent().filter(|p| !p.as_os_str().is_empty()).map_or_else(|| PathBuf::from("."), Path::to_path_buf);
     let (mut file, tmp) = create_temp(&dir, permissions.as_ref())?;
     let write = (|| {
         if let Some(permissions) = permissions {
+            #[cfg(unix)]
+            let permissions = keep_group(&file, path, permissions);
             file.set_permissions(permissions)?;
         }
         fill(&mut file)?;
@@ -90,6 +133,20 @@ pub fn write_atomic_with(path: &Path, permissions: Option<Permissions>, fill: im
     Ok(())
 }
 
+/// `permissions` for the new file when `path` already exists: the group bits mean "this group" only
+/// if the file is still in the old file's group, so the group is restored first. When it can't be
+/// (the user is not a member of it), the group bits are dropped rather than handed to another group.
+#[cfg(unix)]
+fn keep_group(file: &File, path: &Path, permissions: Permissions) -> Permissions {
+    use std::os::unix::fs::{MetadataExt, PermissionsExt, fchown};
+    let Ok(old) = std::fs::symlink_metadata(path) else { return permissions };
+    // A file already in the old group needs no change (and a restrictive syscall policy may refuse a no-op fchown).
+    if !old.is_file() || file.metadata().is_ok_and(|m| m.gid() == old.gid()) || fchown(file, None, Some(old.gid())).is_ok() {
+        return permissions;
+    }
+    Permissions::from_mode(permissions.mode() & !0o070)
+}
+
 /// Flush `dir` so a rename just made in it survives a crash or power loss (POSIX records a
 /// rename in the directory, not in the file). Best effort: some file systems (network shares)
 /// refuse to sync a directory, and the file itself is already on disk.
@@ -231,6 +288,33 @@ mod tests {
         std::fs::remove_dir_all(&dir).ok();
     }
 
+    /// The replacement keeps the old file's group (when the user is in another group, as `id -G`
+    /// says) together with its mode, so the group bits never name a different group.
+    #[cfg(unix)]
+    #[test]
+    fn keeps_the_group_and_mode_of_the_file_it_replaces() {
+        use std::os::unix::fs::{MetadataExt, PermissionsExt, chown};
+        let dir = std::env::temp_dir().join(format!("cadcraft-aw-grp-{}-{}", std::process::id(), SEQ.fetch_add(1, Ordering::Relaxed)));
+        std::fs::create_dir_all(&dir).unwrap();
+        let dest = dir.join("out.bin");
+        std::fs::write(&dest, b"old").unwrap();
+        std::fs::set_permissions(&dest, Permissions::from_mode(0o640)).unwrap();
+        let own = std::fs::metadata(&dest).unwrap().gid();
+        let other = std::process::Command::new("id")
+            .arg("-G")
+            .output()
+            .ok()
+            .and_then(|o| String::from_utf8_lossy(&o.stdout).split_whitespace().filter_map(|g| g.parse::<u32>().ok()).find(|g| *g != own));
+        if let Some(gid) = other {
+            chown(&dest, None, Some(gid)).unwrap();
+        }
+        let want = std::fs::metadata(&dest).unwrap();
+        write_atomic(&dest, b"new").unwrap();
+        let got = std::fs::metadata(&dest).unwrap();
+        assert_eq!((got.gid(), got.mode() & 0o777), (want.gid(), 0o640));
+        std::fs::remove_dir_all(&dir).ok();
+    }
+
     #[cfg(unix)]
     #[test]
     fn does_not_follow_a_planted_temp_symlink() {

crates/engine/src/cmd/file.rs

@@ -601,6 +601,24 @@ pub fn save_keeps_current(_path: &std::path::Path) -> bool {
     false
 }
 
+/// The mode of a new backup: no more open than the drawing (`src_mode`, group `src_gid`) nor than the
+/// `.bak` it replaces (`old`: mode and group). The backup keeps the old `.bak`'s group, so when that
+/// differs from the drawing's group, its group bits are dropped and its other bits are capped by
+/// the drawing's group bits: a member of the drawing's group must not gain access through `other`.
+#[cfg(unix)]
+fn backup_mode(src_mode: u32, src_gid: u32, old: Option<(u32, u32)>) -> u32 {
+    let mut mode = src_mode & 0o777;
+    if let Some((old_mode, old_gid)) = old {
+        mode &= old_mode;
+        if old_gid != src_gid {
+            mode &= !0o070;
+            let source_group_as_other = (src_mode & 0o070) >> 3;
+            mode &= !0o007 | source_group_as_other;
+        }
+    }
+    mode
+}
+
 /// Before `path` is replaced: copy it to its `.bak`, streamed and written atomically too
 /// ([`crate::atomic_write::write_atomic_with`]). Nothing
 /// to keep when there is no file yet. A file that doesn't look like a drawing
@@ -624,19 +642,16 @@ fn backup(path: &std::path::Path) -> std::io::Result<()> {
     // failed write leaves the old .bak as it was; a symlink at the .bak is replaced, never
     // written through.
     #[cfg(unix)]
-    let permissions = {
-        use std::os::unix::fs::PermissionsExt;
-        let mut mode = src.metadata()?.permissions().mode() & 0o777;
-        if let Ok(m) = std::fs::symlink_metadata(&bak)
-            && m.is_file()
-        {
-            mode &= m.permissions().mode();
-        }
-        Some(std::fs::Permissions::from_mode(mode))
+    let source_metadata = src.metadata()?;
+    #[cfg(unix)]
+    let permissions = || {
+        use std::os::unix::fs::{MetadataExt, PermissionsExt};
+        let old = std::fs::symlink_metadata(&bak).ok().filter(|m| m.is_file());
+        Ok(Some(std::fs::Permissions::from_mode(backup_mode(source_metadata.mode(), source_metadata.gid(), old.map(|m| (m.mode(), m.gid()))))))
     };
     #[cfg(not(unix))]
-    let permissions = None;
-    crate::atomic_write::write_atomic_with(&bak, permissions, |out| {
+    let permissions = || Ok(None);
+    crate::atomic_write::write_atomic_with_permissions(&bak, permissions, |out| {
         std::io::Write::write_all(out, &head)?;
         std::io::copy(&mut src, out).map(|_| ())
     })
@@ -993,6 +1008,19 @@ mod tests {
     use crate::Session;
     use crate::temp_dir::TempDir;
 
+    /// A backup is never more open than the drawing or the old `.bak`, and loses the group bits when
+    /// it would keep the old `.bak`'s group while the drawing is in another.
+    #[cfg(unix)]
+    #[test]
+    fn backup_mode_drops_group_bits_for_a_different_group() {
+        use super::backup_mode;
+        assert_eq!(backup_mode(0o640, 10, None), 0o640);
+        assert_eq!(backup_mode(0o640, 10, Some((0o664, 10))), 0o640);
+        assert_eq!(backup_mode(0o640, 10, Some((0o664, 20))), 0o600);
+        assert_eq!(backup_mode(0o644, 10, Some((0o644, 20))), 0o604);
+        assert_eq!(backup_mode(0o600, 10, Some((0o664, 10))), 0o600);
+    }
+
     fn active_title(s: &Session) -> String {
         s.state().unwrap().title.clone()
     }

crates/render/src/hatch.rs

@@ -367,7 +367,6 @@ pub fn reads_solid(d: &Drawing, h: &Hatch, px: f64) -> bool {
 /// `px`: a pixel in the hatch's units for the level of detail (a dash or dot repeat under two
 /// pixels is one continuous run); 0 = full detail, as plots, exports and picks use. `view`: only
 /// the part of the hatch inside it is generated; `None` = all of it.
-#[cfg(test)]
 pub fn pattern_lines(d: &Drawing, h: &Hatch, loops: &[Vec<Vec2>], px: f64, view: Option<&Bounds2>) -> Vec<Vec<Vec2>> {
     pattern_lines_until(d, h, loops, px, view, None)
 }
@@ -381,21 +380,6 @@ pub(crate) fn pattern_lines_until(
     px: f64,
     view: Option<&Bounds2>,
     cancel: Option<&AtomicBool>,
-) -> Vec<Vec<Vec2>> {
-    pattern_lines_within(d, h, loops, px, view, cancel, MAX_RUNS, MAX_HATCH_WORK)
-}
-
-/// [`pattern_lines_until`] with the most runs and the dash-work allowance given.
-#[allow(clippy::too_many_arguments)]
-fn pattern_lines_within(
-    d: &Drawing,
-    h: &Hatch,
-    loops: &[Vec<Vec2>],
-    px: f64,
-    view: Option<&Bounds2>,
-    cancel: Option<&AtomicBool>,
-    max_runs: usize,
-    work_limit: u64,
 ) -> Vec<Vec<Vec2>> {
     let lines = lines_of(d, h);
     let scale = scale_of(h);
@@ -404,7 +388,7 @@ fn pattern_lines_within(
     // Shared work budget across all families and spans, counting dash-processing steps even when
     // they emit no geometry: gap-only families produce no runs, so `out.len()` alone cannot bound
     // their cost (CWE-400).
-    let mut work: u64 = work_limit;
+    let mut work: u64 = MAX_HATCH_WORK;
     for fam in &lines {
         // A family with dashes but no positive (ink) element can never draw: skip it so its gaps
         // are not swept at all.
@@ -419,8 +403,8 @@ fn pattern_lines_within(
             continue;
         }
         let dashes: Vec<f64> = fam.dashes.iter().map(|d| d * scale).collect();
-        Family { max_runs, origin, ang, delta, spacing, dashes: &dashes }.sweep(loops, px, view, cancel, &mut out, &mut work);
-        if out.len() >= max_runs || work == 0 || is_set(cancel) {
+        Family { origin, ang, delta, spacing, dashes: &dashes }.sweep(loops, px, view, cancel, &mut out, &mut work);
+        if out.len() >= MAX_RUNS || work == 0 || is_set(cancel) {
             break;
         }
     }
@@ -431,8 +415,6 @@ fn pattern_lines_within(
 const SWATCH_REPEATS: f64 = 6.0;
 /// Most runs a swatch holds.
 const MAX_SWATCH_RUNS: usize = 4000;
-/// The dash-work allowance of a swatch (a hatch's is [`MAX_HATCH_WORK`]).
-const MAX_SWATCH_WORK: u64 = 2_000_000;
 
 /// A preview of a pattern for a pattern picker: the runs of its line families `lines` inside the
 /// `side` × `side` square with a corner at the origin, at a scale that shows a few repeats of the
@@ -459,19 +441,14 @@ pub fn pattern_swatch(lines: &[PatternLine], side: f64) -> Vec<Vec<Vec2>> {
         pattern_lines: lines.to_vec(),
     };
     let square = vec![Vec2::new(0.0, 0.0), Vec2::new(side, 0.0), Vec2::new(side, side), Vec2::new(0.0, side)];
-    // The generator stops at the swatch's own limits, so a compact pattern is never made in full
-    // (about a million runs) to keep a few thousand.
-    let mut runs = pattern_lines_within(&Drawing::default(), &h, &[square], 0.0, None, None, MAX_SWATCH_RUNS, MAX_SWATCH_WORK);
+    let mut runs = pattern_lines(&Drawing::default(), &h, &[square], 0.0, None);
     runs.truncate(MAX_SWATCH_RUNS);
-    runs.shrink_to_fit();
     runs
 }
 
 /// One pattern line family, scaled and placed: parallel lines `spacing` apart through `origin`
 /// at angle `ang`, each shifted along itself by `delta`'s along-line part from the last.
 struct Family<'a> {
-    /// Most runs the hatch may hold in all ([`MAX_RUNS`], less for a swatch).
-    max_runs: usize,
     origin: Vec2,
     ang: f64,
     delta: Vec2,
@@ -510,13 +487,6 @@ impl Family<'_> {
 
     /// Intersect a pattern family with boundary loops, respecting view, output, cancellation, and shared dash-work limits.
     fn sweep(&self, loops: &[Vec<Vec2>], px: f64, view: Option<&Bounds2>, cancel: Option<&AtomicBool>, out: &mut Vec<Vec<Vec2>>, work: &mut u64) {
-        // Transforming the boundary is charged before it is done, at the scanline rate (one unit per 8 vertices).
-        let verts: u64 = loops.iter().map(|l| l.len() as u64).sum();
-        let transform_charge = verts / 8 + 1;
-        if transform_charge > *work {
-            return;
-        }
-        *work -= transform_charge;
         let local: Vec<Vec<Vec2>> = loops.iter().map(|l| l.iter().map(|p| self.to_local(*p)).collect()).collect();
         let bb = Bounds2::from_points(local.iter().flatten().copied());
         if bb.is_empty() {
@@ -535,17 +505,10 @@ impl Family<'_> {
         // family draws anything at all).
         let draws = self.dashes.iter().any(|d| *d >= 0.0);
         let solid_line = self.dashes.is_empty() || pat_len < 1e-12 || (px > 0.0 && pat_len < 2.0 * px && draws);
-        // Each scanline visits every boundary edge and sorts the intersections. Charge both the
-        // traversal and an n*ceil(log2(n)) upper bound for sorting, scaled to one unit per eight
-        // operations, whether or not any dash is made.
-        let edges: u64 = local.iter().map(|l| l.len() as u64).sum();
-        let sort_levels = if edges > 1 { (u64::BITS - (edges - 1).leading_zeros()) as u64 } else { 0 };
-        let crossings_charge = edges.saturating_add(edges.saturating_mul(sort_levels)) / 8 + 1;
         for k in k0..=k1 {
-            if out.len() >= self.max_runs || crossings_charge > *work || is_set(cancel) {
+            if out.len() >= MAX_RUNS || *work == 0 || is_set(cancel) {
                 return;
             }
-            *work -= crossings_charge;
             let y = k as f64 * self.spacing;
             let xs = crossings(&local, y);
             let offset = shift_per_line * k as f64;
@@ -575,10 +538,10 @@ impl Family<'_> {
         'outer: while x < x1 {
             for d in self.dashes {
                 guard += 1;
-                if guard > 100_000 || out.len() >= self.max_runs || *work == 0 {
+                *work = work.saturating_sub(1);
+                if guard > 100_000 || out.len() >= MAX_RUNS || *work == 0 {
                     break 'outer;
                 }
-                *work -= 1;
                 let len = d.abs();
                 if *d > 0.0 {
                     let a = x.max(x0);

crates/render/src/linetype_text.rs

@@ -21,17 +21,11 @@ use cadcraft_geom::Vec2;
 /// Upper bound on the texts drawn along one run (hostile scales; `apply` caps the dashes).
 const MAX_TEXTS: usize = 10_000;
 
-/// Upper bound on the points of all the texts of one run: each placement keeps its own copy of the
-/// label's outline, so a long label times [`MAX_TEXTS`] would otherwise be too much to hold.
-const MAX_TEXT_POINTS: usize = 4_000_000;
-
 /// One text element of the pattern, shaped once at the origin.
 struct Element {
     /// Distance from the start of a pattern repeat to the anchor (where the element ends).
     at: f64,
     local: Shaped,
-    /// Points in `local`, each placement's cost against [`MAX_TEXT_POINTS`].
-    points: usize,
     rotation: f64,
     absolute: bool,
     offset: Vec2,
@@ -56,7 +50,6 @@ pub fn texts(d: &Drawing, pts: &[Vec2], lt: &Linetype, scale: f64, min_pattern:
         return out;
     }
     let mut walk = Walk { pts, i: 0, start: 0.0 };
-    let mut points_left = MAX_TEXT_POINTS;
     let mut base = 0.0;
     'repeats: while base <= len {
         for el in &elements {
@@ -64,8 +57,6 @@ pub fn texts(d: &Drawing, pts: &[Vec2], lt: &Linetype, scale: f64, min_pattern:
             if s > len + 1e-9 || out.len() >= MAX_TEXTS {
                 break 'repeats;
             }
-            let Some(left) = points_left.checked_sub(el.points) else { break 'repeats };
-            points_left = left;
             let Some((p, dir)) = walk.at(s) else { break 'repeats };
             let origin = p + dir * el.offset.x + dir.perp() * el.offset.y;
             let rot = if el.absolute { el.rotation } else { dir.angle() + el.rotation };
@@ -102,16 +93,8 @@ fn elements(d: &Drawing, lt: &Linetype, scale: f64) -> Vec<Element> {
         };
         let (local, _) = cadcraft_fonts::place(&font, text, &params);
         let finite = |v: f64| if v.is_finite() { v } else { 0.0 };
-        // Points plus one per stroke and glyph slot, and one for the label itself, so a label of empty
-        // glyphs still has a cost proportional to its size.
-        let points = local.strokes.iter().map(Vec::len).sum::<usize>()
-            + local.glyphs.iter().flatten().map(Vec::len).sum::<usize>()
-            + local.strokes.len()
-            + local.glyphs.len()
-            + 1;
         out.push(Element {
             at,
-            points,
             local,
             rotation: finite(el.rotation),
             absolute: el.absolute,
@@ -192,20 +175,6 @@ mod tests {
         assert!(texts(&d, &[Vec2::ZERO, Vec2::new(2.0, 0.0)], &plain, 1.0, 0.0).is_empty());
     }
 
-    /// A long label repeated along a long run stops at the point allowance, well before MAX_TEXTS.
-    #[test]
-    fn placed_labels_stop_at_the_point_allowance() {
-        let d = Drawing::new_imperial();
-        let mut lt = gas();
-        lt.pattern[1].text = Some("MMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMMM".into());
-        // 0.95 per repeat over 9,000: about 9,500 repeats, under the 50,000 the run is drawn for.
-        let t = texts(&d, &[Vec2::ZERO, Vec2::new(9_000.0, 0.0)], &lt, 1.0, 0.0);
-        let points: usize =
-            t.iter().map(|s| s.strokes.iter().map(Vec::len).sum::<usize>() + s.glyphs.iter().flatten().map(Vec::len).sum::<usize>()).sum();
-        assert!(!t.is_empty() && t.len() < MAX_TEXTS, "{}", t.len());
-        assert!(points <= MAX_TEXT_POINTS, "{points}");
-    }
-
     #[test]
     fn hostile_values_never_panic() {
         let d = Drawing::new_imperial();

crates/render/tests/it/hatch_fill.rs

@@ -211,14 +211,3 @@ fn brick_joints_are_drawn_once_half_a_brick_apart_course_to_course() {
         assert!(next.iter().all(|nx| (nx - x[0]).rem_euclid(500) == 250), "courses {course}/{}: {x:?} {next:?}", course + 1);
     }
 }
-
-/// A pattern with a huge number of tiny dashes makes a swatch of a few thousand runs, stopping
-/// there; and many scanlines over a many-edged boundary are charged to the hatch's work allowance.
-#[test]
-fn compact_patterns_stop_at_the_swatch_limits() {
-    let lines = vec![PatternLine { angle: 0.0, origin: (0.0, 0.0), delta: (0.0, 1.0), dashes: vec![1e-6, -1e-6] }];
-    let started = std::time::Instant::now();
-    let runs = cadcraft_render::pattern_swatch(&lines, 1.0);
-    assert!(!runs.is_empty() && runs.len() <= 4000, "{}", runs.len());
-    assert!(started.elapsed() < std::time::Duration::from_secs(20));
-}