Skip to content

Commit

Permalink
Revert "serialize Hash with serde_bytes"
Browse files Browse the repository at this point in the history
This mostly reverts commits 8416b16 and
dd0afd6.

Changing the serialization of Hash can only be backwards-compatible in
self-describing formats like CBOR. In non-self-describing formats like
bincode, the deserializer has to know in advance which serialization
format was used.

Fixes #414.

Reopens #412.
  • Loading branch information
oconnor663 committed Jul 15, 2024
1 parent fc2f7e4 commit 43ce639
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 22 deletions.
3 changes: 0 additions & 3 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@ mmap = ["std", "dep:memmap2"]
# Implement the zeroize::Zeroize trait for types in this crate.
zeroize = ["dep:zeroize", "arrayvec/zeroize"]

serde = ["dep:serde", "dep:serde_bytes"]

# This crate implements traits from the RustCrypto project, exposed here as the
# "traits-preview" feature. However, these traits aren't stable, and they're
# expected to change in incompatible ways before they reach 1.0. For that
Expand Down Expand Up @@ -105,7 +103,6 @@ digest = { version = "0.10.1", features = [ "mac" ], optional = true }
memmap2 = { version = "0.9", optional = true }
rayon-core = { version = "1.12.1", optional = true }
serde = { version = "1.0", default-features = false, features = ["derive"], optional = true }
serde_bytes = { version = "0.11.15", optional = true }
zeroize = { version = "1", default-features = false, features = ["zeroize_derive"], optional = true }

[dev-dependencies]
Expand Down
7 changes: 1 addition & 6 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,12 +219,7 @@ fn counter_high(counter: u64) -> u32 {
#[cfg_attr(feature = "zeroize", derive(zeroize::Zeroize))]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
#[derive(Clone, Copy, Hash)]
pub struct Hash(
#[rustfmt::skip]
// In formats like CBOR, bytestrings are more compact than lists of ints.
#[cfg_attr(feature = "serde", serde(with = "serde_bytes"))]
[u8; OUT_LEN],
);
pub struct Hash([u8; OUT_LEN]);

impl Hash {
/// The raw bytes of the `Hash`. Note that byte arrays don't provide
Expand Down
29 changes: 16 additions & 13 deletions src/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -826,25 +826,28 @@ fn test_serde() {
assert_eq!(
cbor,
[
0x58, 0x20, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe,
0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe,
0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe
0x98, 0x20, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe,
0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe,
0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe,
0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe,
0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe,
]
);
let hash_from_cbor: crate::Hash = ciborium::from_reader(&cbor[..]).unwrap();
assert_eq!(hash_from_cbor, hash);

// Before we used serde_bytes, the hash [254; 32] would serialize as an array instead of a
// byte string, like this. Make sure we can still deserialize this representation.
let old_cbor: &[u8] = &[
0x98, 0x20, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18,
0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe,
0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18,
0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe,
0x18, 0xfe, 0x18, 0xfe, 0x18, 0xfe,
// Version 1.5.2 of this crate changed the default serialization format to a bytestring
// (instead of an array/list) to save bytes on the wire. That was a backwards compatibility
// mistake for non-self-describing formats, and it's been reverted. Since some small number of
// serialized bytestrings will probably exist forever in the wild, we shold test that we can
// still deserialize these from self-describing formats.
let bytestring_cbor: &[u8] = &[
0x58, 0x20, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe,
0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe, 0xfe,
0xfe, 0xfe, 0xfe, 0xfe,
];
let hash_from_old_cbor: crate::Hash = ciborium::from_reader(old_cbor).unwrap();
assert_eq!(hash_from_old_cbor, hash);
let hash_from_bytestring_cbor: crate::Hash = ciborium::from_reader(bytestring_cbor).unwrap();
assert_eq!(hash_from_bytestring_cbor, hash);
}

// `cargo +nightly miri test` currently works, but it takes forever, because some of our test
Expand Down

0 comments on commit 43ce639

Please sign in to comment.