diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 51c0d2ea7..46ca1398d 100644 --- a/.github/workflows/ci.yml +++ b/.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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 14c046cc0..a5ff4393b 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/hdf5-types/src/dyn_value.rs b/hdf5-types/src/dyn_value.rs index 6ea8bd4c0..b63870889 100644 --- a/hdf5-types/src/dyn_value.rs +++ b/hdf5-types/src/dyn_value.rs @@ -702,7 +702,7 @@ impl Display for DynValue<'_> { pub struct OwnedDynValue { tp: TypeDescriptor, - buf: Vec, + 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) -> 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(mut self) -> Result { + 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::::uninit(); + unsafe { + ptr::copy_nonoverlapping( + self.buf.as_ptr(), + out.as_mut_ptr().cast::(), + 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 From for OwnedDynValue { diff --git a/src/handle.rs b/src/handle.rs index a4485649b..3937434f1 100644 --- a/src/handle.rs +++ b/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 { + 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>>>, -} - -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> { - 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>, + id: hid_t, } impl Handle { + /// Create a handle from object ID, taking ownership of it pub fn try_new(id: hid_t) -> Result { - 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 { + 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()) } } diff --git a/src/hl/file.rs b/src/hl/file.rs index d8dd8cc79..007070b84 100644 --- a/src/hl/file.rs +++ b/src/hl/file.rs @@ -1,12 +1,12 @@ use std::fmt::{self, Debug}; +use std::mem; use std::ops::Deref; use std::path::Path; use hdf5_sys::h5f::{ H5Fclose, H5Fcreate, H5Fflush, H5Fget_access_plist, H5Fget_create_plist, H5Fget_filesize, H5Fget_freespace, H5Fget_intent, H5Fget_obj_count, H5Fget_obj_ids, H5Fopen, H5F_ACC_DEFAULT, - H5F_ACC_EXCL, H5F_ACC_RDONLY, H5F_ACC_RDWR, H5F_ACC_TRUNC, H5F_OBJ_ALL, H5F_OBJ_FILE, - H5F_SCOPE_LOCAL, + H5F_ACC_EXCL, H5F_ACC_RDONLY, H5F_ACC_RDWR, H5F_ACC_TRUNC, H5F_SCOPE_LOCAL, }; use crate::hl::plist::{ @@ -133,6 +133,7 @@ impl File { } /// Returns objects IDs of the contained objects. NOTE: these are borrowed references. + #[allow(unused)] fn get_obj_ids(&self, types: c_uint) -> Vec { h5lock!({ let count = h5call!(H5Fget_obj_count(self.id(), types)).unwrap_or(0) as size_t; @@ -151,26 +152,11 @@ impl File { } /// Closes the file and invalidates all open handles for contained objects. - pub fn close(self) { - h5lock!({ - let file_ids = self.get_obj_ids(H5F_OBJ_FILE); - let object_ids = self.get_obj_ids(H5F_OBJ_ALL & !H5F_OBJ_FILE); - for file_id in &file_ids { - if let Ok(handle) = Handle::try_new(*file_id) { - handle.decref_full(); - } - } - for object_id in &object_ids { - if let Ok(handle) = Handle::try_new(*object_id) { - handle.decref_full(); - } - } - H5Fclose(self.id()); - while self.is_valid() { - self.0.decref(); - } - self.0.decref(); - }); + pub fn close(self) -> Result<()> { + let id = self.id(); + // Ensure we only decref once + mem::forget(self.0); + h5call!(H5Fclose(id)).map(|_| ()) } /// Returns a copy of the file access property list. @@ -500,29 +486,87 @@ pub mod tests { } #[test] - pub fn test_close_automatic() { - // File going out of scope should just close its own handle + fn test_strong_close() { + use crate::hl::plist::file_access::FileCloseDegree; with_tmp_path(|path| { - let file = File::create(&path).unwrap(); + let file = File::with_options() + .with_fapl(|fapl| fapl.fclose_degree(FileCloseDegree::Strong)) + .create(&path) + .unwrap(); + assert_eq!(file.refcount(), 1); + let fileid = file.id(); + let group = file.create_group("foo").unwrap(); + assert_eq!(file.refcount(), 1); + assert_eq!(group.refcount(), 1); + let file_copy = group.file().unwrap(); + assert_eq!(group.refcount(), 1); + assert_eq!(file.refcount(), 2); + assert_eq!(file_copy.refcount(), 2); + drop(file); - assert!(group.is_valid()); - assert!(file_copy.is_valid()); + assert_eq!(crate::handle::refcount(fileid).unwrap(), 1); + assert_eq!(group.refcount(), 1); + assert_eq!(file_copy.refcount(), 1); + + h5lock!({ + // Lock to ensure fileid does not get overwritten + let groupid = group.id(); + drop(file_copy); + assert!(crate::handle::refcount(fileid).is_err()); + assert!(crate::handle::refcount(groupid).is_err()); + assert!(!group.is_valid()); + drop(group); + }); }); } #[test] - pub fn test_close_manual() { - // File::close() should close handles of all related objects + fn test_weak_close() { + use crate::hl::plist::file_access::FileCloseDegree; + with_tmp_path(|path| { + let file = File::with_options() + .with_fapl(|fapl| fapl.fclose_degree(FileCloseDegree::Weak)) + .create(&path) + .unwrap(); + assert_eq!(file.refcount(), 1); + let fileid = file.id(); + + let group = file.create_group("foo").unwrap(); + assert_eq!(file.refcount(), 1); + assert_eq!(group.refcount(), 1); + + let file_copy = group.file().unwrap(); + assert_eq!(group.refcount(), 1); + assert_eq!(file.refcount(), 2); + assert_eq!(file_copy.refcount(), 2); + + drop(file); + assert_eq!(crate::handle::refcount(fileid).unwrap(), 1); + assert_eq!(group.refcount(), 1); + assert_eq!(file_copy.refcount(), 1); + + h5lock!({ + // Lock to ensure fileid does not get overwritten + drop(file_copy); + assert!(crate::handle::refcount(fileid).is_err()); + }); + assert_eq!(group.refcount(), 1); + }); + } + + #[test] + pub fn test_close_automatic() { + // File going out of scope should just close its own handle with_tmp_path(|path| { let file = File::create(&path).unwrap(); let group = file.create_group("foo").unwrap(); let file_copy = group.file().unwrap(); - file.close(); - assert!(!group.is_valid()); - assert!(!file_copy.is_valid()); - }) + drop(file); + assert!(group.is_valid()); + assert!(file_copy.is_valid()); + }); } #[test] @@ -532,7 +576,7 @@ pub mod tests { FileBuilder::new().with_fapl(|p| p.core_filebacked(false)).create(&path).unwrap(); file.create_group("x").unwrap(); assert!(file.is_valid()); - file.close(); + file.close().unwrap(); assert!(fs::metadata(&path).is_err()); assert_err!( FileBuilder::new().with_fapl(|p| p.core()).open(&path), @@ -548,7 +592,7 @@ pub mod tests { FileBuilder::new().with_fapl(|p| p.core_filebacked(true)).create(&path).unwrap(); assert!(file.is_valid()); file.create_group("bar").unwrap(); - file.close(); + file.close().unwrap(); assert!(fs::metadata(&path).is_ok()); File::open(&path).unwrap().group("bar").unwrap(); }) @@ -594,8 +638,8 @@ pub mod tests { let path = dir.join("qwe.h5"); let file = File::create(&path).unwrap(); assert_eq!(format!("{:?}", file), ""); - let root = file.file().unwrap(); - file.close(); + file.close().unwrap(); + let root = File::from_handle(Handle::invalid()); assert_eq!(format!("{:?}", root), ""); let file = File::open(&path).unwrap(); assert_eq!(format!("{:?}", file), ""); diff --git a/src/hl/group.rs b/src/hl/group.rs index 7bb759690..2747d9a3d 100644 --- a/src/hl/group.rs +++ b/src/hl/group.rs @@ -323,8 +323,7 @@ impl Group { unsafe { name.as_ref().expect("iter_visit: null name ptr") }; let name = unsafe { std::ffi::CStr::from_ptr(name) }; let info = unsafe { info.as_ref().expect("iter_vist: null info ptr") }; - let handle = Handle::try_new(id).expect("iter_visit: unable to create a handle"); - handle.incref(); + let handle = Handle::try_borrow(id).expect("iter_visit: unable to create a handle"); let group = Group::from_handle(handle); if (vtable.f)(&group, name.to_string_lossy().as_ref(), info.into(), vtable.d) { 0 @@ -410,7 +409,12 @@ pub mod tests { #[test] pub fn test_debug() { - with_tmp_file(|file| { + use crate::hl::plist::file_access::FileCloseDegree; + with_tmp_path(|path| { + let file = File::with_options() + .with_fapl(|fapl| fapl.fclose_degree(FileCloseDegree::Strong)) + .create(&path) + .unwrap(); file.create_group("a/b/c").unwrap(); file.create_group("/a/d").unwrap(); let a = file.group("a").unwrap(); @@ -419,8 +423,13 @@ pub mod tests { assert_eq!(format!("{:?}", a), ""); assert_eq!(format!("{:?}", ab), ""); assert_eq!(format!("{:?}", abc), ""); - file.close(); - assert_eq!(format!("{:?}", a), ""); + h5lock!({ + file.close().unwrap(); + assert_eq!(format!("{:?}", a), ""); + drop(a); + drop(ab); + drop(abc); + }) }) } diff --git a/src/hl/object.rs b/src/hl/object.rs index afd8e1e59..abddac618 100644 --- a/src/hl/object.rs +++ b/src/hl/object.rs @@ -1,7 +1,5 @@ use std::fmt::{self, Debug}; -use hdf5_sys::h5i::H5Iget_ref; - use crate::internal_prelude::*; /// Any HDF5 object that can be referenced through an identifier. @@ -37,11 +35,7 @@ impl Object { /// Returns reference count if the handle is valid and 0 otherwise. pub fn refcount(&self) -> u32 { - if self.is_valid() { - h5call!(H5Iget_ref(self.id())).unwrap_or(0) as _ - } else { - 0 - } + self.handle().refcount() } /// Returns `true` if the object has a valid unlocked identifier (`false` for pre-defined @@ -56,13 +50,7 @@ impl Object { } pub(crate) fn try_borrow(&self) -> Result { - h5lock!({ - let handle = Handle::try_new(self.id()); - if let Ok(ref handle) = handle { - handle.incref(); - } - handle - }) + Handle::try_borrow(self.id()) } } @@ -130,15 +118,19 @@ pub mod tests { obj.decref(); assert_eq!(obj.refcount(), 1); obj.decref(); - obj.decref(); - assert_eq!(obj.refcount(), 0); - assert!(!obj.is_valid()); - assert!(!is_valid_user_id(obj.id())); - assert!(!is_valid_id(obj.id())); + h5lock!({ + obj.decref(); + assert_eq!(obj.refcount(), 0); + assert!(!obj.is_valid()); + assert!(!is_valid_user_id(obj.id())); + assert!(!is_valid_id(obj.id())); + drop(obj); + }); } #[test] pub fn test_incref_decref_drop() { + use std::mem::ManuallyDrop; let mut obj = TestObject::from_id(h5call!(H5Pcreate(*H5P_FILE_ACCESS)).unwrap()).unwrap(); let obj_id = obj.id(); obj = TestObject::from_id(h5call!(H5Pcreate(*H5P_FILE_ACCESS)).unwrap()).unwrap(); @@ -148,18 +140,40 @@ pub mod tests { assert!(is_valid_id(obj.id())); assert!(is_valid_user_id(obj.id())); assert_eq!(obj.refcount(), 1); - let mut obj2 = TestObject::from_id(obj.id()).unwrap(); + + let obj2 = TestObject::from_id(obj.id()).unwrap(); obj2.incref(); assert_eq!(obj.refcount(), 2); assert_eq!(obj2.refcount(), 2); + drop(obj2); assert!(obj.is_valid()); assert_eq!(obj.refcount(), 1); - obj2 = TestObject::from_id(obj.id()).unwrap(); + + // obj is already owned, we must ensure we do not call drop on this without + // an incref + let mut obj2 = ManuallyDrop::new(TestObject::from_id(obj.id()).unwrap()); + assert_eq!(obj.refcount(), 1); + obj2.incref(); - obj.decref(); - obj.decref(); - assert_eq!(obj.id(), H5I_INVALID_HID); - assert_eq!(obj2.id(), H5I_INVALID_HID); + // We can now take, as we have exactly two handles + let obj2 = unsafe { ManuallyDrop::take(&mut obj2) }; + + h5lock!({ + // We must hold a lock here to prevent another thread creating an object + // with the same identifier as the one we just owned. Failing to do this + // might lead to the wrong object being dropped. + obj.decref(); + obj.decref(); + // We here have to dangling identifiers stored in obj and obj2. As this part + // is locked we know some other object is not going to created with these + // identifiers + assert!(!obj.is_valid()); + assert!(!obj2.is_valid()); + // By manually dropping we don't close some other unrelated objects. + // Dropping/closing an invalid object is allowed + drop(obj); + drop(obj2); + }); } } diff --git a/src/hl/plist/dataset_create.rs b/src/hl/plist/dataset_create.rs index 9baad6e36..151fcc3c9 100644 --- a/src/hl/plist/dataset_create.rs +++ b/src/hl/plist/dataset_create.rs @@ -1,7 +1,6 @@ //! Dataset creation properties. use std::fmt::{self, Debug}; -use std::mem; use std::ops::Deref; use std::ptr; @@ -722,11 +721,11 @@ impl DatasetCreate { FillValue::Default | FillValue::UserDefined => { let dtype = Datatype::from_descriptor(tp)?; let mut buf: Vec = Vec::with_capacity(tp.size()); + h5try!(H5Pget_fill_value(self.id(), dtype.id(), buf.as_mut_ptr().cast())); unsafe { buf.set_len(tp.size()); } - h5try!(H5Pget_fill_value(self.id(), dtype.id(), buf.as_mut_ptr().cast())); - Ok(Some(unsafe { OwnedDynValue::from_raw(tp.clone(), buf) })) + Ok(Some(unsafe { OwnedDynValue::from_raw(tp.clone(), buf.into_boxed_slice()) })) } FillValue::Undefined => Ok(None), } @@ -739,13 +738,13 @@ impl DatasetCreate { #[doc(hidden)] pub fn get_fill_value_as(&self) -> Result> { let dtype = Datatype::from_type::()?; - Ok(self.get_fill_value(&dtype.to_descriptor()?)?.map(|value| unsafe { - let mut out: T = mem::zeroed(); - let buf = value.get_buf(); - ptr::copy_nonoverlapping(buf.as_ptr(), (&mut out as *mut T).cast(), buf.len()); - mem::forget(value); - out - })) + self.get_fill_value(&dtype.to_descriptor()?)? + .map(|value| { + value + .cast::() + .map_err(|_| "The fill value and requested types are not equal".into()) + }) + .transpose() } pub fn fill_value_as(&self) -> Option { diff --git a/src/hl/plist/file_access.rs b/src/hl/plist/file_access.rs index b3809b88e..df3727b31 100644 --- a/src/hl/plist/file_access.rs +++ b/src/hl/plist/file_access.rs @@ -1026,6 +1026,11 @@ impl FileAccessBuilder { Ok(builder) } + /// Sets the file close degree + /// + /// If called with `FileCloseDegree::Strong`, the programmer is responsible + /// for closing all items before closing the file. Failure to do so might + /// invalidate newly created objects. pub fn fclose_degree(&mut self, fc_degree: FileCloseDegree) -> &mut Self { self.fclose_degree = Some(fc_degree); self @@ -1374,6 +1379,8 @@ impl FileAccessBuilder { if let Some(v) = self.chunk_cache { h5try!(H5Pset_cache(id, 0, v.nslots as _, v.nbytes as _, v.w0 as _)); } + // The default is to use CLOSE_SEMI or CLOSE_WEAK, depending on VFL driver. + // Both of these are unproblematic for our ownership if let Some(v) = self.fclose_degree { h5try!(H5Pset_fclose_degree(id, v.into())); } @@ -1536,6 +1543,9 @@ impl FileAccess { } *layout.get_mut(i - 1) = 0xff - mapping[j]; } + for &memb_name in &memb_name { + crate::util::h5_free_memory(memb_name as *mut _); + } let relax = relax > 0; let drv = MultiDriver { files, layout, relax }; drv.validate().map(|_| drv)