clawhdf5: FileEditor unmaps the file before an edit writes it

Each edit planned over the reader's memory map of the file and committed
while that File, and the Image's &[u8] over the mapping, were still alive,
writing the same file through the editor's descriptor. Nothing read the
mapping during the writes, but a shared slice whose memory changes
underneath it is undefined behaviour under Rust's aliasing rules.

Image::into_plan now detaches the edit's writes (patches, end of
allocation) into a Plan that owns all of its bytes and borrows nothing;
edit() takes the user-block size, drops the File — unmapping the file —
and only then commits the Plan. The invariant is documented in the image
module and the editor's module docs.

Test: edit::tests::file_is_not_mapped_while_an_edit_writes_it checks
/proc/self/maps at the moment each commit starts (write, resize, set_attr):
never mapped. With the commit moved back before the reader is dropped (the
previous order) it reports all three commits with the file mapped.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>

crates/clawhdf5/src/edit/image.rs

@@ -4,9 +4,17 @@
 //! An edit never writes to the file while it is being planned. Every change
 //! is recorded here first (reads see them), so an edit that fails half-way —
 //! a filter that cannot encode, a chunk index this code does not handle —
-//! leaves the file exactly as it was. [`Image::commit`] then writes the
-//! changes in an order that keeps the old metadata valid for as long as
+//! leaves the file exactly as it was. [`Image::into_plan`] then detaches the
+//! changes from the bytes they were planned over, and [`Plan::commit`]
+//! writes them in an order that keeps the old metadata valid for as long as
 //! possible (see there).
+//!
+//! **Invariant:** the base bytes an image reads are the reader's view of the
+//! file — a memory map when the `mmap` feature is on. Nothing may write the
+//! file while that view is alive: a write through another descriptor would
+//! change memory behind a live `&[u8]`, which Rust's aliasing rules forbid.
+//! So a [`Plan`] owns everything it writes and borrows nothing, and the
+//! editor drops the reader (unmapping the file) before it commits.
 
 use std::collections::BTreeMap;
 use std::io::{Seek, SeekFrom, Write};
@@ -178,6 +186,26 @@ impl<'a> Image<'a> {
         Ok(())
     }
 
+    /// The edit's writes, detached from the base bytes (see the module's
+    /// invariant: the reader that owns them can then be dropped before
+    /// anything is written).
+    pub(crate) fn into_plan(self) -> Plan {
+        Plan {
+            patches: self.patches,
+            eoa: self.eoa,
+            old_eoa: self.old_eoa,
+        }
+    }
+}
+
+/// The writes of a planned edit, owning all of their bytes.
+pub(crate) struct Plan {
+    patches: BTreeMap<u64, Vec<u8>>,
+    eoa: u64,
+    old_eoa: u64,
+}
+
+impl Plan {
     /// Write the edit to `file`, whose superblock is at `user_block`.
     ///
     /// Order: first everything in newly allocated space (new chunks, new

crates/clawhdf5/src/edit/mod.rs

@@ -10,7 +10,10 @@
 //! Each operation is planned in memory first ([`image::Image`]): if any part
 //! of it is unsupported, nothing is written. The plan is then committed with
 //! the new space (new chunks, new index blocks) written and synced before
-//! the existing bytes that link it in, then synced again.
+//! the existing bytes that link it in, then synced again. The plan owns the
+//! bytes it writes; the reader it was planned with (a memory map of the
+//! file) is dropped before the first write, so no `&[u8]` over the mapping
+//! is alive while the file changes (see `image`).
 
 mod btree1;
 mod earray;
@@ -614,6 +617,12 @@ impl FileEditor {
         &self.path
     }
 
+    /// Plan an edit over the file's current bytes, then commit it.
+    ///
+    /// The reader (a memory map of the file, with the `mmap` feature) is
+    /// dropped before anything is written: the plan owns every byte it
+    /// writes, so no slice over the mapping is alive while the file changes
+    /// underneath it (see `image`'s invariant).
     fn edit<R>(
         &mut self,
         op: impl FnOnce(&File, &mut Image<'_>) -> Result<R, Error>,
@@ -621,13 +630,22 @@ impl FileEditor {
         let f = File::open(&self.path)?;
         check_editable(&f)?;
         let sb = f.superblock().clone();
+        let user_block = f.user_block_size();
         let mut img = Image::new(f.as_bytes(), sb.offset_size, sb.length_size);
         let r = op(&f, &mut img).map_err(unsupported_filter)?;
-        if img.is_dirty() {
+        let plan = if img.is_dirty() {
             if img.eoa() != img.old_eoa() {
                 set_superblock_eof(&mut img, &sb)?;
             }
-            img.commit(&mut self.file, f.user_block_size())?;
+            Some(img.into_plan())
+        } else {
+            None
+        };
+        drop(f);
+        if let Some(plan) = plan {
+            #[cfg(test)]
+            tests::note_commit(&self.path);
+            plan.commit(&mut self.file, user_block)?;
         }
         Ok(r)
     }
@@ -1263,3 +1281,61 @@ fn decode_chunk(
     }
     Ok(out)
 }
+
+#[cfg(test)]
+mod tests {
+    use std::cell::Cell;
+    use std::path::Path;
+
+    use super::FileEditor;
+    use crate::{AttrValue, FileBuilder};
+
+    thread_local! {
+        /// Commits seen on this thread, and how many found the file mapped.
+        static COMMITS: Cell<(usize, usize)> = const { Cell::new((0, 0)) };
+    }
+
+    /// Called just before an edit writes the file: whether this process
+    /// still maps it (`/proc/self/maps` lists every mapping by path).
+    pub(super) fn note_commit(path: &Path) {
+        let path = std::fs::canonicalize(path).unwrap();
+        let maps = std::fs::read_to_string("/proc/self/maps").unwrap_or_default();
+        let mapped = maps
+            .lines()
+            .any(|l| l.ends_with(&format!(" {}", path.display())));
+        COMMITS.with(|c| {
+            let (n, m) = c.get();
+            c.set((n + 1, m + usize::from(mapped)));
+        });
+    }
+
+    /// No edit writes the file while the reader's memory map of it (and so
+    /// a `&[u8]` over it) is alive.
+    #[test]
+    #[cfg(all(target_os = "linux", feature = "mmap"))]
+    fn file_is_not_mapped_while_an_edit_writes_it() {
+        let dir = tempfile::tempdir().unwrap();
+        let path = dir.path().join("mapped.h5");
+        let mut b = FileBuilder::new();
+        b.create_dataset("x")
+            .with_i32_data(&[1, 2, 3, 4])
+            .with_shape(&[4])
+            .with_maxshape(&[u64::MAX])
+            .with_chunks(&[2])
+            .with_deflate(4);
+        b.write(&path).unwrap();
+        let mut ed = FileEditor::open(&path).unwrap();
+        ed.write_values("x", &crate::Selection::All, &[5i32, 6, 7, 8])
+            .unwrap();
+        ed.resize("x", &[6]).unwrap();
+        ed.set_attr("x", "a", &AttrValue::I64(1)).unwrap();
+        drop(ed);
+        let (commits, mapped) = COMMITS.with(Cell::get);
+        assert_eq!((commits, mapped), (3, 0));
+        let f = crate::File::open(&path).unwrap();
+        assert_eq!(
+            f.dataset("x").unwrap().read_i32().unwrap(),
+            [5, 6, 7, 8, 0, 0]
+        );
+    }
+}