From fe377266e1cdbb8923af2260da6bab295f8e8194 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 14:14:58 -0500 Subject: [PATCH] clawhdf5: FileEditor unmaps the file before an edit writes it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- crates/clawhdf5/src/edit/image.rs | 32 +++++++++++- crates/clawhdf5/src/edit/mod.rs | 82 +++++++++++++++++++++++++++++-- 2 files changed, 109 insertions(+), 5 deletions(-) diff --git a/crates/clawhdf5/src/edit/image.rs b/crates/clawhdf5/src/edit/image.rs index a100b66..254abcd 100644 --- a/crates/clawhdf5/src/edit/image.rs +++ b/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>, + 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 diff --git a/crates/clawhdf5/src/edit/mod.rs b/crates/clawhdf5/src/edit/mod.rs index f6a590e..4903672 100644 --- a/crates/clawhdf5/src/edit/mod.rs +++ b/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( &mut self, op: impl FnOnce(&File, &mut Image<'_>) -> Result, @@ -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] + ); + } +}