From 24412a0e5995e7124f905ccc091fad073c4f4996 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 08:52:55 -0500 Subject: [PATCH] fix(py): a panic in the library raises clawhdf5.InternalError, not PanicException MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PanicException derives from BaseException, so `except Exception` let a library bug through. Every call from the bindings into the library now runs under catch_unwind and a panic becomes InternalError (RuntimeError) naming the object. Tests: a hidden hook panics inside the guard; and the v4 chunk indexes are compared with h5py from Python — with the library fix reverted, ds[0:30] of the implicit-index dataset now raises InternalError instead of aborting the test run. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 6 + crates/clawhdf5-py/src/attrs.rs | 80 +++++----- crates/clawhdf5-py/src/dataset.rs | 141 ++++++++++-------- crates/clawhdf5-py/src/file.rs | 46 +++--- crates/clawhdf5-py/src/group.rs | 22 +-- crates/clawhdf5-py/src/lib.rs | 38 +++++ crates/clawhdf5-py/src/node.rs | 88 ++++++----- crates/clawhdf5-py/tests/test_read_vs_h5py.py | 44 ++++++ 8 files changed, 289 insertions(+), 176 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d01a200..d679fbf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,12 @@ h5py 3.16 (HDF5 2.0) on a file h5py writes. One difference is h5py's: it returns variable-length sequences of big-endian floats unswapped; this package returns the stored values. +- **A panic in the library is an ordinary Python exception.** PyO3 turns a + Rust panic into `PanicException`, a `BaseException` that `except + Exception` does not catch. Every call from the bindings into the library + is now guarded and a panic becomes `clawhdf5.InternalError` (a + `RuntimeError`) naming the object; with the implicit-index panic above + restored, `ds[0:30]` raises it. - **CI builds and tests the Python package.** It was excluded from CI. `scripts/ci-test.sh` now lints `clawhdf5-py`, builds the wheel with maturin, unpacks it under `target/` and runs the pytest suite; skipped diff --git a/crates/clawhdf5-py/src/attrs.rs b/crates/clawhdf5-py/src/attrs.rs index ff608ed..d1b06cb 100644 --- a/crates/clawhdf5-py/src/attrs.rs +++ b/crates/clawhdf5-py/src/attrs.rs @@ -191,46 +191,48 @@ fn attr_to_py<'py>( file: &clawhdf5_rs::File, attr: &AttributeMessage, ) -> PyResult> { - let sb = file.superblock(); - let conv = Converter::new(py, &attr.datatype, sb.offset_size) - .map_err(|e| prefix_err(py, &attr.name, e))?; - if node::is_null(&attr.dataspace) { - return Ok(PyEmpty::new(conv.dtype).into_pyobject(py)?.into_any()); - } - let shape: Vec = attr - .dataspace - .dimensions - .iter() - .map(|&d| d as usize) - .collect(); - let n: usize = shape.iter().product(); - let data = if conv.is_vl() { - let want = n * conv.elem_size; - if attr.raw_data.len() < want { - return Err(PyValueError::new_err(format!( - "attribute {}: {} bytes of variable-length references, expected {want}", - attr.name, - attr.raw_data.len(), - ))); + crate::no_panic(|| { + let sb = file.superblock(); + let conv = Converter::new(py, &attr.datatype, sb.offset_size) + .map_err(|e| prefix_err(py, &attr.name, e))?; + if node::is_null(&attr.dataspace) { + return Ok(PyEmpty::new(conv.dtype).into_pyobject(py)?.into_any()); } - let raw = &attr.raw_data[..want]; - let file_data = file.as_bytes(); - let (osz, lsz, unit) = (sb.offset_size, sb.length_size, conv.vl_unit); - Elements::Vl( - py.detach(|| resolve_vl(file_data, raw, n, osz, lsz, unit)) - .map_err(|e| PyValueError::new_err(format!("attribute {}: {e}", attr.name)))?, - ) - } else { - Elements::Bytes(attr.raw_data.clone()) - }; - let arr = conv - .to_array(py, data, &shape, true) - .map_err(|e| prefix_err(py, &attr.name, e))?; - if shape.is_empty() { - // A scalar dataspace: h5py returns the element itself. - return arr.get_item(PyTuple::empty(py)); - } - Ok(arr) + let shape: Vec = attr + .dataspace + .dimensions + .iter() + .map(|&d| d as usize) + .collect(); + let n: usize = shape.iter().product(); + let data = if conv.is_vl() { + let want = n * conv.elem_size; + if attr.raw_data.len() < want { + return Err(PyValueError::new_err(format!( + "attribute {}: {} bytes of variable-length references, expected {want}", + attr.name, + attr.raw_data.len(), + ))); + } + let raw = &attr.raw_data[..want]; + let file_data = file.as_bytes(); + let (osz, lsz, unit) = (sb.offset_size, sb.length_size, conv.vl_unit); + Elements::Vl( + py.detach(|| resolve_vl(file_data, raw, n, osz, lsz, unit)) + .map_err(|e| PyValueError::new_err(format!("attribute {}: {e}", attr.name)))?, + ) + } else { + Elements::Bytes(attr.raw_data.clone()) + }; + let arr = conv + .to_array(py, data, &shape, true) + .map_err(|e| prefix_err(py, &attr.name, e))?; + if shape.is_empty() { + // A scalar dataspace: h5py returns the element itself. + return arr.get_item(PyTuple::empty(py)); + } + Ok(arr) + }) } fn prefix_err(py: Python<'_>, name: &str, e: PyErr) -> PyErr { diff --git a/crates/clawhdf5-py/src/dataset.rs b/crates/clawhdf5-py/src/dataset.rs index cbfc68d..9109a32 100644 --- a/crates/clawhdf5-py/src/dataset.rs +++ b/crates/clawhdf5-py/src/dataset.rs @@ -42,25 +42,27 @@ impl PyDataset { file: Arc, path: String, ) -> PyResult { - let hdr = node::header(&file, &path)?; - let null = node::is_null(&node::dataspace(&file, &hdr)?); - let (shape, datatype) = { - let ds = file.dataset(&path).map_err(to_py_err)?; - let shape = if null { - None - } else { - Some(ds.shape().map_err(to_py_err)?) + crate::no_panic(|| { + let hdr = node::header(&file, &path)?; + let null = node::is_null(&node::dataspace(&file, &hdr)?); + let (shape, datatype) = { + let ds = file.dataset(&path).map_err(to_py_err)?; + let shape = if null { + None + } else { + Some(ds.shape().map_err(to_py_err)?) + }; + (shape, ds.raw_datatype().map_err(to_py_err)?) }; - (shape, ds.raw_datatype().map_err(to_py_err)?) - }; - let conv = Converter::new(py, &datatype, file.superblock().offset_size) - .map_err(|e| e.value(py).to_string()); - Ok(Self { - file, - path, - shape, - datatype, - conv, + let conv = Converter::new(py, &datatype, file.superblock().offset_size) + .map_err(|e| e.value(py).to_string()); + Ok(Self { + file, + path, + shape, + datatype, + conv, + }) }) } @@ -84,39 +86,43 @@ impl PyDataset { let path = self.path.as_str(); let (vl, elem_size, unit) = (conv.is_vl(), conv.elem_size, conv.vl_unit); // Everything below touches only Rust data: release the GIL. - let blocks: Vec<(Elements, Vec)> = py - .detach(|| -> Result<_, ReadError> { - let ds = file.dataset(path)?; - let sb = file.superblock(); - let mut blocks = Vec::with_capacity(reads.len()); - for (sel, shape) in reads { - let raw = ds.read_selection(&sel)?; - let n: usize = shape.iter().product(); - let data = if vl { - if raw.len() != n * elem_size { - return Err(ReadError::Other(format!( - "read {} bytes of variable-length references, expected {}", - raw.len(), - n * elem_size - ))); - } - Elements::Vl( - resolve_vl( - file.as_bytes(), - &raw, - n, - sb.offset_size, - sb.length_size, - unit, - ) - .map_err(ReadError::Other)?, + let read = || -> Result<_, ReadError> { + let ds = file.dataset(path)?; + let sb = file.superblock(); + let mut blocks = Vec::with_capacity(reads.len()); + for (sel, shape) in reads { + let raw = ds.read_selection(&sel)?; + let n: usize = shape.iter().product(); + let data = if vl { + if raw.len() != n * elem_size { + return Err(ReadError::Other(format!( + "read {} bytes of variable-length references, expected {}", + raw.len(), + n * elem_size + ))); + } + Elements::Vl( + resolve_vl( + file.as_bytes(), + &raw, + n, + sb.offset_size, + sb.length_size, + unit, ) - } else { - Elements::Bytes(raw) - }; - blocks.push((data, shape)); - } - Ok(blocks) + .map_err(ReadError::Other)?, + ) + } else { + Elements::Bytes(raw) + }; + blocks.push((data, shape)); + } + Ok(blocks) + }; + let blocks: Vec<(Elements, Vec)> = py + .detach(|| { + std::panic::catch_unwind(std::panic::AssertUnwindSafe(read)) + .unwrap_or_else(|p| Err(ReadError::Panic(crate::panic_text(&*p)))) }) .map_err(|e| e.into_py(&self.path))?; @@ -160,6 +166,7 @@ impl PyDataset { enum ReadError { Lib(clawhdf5_rs::Error), Other(String), + Panic(String), } impl From for ReadError { @@ -173,6 +180,10 @@ impl ReadError { match self { ReadError::Lib(e) => to_py_err(e), ReadError::Other(msg) => PyValueError::new_err(format!("{}: {msg}", node::name(path))), + ReadError::Panic(msg) => crate::InternalError::new_err(format!( + "{}: clawhdf5 internal error (please report it): {msg}", + node::name(path) + )), } } } @@ -223,20 +234,22 @@ impl PyDataset { /// The maximum shape (`None` per unlimited dimension), like h5py. #[getter] fn maxshape<'py>(&self, py: Python<'py>) -> PyResult> { - let Some(shape) = &self.shape else { - return Ok(py.None().into_bound(py)); - }; - let max = self - .file - .dataset(&self.path) - .and_then(|ds| ds.max_dimensions()) - .map_err(to_py_err)? - .unwrap_or_else(|| shape.clone()); - let items: Vec> = max - .into_iter() - .map(|d| (d != u64::MAX).then_some(d)) - .collect(); - Ok(PyTuple::new(py, items)?.into_any()) + crate::no_panic(|| { + let Some(shape) = &self.shape else { + return Ok(py.None().into_bound(py)); + }; + let max = self + .file + .dataset(&self.path) + .and_then(|ds| ds.max_dimensions()) + .map_err(to_py_err)? + .unwrap_or_else(|| shape.clone()); + let items: Vec> = max + .into_iter() + .map(|d| (d != u64::MAX).then_some(d)) + .collect(); + Ok(PyTuple::new(py, items)?.into_any()) + }) } /// The dataset's numpy dtype, as h5py reports it. diff --git a/crates/clawhdf5-py/src/file.rs b/crates/clawhdf5-py/src/file.rs index 4f6bb7a..a2c48ea 100644 --- a/crates/clawhdf5-py/src/file.rs +++ b/crates/clawhdf5-py/src/file.rs @@ -57,9 +57,9 @@ impl PyFile { let filename = path.to_string(); match mode { "r" => { - let file = py - .detach(|| clawhdf5_rs::File::open(path)) - .map_err(to_py_err)?; + let file = py.detach(|| { + crate::no_panic(|| clawhdf5_rs::File::open(path).map_err(to_py_err)) + })?; Ok(Self { inner: Some(FileInner::Read(Arc::new(file))), filename, @@ -277,29 +277,31 @@ fn parse_compression( /// Build and write the HDF5 file from accumulated write state. fn finalize_write(state: WriteState) -> PyResult<()> { - let mut builder = clawhdf5_rs::FileBuilder::new(); + crate::no_panic(|| { + let mut builder = clawhdf5_rs::FileBuilder::new(); - // Root attributes - let root_attrs = state.root_attrs.lock().unwrap_or_else(|e| e.into_inner()); - for (name, val) in root_attrs.iter() { - builder.set_attr(name, val.clone().into()); - } - drop(root_attrs); + // Root attributes + let root_attrs = state.root_attrs.lock().unwrap_or_else(|e| e.into_inner()); + for (name, val) in root_attrs.iter() { + builder.set_attr(name, val.clone().into()); + } + drop(root_attrs); - // Root datasets - for spec in &state.root_datasets { - let db = builder.create_dataset(&spec.name); - apply_dataset_spec(db, spec); - } + // Root datasets + for spec in &state.root_datasets { + let db = builder.create_dataset(&spec.name); + apply_dataset_spec(db, spec); + } - // Groups - for group_arc in &state.groups { - let guard = group_arc.lock().unwrap(); - finalize_write_group(&mut builder, &guard); - } + // Groups + for group_arc in &state.groups { + let guard = group_arc.lock().unwrap(); + finalize_write_group(&mut builder, &guard); + } - builder.write(&state.path).map_err(to_py_err)?; - Ok(()) + builder.write(&state.path).map_err(to_py_err)?; + Ok(()) + }) } #[cfg(test)] diff --git a/crates/clawhdf5-py/src/group.rs b/crates/clawhdf5-py/src/group.rs index c97cc17..365846b 100644 --- a/crates/clawhdf5-py/src/group.rs +++ b/crates/clawhdf5-py/src/group.rs @@ -73,16 +73,18 @@ pub(crate) fn get_item( /// Names of the group's datasets and subgroups, sorted (h5py's order). pub(crate) fn member_names(file: &clawhdf5_rs::File, path: &str) -> PyResult> { - let group = if path.is_empty() { - file.root() - } else { - file.group(path).map_err(to_py_err)? - }; - let mut names = group.datasets().map_err(to_py_err)?; - names.extend(group.groups().map_err(to_py_err)?); - names.sort_by(|a, b| a.as_bytes().cmp(b.as_bytes())); - names.dedup(); - Ok(names) + crate::no_panic(|| { + let group = if path.is_empty() { + file.root() + } else { + file.group(path).map_err(to_py_err)? + }; + let mut names = group.datasets().map_err(to_py_err)?; + names.extend(group.groups().map_err(to_py_err)?); + names.sort_by(|a, b| a.as_bytes().cmp(b.as_bytes())); + names.dedup(); + Ok(names) + }) } pub(crate) fn contains(file: &clawhdf5_rs::File, path: &str, key: &str) -> bool { diff --git a/crates/clawhdf5-py/src/lib.rs b/crates/clawhdf5-py/src/lib.rs index 4c58cd6..e3b5619 100644 --- a/crates/clawhdf5-py/src/lib.rs +++ b/crates/clawhdf5-py/src/lib.rs @@ -24,6 +24,42 @@ pub(crate) use dataset::PyDataset; pub(crate) use file::PyFile; pub(crate) use group::PyGroup; +pyo3::create_exception!( + clawhdf5, + InternalError, + pyo3::exceptions::PyRuntimeError, + "A bug in clawhdf5 met while reading or writing a file (a Rust panic, \ + caught). Derived from RuntimeError, so `except Exception` handles it." +); + +/// The text of a caught panic. +pub(crate) fn panic_text(payload: &(dyn std::any::Any + Send)) -> String { + payload + .downcast_ref::<&str>() + .map(|s| (*s).to_string()) + .or_else(|| payload.downcast_ref::().cloned()) + .unwrap_or_else(|| "unknown panic".to_string()) +} + +/// Run `f`, turning a panic in the library into [`InternalError`] instead of +/// PyO3's `PanicException` (a `BaseException`, which `except Exception` +/// does not catch). Wraps every call into the library. +pub(crate) fn no_panic(f: impl FnOnce() -> PyResult) -> PyResult { + std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)).unwrap_or_else(|p| { + Err(InternalError::new_err(format!( + "clawhdf5 internal error (please report it): {}", + panic_text(&*p) + ))) + }) +} + +/// A test hook: panics inside [`no_panic`], so the tests can check that a +/// library panic reaches Python as an ordinary exception. +#[pyfunction] +fn _panic_for_test() -> PyResult<()> { + no_panic(|| panic!("deliberate panic for the test suite")) +} + /// Convert a `clawhdf5_rs::Error` into a `PyErr`. /// /// Maps different error variants to more specific Python exception types: @@ -276,6 +312,8 @@ fn clawhdf5(m: &Bound<'_, PyModule>) -> PyResult<()> { m.add_class::()?; m.add_class::()?; m.add_class::()?; + m.add("InternalError", m.py().get_type::())?; + m.add_function(wrap_pyfunction!(_panic_for_test, m)?)?; Ok(()) } diff --git a/crates/clawhdf5-py/src/node.rs b/crates/clawhdf5-py/src/node.rs index 1c68101..0482603 100644 --- a/crates/clawhdf5-py/src/node.rs +++ b/crates/clawhdf5-py/src/node.rs @@ -35,22 +35,24 @@ pub(crate) fn name(path: &str) -> String { /// The object header of the object at `path`. pub(crate) fn header(file: &clawhdf5_rs::File, path: &str) -> PyResult { - let sb = file.superblock(); - let data = file.as_bytes(); - let addr = if path.is_empty() { - sb.root_group_address - } else { - clawhdf5_format::group_v2::resolve_path_any(data, sb, path).map_err(|e| { - PyKeyError::new_err(format!( - "Unable to open object (object '{}' doesn't exist): {e}", - name(path) - )) - })? - }; - let addr = usize::try_from(addr) - .map_err(|_| PyValueError::new_err(format!("{}: address out of range", name(path))))?; - ObjectHeader::parse(data, addr, sb.offset_size, sb.length_size) - .map_err(|e| PyValueError::new_err(format!("{}: {e}", name(path)))) + crate::no_panic(|| { + let sb = file.superblock(); + let data = file.as_bytes(); + let addr = if path.is_empty() { + sb.root_group_address + } else { + clawhdf5_format::group_v2::resolve_path_any(data, sb, path).map_err(|e| { + PyKeyError::new_err(format!( + "Unable to open object (object '{}' doesn't exist): {e}", + name(path) + )) + })? + }; + let addr = usize::try_from(addr) + .map_err(|_| PyValueError::new_err(format!("{}: address out of range", name(path))))?; + ObjectHeader::parse(data, addr, sb.offset_size, sb.length_size) + .map_err(|e| PyValueError::new_err(format!("{}: {e}", name(path)))) + }) } /// What an object header describes. @@ -115,20 +117,22 @@ pub(crate) fn exists(file: &clawhdf5_rs::File, path: &str) -> bool { /// The dataspace message of an object header. pub(crate) fn dataspace(file: &clawhdf5_rs::File, hdr: &ObjectHeader) -> PyResult { - let sb = file.superblock(); - let msg = hdr - .messages - .iter() - .find(|m| m.msg_type == MessageType::Dataspace) - .ok_or_else(|| PyValueError::new_err("object has no dataspace message"))?; - let data = clawhdf5_format::shared_message::message_data( - file.as_bytes(), - msg, - sb.offset_size, - sb.length_size, - ) - .map_err(|e| PyValueError::new_err(e.to_string()))?; - Dataspace::parse(&data, sb.length_size).map_err(|e| PyValueError::new_err(e.to_string())) + crate::no_panic(|| { + let sb = file.superblock(); + let msg = hdr + .messages + .iter() + .find(|m| m.msg_type == MessageType::Dataspace) + .ok_or_else(|| PyValueError::new_err("object has no dataspace message"))?; + let data = clawhdf5_format::shared_message::message_data( + file.as_bytes(), + msg, + sb.offset_size, + sb.length_size, + ) + .map_err(|e| PyValueError::new_err(e.to_string()))?; + Dataspace::parse(&data, sb.length_size).map_err(|e| PyValueError::new_err(e.to_string())) + }) } pub(crate) fn is_null(space: &Dataspace) -> bool { @@ -139,17 +143,19 @@ pub(crate) fn is_null(space: &Dataspace) -> bool { /// Attributes whose messages cannot be parsed are left out, as the facade's /// `attrs()` does. pub(crate) fn attributes(file: &clawhdf5_rs::File, path: &str) -> PyResult> { - let hdr = header(file, path)?; - let sb = file.superblock(); - let (mut attrs, _errors) = clawhdf5_format::attribute::extract_attributes_tolerant( - file.as_bytes(), - &hdr, - sb.offset_size, - sb.length_size, - ) - .map_err(|e| PyValueError::new_err(format!("{}: {e}", name(path))))?; - attrs.sort_by(|a, b| a.name.as_bytes().cmp(b.name.as_bytes())); - Ok(attrs) + crate::no_panic(|| { + let hdr = header(file, path)?; + let sb = file.superblock(); + let (mut attrs, _errors) = clawhdf5_format::attribute::extract_attributes_tolerant( + file.as_bytes(), + &hdr, + sb.offset_size, + sb.length_size, + ) + .map_err(|e| PyValueError::new_err(format!("{}: {e}", name(path))))?; + attrs.sort_by(|a, b| a.name.as_bytes().cmp(b.name.as_bytes())); + Ok(attrs) + }) } #[cfg(test)] diff --git a/crates/clawhdf5-py/tests/test_read_vs_h5py.py b/crates/clawhdf5-py/tests/test_read_vs_h5py.py index 9a9178d..66fc643 100644 --- a/crates/clawhdf5-py/tests/test_read_vs_h5py.py +++ b/crates/clawhdf5-py/tests/test_read_vs_h5py.py @@ -467,3 +467,47 @@ def test_threads_read_the_same_file(pair): with ThreadPoolExecutor(8) as pool: list(pool.map(work, range(8))) assert not errors, errors[:3] + + +def _v4_index_fixture(h5py, path): + """One 2-D dataset per v4 chunk index (HDF5 1.10+ layout, libver='latest').""" + data = (np.arange(37 * 23, dtype="