From 839acf56c5455a6e429be3aeb2f23cd3c56716cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Sat, 18 Jul 2026 12:40:31 +0200 Subject: [PATCH 1/7] Remove unnecessary doc(hidden) + pub on private implementation details --- src/index_map.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/index_map.rs b/src/index_map.rs index c33b8f0d55..809109abba 100644 --- a/src/index_map.rs +++ b/src/index_map.rs @@ -83,19 +83,17 @@ impl HashValue { } } -#[doc(hidden)] #[derive(Clone)] #[cfg_attr(feature = "zeroize", derive(Zeroize))] -pub struct Bucket { +struct Bucket { hash: HashValue, key: K, value: V, } -#[doc(hidden)] #[derive(Clone, Copy, PartialEq)] #[cfg_attr(feature = "zeroize", derive(Zeroize))] -pub struct Pos { +struct Pos { // compact representation of `{ hash_value: u16, index: u16 }` // To get the most from `NonZero` we store the *value minus 1*. This way `None::Option` // is equivalent to the very unlikely value of `{ hash_value: 0xffff, index: 0xffff }` instead From 2e73f81ee4090d3ef7fe2d2d95a37d509bc597b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Sat, 18 Jul 2026 12:42:41 +0200 Subject: [PATCH 2/7] Fix UB in index_map::insert Previously, index_map was broken in two ways: Firstly, indices are expected to only be 16 bits but the N parameter could be any usize, including > u16::MAX + 1 This is fixed with a static assertion. Secondly, the indexmap assumed that an index of 0xFFFF and an hash of 0xFFFF would never happen, but this could be wrong, and lead to the safety requirement of NonZeroU32::new_unchecked to be violated. This PR fixes this by removing the option and the automatic niche value optimization of `Pos`, and re-implementing it manually. To make sure that there is no confusion in the case where the index is 0xFFFF and so is the hash, and additional bit of information is used to determine whether the 0xFFFF hash and indices represent a None value or whether it represent Some({hash_value: 0xFF, index: 0xFF}) If the map is full of 0x10000 items, then we know that **all** pos in the indices are valid buffer are valid. Otherwise, we know that index = 0xFFFF is simply not possible, therefore we know that a Pos with such an index is `None`. see https://github.com/rust-embedded/heapless/issues/672 for the original report --- CHANGELOG.md | 3 + src/index_map.rs | 215 +++++++++++++++++++++++++++++++++++++---------- 2 files changed, 174 insertions(+), 44 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 47f4d29569..31d8625326 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Added `retain_back` (aka `truncate_front`) to `Deque` - Fixed unsoundness in `Vec::IntoIter::drop` and `HistoryBuf::write`in the context of panicking drop implementations. - Added `push_mut` to `Vec`. +- Fixed unsoundness in `IndexMap:insert`. +- Limited max size of `IndexMap` to u16::MAX + 1. + The implementation for sizes higher than u16::MAX were unsound anyway. ## [v0.9.3] 2025-04-15 diff --git a/src/index_map.rs b/src/index_map.rs index 809109abba..68282a2c66 100644 --- a/src/index_map.rs +++ b/src/index_map.rs @@ -4,9 +4,7 @@ use core::{ fmt, hash::{BuildHasher, Hash}, iter::FusedIterator, - mem, - num::NonZeroU32, - ops, slice, + mem, ops, slice, }; #[cfg(feature = "zeroize")] @@ -91,33 +89,92 @@ struct Bucket { value: V, } +const MAX_SIZE: usize = 0x10000; + #[derive(Clone, Copy, PartialEq)] #[cfg_attr(feature = "zeroize", derive(Zeroize))] struct Pos { - // compact representation of `{ hash_value: u16, index: u16 }` - // To get the most from `NonZero` we store the *value minus 1*. This way `None::Option` - // is equivalent to the very unlikely value of `{ hash_value: 0xffff, index: 0xffff }` instead - // the more likely of `{ hash_value: 0x00, index: 0x00 }` - nz: NonZeroU32, + maybe_valid: ValidPos, } impl Pos { + const fn none() -> Self { + Self { + maybe_valid: ValidPos { inner: 0 }, + } + } + fn new(index: usize, hash: HashValue) -> Self { + let value = ((u32::from(hash.0) << 16) + index as u32).wrapping_add(1); Self { - nz: unsafe { - NonZeroU32::new_unchecked( - ((u32::from(hash.0) << 16) + index as u32).wrapping_add(1), - ) - }, + maybe_valid: ValidPos { inner: value }, + } + } + + /// Returns a `ValidPos` if the pos doesn't encode `None` + /// + /// This will give the correct information only if the entries vec length + /// is known to be less than 0x10000 + fn assume_not_full(&mut self) -> Option<&mut ValidPos> { + if self == &Self::none() { + None + } else { + Some(&mut self.maybe_valid) + } + } + + /// Returns a `ValidPos` if the pos doesn't encode `None` + /// + /// The argument is the length of the `entries` buffer + fn as_valid(&self, buf_len: usize) -> Option<&ValidPos> { + // Is the buffer full? This uses a const generic to be able to short-circuit + // the check at compile time in most cases + let buffer_max = BUFFER_SIZE == MAX_SIZE && buf_len == MAX_SIZE; + if buffer_max || self != &Self::none() { + Some(&self.maybe_valid) + } else { + None + } + } + + /// Returns a `ValidPos` if the pos doesn't encode `None` + /// + /// The argument is the length of the `entries` buffer + fn as_valid_mut(&mut self, buf_len: usize) -> Option<&mut ValidPos> { + // Is the buffer full? This uses a const generic to be able to short-circuit + // the check at compile time in most cases + let buffer_max = BUFFER_SIZE == MAX_SIZE && buf_len == MAX_SIZE; + if buffer_max || self != &Self::none() { + Some(&mut self.maybe_valid) + } else { + None + } + } +} + +#[cfg_attr(feature = "zeroize", derive(Zeroize))] +#[derive(Clone, Copy, PartialEq)] +struct ValidPos { + /// Contains a simplified representation of `Option<{hash_value, index}>` + /// Where `None` is encoded with 0. + /// + /// In the specific case where the map is full, 0 can encode `Some({hash_value: 0, index: 0})`, + inner: u32, +} + +impl ValidPos { + fn replace(&mut self, pos: Pos) -> Pos { + Pos { + maybe_valid: mem::replace(self, pos.maybe_valid), } } fn hash(&self) -> HashValue { - HashValue((self.nz.get().wrapping_sub(1) >> 16) as u16) + HashValue((self.inner.wrapping_sub(1) >> 16) as u16) } fn index(&self) -> usize { - self.nz.get().wrapping_sub(1) as u16 as usize + self.inner.wrapping_sub(1) as u16 as usize } } @@ -150,16 +207,14 @@ macro_rules! probe_loop { )] struct CoreMap { entries: Vec, N, usize>, - indices: [Option; N], + indices: [Pos; N], } impl CoreMap { const fn new() -> Self { - const INIT: Option = None; - Self { entries: Vec::new(), - indices: [INIT; N], + indices: [Pos::none(); N], } } } @@ -185,7 +240,7 @@ where let mut dist = 0; probe_loop!(probe < self.indices.len(), { - let pos = self.indices[probe]?; + let pos = self.indices[probe].as_valid::(self.indices.len())?; let entry_hash = pos.hash(); // NOTE(i) we use unchecked indexing below let i = pos.index(); @@ -211,7 +266,7 @@ where probe_loop!(probe < self.indices.len(), { let pos = &mut self.indices[probe]; - if let Some(pos) = *pos { + if let Some(pos) = pos.as_valid_mut::(self.entries.len()) { let entry_hash = pos.hash(); // NOTE(i) we use unchecked indexing below let i = pos.index(); @@ -247,7 +302,7 @@ where } // empty bucket, insert here let index = self.entries.len(); - *pos = Some(Pos::new(index, hash)); + *pos = Pos::new(index, hash); unsafe { self.entries.push_unchecked(Bucket { hash, key, value }) }; return Insert::Success(Inserted { index, @@ -259,18 +314,18 @@ where } // phase 2 is post-insert where we forward-shift `Pos` in the indices. - fn insert_phase_2(indices: &mut [Option; N], mut probe: usize, mut old_pos: Pos) -> usize { + fn insert_phase_2(indices: &mut [Pos; N], mut probe: usize, mut old_pos: Pos) -> usize { probe_loop!(probe < indices.len(), { let pos = unsafe { indices.get_unchecked_mut(probe) }; let mut is_none = true; // work around lack of NLL - if let Some(pos) = pos.as_mut() { - old_pos = mem::replace(pos, old_pos); + if let Some(pos) = pos.assume_not_full() { + old_pos = pos.replace(old_pos); is_none = false; } if is_none { - *pos = Some(old_pos); + *pos = old_pos; return probe; } }); @@ -280,7 +335,9 @@ where // index `probe` and entry `found` is to be removed // use swap_remove, but then we need to update the index that points // to the other entry that has to move - self.indices[probe] = None; + self.indices[probe] = Pos::none(); + let old_probe = probe; + let old_len = self.entries.len(); let entry = unsafe { self.entries.swap_remove_unchecked(found) }; // correct index that points to the entry that had to swap places @@ -290,10 +347,13 @@ where let mut probe = entry.hash.desired_pos(Self::mask()); probe_loop!(probe < self.indices.len(), { - if let Some(pos) = self.indices[probe] { + if probe == old_probe { + continue; + } + if let Some(pos) = self.indices[probe].as_valid_mut::(old_len) { if pos.index() >= self.entries.len() { // found it - self.indices[probe] = Some(Pos::new(found, entry.hash)); + self.indices[probe] = Pos::new(found, entry.hash); break; } } @@ -316,11 +376,8 @@ where } fn reinsert_all(&mut self) { - const INIT: Option = None; if self.entries.len() < self.indices.len() { - for index in self.indices.iter_mut() { - *index = INIT; - } + self.indices = [Pos::none(); N]; for (index, entry) in self.entries.iter().enumerate() { let mut probe = entry.hash.desired_pos(Self::mask()); @@ -329,7 +386,7 @@ where probe_loop!(probe < self.indices.len(), { let pos = &mut self.indices[probe]; - if let Some(pos) = *pos { + if let Some(pos) = pos.as_valid_mut::(self.entries.len()) { let entry_hash = pos.hash(); // robin hood: steal the spot if it's better for us @@ -343,7 +400,7 @@ where break; } } else { - *pos = Some(Pos::new(index, entry.hash)); + *pos = Pos::new(index, entry.hash); break; } dist += 1; @@ -359,12 +416,12 @@ where let mut probe = probe_at_remove + 1; probe_loop!(probe < self.indices.len(), { - if let Some(pos) = self.indices[probe] { + if let Some(pos) = self.indices[probe].assume_not_full() { let entry_hash = pos.hash(); if entry_hash.probe_distance(Self::mask(), probe) > 0 { unsafe { *self.indices.get_unchecked_mut(last_probe) = self.indices[probe] } - self.indices[probe] = None; + self.indices[probe] = Pos::none(); } else { break; } @@ -749,6 +806,7 @@ impl IndexMap, N> { const { assert!(N > 1); assert!(N.is_power_of_two()); + assert!(N <= MAX_SIZE); } Self { @@ -980,7 +1038,7 @@ impl IndexMap { impl<'a, K, V, S, const N: usize> Drop for Guard<'a, K, V, S, N> { fn drop(&mut self) { for pos in self.0.core.indices.iter_mut() { - *pos = None; + *pos = Pos::none(); } } } @@ -1292,11 +1350,10 @@ where pub fn truncate(&mut self, len: usize) { self.core.entries.truncate(len); - if self.core.indices.len() > self.core.entries.len() { - for index in self.core.indices.iter_mut() { - match index { - Some(pos) if pos.index() >= len => *index = None, - _ => (), + if N > self.core.entries.len() { + for pos in self.core.indices.iter_mut() { + if pos.maybe_valid.index() >= len { + *pos = Pos::none(); } } } @@ -1374,6 +1431,7 @@ where const { assert!(N > 1); assert!(N.is_power_of_two()); + assert!(N <= MAX_SIZE); } Self { @@ -1625,6 +1683,7 @@ where mod tests { use core::mem; use std::{ + hash::{BuildHasher, Hash, Hasher}, mem::ManuallyDrop, panic::{catch_unwind, AssertUnwindSafe}, sync::atomic::{AtomicI32, Ordering}, @@ -1632,7 +1691,7 @@ mod tests { use static_assertions::assert_not_impl_any; - use super::{BuildHasherDefault, Entry, FnvIndexMap, IndexMap}; + use super::{BuildHasherDefault, Entry, FnvIndexMap, IndexMap, Pos}; // Ensure a `IndexMap` containing `!Send` keys stays `!Send` itself. assert_not_impl_any!(IndexMap<*const (), (), BuildHasherDefault<()>, 4>: Send); @@ -2124,4 +2183,72 @@ mod tests { assert_eq!(map.get(&4), Some(&444)); // ok assert_eq!(map.get(&2), Some(&222)); // <-- key present in iter() but unreachable } + + /// see + #[test] + #[cfg_attr(miri, ignore)] // too slow + fn insert_overflow() { + #[derive(PartialEq, Eq, Debug)] + struct CustomHashU16(u16); + + impl Hash for CustomHashU16 { + fn hash(&self, state: &mut H) { + state.write_u16(self.0); + } + } + + #[derive(Default)] + struct DummyHasher(u16); + + #[derive(Default)] + struct DummyHasherBuilder; + + impl Hasher for DummyHasher { + fn finish(&self) -> u64 { + self.0 as _ + } + + fn write(&mut self, _bytes: &[u8]) {} + fn write_u16(&mut self, i: u16) { + self.0 = i; + } + } + + impl BuildHasher for DummyHasherBuilder { + type Hasher = DummyHasher; + + fn build_hasher(&self) -> Self::Hasher { + DummyHasher(0) + } + } + + // We have to manually allocate and initialize to avoid overflowing the stack in the dev + // profile. + let map: Box>> = + Box::new_zeroed(); + // Safety: the default value of IndexMap is zeros + let mut map = unsafe { map.assume_init() }; + for x in 0..=u16::MAX { + map.insert(CustomHashU16(x), x).unwrap(); + } + assert!(map.is_full()); + assert!(map.core.indices[0xFFFF] == Pos::none()); + for x in 0..=u16::MAX { + assert_eq!(map.get(&CustomHashU16(x)).unwrap(), &x); + } + assert_eq!(map.remove(&CustomHashU16(0x123)).unwrap(), 0x123); + for x in 0..=u16::MAX { + if x == 0x123 { + continue; + } + assert_eq!(map.get(&CustomHashU16(x)).unwrap(), &x); + } + assert_eq!(map.remove(&CustomHashU16(u16::MAX)).unwrap(), u16::MAX); + for x in 0..=u16::MAX { + if x == 0x123 || x == u16::MAX { + continue; + } + assert_eq!(map.get(&CustomHashU16(x)).unwrap(), &x); + } + } } From cae1fab86f2667cd9266ee6877c9733e9f41c9af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Sat, 18 Jul 2026 13:46:56 +0200 Subject: [PATCH 3/7] Fix clippy lints behind feature flags --- .github/workflows/build.yml | 2 +- src/de.rs | 12 ++++++------ src/sorted_linked_list.rs | 2 +- src/vec/mod.rs | 4 +--- 4 files changed, 9 insertions(+), 11 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index db367e8d91..a95a6e95d4 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -113,7 +113,7 @@ jobs: with: components: clippy targets: i686-unknown-linux-musl - - run: cargo clippy --all --target i686-unknown-linux-musl --all-targets + - run: cargo clippy --all --target i686-unknown-linux-musl --all-targets --features "alloc,defmt,portable-atomic-critical-section,serde,ufmt,bytes,zeroize,embedded-io-v0.7" # Compilation check check: diff --git a/src/de.rs b/src/de.rs index bf1d1ecc48..0edbd22b44 100644 --- a/src/de.rs +++ b/src/de.rs @@ -43,7 +43,7 @@ where while let Some(value) = seq.next_element()? { if values.push(value).is_err() { - return Err(A::Error::invalid_length(values.capacity() + 1, &self))?; + return Err(A::Error::invalid_length(values.capacity() + 1, &self)); } } @@ -84,7 +84,7 @@ where while let Some(value) = seq.next_element()? { if values.insert(value).is_err() { - return Err(A::Error::invalid_length(values.capacity() + 1, &self))?; + return Err(A::Error::invalid_length(values.capacity() + 1, &self)); } } @@ -124,7 +124,7 @@ where while let Some(value) = seq.next_element()? { if values.push(value).is_err() { - return Err(A::Error::invalid_length(values.capacity() + 1, &self))?; + return Err(A::Error::invalid_length(values.capacity() + 1, &self)); } } @@ -163,7 +163,7 @@ where while let Some(value) = seq.next_element()? { if values.push_back(value).is_err() { - return Err(A::Error::invalid_length(values.capacity() + 1, &self))?; + return Err(A::Error::invalid_length(values.capacity() + 1, &self)); } } @@ -245,7 +245,7 @@ where while let Some((key, value)) = map.next_entry()? { if values.insert(key, value).is_err() { - return Err(A::Error::invalid_length(values.capacity() + 1, &self))?; + return Err(A::Error::invalid_length(values.capacity() + 1, &self)); } } @@ -286,7 +286,7 @@ where while let Some((key, value)) = map.next_entry()? { if values.insert(key, value).is_err() { - return Err(A::Error::invalid_length(values.capacity() + 1, &self))?; + return Err(A::Error::invalid_length(values.capacity() + 1, &self)); } } diff --git a/src/sorted_linked_list.rs b/src/sorted_linked_list.rs index 2190916a27..479239c943 100644 --- a/src/sorted_linked_list.rs +++ b/src/sorted_linked_list.rs @@ -1019,7 +1019,7 @@ mod tests { list.push(i).unwrap(); } - assert_eq!(list.is_empty(), false); + assert!(!list.is_empty()); assert!(list.is_full()); assert_eq!(list.peek(), Some(&8)); diff --git a/src/vec/mod.rs b/src/vec/mod.rs index e880a93270..86d1d83e45 100644 --- a/src/vec/mod.rs +++ b/src/vec/mod.rs @@ -2468,9 +2468,7 @@ mod tests { #[test] #[cfg(feature = "alloc")] fn alloc_to_heapless() { - let mut av: alloc::vec::Vec = alloc::vec::Vec::new(); - av.push(0); - av.push(1); + let av = vec![0, 1]; let hv: Vec = av.clone().try_into().unwrap(); assert_eq!(hv.as_slice(), av.as_slice()); From 48e2a4ba28a8b5ea9be5cc0eb4ad3627627e3d96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Fri, 24 Jul 2026 14:09:08 +0200 Subject: [PATCH 4/7] Limit lines to 100 char --- .github/workflows/build.yml | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index a95a6e95d4..e1947954be 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -48,7 +48,16 @@ jobs: components: miri - name: Run miri - run: MIRIFLAGS=-Zmiri-ignore-leaks cargo miri test --features="alloc,defmt,mpmc_large,portable-atomic-critical-section,serde,ufmt,bytes,zeroize,embedded-io-v0.7" + run: > + MIRIFLAGS=-Zmiri-ignore-leaks cargo miri test --features="alloc, + defmt, + mpmc_large, + portable-atomic-critical-section, + serde, + ufmt, + bytes, + zeroize, + embedded-io-v0.7" # Run cargo test test: @@ -84,7 +93,17 @@ jobs: toolchain: stable - name: Run cargo test - run: cargo test --features="alloc,defmt,mpmc_large,portable-atomic-critical-section,serde,ufmt,bytes,zeroize,embedded-io-v0.7" + run: > + cargo test --features=" + alloc, + defmt, + mpmc_large, + portable-atomic-critical-section, + serde, + ufmt, + bytes, + zeroize, + embedded-io-v0.7" # Run cargo fmt --check style: @@ -113,7 +132,15 @@ jobs: with: components: clippy targets: i686-unknown-linux-musl - - run: cargo clippy --all --target i686-unknown-linux-musl --all-targets --features "alloc,defmt,portable-atomic-critical-section,serde,ufmt,bytes,zeroize,embedded-io-v0.7" + - run: > + cargo clippy --all --target i686-unknown-linux-musl --all-targets --features "alloc, + defmt, + portable-atomic-critical-section, + serde, + ufmt, + bytes, + zeroize, + embedded-io-v0.7" # Compilation check check: From cd4f43c7f68ad18d48caa75d5bc021837dad225b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Sun, 26 Jul 2026 11:08:17 +0200 Subject: [PATCH 5/7] Fix wrong length being passed to as_valid As_valid expect the number of entries to be passed, not the length of the indices array, which is always `N` --- src/index_map.rs | 98 +++++++++++++++++++++++++++++++++++------------- 1 file changed, 72 insertions(+), 26 deletions(-) diff --git a/src/index_map.rs b/src/index_map.rs index 68282a2c66..8556c7be0b 100644 --- a/src/index_map.rs +++ b/src/index_map.rs @@ -240,7 +240,7 @@ where let mut dist = 0; probe_loop!(probe < self.indices.len(), { - let pos = self.indices[probe].as_valid::(self.indices.len())?; + let pos = self.indices[probe].as_valid::(self.entries.len())?; let entry_hash = pos.hash(); // NOTE(i) we use unchecked indexing below let i = pos.index(); @@ -1693,6 +1693,33 @@ mod tests { use super::{BuildHasherDefault, Entry, FnvIndexMap, IndexMap, Pos}; + /// A hasher that returns the `u16` value as the hash + /// This makes testing weird hashes easier + #[derive(Default)] + struct DummyHasher(u16); + + #[derive(Default)] + struct DummyHasherBuilder; + + impl Hasher for DummyHasher { + fn finish(&self) -> u64 { + self.0 as _ + } + + fn write(&mut self, _bytes: &[u8]) {} + fn write_u16(&mut self, i: u16) { + self.0 = i; + } + } + + impl BuildHasher for DummyHasherBuilder { + type Hasher = DummyHasher; + + fn build_hasher(&self) -> Self::Hasher { + DummyHasher(0) + } + } + // Ensure a `IndexMap` containing `!Send` keys stays `!Send` itself. assert_not_impl_any!(IndexMap<*const (), (), BuildHasherDefault<()>, 4>: Send); // Ensure a `IndexMap` containing `!Send` values stays `!Send` itself. @@ -2197,31 +2224,6 @@ mod tests { } } - #[derive(Default)] - struct DummyHasher(u16); - - #[derive(Default)] - struct DummyHasherBuilder; - - impl Hasher for DummyHasher { - fn finish(&self) -> u64 { - self.0 as _ - } - - fn write(&mut self, _bytes: &[u8]) {} - fn write_u16(&mut self, i: u16) { - self.0 = i; - } - } - - impl BuildHasher for DummyHasherBuilder { - type Hasher = DummyHasher; - - fn build_hasher(&self) -> Self::Hasher { - DummyHasher(0) - } - } - // We have to manually allocate and initialize to avoid overflowing the stack in the dev // profile. let map: Box>> = @@ -2251,4 +2253,48 @@ mod tests { assert_eq!(map.get(&CustomHashU16(x)).unwrap(), &x); } } + + /// Test that `as_valid` doesn't fail in the case N = `0x10000` + /// + /// The first implementation used `indices.len()` instead of `entries.len()` + /// causing `as_valid` to return incorrectly always return `Some` when N = `0x10000` + #[test] + fn indices_valid() { + // We have to manually allocate and initialize to avoid overflowing the stack in the dev + // profile. + let map: Box>> = Box::new_zeroed(); + // Safety: the default value of FnvIndexMap is zeros + let mut map = unsafe { map.assume_init() }; + + map.insert(11, 11).unwrap(); + assert!(map.find(&10).is_none()); + } + + /// Test that `as_valid` doesn't fail in the case N = `0x10000` + /// + /// Unlike the above test, here the queried value compares equal to a zeroed + /// value, which is the case for the uninitialized entries in the `entries` vec + /// because of the zeored allocation. + /// + /// This test doesn't rely on the `debug_assert` to detect incorrect behaviour + #[test] + fn indices_valid2() { + /// A key whose hash is *always* `0xFFFF`. + #[derive(PartialEq, Eq, Debug)] + struct ConstantHash(u16); + + impl Hash for ConstantHash { + fn hash(&self, state: &mut H) { + state.write_u16(0xFFFF); + } + } + + let map: Box>> = + Box::new_zeroed(); + let mut map = unsafe { map.assume_init() }; + + map.insert(ConstantHash(1), 1).unwrap(); + // Distinct key, same hash + assert!(map.get(&ConstantHash(0)).is_none()); + } } From 8a4f6d91a4c05813671818e9af302d36c873a503 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Sun, 26 Jul 2026 11:26:49 +0200 Subject: [PATCH 6/7] indexmap: Improve probe_loop! to support `continue` probe_loop! relied on code running after `$body`, which meant using `continue` in `$body` could break it. --- src/index_map.rs | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/src/index_map.rs b/src/index_map.rs index 8556c7be0b..11ad5891b7 100644 --- a/src/index_map.rs +++ b/src/index_map.rs @@ -189,15 +189,21 @@ struct Inserted { macro_rules! probe_loop { ($probe_var: ident < $len: expr, $body: expr) => { + // We use a separate value to store the probe value increments + // so that the increments are done before $body + // so that $body can use `continue` without + // interfering with the increments + let mut next_probe = $probe_var; loop { + $probe_var = next_probe; + next_probe += 1; if $probe_var < $len { $body - $probe_var += 1; } else { - $probe_var = 0; + next_probe = 0; } } - } + }; } #[cfg_attr( @@ -2297,4 +2303,16 @@ mod tests { // Distinct key, same hash assert!(map.get(&ConstantHash(0)).is_none()); } + + /// Test that `remove_found` doesn't fail + /// + /// Ensures that `probe_loop!` works correctly with the `continue` in the body + #[test] + fn remove_found_loop() { + let mut map: FnvIndexMap = IndexMap::default(); + map.insert(0, 0).unwrap(); + map.insert(4, 4).unwrap(); + map.insert(8, 8).unwrap(); + map.remove(&0).unwrap(); // never returns + } } From 728a114394f5db262ca9a4f593935132986375fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sosth=C3=A8ne=20Gu=C3=A9don?= Date: Tue, 28 Jul 2026 09:57:08 +0200 Subject: [PATCH 7/7] Improve documentation Document all conversions from `Pos` to `ValidPos`. Document all usages of `unsafe` Add a test with a map of maximum size filled to test the `valid_pos` conversions --- src/index_map.rs | 177 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 127 insertions(+), 50 deletions(-) diff --git a/src/index_map.rs b/src/index_map.rs index 11ad5891b7..e120deb16c 100644 --- a/src/index_map.rs +++ b/src/index_map.rs @@ -1,3 +1,5 @@ +#![deny(clippy::undocumented_unsafe_blocks)] + //! A fixed-capacity hash table where the iteration order is independent of the hash of the keys. use core::{ borrow::Borrow, @@ -91,6 +93,12 @@ struct Bucket { const MAX_SIZE: usize = 0x10000; +/// For a given hash in the `indices` table, encodes whether there is a corresponding +/// key/value pair and its index +/// +/// If there is no corresponding entry, its value is `Self::none`. +/// In the rare case where the `IndexMap` is of `MAX_SIZE`, `Self::none()` can encode +/// a valid entry. #[derive(Clone, Copy, PartialEq)] #[cfg_attr(feature = "zeroize", derive(Zeroize))] struct Pos { @@ -111,11 +119,18 @@ impl Pos { } } + /// Get the `ValidPos` from `self` + /// + /// SAFETY: the `ValidPos` is correct only if it is known + /// that `self` cannot be encoding a `None` value. + unsafe fn assume_valid(&mut self) -> &mut ValidPos { + &mut self.maybe_valid + } /// Returns a `ValidPos` if the pos doesn't encode `None` /// - /// This will give the correct information only if the entries vec length - /// is known to be less than 0x10000 - fn assume_not_full(&mut self) -> Option<&mut ValidPos> { + /// SAFETY: This will give the correct information only if it is known + /// that `self.index()` cannot be `0xFFFF` + unsafe fn assume_not_full(&mut self) -> Option<&mut ValidPos> { if self == &Self::none() { None } else { @@ -127,7 +142,7 @@ impl Pos { /// /// The argument is the length of the `entries` buffer fn as_valid(&self, buf_len: usize) -> Option<&ValidPos> { - // Is the buffer full? This uses a const generic to be able to short-circuit + // Is 0xFFFF a valid index ? This uses a const generic to be able to short-circuit // the check at compile time in most cases let buffer_max = BUFFER_SIZE == MAX_SIZE && buf_len == MAX_SIZE; if buffer_max || self != &Self::none() { @@ -141,7 +156,7 @@ impl Pos { /// /// The argument is the length of the `entries` buffer fn as_valid_mut(&mut self, buf_len: usize) -> Option<&mut ValidPos> { - // Is the buffer full? This uses a const generic to be able to short-circuit + // Is 0xFFFF a valid index ? This uses a const generic to be able to short-circuit // the check at compile time in most cases let buffer_max = BUFFER_SIZE == MAX_SIZE && buf_len == MAX_SIZE; if buffer_max || self != &Self::none() { @@ -152,6 +167,7 @@ impl Pos { } } +/// Encodes a hash value and the index of an existing key/value pair in `entries` #[cfg_attr(feature = "zeroize", derive(Zeroize))] #[derive(Clone, Copy, PartialEq)] struct ValidPos { @@ -256,6 +272,7 @@ where // give up when probe distance is too long return None; } else if entry_hash == hash + // SAFETY: We know that the entry is valid ande therefore so is its index && unsafe { self.entries.get_unchecked(i).key.borrow() == query } { return Some((probe, i)); @@ -286,17 +303,23 @@ where } // robin hood: steal the spot if it's better for us let index = self.entries.len(); + // SAFETY: we checked just above that the entries vec is not full unsafe { self.entries.push_unchecked(Bucket { hash, key, value }) }; Self::insert_phase_2(&mut self.indices, probe, Pos::new(index, hash)); return Insert::Success(Inserted { index, old_value: None, }); - } else if entry_hash == hash && unsafe { self.entries.get_unchecked(i).key == key } + } else if entry_hash == hash + // SAFETY: we know that the entry is valid, and therefore + // its index is occupied + && unsafe { self.entries.get_unchecked(i).key == key } { return Insert::Success(Inserted { index: i, old_value: Some(mem::replace( + // SAFETY: we know that the entry is valid, and therefore + // its index is occupied unsafe { &mut self.entries.get_unchecked_mut(i).value }, value, )), @@ -309,6 +332,7 @@ where // empty bucket, insert here let index = self.entries.len(); *pos = Pos::new(index, hash); + // SAFETY: we checked just above that entries is not full unsafe { self.entries.push_unchecked(Bucket { hash, key, value }) }; return Insert::Success(Inserted { index, @@ -322,28 +346,33 @@ where // phase 2 is post-insert where we forward-shift `Pos` in the indices. fn insert_phase_2(indices: &mut [Pos; N], mut probe: usize, mut old_pos: Pos) -> usize { probe_loop!(probe < indices.len(), { - let pos = unsafe { indices.get_unchecked_mut(probe) }; - - let mut is_none = true; // work around lack of NLL - if let Some(pos) = pos.assume_not_full() { + let pos = &mut indices[probe]; + + // SAFETY: We just inserted a key, meaning that the only + // `pos` that can have index `0xFFFF` is the one + // passed as argument to this function + // + // Since `pos` is read from the existing entries + // none can have index `0xFFFF` + if let Some(pos) = unsafe { pos.assume_not_full() } { old_pos = pos.replace(old_pos); - is_none = false; - } - - if is_none { + } else { *pos = old_pos; + return probe; } }); } - fn remove_found(&mut self, probe: usize, found: usize) -> (K, V) { + /// SAFETY: found must be a valid entry and probe the index of the corresponding `Pos` + unsafe fn remove_found(&mut self, probe: usize, found: usize) -> (K, V) { // index `probe` and entry `found` is to be removed // use swap_remove, but then we need to update the index that points // to the other entry that has to move self.indices[probe] = Pos::none(); let old_probe = probe; - let old_len = self.entries.len(); + // SAFETY: We know that the entry at `found` is valid as an invariant + // held by the caller let entry = unsafe { self.entries.swap_remove_unchecked(found) }; // correct index that points to the entry that had to swap places @@ -356,12 +385,15 @@ where if probe == old_probe { continue; } - if let Some(pos) = self.indices[probe].as_valid_mut::(old_len) { - if pos.index() >= self.entries.len() { - // found it - self.indices[probe] = Pos::new(found, entry.hash); - break; - } + // SAFETY: We know that the `Pos` are all valid because + // because any `None` would mean the index of the removed entry + // is further away than it has to. + let pos = unsafe { self.indices[probe].assume_valid() }; + debug_assert!(pos.index() <= self.entries.len()); + if pos.index() == self.entries.len() { + // found it + self.indices[probe] = Pos::new(found, entry.hash); + break; } }); } @@ -392,7 +424,8 @@ where probe_loop!(probe < self.indices.len(), { let pos = &mut self.indices[probe]; - if let Some(pos) = pos.as_valid_mut::(self.entries.len()) { + // SAFETY: we checked above that the entries vec is not full + if let Some(pos) = unsafe { pos.assume_not_full() } { let entry_hash = pos.hash(); // robin hood: steal the spot if it's better for us @@ -422,11 +455,16 @@ where let mut probe = probe_at_remove + 1; probe_loop!(probe < self.indices.len(), { - if let Some(pos) = self.indices[probe].assume_not_full() { + // SAFETY: We know that `pos.index() == 0xFFFF `is not possible because + // we just removed the last entry and removed the corresponding + // `Pos` in `self.indices` + if let Some(pos) = unsafe { self.indices[probe].assume_not_full() } { let entry_hash = pos.hash(); if entry_hash.probe_distance(Self::mask(), probe) > 0 { - unsafe { *self.indices.get_unchecked_mut(last_probe) = self.indices[probe] } + // SAFETY: we know that last_probe is < than N from the caller + // or because last_probe was overwritten by the loop + *unsafe { self.indices.get_unchecked_mut(last_probe) } = self.indices[probe]; self.indices[probe] = Pos::none(); } else { break; @@ -655,7 +693,9 @@ where /// Removes this entry from the map and yields its corresponding key and value pub fn remove_entry(self) -> (K, V) { - self.core.remove_found(self.probe, self.pos) + // SAFETY: We know that `pos` is valid from the creation of the entry + // and that cannot have changed since we held a mutable entry to the map + unsafe { self.core.remove_found(self.probe, self.pos) } } /// Gets a reference to the value associated with this entry @@ -725,11 +765,9 @@ where } else { match self.core.insert(self.hash_val, self.key, value) { Insert::Success(inserted) => { - unsafe { - // SAFETY: Already checked existence at instantiation and the only mutable - // reference to the map is internally held. - Ok(&mut (*self.core.entries.as_mut_ptr().add(inserted.index)).value) - } + // SAFETY: Already checked existence at instantiation and the only mutable + // reference to the map is internally held. + unsafe { Ok(&mut (*self.core.entries.as_mut_ptr().add(inserted.index)).value) } } Insert::Full((_, v)) => Err(v), } @@ -1114,8 +1152,10 @@ where K: Borrow, Q: ?Sized + Hash + Eq, { - self.find(key) - .map(|(_, found)| unsafe { &self.core.entries.get_unchecked(found).value }) + self.find(key).map(|(_, found)| { + // SAFETY: Find gives a correct bucket and the corresponding probe + unsafe { &self.core.entries.get_unchecked(found).value } + }) } /// Returns true if the map contains a value for the specified key. @@ -1168,6 +1208,7 @@ where Q: ?Sized + Hash + Eq, { if let Some((_, found)) = self.find(key) { + // SAFETY: Find gives a correct bucket and the corresponding probe Some(unsafe { &mut self.core.entries.get_unchecked_mut(found).value }) } else { None @@ -1315,8 +1356,10 @@ where K: Borrow, Q: ?Sized + Hash + Eq, { - self.find(key) - .map(|(probe, found)| self.core.remove_found(probe, found).1) + self.find(key).map(|(probe, found)| { + // SAFETY: Find gives a correct bucket and the corresponding probe + unsafe { self.core.remove_found(probe, found) }.1 + }) } /// Retains only the elements specified by the predicate. @@ -1697,6 +1740,8 @@ mod tests { use static_assertions::assert_not_impl_any; + use crate::index_map::MAX_SIZE; + use super::{BuildHasherDefault, Entry, FnvIndexMap, IndexMap, Pos}; /// A hasher that returns the `u16` value as the hash @@ -2230,12 +2275,7 @@ mod tests { } } - // We have to manually allocate and initialize to avoid overflowing the stack in the dev - // profile. - let map: Box>> = - Box::new_zeroed(); - // Safety: the default value of IndexMap is zeros - let mut map = unsafe { map.assume_init() }; + let mut map = max_size_map::(); for x in 0..=u16::MAX { map.insert(CustomHashU16(x), x).unwrap(); } @@ -2260,18 +2300,21 @@ mod tests { } } + fn max_size_map() -> Box> { + // We have to manually allocate and initialize to avoid overflowing the stack in the dev + // profile. + let map: Box>> = Box::new_zeroed(); + // SAFETY: the default value of FnvIndexMap is zeros + unsafe { map.assume_init() } + } + /// Test that `as_valid` doesn't fail in the case N = `0x10000` /// /// The first implementation used `indices.len()` instead of `entries.len()` /// causing `as_valid` to return incorrectly always return `Some` when N = `0x10000` #[test] fn indices_valid() { - // We have to manually allocate and initialize to avoid overflowing the stack in the dev - // profile. - let map: Box>> = Box::new_zeroed(); - // Safety: the default value of FnvIndexMap is zeros - let mut map = unsafe { map.assume_init() }; - + let mut map = max_size_map::(); map.insert(11, 11).unwrap(); assert!(map.find(&10).is_none()); } @@ -2295,15 +2338,49 @@ mod tests { } } - let map: Box>> = - Box::new_zeroed(); - let mut map = unsafe { map.assume_init() }; + let mut map = max_size_map::(); map.insert(ConstantHash(1), 1).unwrap(); // Distinct key, same hash assert!(map.get(&ConstantHash(0)).is_none()); } + #[test] + #[cfg_attr(miri, ignore)] // too slow + fn index_none_valid() { + /// A key whose hash is *always* `0xFFFF`. + #[derive(PartialEq, Eq, Debug)] + struct ControlledHash(u16, u16); + + impl Hash for ControlledHash { + fn hash(&self, state: &mut H) { + state.write_u16(self.0); + } + } + + let mut map = max_size_map::(); + for i in 0..=u16::MAX { + map.insert(ControlledHash(i, 0), i as u32).unwrap(); + } + + let Entry::Vacant(entry) = map.entry(ControlledHash(0xFFFF, 1)) else { + panic!("Should be vacant"); + }; + entry.insert(0xFFFF).unwrap_err(); + let Entry::Occupied(entry) = map.entry(ControlledHash(0xFFFF, 0)) else { + panic!("Should be occupied"); + }; + entry.insert(0x10000); + + assert_eq!(map.get(&ControlledHash(0xFFFF, 0)), Some(&0x10000)); + assert_eq!(map.remove(&ControlledHash(0xFFFF, 0)), Some(0x10000)); + map.insert(ControlledHash(0xFFFF, 0), 0xFFFF).unwrap(); + assert_eq!(map.remove(&ControlledHash(0xFFFE, 0)), Some(0xFFFE)); + assert_eq!(map.get(&ControlledHash(0xFFFF, 0)), Some(&0xFFFF)); + assert_eq!(map.remove(&ControlledHash(0xFFFF, 0)), Some(0xFFFF)); + assert!(map.get(&ControlledHash(0xFFFF, 0)).is_none()); + } + /// Test that `remove_found` doesn't fail /// /// Ensures that `probe_loop!` works correctly with the `continue` in the body