From b9c295c1baaa9b0df81c6496556620e6539908cc Mon Sep 17 00:00:00 2001 From: Rain Date: Tue, 21 Jul 2026 15:13:23 -0700 Subject: [PATCH] [spr] initial version Created using spr 1.3.6-beta.1 --- CHANGELOG.md | 2 + crates/iddqd/src/bi_hash_map/serde_impls.rs | 44 +++--- crates/iddqd/src/id_hash_map/serde_impls.rs | 44 +++--- crates/iddqd/src/id_ord_map/serde_impls.rs | 12 +- crates/iddqd/src/support/mod.rs | 2 + crates/iddqd/src/support/size_hint.rs | 14 ++ crates/iddqd/src/tri_hash_map/serde_impls.rs | 44 +++--- crates/iddqd/tests/integration/main.rs | 2 + .../tests/integration/serde_size_hint.rs | 133 ++++++++++++++++++ 9 files changed, 216 insertions(+), 81 deletions(-) create mode 100644 crates/iddqd/src/support/size_hint.rs create mode 100644 crates/iddqd/tests/integration/serde_size_hint.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 45d81adb..d583c83e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,8 @@ ### Fixed +- Deserialization no longer preallocates based on an unbounded size hint. Length-prefixed formats such as bincode and postcard derive their size hint from the input, so a small hostile payload claiming a huge number of elements could previously cause an excessively large allocation before any element was read. Preallocation is now capped at 1 MiB worth of items, matching what `serde` does for the standard library's collections. + - The `insert_overwrite` path on `IdHashMap` no longer aborts when an allocation fails, matching the existing guarantee on `BiHashMap` and `TriHashMap`. Instead, it results in a catchable panic. (The map is left unchanged, similar to `BiHashMap` and `TriHashMap`.) Note that `BTreeMap::insert_overwrite` will abort on allocation failure, because it calls into `std` which doesn't have an equivalent to `HashMap::try_reserve`. diff --git a/crates/iddqd/src/bi_hash_map/serde_impls.rs b/crates/iddqd/src/bi_hash_map/serde_impls.rs index 9173e26e..53cab93c 100644 --- a/crates/iddqd/src/bi_hash_map/serde_impls.rs +++ b/crates/iddqd/src/bi_hash_map/serde_impls.rs @@ -1,6 +1,9 @@ use crate::{ BiHashItem, BiHashMap, DefaultHashBuilder, - support::alloc::{Allocator, Global}, + support::{ + alloc::{Allocator, Global}, + size_hint::cautious, + }, }; use core::{fmt, hash::BuildHasher, marker::PhantomData}; use serde_core::{ @@ -193,14 +196,11 @@ where where Access: SeqAccess<'de>, { - let mut map = match seq.size_hint() { - Some(size) => BiHashMap::with_capacity_and_hasher_in( - size, - self.hasher, - self.alloc, - ), - None => BiHashMap::with_hasher_in(self.hasher, self.alloc), - }; + let mut map = BiHashMap::with_capacity_and_hasher_in( + cautious::(seq.size_hint()), + self.hasher, + self.alloc, + ); while let Some(element) = seq.next_element()? { map.insert_unique(element) @@ -217,14 +217,11 @@ where where Access: MapAccess<'de>, { - let mut map = match map_access.size_hint() { - Some(size) => BiHashMap::with_capacity_and_hasher_in( - size, - self.hasher, - self.alloc, - ), - None => BiHashMap::with_hasher_in(self.hasher, self.alloc), - }; + let mut map = BiHashMap::with_capacity_and_hasher_in( + cautious::(map_access.size_hint()), + self.hasher, + self.alloc, + ); while let Some((_, value)) = map_access.next_entry::()? @@ -312,14 +309,11 @@ where where Access: MapAccess<'de>, { - let mut map = match map_access.size_hint() { - Some(size) => BiHashMap::with_capacity_and_hasher_in( - size, - self.hasher, - self.alloc, - ), - None => BiHashMap::with_hasher_in(self.hasher, self.alloc), - }; + let mut map = BiHashMap::with_capacity_and_hasher_in( + cautious::(map_access.size_hint()), + self.hasher, + self.alloc, + ); while let Some((_, value)) = map_access.next_entry::()? diff --git a/crates/iddqd/src/id_hash_map/serde_impls.rs b/crates/iddqd/src/id_hash_map/serde_impls.rs index 3b7ff63e..6347afad 100644 --- a/crates/iddqd/src/id_hash_map/serde_impls.rs +++ b/crates/iddqd/src/id_hash_map/serde_impls.rs @@ -1,6 +1,9 @@ use crate::{ DefaultHashBuilder, IdHashItem, IdHashMap, - support::alloc::{Allocator, Global}, + support::{ + alloc::{Allocator, Global}, + size_hint::cautious, + }, }; use core::{fmt, hash::BuildHasher, marker::PhantomData}; use serde_core::{ @@ -188,14 +191,11 @@ where where Access: SeqAccess<'de>, { - let mut map = match seq.size_hint() { - Some(size) => IdHashMap::with_capacity_and_hasher_in( - size, - self.hasher, - self.alloc, - ), - None => IdHashMap::with_hasher_in(self.hasher, self.alloc), - }; + let mut map = IdHashMap::with_capacity_and_hasher_in( + cautious::(seq.size_hint()), + self.hasher, + self.alloc, + ); while let Some(element) = seq.next_element()? { map.insert_unique(element) @@ -212,14 +212,11 @@ where where Access: MapAccess<'de>, { - let mut map = match map_access.size_hint() { - Some(size) => IdHashMap::with_capacity_and_hasher_in( - size, - self.hasher, - self.alloc, - ), - None => IdHashMap::with_hasher_in(self.hasher, self.alloc), - }; + let mut map = IdHashMap::with_capacity_and_hasher_in( + cautious::(map_access.size_hint()), + self.hasher, + self.alloc, + ); while let Some((_, value)) = map_access.next_entry::()? @@ -301,14 +298,11 @@ where where Access: MapAccess<'de>, { - let mut map = match map_access.size_hint() { - Some(size) => IdHashMap::with_capacity_and_hasher_in( - size, - self.hasher, - self.alloc, - ), - None => IdHashMap::with_hasher_in(self.hasher, self.alloc), - }; + let mut map = IdHashMap::with_capacity_and_hasher_in( + cautious::(map_access.size_hint()), + self.hasher, + self.alloc, + ); while let Some((_, value)) = map_access.next_entry::()? diff --git a/crates/iddqd/src/id_ord_map/serde_impls.rs b/crates/iddqd/src/id_ord_map/serde_impls.rs index dd93ac03..c110a3c0 100644 --- a/crates/iddqd/src/id_ord_map/serde_impls.rs +++ b/crates/iddqd/src/id_ord_map/serde_impls.rs @@ -1,4 +1,5 @@ use super::{IdOrdItem, IdOrdMap}; +use crate::support::size_hint::cautious; use core::{fmt, marker::PhantomData}; use serde_core::{ Deserialize, Deserializer, Serialize, Serializer, @@ -110,10 +111,7 @@ where where Access: SeqAccess<'de>, { - let mut map = match seq.size_hint() { - Some(size) => IdOrdMap::with_capacity(size), - None => IdOrdMap::new(), - }; + let mut map = IdOrdMap::with_capacity(cautious::(seq.size_hint())); while let Some(element) = seq.next_element()? { map.insert_unique(element) @@ -130,7 +128,8 @@ where where Access: MapAccess<'de>, { - let mut map = IdOrdMap::new(); + let mut map = + IdOrdMap::with_capacity(cautious::(map_access.size_hint())); while let Some((_, value)) = map_access.next_entry::()? @@ -203,7 +202,8 @@ where where Access: MapAccess<'de>, { - let mut map = IdOrdMap::new(); + let mut map = + IdOrdMap::with_capacity(cautious::(map_access.size_hint())); while let Some((_, value)) = map_access.next_entry::()? diff --git a/crates/iddqd/src/support/mod.rs b/crates/iddqd/src/support/mod.rs index 1da16df5..1b63e35a 100644 --- a/crates/iddqd/src/support/mod.rs +++ b/crates/iddqd/src/support/mod.rs @@ -14,5 +14,7 @@ pub(crate) mod item_set; pub(crate) mod map_hash; #[cfg(feature = "schemars08")] pub(crate) mod schemars_utils; +#[cfg(feature = "serde")] +pub(crate) mod size_hint; pub(crate) use item_index::ItemIndex; diff --git a/crates/iddqd/src/support/size_hint.rs b/crates/iddqd/src/support/size_hint.rs new file mode 100644 index 00000000..0ce42786 --- /dev/null +++ b/crates/iddqd/src/support/size_hint.rs @@ -0,0 +1,14 @@ +use core::{cmp, mem}; + +// A mirror of +// https://github.com/serde-rs/serde/blob/7fc3b4c30c94f73a96ebd1553f2b090d928fc3a8/serde_core/src/private/size_hint.rs#L12, +// used to cap size_hint-based preallocation to a reasonable value. +pub(crate) fn cautious(hint: Option) -> usize { + const MAX_PREALLOC_BYTES: usize = 1024 * 1024; + + if mem::size_of::() == 0 { + 0 + } else { + cmp::min(hint.unwrap_or(0), MAX_PREALLOC_BYTES / mem::size_of::()) + } +} diff --git a/crates/iddqd/src/tri_hash_map/serde_impls.rs b/crates/iddqd/src/tri_hash_map/serde_impls.rs index 6fa8c481..6c4cce1d 100644 --- a/crates/iddqd/src/tri_hash_map/serde_impls.rs +++ b/crates/iddqd/src/tri_hash_map/serde_impls.rs @@ -1,6 +1,9 @@ use crate::{ DefaultHashBuilder, TriHashItem, TriHashMap, - support::alloc::{Allocator, Global}, + support::{ + alloc::{Allocator, Global}, + size_hint::cautious, + }, }; use core::{fmt, hash::BuildHasher, marker::PhantomData}; use serde_core::{ @@ -197,14 +200,11 @@ where where Access: SeqAccess<'de>, { - let mut map = match seq.size_hint() { - Some(size) => TriHashMap::with_capacity_and_hasher_in( - size, - self.hasher, - self.alloc, - ), - None => TriHashMap::with_hasher_in(self.hasher, self.alloc), - }; + let mut map = TriHashMap::with_capacity_and_hasher_in( + cautious::(seq.size_hint()), + self.hasher, + self.alloc, + ); while let Some(element) = seq.next_element()? { map.insert_unique(element) @@ -221,14 +221,11 @@ where where Access: MapAccess<'de>, { - let mut map = match map_access.size_hint() { - Some(size) => TriHashMap::with_capacity_and_hasher_in( - size, - self.hasher, - self.alloc, - ), - None => TriHashMap::with_hasher_in(self.hasher, self.alloc), - }; + let mut map = TriHashMap::with_capacity_and_hasher_in( + cautious::(map_access.size_hint()), + self.hasher, + self.alloc, + ); while let Some((_, value)) = map_access.next_entry::()? @@ -321,14 +318,11 @@ where where Access: MapAccess<'de>, { - let mut map = match map_access.size_hint() { - Some(size) => TriHashMap::with_capacity_and_hasher_in( - size, - self.hasher, - self.alloc, - ), - None => TriHashMap::with_hasher_in(self.hasher, self.alloc), - }; + let mut map = TriHashMap::with_capacity_and_hasher_in( + cautious::(map_access.size_hint()), + self.hasher, + self.alloc, + ); while let Some((_, value)) = map_access.next_entry::()? diff --git a/crates/iddqd/tests/integration/main.rs b/crates/iddqd/tests/integration/main.rs index 83f2cf2d..503d8698 100644 --- a/crates/iddqd/tests/integration/main.rs +++ b/crates/iddqd/tests/integration/main.rs @@ -7,6 +7,8 @@ mod id_ord_map; mod pathological; #[cfg(feature = "schemars08")] mod schemars_tests; +#[cfg(all(feature = "serde", feature = "std", feature = "default-hasher"))] +mod serde_size_hint; #[cfg(all( feature = "std", feature = "default-hasher", diff --git a/crates/iddqd/tests/integration/serde_size_hint.rs b/crates/iddqd/tests/integration/serde_size_hint.rs new file mode 100644 index 00000000..f6f87781 --- /dev/null +++ b/crates/iddqd/tests/integration/serde_size_hint.rs @@ -0,0 +1,133 @@ +use iddqd::{ + BiHashMap, IdHashMap, IdOrdMap, TriHashMap, bi_hash_map::BiHashMapAsMap, + id_hash_map::IdHashMapAsMap, id_ord_map::IdOrdMapAsMap, + tri_hash_map::TriHashMapAsMap, +}; +use iddqd_test_utils::test_item::TestItem; +use serde::{ + Deserialize, + de::{ + DeserializeSeed, Deserializer, MapAccess, SeqAccess, Visitor, + value::Error, + }, +}; + +#[derive(Clone, Copy)] +enum Shape { + Seq, + Map, +} + +struct LyingDeserializer(Shape); + +impl<'de> Deserializer<'de> for LyingDeserializer { + type Error = Error; + + fn deserialize_any(self, visitor: V) -> Result + where + V: Visitor<'de>, + { + match self.0 { + Shape::Seq => visitor.visit_seq(LyingAccess), + Shape::Map => visitor.visit_map(LyingAccess), + } + } + + serde::forward_to_deserialize_any! { + bool i8 i16 i32 i64 i128 u8 u16 u32 u64 u128 f32 f64 char str string + bytes byte_buf option unit unit_struct newtype_struct seq tuple + tuple_struct map struct enum identifier ignored_any + } +} + +struct LyingAccess; + +impl<'de> SeqAccess<'de> for LyingAccess { + type Error = Error; + + fn next_element_seed( + &mut self, + _seed: T, + ) -> Result, Self::Error> + where + T: DeserializeSeed<'de>, + { + Ok(None) + } + + fn size_hint(&self) -> Option { + // This would cause a crash if we didn't cap the size hint to a + // reasonable value. + Some(usize::MAX) + } +} + +impl<'de> MapAccess<'de> for LyingAccess { + type Error = Error; + + fn next_key_seed( + &mut self, + _seed: K, + ) -> Result, Self::Error> + where + K: DeserializeSeed<'de>, + { + Ok(None) + } + + fn next_value_seed(&mut self, _seed: V) -> Result + where + V: DeserializeSeed<'de>, + { + unreachable!("next_key_seed always returns None") + } + + fn size_hint(&self) -> Option { + // This would cause a crash if we didn't cap the size hint to a + // reasonable value. + Some(usize::MAX) + } +} + +/// Test that a very large size_hint is not trusted, and that the preallocated +/// size is capped to a reasonable amount instead. +#[test] +fn huge_size_hint_does_not_preallocate() { + for shape in [Shape::Seq, Shape::Map] { + let map = IdHashMap::::deserialize(LyingDeserializer(shape)) + .expect("deserialized empty IdHashMap"); + assert_eq!(map.len(), 0); + + let map = IdOrdMap::::deserialize(LyingDeserializer(shape)) + .expect("deserialized empty IdOrdMap"); + assert_eq!(map.len(), 0); + + let map = BiHashMap::::deserialize(LyingDeserializer(shape)) + .expect("deserialized empty BiHashMap"); + assert_eq!(map.len(), 0); + + let map = TriHashMap::::deserialize(LyingDeserializer(shape)) + .expect("deserialized empty TriHashMap"); + assert_eq!(map.len(), 0); + } + + let map = + IdHashMapAsMap::::deserialize(LyingDeserializer(Shape::Map)) + .expect("deserialized empty IdHashMap from map"); + assert_eq!(map.len(), 0); + + let map = + IdOrdMapAsMap::::deserialize(LyingDeserializer(Shape::Map)) + .expect("deserialized empty IdOrdMap from map"); + assert_eq!(map.len(), 0); + + let map = + BiHashMapAsMap::::deserialize(LyingDeserializer(Shape::Map)) + .expect("deserialized empty BiHashMap from map"); + assert_eq!(map.len(), 0); + + let map = + TriHashMapAsMap::::deserialize(LyingDeserializer(Shape::Map)) + .expect("deserialized empty TriHashMap from map"); + assert_eq!(map.len(), 0); +}