craft-cms: confirm ICC identity by bytes, not the 64-bit hash alone (#822)

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

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)]

crates/cms/src/transform.rs

@@ -805,20 +805,34 @@ pub fn black_point(p: &Profile, intent: Intent, _input: bool) -> Result<f64, Cms
 
 type Key = (u64, u64, TransformOptions);
 
-/// Process-wide cache of transforms keyed by profile content and options.
+/// A cached transform with the ICC bytes of the profiles it was built from.
+struct Entry {
+    src: Arc<Vec<u8>>,
+    dst: Arc<Vec<u8>>,
+    transform: Arc<Transform>,
+}
+
+/// 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<Arc<Transform>, CmsError> {
-    static CACHE: OnceLock<Mutex<HashMap<Key, Arc<Transform>>>> = OnceLock::new();
+    static CACHE: OnceLock<Mutex<HashMap<Key, Entry>>> = 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)
 }