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

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/pivot/charts.rs

@@ -57,10 +57,6 @@ fn rows_ref(sheet: &str, origin: CellRef, col: u32, rows: &[u32]) -> Option<Stri
 /// Most category rows a PivotChart plots.
 const MAX_CHART_ROWS: usize = 10_000;
 
-/// Most series a PivotChart gets: the chart renderer reads at most 255 series, and each one owns
-/// reference text as long as its category rows, copied into every linked chart.
-const MAX_CHART_SERIES: usize = 255;
-
 /// A PivotChart's series for a report written at `origin`: categories are the innermost row
 /// items, one series per item column (column items × value fields); totals are left out.
 pub fn chart_series(sheet: &str, origin: CellRef, out: &Output) -> Vec<Series> {
@@ -68,7 +64,6 @@ pub fn chart_series(sheet: &str, origin: CellRef, out: &Output) -> Vec<Series> {
     let categories = out.label_col.and_then(|c| rows_ref(sheet, origin, c, &rows));
     out.chart_cols
         .iter()
-        .take(MAX_CHART_SERIES)
         .filter_map(|(c, name)| {
             Some(Series {
                 name: Some(format!("\"{}\"", name.replace('"', "\"\""))),
@@ -111,22 +106,3 @@ pub fn clear_output(wb: &mut Workbook, sheet: usize, idx: usize) {
         p.last_range = None;
     }
 }
-
-#[cfg(test)]
-mod tests {
-    use super::*;
-
-    #[test]
-    fn a_pivot_chart_gets_at_most_the_series_the_renderer_reads() {
-        let out = Output {
-            grid: vec![],
-            row_kinds: vec![],
-            body_offset: 0,
-            width: 1000,
-            chart_rows: vec![1, 2, 3],
-            chart_cols: (1..1000).map(|c| (c, format!("S{c}"))).collect(),
-            label_col: Some(0),
-        };
-        assert_eq!(chart_series("Sheet1", CellRef::new(0, 0), &out).len(), MAX_CHART_SERIES);
-    }
-}

crates/engine/src/pivot/compute.rs

@@ -1,7 +1,5 @@
 use super::items::{Acc, FieldItems, build_tree, collect_siblings, is_collapsed, item_label};
-use super::output::{
-    AggMap, CellKind, ColKind, ColSpec, Ctx, Grid, Line, LineKind, Output, RowKind, ValSrc, caption, data_cols_exceed_sheet, flatten_cols,
-};
+use super::output::{AggMap, CellKind, ColKind, ColSpec, Ctx, Grid, Line, LineKind, Output, RowKind, ValSrc, data_cols_exceed_sheet, flatten_cols};
 use super::{
     CalcSet, CellError, CellRef, HashMap, HashSet, MAX_AXIS_FIELDS, MAX_COLS, MAX_VALUE_FIELDS, PivotDateGroup, PivotField, PivotFilter, PivotFunc,
     PivotLayout, PivotShowAs, PivotTable, PivotValue, Source, Value, Workbook, read_source,
@@ -94,10 +92,6 @@ struct AggregateData {
     col_leaves: HashSet<Vec<u32>>,
 }
 
-/// Most accumulators (groups times value columns) one report may hold: twice what the 5 million
-/// group limit allowed at one value field.
-const MAX_ACCUMULATORS: usize = 16_000_000;
-
 fn aggregate_record(
     input: &AggregateInput<'_>,
     data: &mut AggregateData,
@@ -156,9 +150,7 @@ fn aggregate_record(
     if !input.cols.is_empty() && data_cols_exceed_sheet(data.col_leaves.len(), input.pt.grand_totals_rows, input.vals.len()) {
         return Err("The PivotTable report will not fit on the sheet.".into());
     }
-    // Each entry holds one accumulator per value field and calculated-field source column, so the
-    // width counts too: a handful of groups over hundreds of fields is large as well.
-    if data.aggs.len() > 5_000_000 || data.aggs.len().saturating_mul(input.vals.len() + input.extra.len()) > MAX_ACCUMULATORS {
+    if data.aggs.len() > 5_000_000 {
         return Err("The PivotTable report is too large.".into());
     }
     Ok(())
@@ -265,8 +257,8 @@ fn write_headers(g: &mut Grid, input: HeaderInput<'_>) -> Result<(), String> {
                     let lv = spec.path.len().saturating_sub(1);
                     let label = spec.path.last().and_then(|it| col_items.get(lv).map(|fi| fi.label(*it))).unwrap_or_default();
                     let text = match vi {
-                        Some(vi) if k > 1 => caption(&label, &val_name(*vi)),
-                        _ => caption(&label, "Total"),
+                        Some(vi) if k > 1 => format!("{label} {}", val_name(*vi)),
+                        _ => format!("{label} Total"),
                     };
                     g.set(first_level + lv, c, Value::text(text), CellKind::ColLabel, 0, None);
                 }
@@ -324,7 +316,6 @@ struct ShowContext<'a> {
     value_index: usize,
     level0: &'a [u32],
     siblings: &'a HashMap<Vec<u32>, Vec<u32>>,
-    cache: &'a ShowCache,
 }
 
 fn number(value: &Option<Value>) -> Option<f64> {
@@ -341,67 +332,42 @@ fn ratio(a: Value, b: Option<Value>) -> Value {
     }
 }
 
-/// What Running Total and Rank need from other cells, computed once per sibling set instead of
-/// once per displayed value (which rescanned the whole set every time).
-type ShowKey = (Vec<u32>, Vec<u32>, usize);
-/// Cumulative values by top-level item, and the total of all of them.
-type RunningTotals = (HashMap<u32, f64>, f64);
-
-#[derive(Default)]
-struct ShowCache {
-    /// Ranked values' sibling numbers, sorted ascending without repeats, by (parent, column, value).
-    ranks: std::cell::RefCell<HashMap<ShowKey, Vec<f64>>>,
-    /// Running totals by top-level item, with the total of all of them, by (rest of path, column, value).
-    running: std::cell::RefCell<HashMap<ShowKey, RunningTotals>>,
-}
-
 fn running_total(raw: Value, lookup: &ValueLookup<'_>, context: &ShowContext<'_>) -> Value {
-    let Some((first, rest)) = context.row_path.split_first() else { return raw };
+    let Some((first, _)) = context.row_path.split_first() else { return raw };
     if !matches!(raw, Value::Number(_)) {
         return raw;
     }
-    let mut cache = context.cache.running.borrow_mut();
-    let (by_item, all) = cache.entry((rest.to_vec(), context.col_path.to_vec(), context.value_index)).or_insert_with(|| {
-        let mut total = 0.0;
-        let mut by_item = HashMap::new();
-        for item in context.level0 {
-            let mut path = Vec::with_capacity(rest.len() + 1);
-            path.push(*item);
-            path.extend_from_slice(rest);
-            // An item with no number here adds nothing and is not stored (the shown cell always has one).
-            let Some(value) = number(&lookup.get(&path, context.col_path, context.value_index)) else { continue };
-            total += value;
-            by_item.entry(*item).or_insert(total);
+    let mut total = 0.0;
+    for item in context.level0 {
+        let mut path = context.row_path.to_vec();
+        if let Some(first_item) = path.first_mut() {
+            *first_item = *item;
         }
-        (by_item, total)
-    });
-    Value::number(by_item.get(first).copied().unwrap_or(*all))
+        total += number(&lookup.get(&path, context.col_path, context.value_index)).unwrap_or(0.0);
+        if item == first {
+            break;
+        }
+    }
+    Value::number(total)
 }
 
 fn ranked_value(raw: Value, lookup: &ValueLookup<'_>, context: &ShowContext<'_>) -> Value {
     let Some((_, parent)) = context.row_path.split_last() else { return Value::Empty };
     let Some(me) = raw.as_f64() else { return raw };
-    let mut cache = context.cache.ranks.borrow_mut();
-    let sorted = cache.entry((parent.to_vec(), context.col_path.to_vec(), context.value_index)).or_insert_with(|| {
-        let mut all: Vec<f64> = context
-            .siblings
-            .get(parent)
-            .into_iter()
-            .flatten()
-            .filter_map(|sibling| {
-                let mut path = parent.to_vec();
-                path.push(*sibling);
-                number(&lookup.get(&path, context.col_path, context.value_index))
-            })
-            .collect();
-        all.sort_by(f64::total_cmp);
-        all.dedup();
-        all
-    });
+    let siblings = context.siblings.get(parent).cloned().unwrap_or_default();
     // Excel's ranks are dense: ties share a rank and the next value takes the next one
     // (5, 4, 3, 3, 4 rank 1, 2, 3, 3, 2), so count the distinct larger values.
-    let larger = sorted.len() - sorted.partition_point(|value| *value <= me);
-    Value::number(larger as f64 + 1.0)
+    let mut larger: Vec<f64> = siblings
+        .into_iter()
+        .filter_map(|sibling| {
+            let mut path = parent.to_vec();
+            path.push(sibling);
+            number(&lookup.get(&path, context.col_path, context.value_index)).filter(|value| *value > me)
+        })
+        .collect();
+    larger.sort_by(f64::total_cmp);
+    larger.dedup();
+    Value::number(larger.len() as f64 + 1.0)
 }
 
 fn show_value(definition: &PivotValue, raw: Value, lookup: &ValueLookup<'_>, context: &ShowContext<'_>) -> Value {
@@ -671,7 +637,6 @@ pub(super) fn compute_from(wb: &Workbook, pt: &PivotTable, src: &Source, mut pro
         })
         .collect();
     let lookup = ValueLookup { wb, source: src, calc: &calc, vals: &vals, extra: &extra, aggs: &aggs, value_count: k };
-    let show_cache = ShowCache::default();
     let mut chart_rows: Vec<u32> = Vec::new();
     let chart_cols: Vec<(u32, String)> = data_cols
         .iter()
@@ -729,7 +694,7 @@ pub(super) fn compute_from(wb: &Workbook, pt: &PivotTable, src: &Source, mut pro
             let Some((_, vdef)) = vals.get(*vi) else { continue };
             let cp = spec.path.as_slice();
             let Some(raw) = lookup.get(rp, cp, *vi) else { continue };
-            let context = ShowContext { row_path: rp, col_path: cp, value_index: *vi, level0: &level0, siblings: &siblings, cache: &show_cache };
+            let context = ShowContext { row_path: rp, col_path: cp, value_index: *vi, level0: &level0, siblings: &siblings };
             let shown = show_value(vdef, raw, &lookup, &context);
             if let Some(pr) = probe.as_deref_mut() {
                 if pr.at == Some((r, l + j)) {

crates/engine/src/pivot/output.rs

@@ -121,18 +121,6 @@ pub(super) enum ColKind {
     Grand,
 }
 
-/// Longest item caption Excel keeps; a subtotal caption repeats its parent's label for every value
-/// field at every level, so a long source label is cut rather than multiplied.
-const MAX_CAPTION_LABEL_CHARS: usize = 255;
-
-/// `label` (cut to [`MAX_CAPTION_LABEL_CHARS`]) and `suffix`, as in "East Total".
-pub(super) fn caption(label: &str, suffix: &str) -> String {
-    let mut out: String = label.chars().take(MAX_CAPTION_LABEL_CHARS).collect();
-    out.push(' ');
-    out.push_str(suffix);
-    out
-}
-
 pub(super) struct ColSpec {
     pub(super) kind: ColKind,
     pub(super) path: Vec<u32>,
@@ -174,7 +162,7 @@ impl Ctx<'_> {
                 leaf: false,
                 path: prefix.to_vec(),
                 values: true,
-                labels: vec![(col, Value::text(caption(label, name)), indent, None)],
+                labels: vec![(col, Value::text(format!("{label} {name}")), indent, None)],
                 vi: Some(vi),
             });
         }
@@ -261,7 +249,7 @@ impl Ctx<'_> {
                         leaf: false,
                         path: prefix.clone(),
                         values: true,
-                        labels: vec![(col, Value::text(caption(&label, "Total")), indent, None)],
+                        labels: vec![(col, Value::text(format!("{label} Total")), indent, None)],
                         vi,
                     });
                 }
@@ -328,7 +316,7 @@ impl Ctx<'_> {
                         leaf: false,
                         path: prefix.clone(),
                         values: true,
-                        labels: vec![(col, Value::text(caption(&label, "Total")), 0, None)],
+                        labels: vec![(col, Value::text(format!("{label} Total")), 0, None)],
                         vi,
                     });
                 }
@@ -450,17 +438,3 @@ pub(super) fn cell_style(lk: &Look, rk: RowKind, cell: &OutCell, last_header: bo
     }
     s
 }
-
-#[cfg(test)]
-mod tests {
-    use super::*;
-
-    #[test]
-    fn captions_cut_a_long_parent_label() {
-        assert_eq!(caption("East", "Total"), "East Total");
-        let long = "é".repeat(100_000);
-        let c = caption(&long, "Sum of Sales");
-        assert_eq!(c.chars().count(), MAX_CAPTION_LABEL_CHARS + " Sum of Sales".len());
-        assert!(c.ends_with(" Sum of Sales"));
-    }
-}