diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c0e719..98046ea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,26 @@ ## Unreleased +### Correctness: edits planned from another file after a rename or `chdir` (2026-09-27) +- **`FileEditor` planned each edit by re-opening its path but wrote + through the file it held open** (fixed 2026-09-27; on main since PR #18, + no release). When the path came to name another file between edits — a + rename or replacement, or, for a relative path, a change of working + directory — an edit was laid out from the other file's metadata and + written into the held one, corrupting it (h5py: "invalid dataset size, + likely file corruption"). The Python `'r+'` handle had the same flaw in + its reads: it reopened the path after every edit, so reads came from the + other file. The editor now plans every edit from the file it holds, and + its path is canonicalised at open. New `FileEditor::reader()` opens the + held file anew for reading (on Linux through `/proc/self/fd`, so it + follows a renamed file; elsewhere by the path, refused when the path no + longer names the held file), without sharing the editor's lock; the + Python handle reads through it, and a `'w'` file is written at the + absolute path it was opened with. Tests: `edit_tests.rs`'s + `edits_go_to_the_file_held_not_the_path`; `test_edit.py`'s + `test_relative_path_and_chdir` and `test_path_replaced_between_edits` + (the review's repro). + ### Correctness: zero extents in Fixed/Extensible Array chunk indexes (2026-09-27) - **A chunked dataset whose maximum (or, with none recorded, current) extent is 0 along a dimension made the reader divide by zero** (fixed diff --git a/crates/clawhdf5-io/src/mmap.rs b/crates/clawhdf5-io/src/mmap.rs index 756d5e3..113693a 100644 --- a/crates/clawhdf5-io/src/mmap.rs +++ b/crates/clawhdf5-io/src/mmap.rs @@ -39,6 +39,23 @@ impl MmapReader { Ok(Self { _file: file, mmap }) } + /// Memory-map a file that is already open (for reading). + /// + /// The mapping references `file`'s open file description for as long as + /// it lives, so a `flock` taken through that description (or a + /// `try_clone` of it) is held until the reader is dropped. + /// + /// # Safety + /// + /// The same contract as [`open`](Self::open): the file must not be + /// modified while the mapping is active. + pub fn from_file(file: fs::File) -> io::Result { + // SAFETY: a read-only mapping; the caller keeps the file unmodified + // while it is alive. + let mmap = unsafe { Mmap::map(&file)? }; + Ok(Self { _file: file, mmap }) + } + /// Zero-copy access to the entire file contents. pub fn as_bytes(&self) -> &[u8] { &self.mmap diff --git a/crates/clawhdf5-py/src/file.rs b/crates/clawhdf5-py/src/file.rs index 2764e61..bfb6d57 100644 --- a/crates/clawhdf5-py/src/file.rs +++ b/crates/clawhdf5-py/src/file.rs @@ -110,7 +110,9 @@ impl PyFile { "w" => Ok(Self { filename, inner: Some(FileInner::Write(WriteState { - path: PathBuf::from(path), + // Absolute now: the file is written at close, possibly + // after the working directory changed. + path: std::path::absolute(path).unwrap_or_else(|_| PathBuf::from(path)), root_datasets: Vec::new(), root_attrs: Arc::new(Mutex::new(Vec::new())), groups: Vec::new(), diff --git a/crates/clawhdf5-py/src/handle.rs b/crates/clawhdf5-py/src/handle.rs index 754de6f..4bb8364 100644 --- a/crates/clawhdf5-py/src/handle.rs +++ b/crates/clawhdf5-py/src/handle.rs @@ -8,7 +8,8 @@ //! //! A file opened with `'r+'` also holds a [`FileEditor`]. An edit takes the //! file's write lock, so no read runs while the file changes underneath it, -//! and reopens the file afterwards: reads after an edit see the new bytes +//! and reopens the file afterwards, through the editor's own open file +//! rather than its path: reads after an edit see the new bytes //! (a grown file, a new dataspace), never a stale mapping or chunk cache. //! Objects that cache something an edit can change compare //! [`Handle::generation`] with the value they cached it at. @@ -16,7 +17,6 @@ //! Lock discipline (no deadlock with the GIL): the file lock is only taken //! with the GIL released, and code that holds it never touches Python. -use std::path::PathBuf; use std::sync::atomic::{AtomicU64, Ordering}; use std::sync::{Arc, Mutex, PoisonError, RwLock}; @@ -28,8 +28,9 @@ use crate::{panic_text, to_py_err}; /// Where the file's bytes come from. pub(crate) enum Source { - /// A local path (memory-mapped). - Local(PathBuf), + /// A local file (memory-mapped). Its path is not kept: nothing reopens + /// it by path (see `open_editable`). + Local, /// A URL, read through `clawhdf5-remote`'s block cache. Remote { url: String, @@ -74,25 +75,23 @@ impl Handle { /// A local file, read-only. pub(crate) fn open_local(py: Python<'_>, path: &str) -> PyResult> { let file = py.detach(|| crate::no_panic(|| File::open(path).map_err(to_py_err)))?; - Ok(Self::new(file, Source::Local(PathBuf::from(path)), None)) + Ok(Self::new(file, Source::Local, None)) } /// A local file, open for in-place editing (`'r+'`): the editor takes /// the file's exclusive lock and checks that it can edit the file, then - /// the file is opened for reading. + /// the file is read through the editor's own file — never by path + /// again, so a later `os.chdir` or a rename or replacement of the path + /// cannot make reads (or the editor's plans) come from another file. pub(crate) fn open_editable(py: Python<'_>, path: &str) -> PyResult> { let (file, editor) = py.detach(|| { crate::no_panic(|| { let editor = FileEditor::open(path).map_err(to_py_err)?; - let file = File::open(path).map_err(to_py_err)?; + let file = editor.reader().map_err(to_py_err)?; Ok((file, editor)) }) })?; - Ok(Self::new( - file, - Source::Local(PathBuf::from(path)), - Some(editor), - )) + Ok(Self::new(file, Source::Local, Some(editor))) } /// A remote file (`http(s)://`, `s3://`, ...). @@ -153,7 +152,7 @@ impl Handle { pub(crate) fn remote_storage(&self) -> Option<&clawhdf5_remote::RemoteStorage> { match &self.source { Source::Remote { storage, .. } => Some(storage), - Source::Local(_) => None, + Source::Local => None, } } @@ -161,7 +160,7 @@ impl Handle { pub(crate) fn redacted_url(&self) -> Option { match &self.source { Source::Remote { url, .. } => Some(clawhdf5_remote::redact_url(url)), - Source::Local(_) => None, + Source::Local => None, } } @@ -185,14 +184,12 @@ impl Handle { let Some(editor) = &self.editor else { return Err(PyOSError::new_err(match self.source { Source::Remote { .. } => "remote files are read-only", - Source::Local(_) => { - "the file is open read-only; open it with mode 'r+' to change it" - } + Source::Local => "the file is open read-only; open it with mode 'r+' to change it", })); }; - let Source::Local(path) = &self.source else { + if matches!(self.source, Source::Remote { .. }) { return Err(PyOSError::new_err("remote files are read-only")); - }; + } py.detach(|| { let mut ed = editor.lock().unwrap_or_else(PoisonError::into_inner); let ed = ed @@ -202,7 +199,8 @@ impl Handle { let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| f(ed))); // Drop the old mapping and its chunk cache before reopening. *file = None; - let reopened = std::panic::catch_unwind(|| File::open(path)); + // Through the editor's file, not the path (see `open_editable`). + let reopened = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| ed.reader())); self.generation.fetch_add(1, Ordering::AcqRel); match reopened { Ok(Ok(f)) => *file = Some(f), diff --git a/crates/clawhdf5-py/tests/test_edit.py b/crates/clawhdf5-py/tests/test_edit.py index 690bd0f..c0b7ba6 100644 --- a/crates/clawhdf5-py/tests/test_edit.py +++ b/crates/clawhdf5-py/tests/test_edit.py @@ -823,3 +823,76 @@ def test_close_releases_the_file(h5py, tmp_path): ds[0] = 1 with h5py.File(path, "r+") as g: g["d"][0] = 9 + + +def _two_files(h5py, tmp_path): + """d1/f.h5 and d2/f.h5: same name, different layouts (the review's + repro).""" + (tmp_path / "d1").mkdir() + (tmp_path / "d2").mkdir() + with h5py.File(tmp_path / "d1" / "f.h5", "w") as f: + f.create_dataset("x", data=np.arange(10, dtype=">(path: P) -> Result { - let path = path.as_ref().to_path_buf(); - let file = OpenOptions::new().read(true).write(true).open(&path)?; + let file = OpenOptions::new() + .read(true) + .write(true) + .open(path.as_ref())?; + let path = std::fs::canonicalize(path.as_ref())?; match file.try_lock() { Ok(()) => {} Err(TryLockError::WouldBlock) => { @@ -768,16 +777,68 @@ impl FileEditor { file, free: FreeList::default(), }; - let f = File::open(&ed.path)?; + let f = ed.plan_reader()?; check_editable(&f)?; Ok(ed) } - /// The file's path. + /// The file's absolute path when it was opened (it may have been + /// renamed since; the editor keeps editing the file it opened). pub fn path(&self) -> &Path { &self.path } + /// A reader over the file this editor holds, as last written: the file + /// opened by [`open`](Self::open), not whatever its path names now. + /// + /// It opens the file anew (read-only), so it does not share the + /// editor's lock and stays usable after the editor is dropped: on Linux + /// through `/proc/self/fd`, which reaches the held file even after its + /// path was renamed or replaced; elsewhere by the path the file had at + /// open, refused with [`Error::Io`] when that path no longer names the + /// held file (on Unix, compared by device and inode; Windows cannot + /// check). With the `mmap` feature the reader maps the file: edits + /// through the editor change the bytes it sees, so take a new reader + /// after each edit rather than reading through an old one while an edit + /// runs. + pub fn reader(&self) -> Result { + let dir = self.path.parent().map(Path::to_path_buf); + File::from_std_file(self.reopen()?, dir) + } + + /// A new read-only open file description of the held file (see + /// [`reader`](Self::reader)). + fn reopen(&self) -> Result { + #[cfg(target_os = "linux")] + { + use std::os::fd::AsRawFd; + let proc = format!("/proc/self/fd/{}", self.file.as_raw_fd()); + if let Ok(f) = std::fs::File::open(proc) { + return Ok(f); + } + } + let f = std::fs::File::open(&self.path)?; + #[cfg(unix)] + { + use std::os::unix::fs::MetadataExt; + let (a, b) = (self.file.metadata()?, f.metadata()?); + if (a.dev(), a.ino()) != (b.dev(), b.ino()) { + return Err(Error::Io(std::io::Error::other(format!( + "{} no longer names the file being edited (renamed or replaced)", + self.path.display() + )))); + } + } + Ok(f) + } + + /// A reader over the held file itself for planning an edit (it shares + /// the file's lock, and is dropped before the edit writes). + fn plan_reader(&self) -> Result { + let dir = self.path.parent().map(Path::to_path_buf); + File::from_std_file(self.file.try_clone()?, dir) + } + /// Bytes earlier edits of this editor freed that later ones can still /// reuse. pub fn reusable_bytes(&self) -> u64 { @@ -794,7 +855,7 @@ impl FileEditor { &mut self, op: impl FnOnce(&File, &mut Image<'_>) -> Result, ) -> Result { - let f = File::open(&self.path)?; + let f = self.plan_reader()?; check_editable(&f)?; let sb = f.superblock().clone(); let user_block = f.user_block_size(); diff --git a/crates/clawhdf5/src/reader.rs b/crates/clawhdf5/src/reader.rs index 1786f17..d362383 100644 --- a/crates/clawhdf5/src/reader.rs +++ b/crates/clawhdf5/src/reader.rs @@ -405,6 +405,38 @@ impl File { } } + /// A reader over an already open file (the file itself, not whatever + /// its path names now): mapped with the `mmap` feature, else read into + /// memory. `base_dir` resolves external Virtual Dataset sources. + pub(crate) fn from_std_file( + file: std::fs::File, + base_dir: Option, + ) -> Result { + #[cfg(feature = "mmap")] + let mut f = { + let reader = clawhdf5_io::MmapReader::from_file(file).map_err(Error::Io)?; + let (data, superblock) = FileData::new(Backing::Mmap(reader))?; + Self { + data, + superblock, + chunk_cache: ChunkCache::new(), + base_dir: None, + vds_resolver: None, + } + }; + #[cfg(not(feature = "mmap"))] + let mut f = { + use std::io::{Read, Seek, SeekFrom}; + let mut file = file; + let mut bytes = Vec::new(); + file.seek(SeekFrom::Start(0)).map_err(Error::Io)?; + file.read_to_end(&mut bytes).map_err(Error::Io)?; + Self::from_bytes(bytes)? + }; + f.base_dir = base_dir; + Ok(f) + } + /// Open an HDF5 file by reading it entirely into memory. /// /// This is the pre-mmap behaviour and is useful when memory-mapping is diff --git a/crates/clawhdf5/tests/edit_tests.rs b/crates/clawhdf5/tests/edit_tests.rs index 9190a2c..1f83d74 100644 --- a/crates/clawhdf5/tests/edit_tests.rs +++ b/crates/clawhdf5/tests/edit_tests.rs @@ -162,3 +162,59 @@ fn shrink_then_grow_reads_fill() { raw[..4].fill(0.5); assert_eq!(f.dataset("raw").unwrap().read_f64().unwrap(), raw); } + +/// The editor plans every edit from the file it holds open, never by +/// re-opening its path: with the path renamed away and another file put in +/// its place, edits go to the held file, planned from its own metadata, +/// and the file now at the path is untouched (planning from it and writing +/// into the held file corrupted the held one). +#[test] +fn edits_go_to_the_file_held_not_the_path() { + let dir = tempfile::tempdir().unwrap(); + let a = dir.path().join("a.h5"); + let b = dir.path().join("b.h5"); + let mut fb = FileBuilder::new(); + fb.create_dataset("x") + .with_i32_data(&[0; 10]) + .with_shape(&[10]); + fb.create_dataset("big") + .with_f64_data(&[1.5; 5000]) + .with_shape(&[5000]); + fb.set_attr("title", AttrValue::String("a".into())); + fb.write(&a).unwrap(); + let mut fb = FileBuilder::new(); + fb.create_dataset("pad") + .with_f64_data(&[2.5; 3000]) + .with_shape(&[3000]); + fb.create_dataset("x") + .with_i32_data(&[500; 10]) + .with_shape(&[10]); + fb.write(&b).unwrap(); + + let mut ed = FileEditor::open(&a).unwrap(); + assert!(ed.path().is_absolute()); + let moved = dir.path().join("moved.h5"); + std::fs::rename(&a, &moved).unwrap(); + std::fs::rename(&b, &a).unwrap(); + let other = std::fs::read(&a).unwrap(); + + ed.write_values("x", &Selection::All, &[7i32; 10]).unwrap(); + let vals: Vec = (0..50).map(f64::from).collect(); + ed.set_attr("/", "note", &AttrValue::F64Array(vals.clone())) + .unwrap(); + // The editor's own reader sees the held file. + let r = ed.reader().unwrap(); + assert_eq!(r.dataset("x").unwrap().read_i32().unwrap(), [7; 10]); + assert!(r.dataset("pad").is_err()); + drop(r); + drop(ed); + + assert!( + std::fs::read(&a).unwrap() == other, + "the file at the path changed" + ); + let f = File::open(&moved).unwrap(); + assert_eq!(f.dataset("x").unwrap().read_i32().unwrap(), [7; 10]); + assert_eq!(f.dataset("big").unwrap().read_f64().unwrap(), [1.5; 5000]); + assert!(matches!(f.root().attr("note").unwrap(), Some(AttrValue::F64Array(v)) if v == vals)); +} diff --git a/docs/known-issues.md b/docs/known-issues.md index 690bb04..871729d 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -143,6 +143,12 @@ space it leaves is too small for its next, larger version. **No journal.** A crash while an edit patches existing structures can leave the file inconsistent; see the `FileEditor` documentation. +**Renamed files outside Linux.** Edits always go to the file the editor +opened. `FileEditor::reader()` (which the Python `'r+'` handle reads +through) reopens it through `/proc/self/fd` on Linux; elsewhere it reopens +the path, and after the path was renamed or replaced it fails (on Unix; +Windows cannot tell and would read whatever the path names). + ## Python in-place editing (`clawhdf5.File(path, 'r+')`) limits **Status:** open (added 2026-09-27). The Python bindings edit through