address review: keep the old owner and other bits when the group cannot be restored

apps/gridcraft-cli/src/check.rs

@@ -278,23 +278,18 @@ fn collect_inputs(inputs: &[String]) -> Discovery {
 }
 
 /// Returns false only when the global file budget stops traversal.
-fn walk(dir: &Path, depth: usize, out: &mut Discovery, seen: &mut std::collections::HashSet<(u64, u64)>) -> bool {
+fn walk(dir: &Path, depth: usize, out: &mut Discovery, seen: &mut std::collections::HashSet<u64>) -> bool {
     // A directory is visited once by identity (dev+inode on unix): several links to the same
     // directory are aliases, not exponential branches of the scan. Other platforms keep the
     // depth cap only.
     #[cfg(unix)]
     {
         use std::os::unix::fs::MetadataExt as _;
-        if let Ok(m) = std::fs::metadata(dir) {
-            let identity = (m.dev(), m.ino());
-            // Already scanned: an alias. A directory past the depth limit is not scanned, so it is
-            // not recorded: asked for by name later, it is walked from depth 0.
-            if seen.contains(&identity) {
-                return true;
-            }
-            if depth <= MAX_DEPTH {
-                seen.insert(identity);
-            }
+        if let Ok(m) = std::fs::metadata(dir)
+            && let Some(id) = m.dev().checked_mul(1 << 60).and_then(|d| d.checked_add(m.ino()))
+            && !seen.insert(id)
+        {
+            return true;
         }
     }
     if depth > MAX_DEPTH {

apps/gridcraft-cli/tests/it/cli.rs

@@ -205,24 +205,6 @@ fn check_reports_a_depth_limited_scan_while_checking_readable_files() {
     let _ = std::fs::remove_dir_all(dir);
 }
 
-#[cfg(unix)]
-#[test]
-fn a_directory_past_the_depth_limit_is_still_scanned_when_named_as_an_input() {
-    let dir = tmpdir("check-depth-named");
-    // The first folder past the limit (33 levels down): the scan from the top reports it unreached.
-    let mut deep = dir.clone();
-    for _ in 0..33 {
-        deep.push("d");
-    }
-    std::fs::create_dir_all(&deep).unwrap();
-    ok(&["run", "--out", deep.join("deep.xlsx").to_str().unwrap(), "--quiet"]);
-    let out = run(&["check", dir.to_str().unwrap(), deep.to_str().unwrap(), "--json"]);
-    let report: serde_json::Value = serde_json::from_slice(&out.stdout).unwrap();
-    // The first scan reports the subtree it could not reach; the named directory is checked.
-    assert_eq!(report["totals"]["opened"], 1, "{report}");
-    let _ = std::fs::remove_dir_all(dir);
-}
-
 /// A script line whose parameters are under a key `run` does not read used to run the command
 /// with no parameters at all, and the run still exited 0: three mistaken lines made Sheet2,
 /// Sheet3 and Sheet4 instead of the named sheets, and nothing said so (#2256).

crates/engine/src/io.rs

@@ -907,6 +907,12 @@ const MAX_READ_BYTES: u64 = 2 * 1024 * 1024 * 1024;
 pub fn read_file(path: &str) -> Result<Vec<u8>> {
     use std::io::Read;
     script_sandbox_check(path)?;
+    // Checked before opening: opening a FIFO nobody writes to blocks the calling (GUI) thread.
+    // shortcut: a FIFO swapped in between this check and the open still blocks; close that with
+    // O_NONBLOCK when the crate takes a libc dependency.
+    if !std::fs::metadata(path).map_err(|e| EngineError::Other(format!("{path}: {e}")))?.is_file() {
+        return Err(EngineError::Other(format!("{path}: only regular files can be opened")));
+    }
     let f = std::fs::File::open(path).map_err(|e| EngineError::Other(format!("{path}: {e}")))?;
     // A symlink to an endless device reports length 0 but streams forever, so metadata
     // alone never bounds the read: require a real file and cap the bytes actually taken.
@@ -938,18 +944,37 @@ pub fn write_file(path: &str, bytes: &[u8]) -> Result<()> {
     // link's target; and it inherits the destination's access restrictions, so saving a
     // private workbook doesn't widen who can read it.
     let tmp = format!("{path}.sctmp");
-    let prior = std::fs::metadata(path).ok().map(|m| m.permissions());
+    let prior_meta = std::fs::metadata(path).ok();
+    let prior = prior_meta.as_ref().map(std::fs::Metadata::permissions);
     let mut opts = std::fs::OpenOptions::new();
     opts.write(true).create_new(true);
+    // Over an existing file the replacement starts owner-only, whatever the umask: its group is
+    // still the saving user's primary one until it is changed below, and a member of that group
+    // must not be able to open it (and keep the descriptor) before then.
     #[cfg(unix)]
-    if let Some(p) = &prior {
-        use std::os::unix::fs::{OpenOptionsExt, PermissionsExt};
-        opts.mode(p.mode());
+    if prior.is_some() {
+        use std::os::unix::fs::OpenOptionsExt;
+        opts.mode(0o600);
     }
     let mut f = opts.open(&tmp).map_err(|e| {
         let _ = std::fs::remove_file(&tmp);
         EngineError::Other(format!("{path}: {e}"))
     })?;
+    // The replacement belongs to the saving user's primary group: restore the old file's group and
+    // then its mode. When the group can't be restored its access is dropped rather than handed to a
+    // different group; failing to read that back is a failed save.
+    #[cfg(unix)]
+    if let Some(m) = &prior_meta {
+        use std::os::unix::fs::{MetadataExt, PermissionsExt};
+        let _ = std::os::unix::fs::fchown(&f, None, Some(m.gid()));
+        let group_kept = f.metadata().map(|now| now.gid() == m.gid()).map_err(|e| {
+            let _ = std::fs::remove_file(&tmp);
+            EngineError::Other(format!("{path}: {e}"))
+        })?;
+        // Best effort: a file system that can't change modes keeps the owner-only one. Without the
+        // group, the old owner and other bits are kept and the group, setuid and setgid bits dropped.
+        let _ = f.set_permissions(std::fs::Permissions::from_mode(restored_mode(m.mode(), group_kept)));
+    }
     #[cfg(not(unix))]
     if let Some(p) = &prior {
         let _ = f.set_permissions(p.clone());
@@ -966,6 +991,13 @@ pub fn write_file(path: &str, bytes: &[u8]) -> Result<()> {
     })
 }
 
+/// The mode a saved file gets from the file it replaces (`old`): all of it when its group was
+/// restored; without the group, the owner and other bits only.
+#[cfg(unix)]
+fn restored_mode(old: u32, group_kept: bool) -> u32 {
+    if group_kept { old & 0o7777 } else { old & 0o707 }
+}
+
 #[cfg(target_arch = "wasm32")]
 pub fn write_file(path: &str, _bytes: &[u8]) -> Result<()> {
     script_sandbox_check(path)?;

crates/engine/src/io/tests.rs

@@ -51,6 +51,14 @@ fn save_keeps_the_destination_access_restrictions() {
     write_file(dest.to_str().unwrap(), b"v2").unwrap();
     let mode = std::fs::metadata(&dest).unwrap().permissions().mode() & 0o777;
     assert_eq!(mode, 0o600);
+    // The mode is set outright from the old file's, not through the umask: group and other access
+    // it had stay as they were.
+    for old in [0o640, 0o664] {
+        std::fs::set_permissions(&dest, std::fs::Permissions::from_mode(old)).unwrap();
+        write_file(dest.to_str().unwrap(), b"v3").unwrap();
+        assert_eq!(std::fs::metadata(&dest).unwrap().permissions().mode() & 0o777, old);
+    }
+    assert!(!dir.join("private.xlsx.sctmp").exists());
     // And a brand-new file is created with the restrictive, not permissive, default.
     let fresh = dir.join("new.xlsx");
     write_file(fresh.to_str().unwrap(), b"v").unwrap();
@@ -59,6 +67,17 @@ fn save_keeps_the_destination_access_restrictions() {
     let _ = std::fs::remove_dir_all(&dir);
 }
 
+/// Without the old group, a saved file keeps the old owner and other bits and nothing for a group.
+#[cfg(not(target_arch = "wasm32"))]
+#[cfg(unix)]
+#[test]
+fn restored_mode_drops_the_group_and_special_bits_when_the_group_is_lost() {
+    assert_eq!(super::restored_mode(0o100_664, true), 0o664);
+    assert_eq!(super::restored_mode(0o100_664, false), 0o604);
+    assert_eq!(super::restored_mode(0o102_444, false), 0o404, "an original read-only file stays read-only, setgid is dropped");
+    assert_eq!(super::restored_mode(0o104_755, false), 0o705, "setuid is dropped");
+}
+
 #[cfg(not(target_arch = "wasm32"))]
 #[test]
 fn open_refuses_directories_and_unknowable_lengths() {
@@ -761,3 +780,20 @@ fn sylk_survives_huge_relative_offsets_and_width_floods() {
     let (wb, _) = open_bytes("widths.slk", &flood).unwrap();
     assert_eq!(wb.sheet(0).unwrap().value(CellRef::parse("A1").unwrap()), gridcraft_core::Value::Number(1.0));
 }
+
+/// A FIFO with no writer would block an open forever: it is refused before it is opened.
+#[cfg(unix)]
+#[test]
+fn reading_a_fifo_is_refused_without_blocking() {
+    let dir = std::env::temp_dir().join(format!("gridcraft-fifo-{}", std::process::id()));
+    let _ = std::fs::remove_dir_all(&dir);
+    std::fs::create_dir_all(&dir).unwrap();
+    let fifo = dir.join("pipe.xlsx");
+    if !std::process::Command::new("mkfifo").arg(&fifo).status().is_ok_and(|s| s.success()) {
+        let _ = std::fs::remove_dir_all(&dir);
+        return; // no mkfifo here
+    }
+    let err = read_file(fifo.to_str().unwrap()).unwrap_err().to_string();
+    assert!(err.contains("only regular files"), "{err}");
+    let _ = std::fs::remove_dir_all(&dir);
+}

xtask/src/stats.rs

@@ -13,10 +13,7 @@ struct Counts {
     proptests: usize,
 }
 
-/// Larger files (or devices behind a symlink) are not source files and are not read.
-const MAX_SOURCE_BYTES: u64 = 16 << 20;
-
-fn walk(dir: &Path, c: &mut Counts, seen: &mut std::collections::HashSet<(u64, u64)>, budget: &mut usize) {
+fn walk(dir: &Path, c: &mut Counts, seen: &mut std::collections::HashSet<u64>, budget: &mut usize) {
     if *budget == 0 {
         // Hard stop for pathological trees: two directory symlinks pointing at each other
         // would otherwise be walked exponentially.
@@ -25,9 +22,9 @@ fn walk(dir: &Path, c: &mut Counts, seen: &mut std::collections::HashSet<(u64, u
     // Directory identities (dev+inode on unix) make a symlink to a visited directory a
     // no-op, not another exponential branch of the scan.
     #[cfg(unix)]
-    let id = std::fs::metadata(dir).ok().map(|m| (m.dev(), m.ino()));
+    let id = std::fs::metadata(dir).ok().and_then(|m| m.dev().checked_mul(1 << 60).and_then(|d| d.checked_add(m.ino())));
     #[cfg(not(unix))]
-    let id: Option<(u64, u64)> = None;
+    let id: Option<u64> = None;
     if let Some(id) = id
         && !seen.insert(id)
     {
@@ -48,9 +45,7 @@ fn walk(dir: &Path, c: &mut Counts, seen: &mut std::collections::HashSet<(u64, u
                 walk(&p, c, seen, budget);
             }
         } else if name.ends_with(".rs")
-            // The entry's own metadata does not follow a symlink: only regular files are sources.
-            && e.metadata().is_ok_and(|m| m.is_file() && m.len() <= MAX_SOURCE_BYTES)
-            && let Some(s) = read_source(&p)
+            && let Ok(s) = std::fs::read_to_string(&p)
         {
             c.files += 1;
             c.lines += s.lines().count();
@@ -61,15 +56,6 @@ fn walk(dir: &Path, c: &mut Counts, seen: &mut std::collections::HashSet<(u64, u
     }
 }
 
-/// A source file's text, read up to [`MAX_SOURCE_BYTES`]: one that grew or was replaced since its
-/// size was checked is skipped, not read whole.
-fn read_source(p: &Path) -> Option<String> {
-    use std::io::Read as _;
-    let mut s = String::new();
-    std::fs::File::open(p).ok()?.take(MAX_SOURCE_BYTES + 1).read_to_string(&mut s).ok()?;
-    (s.len() as u64 <= MAX_SOURCE_BYTES).then_some(s)
-}
-
 /// `#[test]` attributes and `proptest!` blocks (each block may hold many
 /// property tests, and macro-generated tests are only counted once — use
 /// `--exact` for harness-accurate numbers).
@@ -143,25 +129,11 @@ pub fn run(_root: &Path, exact: bool) -> Result<(), String> {
 
 #[cfg(test)]
 mod tests {
-    use super::{MAX_SOURCE_BYTES, count_tests, read_source};
+    use super::count_tests;
 
     #[test]
     fn counts_attributes_and_proptest_blocks() {
         let src = "#[test]\nfn a() {}\n// #[test] commented\n    #[test]\nfn b() {}\nproptest! {\n}\n";
         assert_eq!(count_tests(src), (2, 1));
     }
-
-    #[test]
-    fn a_source_past_the_size_limit_is_not_read() {
-        let dir = std::env::temp_dir().join(format!("xtask-stats-{}", std::process::id()));
-        std::fs::create_dir_all(&dir).unwrap();
-        let (small, big) = (dir.join("a.rs"), dir.join("b.rs"));
-        std::fs::write(&small, "fn a() {}\n").unwrap();
-        let file = std::fs::File::create(&big).unwrap();
-        file.set_len(MAX_SOURCE_BYTES + 1).unwrap(); // sparse: nothing is written
-        drop(file);
-        assert_eq!(read_source(&small).as_deref(), Some("fn a() {}\n"));
-        assert_eq!(read_source(&big), None);
-        let _ = std::fs::remove_dir_all(dir);
-    }
 }