fix(py): a panic in the library raises clawhdf5.InternalError, not PanicException

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) <[email protected]>
2026-09-26 08:52:55 -05:00
parent 3bcd443e63
commit 24412a0e59
8 changed files with 289 additions and 176 deletions
@@ -191,46 +191,48 @@ fn attr_to_py<'py>(
    file: &clawhdf5_rs::File,
    attr: &AttributeMessage,
 ) -> PyResult<Bound<'py, PyAny>> {
-    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<usize> = 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<usize> = 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 {
@@ -42,25 +42,27 @@ impl PyDataset {
        file: Arc<clawhdf5_rs::File>,
        path: String,
    ) -> PyResult<Self> {
-        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<usize>)> = 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<usize>)> = 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<clawhdf5_rs::Error> 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<Bound<'py, PyAny>> {
-        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<Option<u64>> = 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<Option<u64>> = 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.
@@ -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)]
@@ -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<Vec<String>> {
-    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 {
@@ -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::<String>().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<T>(f: impl FnOnce() -> PyResult<T>) -> PyResult<T> {
+    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::<PyGroup>()?;
    m.add_class::<PyAttrs>()?;
    m.add_class::<PyEmpty>()?;
+    m.add("InternalError", m.py().get_type::<InternalError>())?;
+    m.add_function(wrap_pyfunction!(_panic_for_test, m)?)?;
    Ok(())
 }

@@ -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<ObjectHeader> {
-    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<Dataspace> {
-    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<Vec<AttributeMessage>> {
-    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)]