From 4c6fe7c8648c88b79954630fa1fc3e5aee0227f2 Mon Sep 17 00:00:00 2001 From: "Dylan M. Taylor" Date: Wed, 11 Nov 2026 13:53:39 -0500 Subject: [PATCH] craft-cms: confirm ICC identity by bytes, not the 64-bit hash alone (#822) --- crates/cms/README.md | 3 +- crates/cms/src/profile.rs | 57 +++++++++++++++++++++++++++++++++++-- crates/cms/src/transform.rs | 24 ++++++++++++---- 3 files changed, 76 insertions(+), 8 deletions(-) diff --git a/crates/cms/README.md b/crates/cms/README.md index 585eb708c..7953539ba 100644 --- a/crates/cms/README.md +++ b/crates/cms/README.md @@ -42,7 +42,8 @@ Buffers are checked once per call, before any pixel is written. when its ICC bytes are not available (see [Selecting by profile name](#selecting-by-profile-name)). * `Profile::same_colors(&other)`: the same colour space whatever the encoding (every sample of a device grid within ΔE 0.5 through both), e.g. Photoshop's sRGB IEC61966-2.1 and the built-in - sRGB. `content_hash()` compares bytes and suits cache keys only. + sRGB. `same_bytes(&other)` compares the ICC bytes. `content_hash()` is a 64-bit hash of them + that another profile can share: a cache key, confirmed with `same_bytes`. * `Profile::named_colors()`: a named-colour profile's `ncl2` table (spot-colour libraries), with names joined from prefix, root and suffix, and Lab read in the legacy 16-bit encoding it uses in every profile version. `NamedColors::to_device(name, dst)` converts one into a destination diff --git a/crates/cms/src/profile.rs b/crates/cms/src/profile.rs index c5e4f0101..b7e389b7b 100644 --- a/crates/cms/src/profile.rs +++ b/crates/cms/src/profile.rs @@ -213,7 +213,9 @@ impl Profile { self.color_space.channels() } - /// Hash of the profile's ICC bytes (stable within a process; used as a cache key). + /// Hash of the profile's ICC bytes (stable within a process; used as a cache key). It is a + /// fixed-key 64-bit hash, so a crafted profile can share another's: equal hashes only narrow + /// the search, [`Profile::same_bytes`] decides. pub fn content_hash(&self) -> u64 { *self.hash.get_or_init(|| { use std::hash::{Hash, Hasher}; @@ -223,6 +225,16 @@ impl Profile { }) } + /// Whether `self` and `other` have the same ICC bytes. + pub fn same_bytes(&self, other: &Profile) -> bool { + if let (Some(a), Some(b)) = (&self.bytes, &other.bytes) + && Arc::ptr_eq(a, b) + { + return true; + } + self.content_hash() == other.content_hash() && self.to_bytes() == other.to_bytes() + } + /// Whether `self` and `other` describe the same colours: same colour space, and every sample /// of a device-value grid has the same colour through both (relative colorimetric, within /// [`SAME_COLORS_MAX_DELTA_E`]). Different encodings of one space are the same colours: the @@ -230,7 +242,7 @@ impl Profile { /// parametric curves) differ in every byte. Colours are compared rather than round-tripped /// because CMYK → PCS → CMYK is not an identity even through a single profile. pub fn same_colors(&self, other: &Profile) -> bool { - if self.content_hash() == other.content_hash() { + if self.same_bytes(other) { return true; } let n = self.channels(); @@ -848,6 +860,7 @@ fn parse_text(d: &[u8]) -> String { #[cfg(test)] mod tests { use super::*; + use crate::builtin::Builtin; /// The count from a `curv` tag must be bounded before `12 + n * 2` is computed: on a 32-bit /// target the multiplication panics with overflow checks on, which is what the guard exists @@ -873,6 +886,46 @@ mod tests { d.extend_from_slice(&[0u8; 12]); assert!(parse_text(&d).is_empty()); } + + /// `p` with the content hash of `other`: the hash is a fixed-key 64-bit SipHash, so a + /// crafted profile can have another's. Setting it stands in for finding one. + fn with_hash_of(p: &Profile, other: &Profile) -> Profile { + let p = Profile { hash: std::sync::OnceLock::from(other.content_hash()), ..p.clone() }; + assert_eq!(p.content_hash(), other.content_hash()); + p + } + + #[test] + fn a_hash_collision_is_not_the_same_colours() { + let srgb = Builtin::Srgb.profile(); + let p3 = with_hash_of(Builtin::DisplayP3.profile(), srgb); + assert!(!p3.same_bytes(srgb)); + assert!(!p3.same_colors(srgb)); + // Controls: equal bytes are the same, whatever object holds them. + assert!(srgb.same_bytes(srgb)); + assert!(srgb.same_bytes(&Profile::parse(&srgb.to_bytes()).unwrap())); + assert!(Builtin::DisplayP3.profile().same_colors(Builtin::DisplayP3.profile())); + } + + #[test] + fn a_hash_collision_does_not_get_the_cached_transform() { + use crate::transform::{Transform, TransformOptions, cached}; + let (srgb, lab) = (Builtin::Srgb.profile(), Builtin::LabD50.profile()); + let p3 = with_hash_of(Builtin::DisplayP3.profile(), srgb); + let opts = TransformOptions::default(); + let green = [0.0, 1.0, 0.0]; + let eval = |t: &Transform| { + let mut out = [0f32; 3]; + t.eval(&green, &mut out).unwrap(); + out + }; + let want = eval(&Transform::with_options(Builtin::DisplayP3.profile(), lab, opts).unwrap()); + assert_ne!(want, eval(&Transform::with_options(srgb, lab, opts).unwrap()), "P3 green is not sRGB green"); + let sr = eval(&cached(srgb, lab, opts).unwrap()); + assert_eq!(eval(&cached(&p3, lab, opts).unwrap()), want); + // Control: sRGB still gets its own transform after the colliding lookup. + assert_eq!(eval(&cached(srgb, lab, opts).unwrap()), sr); + } } #[cfg(test)] diff --git a/crates/cms/src/transform.rs b/crates/cms/src/transform.rs index 7a8aeb0a8..461881fd6 100644 --- a/crates/cms/src/transform.rs +++ b/crates/cms/src/transform.rs @@ -805,20 +805,34 @@ pub fn black_point(p: &Profile, intent: Intent, _input: bool) -> Result>, + dst: Arc>, + transform: Arc, +} + +/// Process-wide cache of transforms keyed by profile content and options. The key is the +/// profiles' 64-bit hashes, which a crafted profile can share with another, so a hit counts only +/// when the bytes match too; a colliding pair gets its own, uncached transform. pub fn cached(src: &Profile, dst: &Profile, opts: TransformOptions) -> Result, CmsError> { - static CACHE: OnceLock>>> = OnceLock::new(); + static CACHE: OnceLock>> = OnceLock::new(); let key = (src.content_hash(), dst.content_hash(), opts); + let (src_bytes, dst_bytes) = (src.to_bytes(), dst.to_bytes()); let cache = CACHE.get_or_init(Default::default); - if let Some(t) = cache.lock().unwrap_or_else(|e| e.into_inner()).get(&key) { - return Ok(t.clone()); + let hit = + cache.lock().unwrap_or_else(|e| e.into_inner()).get(&key).map(|e| (e.src == src_bytes && e.dst == dst_bytes).then(|| e.transform.clone())); + match hit { + Some(Some(t)) => return Ok(t), + Some(None) => return Ok(Arc::new(Transform::with_options(src, dst, opts)?)), + None => {} } let t = Arc::new(Transform::with_options(src, dst, opts)?); let mut c = cache.lock().unwrap_or_else(|e| e.into_inner()); if c.len() > 64 { c.clear(); } - c.insert(key, t.clone()); + c.insert(key, Entry { src: src_bytes, dst: dst_bytes, transform: t.clone() }); Ok(t) }