diff --git a/CHANGELOG.md b/CHANGELOG.md index 14f8053..70cd926 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,16 @@ types): one attribute, found in dense storage through its name index (record type 8) instead of reading every attribute (`clawhdf5_format::attribute::find_attribute_in_file`). +- **Two links of one name: the first wins everywhere.** A group cannot + validly hold two links of one name, but a damaged or hand-made one can. + The listing, `resolve_child` (`Group::dataset`/`group`) and path + resolution now all use only the first link of a name (header message + order in a compact group, name index order in a dense one) and ignore + the rest, even if the first dangles — libhdf5's rule for compact groups + (h5py fails to open a dangling first link although a later one + resolves). Before, the listing skipped a dangling first link and listed + the name via a later one that lookup did not follow, and path resolution + followed the last. - **`Group::entries()` and `File::group_at(address)`**: a listing's `(name, address)` pairs, to open children without looking names up again. - Test: `crates/clawhdf5/tests/indexed_lookup_interop.rs` — every child of diff --git a/crates/clawhdf5-format/src/group_v2.rs b/crates/clawhdf5-format/src/group_v2.rs index e1bd9f5..0edf972 100644 --- a/crates/clawhdf5-format/src/group_v2.rs +++ b/crates/clawhdf5-format/src/group_v2.rs @@ -6,6 +6,11 @@ #[cfg(not(feature = "std"))] use alloc::{string::String, vec::Vec}; +#[cfg(not(feature = "std"))] +use alloc::collections::BTreeSet; +#[cfg(feature = "std")] +use std::collections::BTreeSet; + use crate::addr::to_usize; use crate::btree_v2::{BTreeV2Header, collect_btree_v2_records, find_btree_v2_records}; use crate::checksum::jenkins_lookup3; @@ -159,45 +164,34 @@ fn resolve_dense_entries( Ok(entries) } -/// The soft or external link called `name` in this group, if there is one. -/// Hard links are what `resolve_group_entries` returns; this is consulted only -/// when a path component isn't among them. -fn find_symbolic_link( +/// The soft link called `name` in a v1 (symbol table) group, if there is +/// one. Hard links are what `resolve_group_entries` returns; this is +/// consulted only when a path component isn't among them. +fn find_v1_symbolic_link( file_data: &[u8], object_header: &ObjectHeader, name: &str, offset_size: u8, length_size: u8, ) -> Result, FormatError> { - if is_v1_group(object_header) { - let Some(sym_msg) = object_header - .messages - .iter() - .find(|m| m.msg_type == MessageType::SymbolTable) - else { - return Ok(None); - }; - let stm = SymbolTableMessage::parse(&sym_msg.data, offset_size)?; - return group_v1::find_v1_soft_link(file_data, &stm, name, offset_size, length_size) - .map(|target| target.map(|target_path| LinkTarget::Soft { target_path })); - } - if !is_v2_group(object_header) { + let Some(sym_msg) = object_header + .messages + .iter() + .find(|m| m.msg_type == MessageType::SymbolTable) + else { return Ok(None); - } - // As when all links were scanned: the last symbolic link of that name. - Ok( - links_named(file_data, object_header, name, offset_size, length_size)? - .into_iter() - .rev() - .map(|link| link.link_target) - .find(|t| !matches!(t, LinkTarget::Hard { .. })), - ) + }; + let stm = SymbolTableMessage::parse(&sym_msg.data, offset_size)?; + group_v1::find_v1_soft_link(file_data, &stm, name, offset_size, length_size) + .map(|target| target.map(|target_path| LinkTarget::Soft { target_path })) } /// B-tree v2 record type of a dense group's link name index. const LINK_NAME_INDEX: u8 = 5; -/// The links called `name` in a v2 group (a valid group has at most one). +/// The links called `name` in a v2 group (a valid group has at most one), +/// in storage order: header message order for a compact group, name index +/// order for a dense one. /// /// In dense storage the link name index (a v2 B-tree of lookup3 name /// hashes, record type 5) is descended to the records with the name's hash, @@ -272,6 +266,32 @@ fn links_named( Ok(found) } +/// The link called `name` in a v2 group, if any. +/// +/// A valid group has at most one; libhdf5 cannot create two. If a damaged +/// or hand-made group has several, the first wins and the rest are +/// ignored, whatever their kind and even if the first cannot be followed. +/// That is libhdf5's rule for a compact group (`H5G__compact_lookup` stops +/// at the first Link message of that name; h5py then fails to open a +/// dangling first link although a later one resolves). For a dense group +/// "first" is first in name index order; libhdf5 binary-searches the index +/// and may land on another of several exact duplicates. The listing +/// ([`resolve_group_children`]), [`resolve_child`] and path resolution all +/// apply this rule, so they agree. +fn first_link_named( + file_data: &[u8], + object_header: &ObjectHeader, + name: &str, + offset_size: u8, + length_size: u8, +) -> Result, FormatError> { + Ok( + links_named(file_data, object_header, name, offset_size, length_size)? + .into_iter() + .next(), + ) +} + /// The link [`resolve_path_any`] follows for one path component `name` of /// the group with header `object_header`: a hard link (as `Hard`), else a /// soft or external link of that name, else `None`. Fails with @@ -293,29 +313,25 @@ fn lookup_link( object_header_address: e.object_header_address, })); } - return find_symbolic_link(file_data, object_header, name, offset_size, length_size); + return find_v1_symbolic_link(file_data, object_header, name, offset_size, length_size); } if !is_v2_group(object_header) { return Err(FormatError::PathNotFound(String::from( "object header is not a group", ))); } - let links = links_named(file_data, object_header, name, offset_size, length_size)?; - if let Some(addr) = links.iter().find_map(|l| match l.link_target { - LinkTarget::Hard { - object_header_address, - } if object_header_address != u64::MAX => Some(object_header_address), - _ => None, - }) { - return Ok(Some(LinkTarget::Hard { - object_header_address: addr, - })); - } - Ok(links - .into_iter() - .rev() - .map(|link| link.link_target) - .find(|t| !matches!(t, LinkTarget::Hard { .. }))) + Ok( + first_link_named(file_data, object_header, name, offset_size, length_size)? + .map(|link| link.link_target) + .filter(|t| { + !matches!( + t, + LinkTarget::Hard { + object_header_address: u64::MAX + } + ) + }), + ) } /// The object header address of the child called `name` of the group at @@ -342,19 +358,14 @@ pub fn resolve_child( .map(|e| e.object_header_address) .ok_or_else(not_found); } - let links = links_named(file_data, &header, name, os, ls)?; - // The listing puts hard links before resolved soft links. - if let Some(addr) = links.iter().find_map(|l| match l.link_target { - LinkTarget::Hard { + // The first link of that name only, as the listing (see + // `first_link_named`). + match first_link_named(file_data, &header, name, os, ls)?.map(|l| l.link_target) { + Some(LinkTarget::Hard { object_header_address, - } => Some(object_header_address), - _ => None, - }) { - return Ok(addr); - } - for link in links { - if let LinkTarget::Soft { target_path } = link.link_target { - return match resolve_path_from(file_data, superblock, group_address, &target_path) { + }) => Ok(object_header_address), + Some(LinkTarget::Soft { target_path }) => { + match resolve_path_from(file_data, superblock, group_address, &target_path) { // Left out of the listing: dangling, cyclic, or in another file. Err( FormatError::PathNotFound(_) @@ -362,10 +373,10 @@ pub fn resolve_child( | FormatError::ExternalLinkUnsupported { .. }, ) => Err(not_found()), other => other, - }; + } } + Some(LinkTarget::External { .. }) | None => Err(not_found()), } - Err(not_found()) } /// Find and parse the Link Info message from an object header. @@ -471,16 +482,23 @@ pub fn resolve_group_children( } entries.extend(all.into_iter().filter(|e| !group_v1::is_v1_soft_link(e))); } else if is_v2_group(&header) { - let mut visit = |link: LinkMessage| match link.link_target { - LinkTarget::Hard { - object_header_address, - } => entries.push(GroupEntry { - name: link.name, - object_header_address, - cache_type: 0, - }), - LinkTarget::Soft { target_path } => soft.push((link.name, target_path)), - LinkTarget::External { .. } => {} + // Only the first link of each name counts (see `first_link_named`). + let mut seen = BTreeSet::new(); + let mut visit = |link: LinkMessage| { + if !seen.insert(link.name.clone()) { + return; + } + match link.link_target { + LinkTarget::Hard { + object_header_address, + } => entries.push(GroupEntry { + name: link.name, + object_header_address, + cache_type: 0, + }), + LinkTarget::Soft { target_path } => soft.push((link.name, target_path)), + LinkTarget::External { .. } => {} + } }; let link_info = find_link_info(&header, os)?; if let Some(fh_addr) = link_info.fractal_heap_address { diff --git a/crates/clawhdf5/tests/indexed_lookup_interop.rs b/crates/clawhdf5/tests/indexed_lookup_interop.rs index 71f48f0..98b04f0 100644 --- a/crates/clawhdf5/tests/indexed_lookup_interop.rs +++ b/crates/clawhdf5/tests/indexed_lookup_interop.rs @@ -495,3 +495,118 @@ fn a_corrupt_internal_index_node_is_an_error_not_a_missing_name() { )); assert_eq!(out, "checksum checksum"); } + +/// Rename the one link called `from` to `to` (same length) in `bytes`, and +/// re-checksum the object header chunk holding it: two links of one name, +/// which libhdf5 cannot write. +fn rename_link_in_header(bytes: &mut [u8], from: &[u8], to: &[u8]) { + assert_eq!(from.len(), to.len()); + let find = |hay: &[u8], needle: &[u8]| hay.windows(needle.len()).position(|w| w == needle); + let at = find(bytes, from).expect("link name"); + assert!(find(&bytes[at + 1..], from).is_none(), "name not unique"); + bytes[at..at + to.len()].copy_from_slice(to); + // The v2 object header (chunk 0) holding it. + let ohdr = bytes[..at] + .windows(4) + .rposition(|w| w == b"OHDR") + .expect("OHDR"); + let flags = bytes[ohdr + 5]; + let mut pos = ohdr + 6; + if flags & 0x20 != 0 { + pos += 16; // times + } + if flags & 0x10 != 0 { + pos += 4; // attribute phase change + } + let width = 1usize << (flags & 3); + let mut size = [0u8; 8]; + size[..width].copy_from_slice(&bytes[pos..pos + width]); + let end = pos + width + usize::try_from(u64::from_le_bytes(size)).unwrap(); + assert!(at < end, "name outside chunk 0"); + let sum = jenkins_lookup3(&bytes[ohdr..end]); + bytes[end..end + 4].copy_from_slice(&sum.to_le_bytes()); +} + +/// Two soft links of one name (a damaged or hand-made group; libhdf5 +/// cannot create one), one dangling: only the first counts, as in libhdf5, +/// which opens the first Link message of a name and fails if it dangles. +/// Lookup, path and listing agree — before, the listing skipped a dangling +/// first link and listed the name via the second, which lookup did not +/// follow, and path resolution followed the last. +#[test] +fn of_two_links_with_one_name_the_first_wins_everywhere() { + skip_if_no_python!(); + let dir = tempfile::tempdir().unwrap(); + for dangling_first in [true, false] { + let path = dir + .path() + .join(format!("dup_{dangling_first}.h5")) + .display() + .to_string(); + let (first, second) = if dangling_first { + ("/nowhere_xyz", "/d") + } else { + ("/d", "/nowhere_xyz") + }; + run_python(&format!( + "import h5py, numpy as np\n\ + with h5py.File(r'{path}', 'w', libver='latest') as f:\n\ + \x20 f.create_dataset('d', data=np.int64(42))\n\ + \x20 s = f.create_group('s')\n\ + \x20 s['dup_A'] = h5py.SoftLink('{first}')\n\ + \x20 s['dup_B'] = h5py.SoftLink('{second}')", + )); + let mut bytes = std::fs::read(&path).unwrap(); + rename_link_in_header(&mut bytes, b"dup_B", b"dup_A"); + std::fs::write(&path, &bytes).unwrap(); + + // What libhdf5 opens under that name: both names listed, first link + // followed. + let out = run_python(&format!( + "import h5py\n\ + with h5py.File(r'{path}', 'r') as f:\n\ + \x20 s = f['s']\n\ + \x20 assert list(s) == ['dup_A', 'dup_A'], list(s)\n\ + \x20 try:\n\ + \x20 print(int(s['dup_A'][()]))\n\ + \x20 except KeyError:\n\ + \x20 print('none')", + )); + let want = if dangling_first { "none" } else { "42" }; + assert_eq!(out, want, "h5py, dangling first: {dangling_first}"); + let want = (!dangling_first).then_some(42i64); + + let f = File::open(&path).unwrap(); + let s = f.group("s").unwrap(); + let got = |r: Result, clawhdf5::Error>| match r { + Ok(ds) => Some(ds.read_i64().unwrap()[0]), + Err(e) => { + assert!(is_not_found(&e), "{e:?}"); + None + } + }; + assert_eq!(got(s.dataset("dup_A")), want, "lookup, {dangling_first}"); + assert_eq!(got(f.dataset("/s/dup_A")), want, "path, {dangling_first}"); + let listed = s.datasets().unwrap(); + let listed_n = listed.iter().filter(|n| *n == "dup_A").count(); + assert_eq!(listed_n, usize::from(want.is_some()), "{listed:?}"); + let entries = s.entries().unwrap(); + assert_eq!(entries.len(), listed_n, "{entries:?}"); + + let m = MmapFile::open(&path).unwrap(); + let l = LazyFile::open_mmap(&path).unwrap(); + let (mg, lg) = (m.group("s").unwrap(), l.group("s").unwrap()); + match want { + Some(v) => { + assert_eq!(mg.dataset("dup_A").unwrap().read_i64().unwrap(), vec![v]); + assert_eq!(lg.dataset("dup_A").unwrap().read_i64().unwrap(), vec![v]); + } + None => { + assert!(mg.dataset("dup_A").is_err_and(|e| is_not_found(&e))); + assert!(lg.dataset("dup_A").is_err_and(|e| is_not_found(&e))); + } + } + assert_eq!(mg.datasets().unwrap(), listed); + assert_eq!(lg.datasets().unwrap(), listed); + } +}