Merge pull request #139 from mulimoen/bugfix/handles

Revamp object identifier ownership

Author: mulimoen

.github/workflows/ci.yml

@@ -232,3 +232,15 @@ jobs:
         run: sudo apt-get install wine64 mingw-w64
       - name: Build and test
         run: env CARGO_TARGET_X86_64_PC_WINDOWS_GNU_RUNNER=wine64 cargo test --features hdf5-sys/static --target x86_64-pc-windows-gnu -- --skip test_compile_fail
+  addr_san:
+    name: Address sanitizer
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout repository
+        uses: actions/checkout@v2
+        with: {submodules: true}
+      - name: Install Rust
+        uses: actions-rs/toolchain@v1
+        with: {toolchain: nightly, profile: minimal, override: true}
+      - name: Run test with sanitizer
+        run: env RUSTFLAGS="-Z sanitizer=address" cargo test --features hdf5-sys/static --target x86_64-unknown-linux-gnu --workspace --exclude hdf5-derive

CHANGELOG.md

@@ -51,7 +51,9 @@
 - Added support for creating external links on a `Group` with `link_external`.
 - Added `Location` methods: `get_info`, `get_info_by_name`, `loc_type`, and `open_by_token`.
 - Added `Group` methods: `iter_visit`, `iter_visit_default`, `get_all_of_type`, `datasets`, `groups`, and `named_datatypes`.
-  
+- Added `Debug` for `Handle`.
+- Added method `try_borrow` on `Handle` for not taking ownership of an HDF5 object.
+
  ### Changed
 
 - Required Rust compiler version is now `1.51`.
@@ -78,6 +80,12 @@
 - `silence_errors` now work globally and will not be reset on dropping `SilenceErrors`.
 - Errors are not expanded when encountered, but only when being used for printing or by
   the library user.
+- Handles to `hdf5` identifiers are no longer tracked via a registry and is instead handled by stricter semantics of ownership.
+- The handle to a `File` will not close all objects in a `File` when dropped, but instead uses a weak file close degree. For the old behaviour see `FileCloseDegree::Strong`.
+
+### Fixed
+
+- A memory leak of handles has been identified and fixed.
 
 ## 0.7.1
 

hdf5-types/src/dyn_value.rs

