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

crates/engine/src/cmd/solver/reports.rs

@@ -52,19 +52,14 @@ pub(super) fn abs_ref(c: CellRef) -> String {
     format!("${}${}", gridcraft_core::col_to_letters(c.col), u64::from(c.row) + 1)
 }
 
-/// How many cells to the left and above a cell its name is searched for.
-const NAME_REACH: u32 = 1_000;
-
 /// A cell's name in the reports: the nearest text to its left and the nearest text above it.
 pub(super) fn cell_name(sh: &Sheet, c: CellRef) -> String {
     let text = |c: CellRef| match sh.value(c) {
         Value::Text(t) if !t.trim().is_empty() => Some(t.to_string()),
         _ => None,
     };
-    // Labels are next to their cells: looking further than this on a sparse sheet only costs time
-    // (a thousand cells near the last row would each probe a million empty coordinates).
-    let left = (c.col.saturating_sub(NAME_REACH)..c.col).rev().find_map(|col| text(CellRef::new(c.row, col)));
-    let top = (c.row.saturating_sub(NAME_REACH)..c.row).rev().find_map(|row| text(CellRef::new(row, c.col)));
+    let left = (0..c.col).rev().find_map(|col| text(CellRef::new(c.row, col)));
+    let top = (0..c.row).rev().find_map(|row| text(CellRef::new(row, c.col)));
     [left, top].into_iter().flatten().collect::<Vec<_>>().join(" ")
 }
 
@@ -443,17 +438,3 @@ pub(super) fn report(s: &mut Session, p: &Json) -> Result<Json> {
     }
     Ok(out)
 }
-
-#[cfg(test)]
-mod tests {
-    use super::*;
-
-    #[test]
-    fn a_name_is_searched_only_a_bounded_distance() {
-        let mut sh = Sheet::new("S".to_string());
-        sh.set_cell(CellRef::new(0, 0), gridcraft_model::Cell::value(Value::text("Far")));
-        sh.set_cell(CellRef::new(5, 4), gridcraft_model::Cell::value(Value::text("Near")));
-        assert_eq!(cell_name(&sh, CellRef::new(5, 5)), "Near");
-        assert_eq!(cell_name(&sh, CellRef::new(0, NAME_REACH + 1)), "");
-    }
-}

crates/engine/src/cmd/whatif/scenarios.rs

@@ -189,9 +189,6 @@ pub(super) fn scenario_delete(s: &mut Session, p: &Json) -> Result<Json> {
     Ok(json!({"deleted": removed.name}))
 }
 
