address review: do not truncate REGEXREPLACE occurrence on 32-bit targets

crates/engine/src/io.rs

@@ -907,12 +907,6 @@ 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.
@@ -944,37 +938,18 @@ 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_meta = std::fs::metadata(path).ok();
-    let prior = prior_meta.as_ref().map(std::fs::Metadata::permissions);
+    let prior = std::fs::metadata(path).ok().map(|m| m.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 prior.is_some() {
-        use std::os::unix::fs::OpenOptionsExt;
-        opts.mode(0o600);
+    if let Some(p) = &prior {
+        use std::os::unix::fs::{OpenOptionsExt, PermissionsExt};
+        opts.mode(p.mode());
     }
     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());
@@ -991,13 +966,6 @@ 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,14 +51,6 @@ 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();
@@ -67,17 +59,6 @@ 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() {
@@ -780,20 +761,3 @@ 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/functions/src/lib.rs

@@ -308,6 +308,12 @@ mod tests {
         assert_eq!(ev("REGEXREPLACE", vec![t("2026-10-22"), t("(\\d+)-(\\d+)-(\\d+)"), t("$3/$2/$1")]), Value::text("22/10/2026"));
         assert_eq!(ev("REGEXREPLACE", vec![t("abc"), t("x*"), t("-")]), Value::text("-a-b-c-"));
         assert_eq!(ev("REGEXREPLACE", vec![t("a1b2"), t("[0-9]"), t("#"), n(2.0)]), Value::text("a1b#"));
+        assert_eq!(ev("REGEXREPLACE", vec![t("a1b2"), t("[0-9]"), t("#"), n(-2.0)]), Value::text("a#b2"));
+        assert_eq!(ev("REGEXREPLACE", vec![t("a1b2"), t("[0-9]"), t("#"), n(-3.0)]), Value::text("a1b2"));
+        // 2^32 is no occurrence the text has (and on 32-bit targets it must not wrap to 0).
+        for occurrence in [4_294_967_296.0, -4_294_967_296.0] {
+            assert_eq!(ev("REGEXREPLACE", vec![t("a1b2"), t("[0-9]"), t("#"), n(occurrence)]), Value::text("a1b2"));
+        }
         // One match whose replacement repeats it 10,000 times: refused before it is built.
         assert_eq!(ev("REGEXREPLACE", vec![t(&many), t("^.*$"), t(&"$0".repeat(10_000))]), Value::Error(CellError::Value));
         assert_eq!(ev("REGEXREPLACE", vec![t(&many), t("^.*$"), t(&"$0".repeat(10_000)), n(1.0)]), Value::Error(CellError::Value));

crates/functions/src/text.rs

@@ -883,13 +883,25 @@ fn regexreplace(a: &[Arg], _c: &mut dyn Ctx) -> R<Value> {
         out.push_str(s.get(last..).unwrap_or_default());
         return check_len(out);
     }
-    let caps: Vec<regex::Captures> = re.captures_iter(&s).collect();
-    let k = occ.unsigned_abs() as usize;
-    let idx = if occ > 0 { k.checked_sub(1) } else { caps.len().checked_sub(k) };
-    let Some(cap) = idx.and_then(|i| caps.get(i)) else { return text_val(s) };
+    // An occurrence past what `usize` holds (32-bit targets) names no match: the text is returned as it was.
+    let Ok(k) = usize::try_from(occ.unsigned_abs()) else { return text_val(s) };
+    // Keeps only the wanted match's captures: the nth from the front, or a window of the last `k`.
+    let cap = if occ > 0 {
+        re.captures_iter(&s).nth(k - 1)
+    } else {
+        let mut last = std::collections::VecDeque::new();
+        for cap in re.captures_iter(&s) {
+            if last.len() == k {
+                last.pop_front();
+            }
+            last.push_back(cap);
+        }
+        if last.len() == k { last.pop_front() } else { None }
+    };
+    let Some(cap) = cap else { return text_val(s) };
     let Some(m) = cap.get(0) else { return text_val(s) };
     out.push_str(s.get(..m.start()).unwrap_or_default());
-    expand_capped(cap, &rep, &mut out, limit).ok_or(CellError::Value)?;
+    expand_capped(&cap, &rep, &mut out, limit).ok_or(CellError::Value)?;
     out.push_str(s.get(m.end()..).unwrap_or_default());
     check_len(out)
 }