From ccfa25b9bfa6fa36c6b88b57118ff0c0cda5193d Mon Sep 17 00:00:00 2001 From: viniciusdutra314 Date: Tue, 30 Jun 2026 19:03:16 -0300 Subject: [PATCH 1/4] feat: added skip attribute to hdf5-derive --- hdf5-derive/src/lib.rs | 25 ++++++++++-- hdf5-derive/tests/test.rs | 86 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 3 deletions(-) diff --git a/hdf5-derive/src/lib.rs b/hdf5-derive/src/lib.rs index 2f73fb0f3..a5031da90 100644 --- a/hdf5-derive/src/lib.rs +++ b/hdf5-derive/src/lib.rs @@ -127,6 +127,22 @@ fn is_phantom_data(ty: &Type) -> bool { } } +fn is_hdf5_skip(attrs: &[Attribute]) -> bool { + let mut skip = false; + let attr = match attrs.iter().find(|a| a.path().is_ident("hdf5")) { + Some(a) => a, + None => return false, + }; + attr.parse_nested_meta(|meta| { + if meta.path.is_ident("skip") { + skip = true; + } + Ok(()) + }) + .ok(); + skip +} + fn find_repr(attrs: &[Attribute], expected: &[&str]) -> Option { let mut repr = None; for attr in attrs.iter() { @@ -181,8 +197,11 @@ fn impl_trait( Fields::Unit => syn::Error::new(ty.span(), "cannot derive `H5Type` for unit structs") .into_compile_error(), Fields::Named(ref fields) => { - let fields: Vec<_> = - fields.named.iter().filter(|f| !is_phantom_data(&f.ty)).collect(); + let fields: Vec<_> = fields + .named + .iter() + .filter(|f| !is_phantom_data(&f.ty) && !is_hdf5_skip(&f.attrs)) + .collect(); if fields.is_empty() { return syn::Error::new(ty.span(), "cannot derive `H5Type` for empty structs") .into_compile_error(); @@ -214,7 +233,7 @@ fn impl_trait( .unnamed .iter() .enumerate() - .filter(|&(_, f)| !is_phantom_data(&f.ty)) + .filter(|&(_, f)| !is_phantom_data(&f.ty) && !is_hdf5_skip(&f.attrs)) .map(|(i, f)| (Index::from(i), f)) .unzip(); if fields.is_empty() { diff --git a/hdf5-derive/tests/test.rs b/hdf5-derive/tests/test.rs index dbb6555ca..122cae0c8 100644 --- a/hdf5-derive/tests/test.rs +++ b/hdf5-derive/tests/test.rs @@ -250,3 +250,89 @@ fn test_phantom_data() { assert_eq!(G3::::type_descriptor(), C3::type_descriptor()); assert_eq!(G4::::type_descriptor(), C4::type_descriptor()); } + +#[cfg(test)] +mod test_hdf5_skip_attribute { + use super::*; + macro_rules! check_fields { + ($ty:ty, $($field_name:expr => $field:tt),+ $(,)?) => {{ + let desc = <$ty as hdf5::types::H5Type>::type_descriptor(); + assert_eq!(desc.size(), std::mem::size_of::<$ty>(), "Total size mismatch"); + + let hdf5::types::TypeDescriptor::Compound(compound) = desc else { + panic!("Expected TypeDescriptor::Compound"); + }; + + let s = std::mem::MaybeUninit::<$ty>::uninit(); + let s_ptr = s.as_ptr(); + let mut expected_field_count = 0; + + $( + expected_field_count += 1; + let found = compound.fields.iter().find(|f| f.name == $field_name) + .unwrap_or_else(|| panic!("Field '{}' not found in HDF5 descriptor", $field_name)); + + let expected_offset = unsafe { + std::ptr::addr_of!((*s_ptr).$field) as usize - s_ptr as usize + }; + + assert_eq!( + found.offset, expected_offset, + "Offset mismatch for field '{}': HDF5 says {}, memory says {}", + $field_name, found.offset, expected_offset + ); + )+ + + assert_eq!( + compound.fields.len(), expected_field_count, + "Expected exactly {} unskipped fields", expected_field_count + ); + }}; + } + #[test] + fn test_skip_repr_c() { + #[derive(H5Type)] + #[repr(C)] + struct ReprCStruct { + a: u8, + #[hdf5(skip)] + _skipped_1: u32, + b: u64, + #[hdf5(skip)] + _skipped_2: Vec, + c: u16, + } + check_fields!(ReprCStruct, "a" => a, "b" => b, "c" => c); + } + #[test] + fn test_skip_repr_packed() { + #[derive(H5Type)] + #[repr(packed)] + struct PackedStruct { + a: u8, + #[hdf5(skip)] + _skipped: u64, + b: u32, + } + check_fields!(PackedStruct, "a" => a, "b" => b); + } + #[test] + fn test_skip_tuple_struct() { + #[derive(H5Type)] + #[repr(C)] + struct TupleStruct(u16, #[hdf5(skip)] String, u64); + check_fields!(TupleStruct, "0" => 0, "1" => 2); + } + #[test] + fn test_skip_generics() { + #[derive(H5Type)] + #[repr(C)] + struct GenericStruct { + x: T, + #[hdf5(skip)] + _skipped: Vec, + y: U, + } + check_fields!(GenericStruct, "x" => x, "y" => y); + } +} From 5de3655d378ad206584bdaa28cd49940f53bc26b Mon Sep 17 00:00:00 2001 From: viniciusdutra314 Date: Wed, 1 Jul 2026 21:19:38 -0300 Subject: [PATCH 2/4] fix: UB on reading of a skipped field in a H5Type struct --- hdf5-derive/src/lib.rs | 61 +++++++++++++++++++++++++-- hdf5-types/src/h5type.rs | 4 ++ hdf5/src/hl/container.rs | 8 +++- hdf5/tests/test_dataset.rs | 84 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 153 insertions(+), 4 deletions(-) diff --git a/hdf5-derive/src/lib.rs b/hdf5-derive/src/lib.rs index a5031da90..82842f892 100644 --- a/hdf5-derive/src/lib.rs +++ b/hdf5-derive/src/lib.rs @@ -2,6 +2,7 @@ use std::iter; use std::mem; +use std::result; use std::str::FromStr; use proc_macro2::{Ident, Span, TokenStream}; @@ -17,7 +18,8 @@ pub fn derive(input: proc_macro::TokenStream) -> proc_macro::TokenStream { let input = parse_macro_input!(input as DeriveInput); let name = input.ident; let (impl_generics, ty_generics, where_clause) = input.generics.split_for_impl(); - let body = impl_trait(&name, &input.data, &input.attrs, &ty_generics); + let type_descriptor_body = impl_type_descriptor(&name, &input.data, &input.attrs, &ty_generics); + let initialize_skipped_fields_body = impl_initialize_skipped_fields(&input.data); // Determine name of parent crate, even if renamed using "package" // CARGO_CRATE_NAME is the name of the actual crate being compiled (e.g., "simple" for examples) @@ -50,7 +52,11 @@ pub fn derive(input: proc_macro::TokenStream) -> proc_macro::TokenStream { unsafe impl #impl_generics _h5::types::H5Type for #name #ty_generics #where_clause { #[inline] fn type_descriptor() -> _h5::types::TypeDescriptor { - #body + #type_descriptor_body + } + #[inline] + unsafe fn initialize_skipped_fields(ptr: *mut Self, size: usize) { + #initialize_skipped_fields_body } } }; @@ -189,7 +195,7 @@ where iter.map(func).collect() } -fn impl_trait( +fn impl_type_descriptor( ty: &Ident, data: &Data, attrs: &[Attribute], ty_generics: &TypeGenerics, ) -> TokenStream { match *data { @@ -304,3 +310,52 @@ fn impl_trait( .to_compile_error(), } } + +fn impl_initialize_skipped_fields(data: &Data) -> TokenStream { + match *data { + Data::Struct(ref data) => match data.fields { + Fields::Named(ref fields) => { + let skipped_fields = fields + .named + .iter() + .filter(|field| is_hdf5_skip(&field.attrs)) + .filter_map(|field| field.ident.as_ref()) + .map(|field| quote!(#field)) + .collect::>(); + impl_initialize_skipped_fields_body(&skipped_fields) + } + Fields::Unnamed(ref fields) => { + let skipped_fields = fields + .unnamed + .iter() + .enumerate() + .filter(|&(_, field)| is_hdf5_skip(&field.attrs)) + .map(|(index, _)| Index::from(index)) + .map(|index| quote!(#index)) + .collect::>(); + impl_initialize_skipped_fields_body(&skipped_fields) + } + Fields::Unit => quote! {}, + }, + _ => quote! {}, + } +} + +fn impl_initialize_skipped_fields_body(fields: &[TokenStream]) -> TokenStream { + if fields.is_empty() { + return quote! {}; + } + + let fields = fields.iter(); + quote! { + for i in 0..size { + let ptr = ptr.add(i); + #( + ::std::ptr::write_unaligned( + ::std::ptr::addr_of_mut!((*ptr).#fields), + ::core::default::Default::default(), + ); + )* + } + } +} diff --git a/hdf5-types/src/h5type.rs b/hdf5-types/src/h5type.rs index 00aa2f1b0..d50d7ac12 100644 --- a/hdf5-types/src/h5type.rs +++ b/hdf5-types/src/h5type.rs @@ -302,6 +302,10 @@ impl TypeDescriptor { pub unsafe trait H5Type: 'static { /// Returns a descriptor for an equivalent HDF5 datatype. fn type_descriptor() -> TypeDescriptor; + /// Structs with `#[hdf5(skip)]` fields need to have them initialized because they are not present in the HDF5 file. + /// The derive macro implementation initializes skipped fields + /// using the Default trait. + unsafe fn initialize_skipped_fields(_ptr: *mut Self, _size: usize) {} } macro_rules! impl_h5type { diff --git a/hdf5/src/hl/container.rs b/hdf5/src/hl/container.rs index 9b68d55e6..adfc496df 100644 --- a/hdf5/src/hl/container.rs +++ b/hdf5/src/hl/container.rs @@ -46,7 +46,13 @@ impl<'a> Reader<'a> { let mem_dtype = Datatype::from_type::()?; file_dtype.ensure_convertible(&mem_dtype, self.conv)?; let (obj_id, tp_id) = (self.obj.id(), mem_dtype.id()); - + let n_elements = match mspace { + Some(space) => space.size(), + None => self.obj.space()?.size(), + }; + unsafe { + T::initialize_skipped_fields(buf, n_elements); + } if self.obj.is_attr() { h5try!(H5Aread(obj_id, tp_id, buf.cast())); } else { diff --git a/hdf5/tests/test_dataset.rs b/hdf5/tests/test_dataset.rs index a775dadf2..9b2d54d8a 100644 --- a/hdf5/tests/test_dataset.rs +++ b/hdf5/tests/test_dataset.rs @@ -480,3 +480,87 @@ fn remove_attr() { ds.delete_attr("bar").unwrap(); assert!(ds.attr("bar").is_err()); } +mod test_reading_and_written_skipped_structs { + use super::*; + #[derive(hdf5_derive::H5Type)] + #[repr(C)] + struct NamedSkippedStruct { + id: u32, + #[hdf5(skip)] + skipped: String, + value: u8, + } + + #[derive(hdf5_derive::H5Type)] + #[repr(C)] + struct TupleSkippedStruct(u32, #[hdf5(skip)] Vec, u8); + + #[test] + fn named_struct_defaults_skipped_field() { + let written = vec![ + NamedSkippedStruct { id: 1, skipped: String::from("first"), value: 10 }, + NamedSkippedStruct { id: 2, skipped: String::from("second"), value: 20 }, + ]; + + let dataset = new_in_memory_file() + .unwrap() + .new_dataset::() + .shape((written.len(),)) + .create("named") + .unwrap(); + + dataset.write_raw(&written).unwrap(); + + let read = dataset.read_raw::().unwrap(); + + assert_eq!(read.len(), 2); + assert_eq!(read[0].id, 1); + assert_eq!(read[0].value, 10); + assert_eq!(read[0].skipped, String::default()); + assert_eq!(read[1].id, 2); + assert_eq!(read[1].value, 20); + assert_eq!(read[1].skipped, String::default()); + } + + #[test] + fn tuple_struct_defaults_skipped_field_in_array_roundtrip() { + let written = vec![ + TupleSkippedStruct(11, vec![1, 2, 3], 9), + TupleSkippedStruct(21, vec![4, 5, 6], 19), + ]; + + let dataset = new_in_memory_file() + .unwrap() + .new_dataset::() + .shape((written.len(),)) + .create("tuple") + .unwrap(); + + dataset.write_raw(&written).unwrap(); + + let read = dataset.read_raw::().unwrap(); + + assert_eq!(read.len(), 2); + assert_eq!(read[0].0, 11); + assert_eq!(read[0].2, 9); + assert_eq!(read[0].1, Vec::::default()); + assert_eq!(read[1].0, 21); + assert_eq!(read[1].2, 19); + assert_eq!(read[1].1, Vec::::default()); + } + + #[test] + fn attribute_defaults_skipped_field() { + let written = NamedSkippedStruct { id: 99, skipped: String::from("meta"), value: 5 }; + + let file = new_in_memory_file().unwrap(); + let attr = file.new_attr::().shape(()).create("my_attr").unwrap(); + + attr.write_scalar(&written).unwrap(); + let read = attr.read_scalar::().unwrap(); + + assert_eq!(read.id, 99); + assert_eq!(read.value, 5); + assert_eq!(read.skipped, String::default()); + } +} From 4db80b1efeebcccabd8a5d1e7d96c2e0d05593ec Mon Sep 17 00:00:00 2001 From: viniciusdutra314 Date: Thu, 2 Jul 2026 11:03:37 -0300 Subject: [PATCH 3/4] feat: better semantics for the H5Type trait function --- hdf5-derive/src/lib.rs | 24 +++++++++++------------- hdf5-types/src/h5type.rs | 17 ++++++++++++----- hdf5/src/hl/container.rs | 10 +++++++--- hdf5/tests/test_dataset.rs | 34 +++++++++++++++++++++++++++++++--- 4 files changed, 61 insertions(+), 24 deletions(-) diff --git a/hdf5-derive/src/lib.rs b/hdf5-derive/src/lib.rs index 82842f892..6028fc148 100644 --- a/hdf5-derive/src/lib.rs +++ b/hdf5-derive/src/lib.rs @@ -2,7 +2,6 @@ use std::iter; use std::mem; -use std::result; use std::str::FromStr; use proc_macro2::{Ident, Span, TokenStream}; @@ -19,7 +18,7 @@ pub fn derive(input: proc_macro::TokenStream) -> proc_macro::TokenStream { let name = input.ident; let (impl_generics, ty_generics, where_clause) = input.generics.split_for_impl(); let type_descriptor_body = impl_type_descriptor(&name, &input.data, &input.attrs, &ty_generics); - let initialize_skipped_fields_body = impl_initialize_skipped_fields(&input.data); + let init_skipped_fields_body = impl_initialize_skipped_fields(&input.data); // Determine name of parent crate, even if renamed using "package" // CARGO_CRATE_NAME is the name of the actual crate being compiled (e.g., "simple" for examples) @@ -55,8 +54,8 @@ pub fn derive(input: proc_macro::TokenStream) -> proc_macro::TokenStream { #type_descriptor_body } #[inline] - unsafe fn initialize_skipped_fields(ptr: *mut Self, size: usize) { - #initialize_skipped_fields_body + unsafe fn init_skipped_fields(element: &mut ::std::mem::MaybeUninit) { + #init_skipped_fields_body } } }; @@ -348,14 +347,13 @@ fn impl_initialize_skipped_fields_body(fields: &[TokenStream]) -> TokenStream { let fields = fields.iter(); quote! { - for i in 0..size { - let ptr = ptr.add(i); - #( - ::std::ptr::write_unaligned( - ::std::ptr::addr_of_mut!((*ptr).#fields), - ::core::default::Default::default(), - ); - )* - } + let ptr = element.as_mut_ptr(); + + #( + ::std::ptr::write_unaligned( + ::std::ptr::addr_of_mut!((*ptr).#fields), + ::core::default::Default::default(), + ); + )* } } diff --git a/hdf5-types/src/h5type.rs b/hdf5-types/src/h5type.rs index d50d7ac12..461a69b7e 100644 --- a/hdf5-types/src/h5type.rs +++ b/hdf5-types/src/h5type.rs @@ -1,5 +1,5 @@ use std::fmt::{self, Display}; -use std::mem; +use std::mem::{self, MaybeUninit}; use std::os::raw::c_void; use crate::array::VarLenArray; @@ -302,10 +302,17 @@ impl TypeDescriptor { pub unsafe trait H5Type: 'static { /// Returns a descriptor for an equivalent HDF5 datatype. fn type_descriptor() -> TypeDescriptor; - /// Structs with `#[hdf5(skip)]` fields need to have them initialized because they are not present in the HDF5 file. - /// The derive macro implementation initializes skipped fields - /// using the Default trait. - unsafe fn initialize_skipped_fields(_ptr: *mut Self, _size: usize) {} + /** + Structs with `#[hdf5(skip)]` fields need to have them initialized because they are not present in the HDF5 file. + The derive macro implementation initializes skipped fields + using the Default trait. This function is called for each element of a read buffer before reading the data from the HDF5 file, it partially initialize the struct by only initializing the skipped fields. + */ + #[allow(unused_variables)] + unsafe fn init_skipped_fields(element: &mut MaybeUninit) + where + Self: Sized, + { + } } macro_rules! impl_h5type { diff --git a/hdf5/src/hl/container.rs b/hdf5/src/hl/container.rs index adfc496df..07692bab4 100644 --- a/hdf5/src/hl/container.rs +++ b/hdf5/src/hl/container.rs @@ -1,7 +1,7 @@ use std::convert::TryInto; use std::fmt::{self, Debug}; use std::io; -use std::mem; +use std::mem::{self, MaybeUninit}; use std::ops::Deref; use ndarray::{Array, Array1, Array2, ArrayD, ArrayView, ArrayView1}; @@ -50,8 +50,12 @@ impl<'a> Reader<'a> { Some(space) => space.size(), None => self.obj.space()?.size(), }; - unsafe { - T::initialize_skipped_fields(buf, n_elements); + let uninit_buf = buf.cast::<::std::mem::MaybeUninit>(); + for i in 0..n_elements { + unsafe { + let uninit_ref: &mut MaybeUninit = &mut *(uninit_buf.add(i)); + T::init_skipped_fields(uninit_ref); + } } if self.obj.is_attr() { h5try!(H5Aread(obj_id, tp_id, buf.cast())); diff --git a/hdf5/tests/test_dataset.rs b/hdf5/tests/test_dataset.rs index 9b2d54d8a..001966fff 100644 --- a/hdf5/tests/test_dataset.rs +++ b/hdf5/tests/test_dataset.rs @@ -484,17 +484,46 @@ mod test_reading_and_written_skipped_structs { use super::*; #[derive(hdf5_derive::H5Type)] #[repr(C)] + struct PlainStruct { + id: u32, + value: u8, + } + #[derive(hdf5_derive::H5Type)] + #[repr(C)] struct NamedSkippedStruct { id: u32, #[hdf5(skip)] skipped: String, value: u8, } - #[derive(hdf5_derive::H5Type)] #[repr(C)] struct TupleSkippedStruct(u32, #[hdf5(skip)] Vec, u8); + #[test] + fn skipped_and_plain_structs_deserialize_the_same() { + let written = vec![PlainStruct { id: 1, value: 10 }, PlainStruct { id: 2, value: 20 }]; + + let dataset = new_in_memory_file() + .unwrap() + .new_dataset::() + .shape((written.len(),)) + .create("plain") + .unwrap(); + + dataset.write_raw(&written).unwrap(); + + let plain = dataset.read_raw::().unwrap(); + let skipped = dataset.read_raw::().unwrap(); + + assert_eq!(plain.len(), skipped.len()); + for (p, s) in plain.iter().zip(skipped.iter()) { + assert_eq!(p.id, s.id); + assert_eq!(p.value, s.value); + assert_eq!(s.skipped, String::default()); + } + } + #[test] fn named_struct_defaults_skipped_field() { let written = vec![ @@ -523,7 +552,7 @@ mod test_reading_and_written_skipped_structs { } #[test] - fn tuple_struct_defaults_skipped_field_in_array_roundtrip() { + fn tuple_struct_defaults_skipped_field() { let written = vec![ TupleSkippedStruct(11, vec![1, 2, 3], 9), TupleSkippedStruct(21, vec![4, 5, 6], 19), @@ -558,7 +587,6 @@ mod test_reading_and_written_skipped_structs { attr.write_scalar(&written).unwrap(); let read = attr.read_scalar::().unwrap(); - assert_eq!(read.id, 99); assert_eq!(read.value, 5); assert_eq!(read.skipped, String::default()); From 67fe5e9ee7c6edc534651649ce6d692a42f6f233 Mon Sep 17 00:00:00 2001 From: viniciusdutra314 Date: Thu, 2 Jul 2026 11:16:12 -0300 Subject: [PATCH 4/4] fix: test with wrong struct being serialized --- hdf5/tests/test_dataset.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/hdf5/tests/test_dataset.rs b/hdf5/tests/test_dataset.rs index 001966fff..baf954799 100644 --- a/hdf5/tests/test_dataset.rs +++ b/hdf5/tests/test_dataset.rs @@ -502,13 +502,16 @@ mod test_reading_and_written_skipped_structs { #[test] fn skipped_and_plain_structs_deserialize_the_same() { - let written = vec![PlainStruct { id: 1, value: 10 }, PlainStruct { id: 2, value: 20 }]; + let written = vec![ + NamedSkippedStruct { id: 1, skipped: String::from("Test1"), value: 10 }, + NamedSkippedStruct { id: 2, skipped: String::from("Test2"), value: 20 }, + ]; let dataset = new_in_memory_file() .unwrap() - .new_dataset::() + .new_dataset::() .shape((written.len(),)) - .create("plain") + .create("named") .unwrap(); dataset.write_raw(&written).unwrap();