@@ -702,7 +702,7 @@ impl Display for DynValue<'_> {
 
 pub struct OwnedDynValue {
     tp: TypeDescriptor,
-    buf: Vec<u8>,
+    buf: Box<[u8]>,
 }
 
 impl OwnedDynValue {
@@ -711,7 +711,7 @@ impl OwnedDynValue {
         let len = mem::size_of_val(&value);
         let buf = unsafe { std::slice::from_raw_parts(ptr, len) };
         mem::forget(value);
-        Self { tp: T::type_descriptor(), buf: buf.to_owned() }
+        Self { tp: T::type_descriptor(), buf: buf.to_owned().into_boxed_slice() }
     }
 
     pub fn get(&self) -> DynValue {
@@ -728,9 +728,40 @@ impl OwnedDynValue {
     }
 
     #[doc(hidden)]
-    pub unsafe fn from_raw(tp: TypeDescriptor, buf: Vec<u8>) -> Self {
+    pub unsafe fn from_raw(tp: TypeDescriptor, buf: Box<[u8]>) -> Self {
         Self { tp, buf }
     }
+
+    /// Cast to the concrete type
+    ///
+    /// Will fail if the type-descriptors are not equal
+    pub fn cast<T: H5Type>(mut self) -> Result<T, Self> {
+        use mem::MaybeUninit;
+        if self.tp != T::type_descriptor() {
+            return Err(self);
+        }
+        debug_assert_eq!(self.tp.size(), self.buf.len());
+        let mut out = MaybeUninit::<T>::uninit();
+        unsafe {
+            ptr::copy_nonoverlapping(
+                self.buf.as_ptr(),
+                out.as_mut_ptr().cast::<u8>(),
+                self.buf.len(),
+            );
+        }
+        // For safety we must ensure any nested structures are not live at the same time,
+        // as this could cause a double free in `dyn_drop`.
+        // We must deallocate only the top level of Self
+
+        // The zero-sized array has a special case to not drop ptr if len is zero,
+        // so `dyn_drop` of `DynArray` is a nop
+        self.tp = <[u8; 0]>::type_descriptor();
+        // We must also swap out the buffer to ensure we can create the `DynValue`
+        let mut b: Box<[u8]> = Box::new([]);
+        mem::swap(&mut self.buf, &mut b);
+
+        Ok(unsafe { out.assume_init() })
+    }
 }
 
 impl<T: H5Type> From<T> for OwnedDynValue {

src/handle.rs

@@ -1,10 +1,4 @@
-use std::collections::HashMap;
-use std::sync::Arc;
-
-use lazy_static::lazy_static;
-use parking_lot::{Mutex, RwLock};
-
-use hdf5_sys::h5i::{H5I_type_t, H5Idec_ref, H5Iget_type, H5Iinc_ref, H5Iis_valid};
+use hdf5_sys::h5i::{H5I_type_t, H5Idec_ref, H5Iget_ref, H5Iget_type, H5Iinc_ref, H5Iis_valid};
 
 use crate::internal_prelude::*;
 
@@ -20,6 +14,10 @@ pub fn get_id_type(id: hid_t) -> H5I_type_t {
     })
 }
 
+pub(crate) fn refcount(id: hid_t) -> Result<hsize_t> {
+    h5call!(H5Iget_ref(id)).map(|x| x as _)
+}
+
 pub fn is_valid_id(id: hid_t) -> bool {
     h5lock!({
         let tp = get_id_type(id);
@@ -31,77 +29,60 @@ pub fn is_valid_user_id(id: hid_t) -> bool {
     h5lock!({ H5Iis_valid(id) == 1 })
 }
 
-struct Registry {
-    registry: Mutex<HashMap<hid_t, Arc<RwLock<hid_t>>>>,
-}
-
-impl Default for Registry {
-    fn default() -> Self {
-        Self::new()
-    }
-}
-
-impl Registry {
-    pub fn new() -> Self {
-        Self { registry: Mutex::new(HashMap::new()) }
-    }
-
-    pub fn new_handle(&self, id: hid_t) -> Arc<RwLock<hid_t>> {
-        let mut registry = self.registry.lock();
-        let handle = registry.entry(id).or_insert_with(|| Arc::new(RwLock::new(id)));
-        if *handle.read() != id {
-            // an id may be left dangling by previous invalidation of a linked handle
-            *handle = Arc::new(RwLock::new(id));
-        }
-        handle.clone()
-    }
-}
-
+/// A handle to an HDF5 object
+#[derive(Debug)]
 pub struct Handle {
-    id: Arc<RwLock<hid_t>>,
+    id: hid_t,
 }
 
 impl Handle {
+    /// Create a handle from object ID, taking ownership of it
     pub fn try_new(id: hid_t) -> Result<Self> {
-        lazy_static! {
-            static ref REGISTRY: Registry = Registry::new();
-        }
         h5lock!({
             if is_valid_user_id(id) {
-                Ok(Self { id: REGISTRY.new_handle(id) })
+                Ok(Self { id })
             } else {
                 Err(From::from(format!("Invalid handle id: {}", id)))
             }
         })
     }
 
-    pub fn invalid() -> Self {
-        Self { id: Arc::new(RwLock::new(H5I_INVALID_HID)) }
+    /// Create a handle from object ID by cloning it
+    pub fn try_borrow(id: hid_t) -> Result<Self> {
+        h5lock!({
+            if is_valid_user_id(id) {
+                h5call!(H5Iinc_ref(id))?;
+                Ok(Self { id })
+            } else {
+                Err(From::from(format!("Invalid handle id: {}", id)))
+            }
+        })
     }
 
-    pub fn id(&self) -> hid_t {
-        *self.id.read()
+    pub const fn invalid() -> Self {
+        Self { id: H5I_INVALID_HID }
     }
 
-    pub fn invalidate(&self) {
-        *self.id.write() = H5I_INVALID_HID;
+    pub const fn id(&self) -> hid_t {
+        self.id
     }
 
+    /// Increment the reference count of the handle
     pub fn incref(&self) {
         if is_valid_user_id(self.id()) {
             h5lock!(H5Iinc_ref(self.id()));
         }
     }
 
+    /// Decrease the reference count of the handle
+    ///
+    /// Note: This function should only be used if `incref` has been
+    /// previously called.
     pub fn decref(&self) {
         h5lock!({
             if self.is_valid_id() {
                 H5Idec_ref(self.id());
             }
-            // must invalidate all linked IDs because the library reuses them internally
-            if !self.is_valid_user_id() && !self.is_valid_id() {
-                self.invalidate();
-            }
         });
     }
 
@@ -115,19 +96,15 @@ impl Handle {
         is_valid_id(self.id())
     }
 
-    pub fn decref_full(&self) {
-        while self.is_valid_user_id() {
-            self.decref();
-        }
+    /// Return the reference count of the object
+    pub fn refcount(&self) -> u32 {
+        refcount(self.id).unwrap_or(0) as _
     }
 }
 
 impl Clone for Handle {
     fn clone(&self) -> Self {
-        h5lock!({
-            self.incref();
-            Self::try_new(self.id()).unwrap_or_else(|_| Self::invalid())
-        })
+        Self::try_borrow(self.id()).unwrap_or_else(|_| Self::invalid())
     }
 }