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

crates/constraints/src/auto.rs

@@ -232,10 +232,6 @@ pub fn infer_placed(d: &Drawing, placed: &[Handle], others: &[Handle], o: &Infer
     let mut extra = Vec::new();
     let mut on = Vec::new();
     for (rp, p) in g.points.iter().filter(|(r, _)| placed.contains(&r.handle)) {
-        // Nothing is added once the candidates are full, so the points left need no search.
-        if extra.len() + on.len() + out.len() >= MAX_CANDIDATES {
-            break;
-        }
         if out.iter().any(|c| c.kind == ConstraintKind::Coincident && c.refs.contains(rp)) {
             continue;
         }
@@ -271,22 +267,20 @@ pub fn infer_placed(d: &Drawing, placed: &[Handle], others: &[Handle], o: &Infer
     // Perpendicular lines meeting in a T (a perpendicular snap onto the middle of a line).
     let atol = if o.angle_tolerance.is_finite() { o.angle_tolerance.clamp(0.0, 45.0).to_radians() } else { 0.0 };
     let allow_per = o.types.is_empty() || o.types.iter().any(|t| t.eq_ignore_ascii_case(ConstraintKind::Perpendicular.name()));
-    'per: for (r1, a1, b1) in g.lines.iter().filter(|(r, _, _)| allow_per && placed.contains(&r.handle)) {
+    for (r1, a1, b1) in g.lines.iter().filter(|(r, _, _)| placed.contains(&r.handle)) {
         for (r2, a2, b2) in g.lines.iter().filter(|(r, _, _)| r.handle != r1.handle) {
-            // Full: nothing more can be added, so the pairs left need no geometry or duplicate search.
-            if out.len() >= MAX_CANDIDATES {
-                break 'per;
-            }
             let (l1, l2) = (Line::new(*a1, *b1), Line::new(*a2, *b2));
             let touches = |l: &Line, p: Vec2| l.dist(p) <= tol && (0.0..=1.0).contains(&l.param_of(p));
             let tee = [*a1, *b1].iter().any(|p| touches(&l2, *p)) || [*a2, *b2].iter().any(|p| touches(&l1, *p));
             let ang = ang_between(*b1 - *a1, *b2 - *a2);
-            // The duplicate search scans every candidate, so it comes after the cheap tests.
-            if tee
+            let dup = out.iter().any(|c| c.kind == ConstraintKind::Perpendicular && c.refs.contains(r1) && c.refs.contains(r2));
+            if allow_per
+                && tee
+                && !dup
                 && (std::f64::consts::FRAC_PI_2 - ang).abs() <= atol
                 && l1.len() > tol
                 && l2.len() > tol
-                && !out.iter().any(|c| c.kind == ConstraintKind::Perpendicular && c.refs.contains(r1) && c.refs.contains(r2))
+                && out.len() < MAX_CANDIDATES
             {
                 out.push(Constraint { id: 0, kind: ConstraintKind::Perpendicular, refs: vec![*r1, *r2], name: String::new(), expr: String::new() });
             }

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/fonts/src/ttf.rs

@@ -89,11 +89,7 @@ impl Face {
         let Some(f) = self.font() else { return Vec::new() };
         [skrifa::string::StringId::FAMILY_NAME, skrifa::string::StringId::TYPOGRAPHIC_FAMILY_NAME, skrifa::string::StringId::FULL_NAME]
             .into_iter()
-            // A name table can hold many records that point at one big string: at most
-            // MAX_NAME_RECORDS per name id, each decoded only up to MAX_NAME_CHARS characters.
-            .flat_map(|id| {
-                f.localized_strings(id).take(MAX_NAME_RECORDS).map(|s| norm(&s.chars().take(MAX_NAME_CHARS).collect::<String>())).collect::<Vec<_>>()
-            })
+            .flat_map(|id| f.localized_strings(id).map(|s| norm(&s.to_string())).collect::<Vec<_>>())
             .collect()
     }
 }
@@ -108,10 +104,6 @@ pub fn faces(bytes: &Arc<Vec<u8>>) -> Vec<Face> {
 }
 
 const MAX_FACES_PER_FILE: u32 = 32;
-/// Name records read per name id of a face, and characters read of each: real fonts have a few
-/// dozen short ones.
-const MAX_NAME_RECORDS: usize = 64;
-const MAX_NAME_CHARS: usize = 256;
 /// Bounds of the missing-glyph search: files loaded, bytes loaded, faces checked per character.
 const MAX_FALLBACK_FILES: usize = 24;
 const MAX_FALLBACK_BYTES: usize = 160 << 20;

crates/geom/src/region.rs

@@ -110,10 +110,6 @@ fn build(polys: &[Vec<Vec2>], tol: f64) -> Option<Graph> {
     };
     let mut grid: HashMap<(usize, usize), Vec<usize>> = HashMap::new();
     let mut long: Vec<usize> = Vec::new();
-    // Every cell a segment is registered in is an index kept: past this many in all (each segment
-    // may take up to 4096 cells), the input is refused as it is for too much pair work.
-    const MAX_REGISTRATIONS: usize = 20_000_000;
-    let mut registered = 0usize;
     for (i, (a, b)) in segs.iter().enumerate() {
         let (x0, y0) = cell(a.min(*b));
         let (x1, y1) = cell(a.max(*b));
@@ -122,10 +118,6 @@ fn build(polys: &[Vec<Vec2>], tol: f64) -> Option<Graph> {
             long.push(i);
             continue;
         }
-        registered = registered.saturating_add((x1 - x0 + 1) * (y1 - y0 + 1));
-        if registered > MAX_REGISTRATIONS {
-            return None;
-        }
         for gx in x0..=x1 {
             for gy in y0..=y1 {
                 grid.entry((gx, gy)).or_default().push(i);
@@ -399,10 +391,10 @@ fn outside(core: &Graph, u: usize, steps: usize) -> Option<Vec<Vec2>> {
 }
 
 /// Outlines of the groups of curves inside the face `outer` (vertex indices into `g`).
-fn islands(g: &Graph, outer: &[usize], p: Vec2) -> Option<Vec<Island>> {
+fn islands(g: &Graph, outer: &[usize], p: Vec2) -> Vec<Island> {
     let comp = components(g);
     let outer_comp: std::collections::HashSet<usize> = outer.iter().filter_map(|&i| comp.get(i).copied()).collect();
-    let outer_pts = points(g, outer)?;
+    let Some(outer_pts) = points(g, outer) else { return Vec::new() };
     let ob = Bounds2::from_points(outer_pts.iter().copied());
     let core = prune(g);
     // Leftmost (then lowest) vertex still on a cycle, per component.
@@ -432,32 +424,19 @@ fn islands(g: &Graph, outer: &[usize], p: Vec2) -> Option<Vec<Island>> {
     let mut starts: Vec<(usize, usize)> = start.into_iter().collect();
     starts.sort_unstable();
     let mut out = Vec::new();
-    // Each containment test scans a whole polygon, accepted or not: they share one allowance of
-    // polygon edges, and the search ends when it is spent.
-    let mut edges_left: usize = 100_000_000;
     for (c, u) in starts {
         if out.len() >= MAX_ISLANDS {
             break;
         }
         let Some(&pu) = core.pts.get(u) else { continue };
-        if !ob.contains(pu) {
-            continue;
-        }
-        let left = edges_left.checked_sub(outer_pts.len())?;
-        edges_left = left;
-        if !point_in_polygon(&outer_pts, pu) {
+        if !ob.contains(pu) || !point_in_polygon(&outer_pts, pu) {
             continue;
         }
         let steps = edges.get(&c).copied().unwrap_or(0).saturating_add(4);
         let Some(lp) = outside(&core, u, steps) else { continue };
         let area = shoelace(&lp);
         let ib = Bounds2::from_points(lp.iter().copied());
-        if lp.len() < 3 || !area.is_finite() || area <= 0.0 || !ob.contains_box(&ib) {
-            continue;
-        }
-        let left = edges_left.checked_sub(lp.len())?;
-        edges_left = left;
-        if point_in_polygon(&lp, p) {
+        if lp.len() < 3 || !area.is_finite() || area <= 0.0 || !ob.contains_box(&ib) || point_in_polygon(&lp, p) {
             continue;
         }
         let mut src = sources.remove(&c).unwrap_or_default();
@@ -465,7 +444,7 @@ fn islands(g: &Graph, outer: &[usize], p: Vec2) -> Option<Vec<Island>> {
         src.dedup();
         out.push(Island { outline: lp, sources: src });
     }
-    Some(out)
+    out
 }
 
 /// The point of the nearest other curve within `snap` of the end `e` of curve `i`, as (curve,
@@ -478,10 +457,6 @@ fn nearest_curve(polys: &[Vec<Vec2>], i: usize, e: Vec2, snap: f64, tol: f64, bu
         if j == i {
             continue;
         }
-        // A curve with no segment costs a visit too, so many of them can't multiply the searches.
-        if other.len() < 2 {
-            *budget = budget.checked_sub(1).ok_or(())?;
-        }
         for (k, w) in other.windows(2).enumerate() {
             if *budget == 0 {
                 return Err(());
@@ -583,7 +558,7 @@ pub fn boundary_at_gap(polys: &[Vec<Vec2>], p: Vec2, tol: f64, gap: f64) -> Opti
     let g = build_gap(polys, tol, gap)?;
     let ids = outer_face(&g, p)?;
     let outer = points(&g, &ids)?;
-    let islands = islands(&g, &ids, p)?;
+    let islands = islands(&g, &ids, p);
     let mut outer_sources: Vec<usize> =
         ids.iter().zip(ids.iter().cycle().skip(1)).filter_map(|(&u, &v)| g.edge_src.get(&(u.min(v), u.max(v))).copied()).collect();
     outer_sources.sort_unstable();