mcf: avoid intermediate allocations in push_displayable

mcf/Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "mcf"
-version = "0.6.0-rc.0"
+version = "0.6.0-rc.1"
 authors = ["RustCrypto Developers"]
 edition = "2024"
 rust-version = "1.85"

mcf/src/lib.rs

@@ -113,7 +113,12 @@ impl<'a> TryFrom<&'a str> for &'a PasswordHashRef {
 mod allocating {
     use crate::{Error, Field, PasswordHashRef, Result, fields, validate, validate_id};
     use alloc::{borrow::ToOwned, string::String};
-    use core::{borrow::Borrow, fmt, ops::Deref, str::FromStr};
+    use core::{
+        borrow::Borrow,
+        fmt::{self, Write as _},
+        ops::Deref,
+        str::FromStr,
+    };
 
     #[cfg(feature = "base64")]
     use crate::Base64;
@@ -181,11 +186,27 @@ mod allocating {
 
         /// Push a type which impls [`fmt::Display`], first adding a `$` delimiter and ensuring the
         /// added characters comprise a valid field.
+        ///
+        /// # Errors
+        /// - If the added field fails to validate as a [`Field`] (i.e. contains characters outside
+        ///   the allowed set)
+        /// - If the [`fmt::Display`] impl returns [`fmt::Error`].
         pub fn push_displayable<D: fmt::Display>(&mut self, displayable: D) -> Result<()> {
-            // TODO(tarcieri): avoid intermediate allocation?
-            let mut buf = String::new();
-            fmt::write(&mut buf, format_args!("{displayable}"))?;
-            self.push_str(&buf)
+            // Cache original length to truncate back to on error
+            let old_len = self.0.len();
+            self.0.push(fields::DELIMITER);
+
+            if let Err(e) = write!(&mut self.0, "{displayable}") {
+                self.0.truncate(old_len);
+                return Err(e.into());
+            }
+
+            if let Err(e) = Field::new(&self.0[(old_len + 1)..]) {
+                self.0.truncate(old_len);
+                return Err(e);
+            }
+
+            Ok(())
         }
 
         /// Push an additional field onto the password hash string, first adding a `$` delimiter.
@@ -357,3 +378,103 @@ fn validate_id(id: &str) -> Result<()> {
 
     Ok(())
 }
+
+#[cfg(all(test, feature = "alloc"))]
+#[allow(clippy::unwrap_used)]
+mod tests {
+    use crate::{Error, PasswordHash};
+
+    #[cfg(feature = "base64")]
+    use {crate::Base64, hex_literal::hex};
+
+    const SHA512_HASH: &str = "$6$rounds=100000$exn6tVc2j/MZD8uG$BI1Xh8qQSK9J4m14uwy7abn.ctj/TIAzlaVCto0MQrOFIeTXsc1iwzH16XEWo/a7c7Y9eVJvufVzYAs4EsPOy0";
+
+    #[cfg(feature = "base64")]
+    const EXAMPLE_SALT: &[u8] = &hex!("6a3f237988126f80958fa24b");
+    #[cfg(feature = "base64")]
+    const EXAMPLE_HASH: &[u8] = &hex!(
+        "0d358cad62739eb554863c183aef27e6390368fe061fc5fcb1193a392d60dcad4594fa8d383ab8fc3f0dc8088974602668422e6a58edfa1afe24831b10be69be"
+    );
+
+    #[test]
+    fn from_id() {
+        let mcf_hash = PasswordHash::from_id("6").unwrap();
+        assert_eq!("$6", mcf_hash.as_str());
+    }
+
+    #[test]
+    fn parse_malformed() {
+        assert!("Hello, world!".parse::<PasswordHash>().is_err());
+        assert!("$".parse::<PasswordHash>().is_err());
+        assert!("$$".parse::<PasswordHash>().is_err());
+        assert!("$$foo".parse::<PasswordHash>().is_err());
+        assert!("$foo$".parse::<PasswordHash>().is_err());
+        assert!("$-$foo".parse::<PasswordHash>().is_err());
+        assert!("$foo-$bar".parse::<PasswordHash>().is_err());
+        assert!("$-foo$bar".parse::<PasswordHash>().is_err());
+    }
+
+    #[test]
+    fn parse_id_only() {
+        let hash: PasswordHash = "$6".parse().unwrap();
+        assert_eq!("6", hash.id());
+    }
+
+    #[test]
+    fn parse_sha512_hash() {
+        let hash: PasswordHash = SHA512_HASH.parse().unwrap();
+        assert_eq!("6", hash.id());
+
+        let mut fields = hash.fields();
+        assert_eq!("rounds=100000", fields.next().unwrap().as_str());
+
+        let salt = fields.next().unwrap();
+        assert_eq!("exn6tVc2j/MZD8uG", salt.as_str());
+
+        #[cfg(feature = "base64")]
+        {
+            let salt_bytes = salt.decode_base64(Base64::ShaCrypt).unwrap();
+            assert_eq!(EXAMPLE_SALT, salt_bytes.as_slice());
+        }
+
+        let hash = fields.next().unwrap();
+        assert_eq!(
+            "BI1Xh8qQSK9J4m14uwy7abn.ctj/TIAzlaVCto0MQrOFIeTXsc1iwzH16XEWo/a7c7Y9eVJvufVzYAs4EsPOy0",
+            hash.as_str()
+        );
+
+        #[cfg(feature = "base64")]
+        {
+            let hash_bytes = hash.decode_base64(Base64::ShaCrypt).unwrap();
+            assert_eq!(EXAMPLE_HASH, hash_bytes.as_slice());
+        }
+
+        assert_eq!(None, fields.next());
+    }
+
+    #[cfg(feature = "base64")]
+    #[test]
+    fn push_base64() {
+        let mut hash = PasswordHash::new("$6$rounds=100000").unwrap();
+        hash.push_base64(EXAMPLE_SALT, Base64::ShaCrypt);
+        hash.push_base64(EXAMPLE_HASH, Base64::ShaCrypt);
+        assert_eq!(SHA512_HASH, hash.as_str());
+    }
+
+    #[test]
+    fn push_displayable() {
+        let mut hash = PasswordHash::from_id("6").unwrap();
+        hash.push_displayable("rounds=100000").unwrap();
+        assert_eq!("$6$rounds=100000", hash.as_str());
+    }
+
+    #[test]
+    fn push_displayable_malformed() {
+        let mut hash = PasswordHash::from_id("6").unwrap();
+        assert_eq!(
+            hash.push_displayable("$$$").unwrap_err(),
+            Error::EncodingInvalid
+        );
+        assert_eq!("$6", hash.as_str());
+    }
+}