From 739f50d69b76190c653a7ce717ce66977970a6cf Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Wed, 17 Feb 2021 17:17:28 +0100 Subject: [PATCH 01/19] Remove Registry --- src/handle.rs | 69 +++++++--------------------- src/hl/file.rs | 116 ++++++++++++++++++++++++++++++++--------------- src/hl/group.rs | 13 ++++-- src/hl/object.rs | 32 +++++++------ 4 files changed, 124 insertions(+), 106 deletions(-) diff --git a/src/handle.rs b/src/handle.rs index a4485649b..f9894bafb 100644 --- a/src/handle.rs +++ b/src/handle.rs @@ -1,9 +1,3 @@ -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 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!(hdf5_sys::h5i::H5Iget_ref(id)).map(|x| x as _) +} + pub fn is_valid_id(id: hid_t) -> bool { h5lock!({ let tp = get_id_type(id); @@ -31,44 +29,17 @@ 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 a `hdf5` object pub struct Handle { - id: Arc>, + id: hid_t, } impl Handle { + /// Take ownership of the object id 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))) } @@ -76,15 +47,11 @@ impl Handle { } pub fn invalid() -> Self { - Self { id: Arc::new(RwLock::new(H5I_INVALID_HID)) } + Self { id: H5I_INVALID_HID } } pub fn id(&self) -> hid_t { - *self.id.read() - } - - pub fn invalidate(&self) { - *self.id.write() = H5I_INVALID_HID; + self.id } pub fn incref(&self) { @@ -93,16 +60,14 @@ impl Handle { } } - pub fn decref(&self) { + /// An object should not be decreffed unless it has an + /// associated incref + pub unsafe 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(); - } - }); + }) } /// Returns `true` if the object has a valid unlocked identifier (`false` for pre-defined @@ -115,10 +80,8 @@ impl Handle { is_valid_id(self.id()) } - pub fn decref_full(&self) { - while self.is_valid_user_id() { - self.decref(); - } + pub(crate) fn refcount(&self) -> u32 { + refcount(self.id).unwrap_or(0) as u32 } } diff --git a/src/hl/file.rs b/src/hl/file.rs index d8dd8cc79..57f403de4 100644 --- a/src/hl/file.rs +++ b/src/hl/file.rs @@ -5,8 +5,7 @@ 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 +132,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 +151,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 + std::mem::forget(self.0); + h5call!(H5Fclose(id)).map(|_| ()) } /// Returns a copy of the file access property list. @@ -500,29 +485,86 @@ 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()); + }); }); } #[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 +574,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 +590,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 +636,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..2fb3dbe0f 100644 --- a/src/hl/group.rs +++ b/src/hl/group.rs @@ -410,7 +410,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 +424,10 @@ 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), ""); + }) }) } diff --git a/src/hl/object.rs b/src/hl/object.rs index afd8e1e59..3fd1bdc16 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 @@ -105,7 +99,7 @@ pub mod tests { } fn decref(&self) { - self.0.decref() + unsafe { self.0.decref() } } } @@ -148,18 +142,30 @@ 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 = std::mem::ManuallyDrop::new(TestObject::from_id(obj.id()).unwrap()); + assert_eq!(obj.refcount(), 1); + obj2.incref(); + // We can now take, as we have exactly two handles + let obj2 = unsafe { std::mem::ManuallyDrop::take(&mut obj2) }; + obj.decref(); - obj.decref(); - assert_eq!(obj.id(), H5I_INVALID_HID); - assert_eq!(obj2.id(), H5I_INVALID_HID); + h5lock!({ + obj.decref(); + assert!(!obj.is_valid()); + assert!(!obj2.is_valid()); + }); } } From 22ee768d9a4dbe8edd3fc098da1ec3f528965938 Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Wed, 17 Feb 2021 18:17:24 +0100 Subject: [PATCH 02/19] Ensure drops don't close unrelated objects --- src/hl/file.rs | 1 + src/hl/group.rs | 3 +++ src/hl/object.rs | 15 ++++++++++----- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/hl/file.rs b/src/hl/file.rs index 57f403de4..0376f9e67 100644 --- a/src/hl/file.rs +++ b/src/hl/file.rs @@ -516,6 +516,7 @@ pub mod tests { assert!(crate::handle::refcount(fileid).is_err()); assert!(crate::handle::refcount(groupid).is_err()); assert!(!group.is_valid()); + drop(group); }); }); } diff --git a/src/hl/group.rs b/src/hl/group.rs index 2fb3dbe0f..3dd37b8eb 100644 --- a/src/hl/group.rs +++ b/src/hl/group.rs @@ -427,6 +427,9 @@ pub mod tests { 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 3fd1bdc16..a1e292510 100644 --- a/src/hl/object.rs +++ b/src/hl/object.rs @@ -124,11 +124,14 @@ 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] @@ -166,6 +169,8 @@ pub mod tests { obj.decref(); assert!(!obj.is_valid()); assert!(!obj2.is_valid()); + drop(obj); + drop(obj2); }); } } From 31c5e62eee390989ad09094281f6dd99a50be86a Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Fri, 19 Feb 2021 17:31:51 +0100 Subject: [PATCH 03/19] Document file close degree --- src/handle.rs | 11 +++++++---- src/hl/object.rs | 12 ++++++++++-- src/hl/plist/file_access.rs | 7 +++++++ 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/src/handle.rs b/src/handle.rs index f9894bafb..2ed45956e 100644 --- a/src/handle.rs +++ b/src/handle.rs @@ -54,15 +54,17 @@ impl Handle { 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())); } } - /// An object should not be decreffed unless it has an - /// associated incref - pub unsafe fn decref(&self) { + /// Decrease the reference count of the handle + /// + /// This function should only be used if `incref` has been used + pub fn decref(&self) { h5lock!({ if self.is_valid_id() { H5Idec_ref(self.id()); @@ -80,7 +82,8 @@ impl Handle { is_valid_id(self.id()) } - pub(crate) fn refcount(&self) -> u32 { + /// Return the reference count of the object + pub fn refcount(&self) -> u32 { refcount(self.id).unwrap_or(0) as u32 } } diff --git a/src/hl/object.rs b/src/hl/object.rs index a1e292510..416962604 100644 --- a/src/hl/object.rs +++ b/src/hl/object.rs @@ -99,7 +99,7 @@ pub mod tests { } fn decref(&self) { - unsafe { self.0.decref() } + self.0.decref() } } @@ -164,11 +164,19 @@ pub mod tests { // We can now take, as we have exactly two handles let obj2 = unsafe { std::mem::ManuallyDrop::take(&mut obj2) }; - obj.decref(); 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/file_access.rs b/src/hl/plist/file_access.rs index b3809b88e..482f65e25 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())); } From 8e327e2aa715f8cbe168c8964ae82081942c6c9a Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Fri, 19 Mar 2021 20:15:10 +0100 Subject: [PATCH 04/19] Add address sanitizer to CI --- .github/workflows/ci.yml | 12 ++++++++++++ 1 file changed, 12 insertions(+) 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 From 4642da128cec35b794627083f236f39d15749f64 Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Fri, 19 Mar 2021 20:15:37 +0100 Subject: [PATCH 05/19] Fix memory leak --- src/hl/plist/file_access.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/hl/plist/file_access.rs b/src/hl/plist/file_access.rs index 482f65e25..df3727b31 100644 --- a/src/hl/plist/file_access.rs +++ b/src/hl/plist/file_access.rs @@ -1543,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) From 55b084cb8f9ffc0c34b64d3ff3189f90fca316f2 Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Wed, 7 Apr 2021 19:48:28 +0200 Subject: [PATCH 06/19] Add entry to CHANGELOG --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 14c046cc0..261110e04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -79,6 +79,10 @@ - Errors are not expanded when encountered, but only when being used for printing or by the library user. +### Fixed + +- A memory leak of handles has been identified and fixed. + ## 0.7.1 ### Added From bc19ab23c579118f4c7dc8f8213560af37161951 Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Wed, 7 Apr 2021 21:26:41 +0200 Subject: [PATCH 07/19] Fix memory leak of DynValue --- hdf5-types/src/dyn_value.rs | 16 +++++++++++++--- src/hl/plist/dataset_create.rs | 14 ++++++++------ 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/hdf5-types/src/dyn_value.rs b/hdf5-types/src/dyn_value.rs index 6ea8bd4c0..c123c3183 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,19 @@ 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 } } + + #[doc(hidden)] + /// Use this if the values should still be used after the buffer has been + /// copied to the concrete type + pub fn drop_nonrecursive(mut self) { + // We can't take ownership of the contents of self, + // but we can replace the items with a zero-sized object + self.tp = <[u8; 0] as H5Type>::type_descriptor(); + self.buf = Box::new([]); + } } impl From for OwnedDynValue { diff --git a/src/hl/plist/dataset_create.rs b/src/hl/plist/dataset_create.rs index 9baad6e36..7f857a3b6 100644 --- a/src/hl/plist/dataset_create.rs +++ b/src/hl/plist/dataset_create.rs @@ -722,11 +722,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), } @@ -740,11 +740,13 @@ impl DatasetCreate { 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 mut out: mem::MaybeUninit = mem::MaybeUninit::uninit(); let buf = value.get_buf(); - ptr::copy_nonoverlapping(buf.as_ptr(), (&mut out as *mut T).cast(), buf.len()); - mem::forget(value); - out + ptr::copy_nonoverlapping(buf.as_ptr(), out.as_mut_ptr().cast(), buf.len()); + // Drop the Box<[u8]>, but not subfields + value.drop_nonrecursive(); + // We now have exclusive access to all subfields + out.assume_init() })) } From 469519382e634dbd3cf92e1fc6462adcaf5e16b9 Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Sat, 10 Apr 2021 11:26:06 +0200 Subject: [PATCH 08/19] Add cast to OwnedDynValue --- hdf5-types/src/dyn_value.rs | 37 ++++++++++++++++++++++++++-------- src/hl/plist/dataset_create.rs | 17 +++++++--------- 2 files changed, 36 insertions(+), 18 deletions(-) diff --git a/hdf5-types/src/dyn_value.rs b/hdf5-types/src/dyn_value.rs index c123c3183..2ca8d532e 100644 --- a/hdf5-types/src/dyn_value.rs +++ b/hdf5-types/src/dyn_value.rs @@ -732,14 +732,35 @@ impl OwnedDynValue { Self { tp, buf } } - #[doc(hidden)] - /// Use this if the values should still be used after the buffer has been - /// copied to the concrete type - pub fn drop_nonrecursive(mut self) { - // We can't take ownership of the contents of self, - // but we can replace the items with a zero-sized object - self.tp = <[u8; 0] as H5Type>::type_descriptor(); - self.buf = Box::new([]); + /// Cast to the concrete type + /// + /// Will fail if the type-descriptors are not equal + pub fn cast(mut self) -> Result { + use std::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([]); + std::mem::swap(&mut self.buf, &mut b); + + Ok(unsafe { out.assume_init() }) } } diff --git a/src/hl/plist/dataset_create.rs b/src/hl/plist/dataset_create.rs index 7f857a3b6..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; @@ -739,15 +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: mem::MaybeUninit = mem::MaybeUninit::uninit(); - let buf = value.get_buf(); - ptr::copy_nonoverlapping(buf.as_ptr(), out.as_mut_ptr().cast(), buf.len()); - // Drop the Box<[u8]>, but not subfields - value.drop_nonrecursive(); - // We now have exclusive access to all subfields - out.assume_init() - })) + 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 { From fde768e425f885e4a2db3cffe4939ce50dfd1b0f Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Tue, 17 Aug 2021 08:30:01 +0000 Subject: [PATCH 09/19] Clippy lint --- src/handle.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/handle.rs b/src/handle.rs index 2ed45956e..8034fd7da 100644 --- a/src/handle.rs +++ b/src/handle.rs @@ -69,7 +69,7 @@ impl Handle { if self.is_valid_id() { H5Idec_ref(self.id()); } - }) + }); } /// Returns `true` if the object has a valid unlocked identifier (`false` for pre-defined From 9703771b72c28a6c7339ec7ada18e21ef4369f22 Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Wed, 6 Oct 2021 19:49:18 +0000 Subject: [PATCH 10/19] Add entry to changelog --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 261110e04..81b4b1a62 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,7 +51,7 @@ - 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`. - + ### Changed - Required Rust compiler version is now `1.51`. @@ -78,6 +78,7 @@ - `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. ### Fixed From 3050354e53de38a6c6e21ad29d3671587714347b Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Wed, 13 Oct 2021 18:31:08 +0000 Subject: [PATCH 11/19] Remove unnecessary prefix --- hdf5-types/src/dyn_value.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hdf5-types/src/dyn_value.rs b/hdf5-types/src/dyn_value.rs index 2ca8d532e..b63870889 100644 --- a/hdf5-types/src/dyn_value.rs +++ b/hdf5-types/src/dyn_value.rs @@ -736,7 +736,7 @@ impl OwnedDynValue { /// /// Will fail if the type-descriptors are not equal pub fn cast(mut self) -> Result { - use std::mem::MaybeUninit; + use mem::MaybeUninit; if self.tp != T::type_descriptor() { return Err(self); } @@ -758,7 +758,7 @@ impl OwnedDynValue { 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([]); - std::mem::swap(&mut self.buf, &mut b); + mem::swap(&mut self.buf, &mut b); Ok(unsafe { out.assume_init() }) } From 2e5f8975a7cf2a9a62ec2a6d7645c4be1c61da23 Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Wed, 13 Oct 2021 18:32:29 +0000 Subject: [PATCH 12/19] Import ManuallyDrop in function --- src/hl/object.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/hl/object.rs b/src/hl/object.rs index 416962604..c0b4970e0 100644 --- a/src/hl/object.rs +++ b/src/hl/object.rs @@ -136,6 +136,7 @@ pub mod tests { #[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(); @@ -157,12 +158,12 @@ pub mod tests { // obj is already owned, we must ensure we do not call drop on this without // an incref - let mut obj2 = std::mem::ManuallyDrop::new(TestObject::from_id(obj.id()).unwrap()); + let mut obj2 = ManuallyDrop::new(TestObject::from_id(obj.id()).unwrap()); assert_eq!(obj.refcount(), 1); obj2.incref(); // We can now take, as we have exactly two handles - let obj2 = unsafe { std::mem::ManuallyDrop::take(&mut obj2) }; + let obj2 = unsafe { ManuallyDrop::take(&mut obj2) }; h5lock!({ // We must hold a lock here to prevent another thread creating an object From 573f5963b12c45106607aaebfe7003f69e734cbe Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Wed, 13 Oct 2021 18:36:30 +0000 Subject: [PATCH 13/19] Import std::mem --- src/hl/file.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/hl/file.rs b/src/hl/file.rs index 0376f9e67..007070b84 100644 --- a/src/hl/file.rs +++ b/src/hl/file.rs @@ -1,4 +1,5 @@ use std::fmt::{self, Debug}; +use std::mem; use std::ops::Deref; use std::path::Path; @@ -154,7 +155,7 @@ impl File { pub fn close(self) -> Result<()> { let id = self.id(); // Ensure we only decref once - std::mem::forget(self.0); + mem::forget(self.0); h5call!(H5Fclose(id)).map(|_| ()) } From 92b9ebbcb1156a3a53809b8dee70cc0a7e39107f Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Wed, 13 Oct 2021 18:42:06 +0000 Subject: [PATCH 14/19] Implicit casting with as _ --- src/handle.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/handle.rs b/src/handle.rs index 8034fd7da..f0bd64a77 100644 --- a/src/handle.rs +++ b/src/handle.rs @@ -84,7 +84,7 @@ impl Handle { /// Return the reference count of the object pub fn refcount(&self) -> u32 { - refcount(self.id).unwrap_or(0) as u32 + refcount(self.id).unwrap_or(0) as _ } } From 095aa373eb568f0e71edb51e5d599235368486d0 Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Wed, 13 Oct 2021 19:02:24 +0000 Subject: [PATCH 15/19] Clarify behaviour in README for closing a File --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 81b4b1a62..d36a26e77 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -79,6 +79,7 @@ - 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 From 2a13aebd92f278164773253b5b55adaf87caba2e Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Wed, 13 Oct 2021 19:48:01 +0000 Subject: [PATCH 16/19] Minor changes following review --- src/handle.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/handle.rs b/src/handle.rs index f0bd64a77..eccdd6d4b 100644 --- a/src/handle.rs +++ b/src/handle.rs @@ -1,4 +1,4 @@ -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::*; @@ -15,7 +15,7 @@ pub fn get_id_type(id: hid_t) -> H5I_type_t { } pub(crate) fn refcount(id: hid_t) -> Result { - h5call!(hdf5_sys::h5i::H5Iget_ref(id)).map(|x| x as _) + h5call!(H5Iget_ref(id)).map(|x| x as _) } pub fn is_valid_id(id: hid_t) -> bool { @@ -29,13 +29,14 @@ pub fn is_valid_user_id(id: hid_t) -> bool { h5lock!({ H5Iis_valid(id) == 1 }) } -/// A handle to a `hdf5` object +/// A handle to an HDF5 object +#[derive(Debug)] pub struct Handle { id: hid_t, } impl Handle { - /// Take ownership of the object id + /// Create a handle from object ID, taking ownership of it pub fn try_new(id: hid_t) -> Result { h5lock!({ if is_valid_user_id(id) { @@ -46,11 +47,11 @@ impl Handle { }) } - pub fn invalid() -> Self { + pub const fn invalid() -> Self { Self { id: H5I_INVALID_HID } } - pub fn id(&self) -> hid_t { + pub const fn id(&self) -> hid_t { self.id } @@ -63,7 +64,8 @@ impl Handle { /// Decrease the reference count of the handle /// - /// This function should only be used if `incref` has been used + /// Note: This function should only be used if `incref` has been + /// previously called. pub fn decref(&self) { h5lock!({ if self.is_valid_id() { From 13adc7573803e78f9f57e640854ee9d1e5eb66fd Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Wed, 13 Oct 2021 19:58:01 +0000 Subject: [PATCH 17/19] Add try_borrow for Handle --- src/handle.rs | 12 ++++++++++++ src/hl/group.rs | 3 +-- src/hl/object.rs | 8 +------- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/handle.rs b/src/handle.rs index eccdd6d4b..6b77de6d8 100644 --- a/src/handle.rs +++ b/src/handle.rs @@ -47,6 +47,18 @@ impl Handle { }) } + /// 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 const fn invalid() -> Self { Self { id: H5I_INVALID_HID } } diff --git a/src/hl/group.rs b/src/hl/group.rs index 3dd37b8eb..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 diff --git a/src/hl/object.rs b/src/hl/object.rs index c0b4970e0..abddac618 100644 --- a/src/hl/object.rs +++ b/src/hl/object.rs @@ -50,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()) } } From 105b53ffead6a010c9e457e1c16660d96d0f57e5 Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Wed, 13 Oct 2021 20:00:35 +0000 Subject: [PATCH 18/19] Add entries to CHANGELOG --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d36a26e77..a5ff4393b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -51,6 +51,8 @@ - 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 From 061848c4e90353e1d6a1c036935a4d1d70002e3f Mon Sep 17 00:00:00 2001 From: Magnus Ulimoen Date: Wed, 13 Oct 2021 20:17:55 +0000 Subject: [PATCH 19/19] Remove manual incref --- src/handle.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/handle.rs b/src/handle.rs index 6b77de6d8..3937434f1 100644 --- a/src/handle.rs +++ b/src/handle.rs @@ -104,10 +104,7 @@ impl Handle { 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()) } }