[pbs-devel] [PATCH pbs-api-types] crypto: improve deserialization for Fingerprint with custom visitor

* [pbs-devel] [PATCH pbs-api-types] crypto: improve deserialization for Fingerprint with custom visitor
@ 2025-09-26 11:08 Nicolas Frey
  2025-10-30 12:43 ` [pbs-devel] applied: " Fabian Grünbichler
  0 siblings, 1 reply; 2+ messages in thread
From: Nicolas Frey @ 2025-09-26 11:08 UTC (permalink / raw)
  To: pbs-devel

Improve Fingerprint deserialization by introducing a custom Visitor,
which:
- eliminates unsafe code by zero-initializing buffers
- removes unnecessary allocations by using a fixed-size buffer
- improves efficiency by filtering and decoding directly from input

Also added tests to establish baseline behavior and verify
correctness.

Signed-off-by: Nicolas Frey <n.frey@proxmox.com>
---
 pbs-api-types/src/crypto.rs | 102 ++++++++++++++++++++++++++++++++----
 1 file changed, 92 insertions(+), 10 deletions(-)

diff --git a/pbs-api-types/src/crypto.rs b/pbs-api-types/src/crypto.rs
index cdc1ba64..a9528022 100644
--- a/pbs-api-types/src/crypto.rs
+++ b/pbs-api-types/src/crypto.rs
@@ -67,9 +67,9 @@ fn as_fingerprint(bytes: &[u8]) -> String {
 }
 
 pub mod bytes_as_fingerprint {
-    use std::mem::MaybeUninit;
+    use std::fmt;
 
-    use serde::{Deserialize, Deserializer, Serializer};
+    use serde::{de, Deserializer, Serializer};
 
     pub fn serialize<S>(bytes: &[u8; 32], serializer: S) -> Result<S::Ok, S::Error>
     where
@@ -83,13 +83,95 @@ pub mod bytes_as_fingerprint {
     where
         D: Deserializer<'de>,
     {
-        // TODO: more efficiently implement with a Visitor implementing visit_str using split() and
-        // hex::decode by-byte
-        let mut s = String::deserialize(deserializer)?;
-        s.retain(|c| c != ':');
-        let mut out = MaybeUninit::<[u8; 32]>::uninit();
-        hex::decode_to_slice(s.as_bytes(), unsafe { &mut (*out.as_mut_ptr())[..] })
-            .map_err(serde::de::Error::custom)?;
-        Ok(unsafe { out.assume_init() })
+        struct FingerprintVisitor;
+
+        impl<'de> de::Visitor<'de> for FingerprintVisitor {
+            type Value = [u8; 32];
+
+            fn expecting(&self, f: &mut fmt::Formatter) -> fmt::Result {
+                f.write_str("a 32-byte fingerprint hex string with colons")
+            }
+
+            fn visit_str<E>(self, val: &str) -> Result<Self::Value, E>
+            where
+                E: de::Error,
+            {
+                let mut filtered = [0u8; 64];
+                let mut idx = 0;
+
+                for &b in val.as_bytes().iter().filter(|&&b| b != b':') {
+                    if idx == 64 {
+                        return Err(E::custom("fingerprint too long"));
+                    }
+                    filtered[idx] = b;
+                    idx += 1;
+                }
+
+                if idx != 64 {
+                    return Err(E::custom("fingerprint too short"));
+                }
+
+                let mut out = [0u8; 32];
+                hex::decode_to_slice(&filtered, &mut out).map_err(serde::de::Error::custom)?;
+                Ok(out)
+            }
+        }
+
+        deserializer.deserialize_str(FingerprintVisitor)
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::Fingerprint;
+
+    static SAMPLE_BYTES: [u8; 32] = [
+        0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88, 0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee,
+        0xff, 0x10, 0x20, 0x30, 0x40, 0x50, 0x60, 0x70, 0x80, 0x90, 0xa0, 0xb0, 0xc0, 0xd0, 0xe0,
+        0xf0, 0x01,
+    ];
+
+    #[test]
+    fn serialize_valid() {
+        let s = Fingerprint::new(SAMPLE_BYTES);
+        let encoded = serde_plain::to_string(&s).unwrap();
+        assert!(encoded.contains("00:11:22:33:44:55:66:77"));
+        assert!(encoded.contains("f0:01"));
+    }
+
+    #[test]
+    fn deserialize_valid() {
+        let s = "00:11:22:33:44:55:66:77:88:99:aa:bb:cc:dd:ee:ff:10:20:30:40:50:60:70:80:90:a0:b0:c0:d0:e0:f0:01";
+        let parsed = serde_plain::from_str::<Fingerprint>(s).unwrap();
+        assert_eq!(parsed.bytes(), &SAMPLE_BYTES);
+    }
+
+    #[test]
+    fn roundtrip() {
+        let original = Fingerprint::new(SAMPLE_BYTES);
+        let encoded = serde_plain::to_string(&original).unwrap();
+        let decoded: Fingerprint = serde_plain::from_str(&encoded).unwrap();
+        assert_eq!(decoded, original);
+    }
+
+    #[test]
+    fn deserialize_invalid_char() {
+        let s = "zz:11:22:33:44:55:66:77:88:99:aa:bb:cc:dd:ee:ff:10:20:30:40:50:60:70:80:90:a0:b0:c0:d0:e0:f0:01";
+        let parsed = serde_plain::from_str::<Fingerprint>(s);
+        assert!(parsed.is_err());
+    }
+
+    #[test]
+    fn deserialize_too_short() {
+        let s = "00:11:22:33";
+        let parsed = serde_plain::from_str::<Fingerprint>(s);
+        assert!(parsed.is_err());
+    }
+
+    #[test]
+    fn deserialize_too_long() {
+        let s = &"00:".repeat(33);
+        let parsed = serde_plain::from_str::<Fingerprint>(&s);
+        assert!(parsed.is_err());
     }
 }
-- 
2.47.3


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 2+ messages in thread