read: of two links with one name, the first wins everywhere

A valid group has one link per name, but a damaged or hand-made one can
have two. resolve_child followed the first soft link of the name, the
listing skipped a dangling one and listed the name via a later link, and
path resolution followed the last symbolic link: three answers. All now
take the first link of the name (header message order in a compact group,
name index order in a dense one) and ignore the rest, even if the first
dangles. That is libhdf5's rule for compact groups (H5G__compact_lookup
stops at the first Link message); h5py opens nothing for a dangling first
link although a later one resolves. For a dense group libhdf5
binary-searches the index and may land on another of several exact
duplicates; documented on first_link_named. find_symbolic_link's v2 branch
was dead (only v1 groups reach it) and is now v1-only.

Test: an h5py compact group with soft links dup_A (dangling, or to /d) and
dup_B (the other), dup_B renamed to dup_A in the header and re-checksummed.
Lookup, path and listing through all three readers match h5py for both
orders. With the old group_v2.rs the path lookup returned 42 where h5py
opens nothing.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>

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

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.
+/// The soft link called `name` in a v1 (symbol table) group, if there is
-/// Hard links are what `resolve_group_entries` returns; this is consulted only
+/// one. Hard links are what `resolve_group_entries` returns; this is
-/// when a path component isn't among them.
+/// consulted only when a path component isn't among them.
-fn find_symbolic_link(
+fn find_v1_symbolic_link(
     file_data: &[u8],
     object_header: &ObjectHeader,
     name: &str,
     offset_size: u8,
     length_size: u8,
 ) -> Result<Option<LinkTarget>, FormatError> {
-    if is_v1_group(object_header) {
+    let Some(sym_msg) = object_header
-        let Some(sym_msg) = object_header
+        .messages
-            .messages
+        .iter()
-            .iter()
+        .find(|m| m.msg_type == MessageType::SymbolTable)
-            .find(|m| m.msg_type == MessageType::SymbolTable)
+    else {
-        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) {
         return Ok(None);
-    }
+    };
-    // As when all links were scanned: the last symbolic link of that name.
+    let stm = SymbolTableMessage::parse(&sym_msg.data, offset_size)?;
-    Ok(
+    group_v1::find_v1_soft_link(file_data, &stm, name, offset_size, length_size)
-        links_named(file_data, object_header, name, offset_size, length_size)?
+        .map(|target| target.map(|target_path| LinkTarget::Soft { target_path }))
-            .into_iter()
-            .rev()
-            .map(|link| link.link_target)
-            .find(|t| !matches!(t, LinkTarget::Hard { .. })),
-    )
 }
 
 /// 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<Option<LinkMessage>, 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)?;
+    Ok(
-    if let Some(addr) = links.iter().find_map(|l| match l.link_target {
+        first_link_named(file_data, object_header, name, offset_size, length_size)?
-        LinkTarget::Hard {
+            .map(|link| link.link_target)
-            object_header_address,
+            .filter(|t| {
-        } if object_header_address != u64::MAX => Some(object_header_address),
+                !matches!(
-        _ => None,
+                    t,
-    }) {
+                    LinkTarget::Hard {
-        return Ok(Some(LinkTarget::Hard {
+                        object_header_address: u64::MAX
-            object_header_address: addr,
+                    }
-        }));
+                )
-    }
+            }),
-    Ok(links
+    )
-        .into_iter()
-        .rev()
-        .map(|link| link.link_target)
-        .find(|t| !matches!(t, LinkTarget::Hard { .. })))
 }
 
 /// 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 first link of that name only, as the listing (see
-    // The listing puts hard links before resolved soft links.
+    // `first_link_named`).
-    if let Some(addr) = links.iter().find_map(|l| match l.link_target {
+    match first_link_named(file_data, &header, name, os, ls)?.map(|l| l.link_target) {
-        LinkTarget::Hard {
+        Some(LinkTarget::Hard {
             object_header_address,
-        } => Some(object_header_address),
+        }) => Ok(object_header_address),
-        _ => None,
+        Some(LinkTarget::Soft { target_path }) => {
-    }) {
+            match resolve_path_from(file_data, superblock, group_address, &target_path) {
-        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) {
                 // 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 {
+        // Only the first link of each name counts (see `first_link_named`).
-            LinkTarget::Hard {
+        let mut seen = BTreeSet::new();
-                object_header_address,
+        let mut visit = |link: LinkMessage| {
-            } => entries.push(GroupEntry {
+            if !seen.insert(link.name.clone()) {
-                name: link.name,
+                return;
-                object_header_address,
+            }
-                cache_type: 0,
+            match link.link_target {
-            }),
+                LinkTarget::Hard {
-            LinkTarget::Soft { target_path } => soft.push((link.name, target_path)),
+                    object_header_address,
-            LinkTarget::External { .. } => {}
+                } => 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 {

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::Dataset<'_>, 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);
+    }
+}