-/// Most changing-cell-by-scenario cells one Scenario Summary writes.
-const MAX_SUMMARY_CELLS: usize = 1_000_000;
-
 pub(super) fn scenario_summary(s: &mut Session, p: &Json) -> Result<Json> {
     const CMD: &str = "data.scenarioSummary";
     let d = s.doc()?;
@@ -214,24 +211,17 @@ pub(super) fn scenario_summary(s: &mut Session, p: &Json) -> Result<Json> {
     // Changing cells of every scenario, first-seen order; per scenario its values.
     let mut changing: Vec<CellRef> = Vec::new();
     let mut per: Vec<HashMap<CellRef, Value>> = Vec::new();
-    let mut seen: std::collections::HashSet<CellRef> = Default::default();
     for sc in &scenarios {
         let (_, cells) = cell_list(&d.wb, &sc.changing, CMD, MAX_SCENARIO_CELLS)?;
         let mut m = HashMap::new();
         for (c, v) in cells.iter().zip(sc.values.iter()) {
-            if seen.insert(*c) {
+            if !changing.contains(c) {
                 changing.push(*c);
             }
             m.insert(*c, json_value(v, &d.wb));
         }
         per.push(m);
     }
-    // The summary has a cell for every changing cell in every scenario, filled from the current
-    // value where a scenario doesn't set it, so small disjoint scenarios multiply.
-    // Each result cell is a row of the summary too, with a cell per scenario.
-    if changing.len().saturating_add(results.len()).saturating_mul(per.len()) > MAX_SUMMARY_CELLS {
-        return Err(bad(CMD, "the scenarios together change too many cells for a summary"));
-    }
     let current: Vec<Value> = changing.iter().map(|c| sh.value(*c)).collect();
     let current_results: Vec<Value> = results.iter().map(|c| sh.value(*c)).collect();
     let mut trial = Trial::new(d);

crates/engine/src/cmd/whatif/tests.rs

@@ -297,25 +297,3 @@ fn consolidate_by_position_and_label() {
     assert!(s.execute("data.consolidate", json!({"sources": []})).is_err());
     assert!(s.execute("data.consolidate", json!({"sources": ["A1"], "function": "median"})).is_err());
 }
-
-#[test]
-fn scenario_summary_refuses_a_matrix_over_its_cell_limit() {
-    let mut s = s();
-    // 200 scenarios of 32 disjoint cells: 6,400 changing cells x 200 scenarios = 1.28 million cells.
-    for i in 0..200 {
-        let (a, b) = (i * 32 + 1, i * 32 + 32);
-        s.execute("data.scenarioAdd", json!({"name": format!("S{i}"), "changing": format!("A{a}:A{b}")})).unwrap();
-    }
-    assert!(s.execute("data.scenarioSummary", json!({})).is_err());
-}
-
-#[test]
-fn scenario_summary_counts_result_rows_in_its_cell_limit() {
-    let mut s = s();
-    // 900 scenarios of one distinct cell each are 810,000 cells; 255 result rows take it past the limit.
-    for i in 0..900 {
-        s.execute("data.scenarioAdd", json!({"name": format!("S{i}"), "changing": format!("A{}", i + 1)})).unwrap();
-    }
-    let err = s.execute("data.scenarioSummary", json!({"resultCells": "C1:C255"})).unwrap_err().to_string();
-    assert!(err.contains("too many cells"), "{err}");
-}

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);
+}

crates/engine/src/lib.rs

@@ -237,22 +237,6 @@ const JOURNAL_MAX_ENTRIES: usize = 10_000;
 pub(crate) const JOURNAL_MAX_BYTES: usize = 64 * 1024 * 1024;
 const UNDO_BYTES_LIMIT: usize = 128 * 1024 * 1024;
 
-/// Comment and hyperlink text of a sheet: a changed sheet's snapshot keeps its own copy of these
-/// strings (an edit deep-copies them), so they count against the history budget with the cells.
-fn sheet_text_bytes(sheet: &gridcraft_model::Sheet) -> usize {
-    let comments = sheet.comments.values().fold(0usize, |sum, c| {
-        // The vectors' own storage counts too: an empty reply still takes an element.
-        let replies = c.replies.iter().fold(0usize, |n, (a, t)| n.saturating_add(a.len()).saturating_add(t.len()));
-        let replies = replies.saturating_add(c.replies.capacity().saturating_mul(std::mem::size_of::<(String, String)>()));
-        let dates = c.dates.iter().fold(0usize, |n, d| n.saturating_add(d.len()));
-        let dates = dates.saturating_add(c.dates.capacity().saturating_mul(std::mem::size_of::<String>()));
-        sum.saturating_add(std::mem::size_of::<gridcraft_model::Comment>() + c.author.len() + c.text.len() + replies + dates)
-    });
-    sheet.hyperlinks.values().fold(comments, |sum, h| {
-        sum.saturating_add(std::mem::size_of::<gridcraft_model::Hyperlink>() + h.target.len() + h.tooltip.as_ref().map_or(0, String::len))
-    })
-}
-
 fn history_retained_bytes(older: &Workbook, newer: &Workbook) -> usize {
     let mut bytes = std::mem::size_of::<Workbook>();
     for (index, sheet) in older.sheets.iter().enumerate() {
@@ -264,7 +248,6 @@ fn history_retained_bytes(older: &Workbook, newer: &Workbook) -> usize {
         let empty = gridcraft_model::CellStore::new();
         let next = newer.sheets.get(index).map_or(&empty, |next| &next.cells);
         bytes = bytes.saturating_add(sheet.cells.retained_bytes_against(next));
-        bytes = bytes.saturating_add(sheet_text_bytes(sheet));
     }
     // Snapshots share the macro list until a change to it; a snapshot with its own list keeps it.
     if !older.macros.shares(&newer.macros) {

crates/engine/src/tests.rs

@@ -5572,26 +5572,3 @@ fn clearing_links_and_notes_keeps_allow_edit_ranges() {
     let sh = s.doc().unwrap().wb.active().unwrap().clone();
     assert!(sh.hyperlinks.is_empty() && sh.comments.is_empty());
 }
-
-#[test]
-fn history_charges_a_changed_sheets_comment_text() {
-    let mut s = s();
-    s.execute("review.newNote", json!({"cell": "A1", "text": "n".repeat(30_000), "author": "Example"})).unwrap();
-    let older = (*s.doc().unwrap().wb).clone();
-    s.execute("cell.set", json!({"cell": "B2", "input": "x"})).unwrap();
-    let newer = (*s.doc().unwrap().wb).clone();
-    // The older snapshot owns the comment text now that the sheet changed under it.
-    assert!(crate::history_retained_bytes(&older, &newer) >= 30_000);
-}
-
-#[test]
-fn history_charges_the_storage_of_empty_comment_replies() {
-    let mut s = s();
-    s.execute("review.newComment", json!({"cell": "A1", "text": "c", "author": "Ann"})).unwrap();
-    for _ in 0..500 {
-        s.execute("review.replyComment", json!({"cell": "A1", "text": "", "author": ""})).unwrap();
-    }
-    let sheet = s.doc().unwrap().wb.sheet(0).unwrap().clone();
-    // 500 replies of no text are still 500 elements.
-    assert!(crate::sheet_text_bytes(&sheet) >= 500 * std::mem::size_of::<(String, String)>());
-}