From f4f3f340ba29b3800cd8272e34023606def23855 Mon Sep 17 00:00:00 2001
From: Akosh Farkash <akosh.farkash@protocol.ai>
Date: Fri, 5 May 2023 16:05:30 +0100
Subject: [PATCH] FIX: DelegateAddress Arbitary (#1693)

* FIX: DelegateAddress Arbitary instances generated inconsistent length and buffer.

* FIX: Clippy suggestions

* FIX: Wider clippy checks in make
---
 Cargo.lock                    |  2 ++
 Makefile                      |  2 +-
 shared/Cargo.toml             |  5 ++++-
 shared/src/address/payload.rs | 20 ++++++++++++++------
 shared/src/sector/post.rs     |  2 +-
 shared/tests/address_test.rs  | 12 ++++++++++++
 6 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index 317fcc8d3..45e090c0e 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -2731,6 +2731,7 @@ dependencies = [
  "data-encoding-macro",
  "filecoin-proofs-api",
  "fvm_ipld_encoding 0.3.3",
+ "fvm_shared 3.3.1",
  "lazy_static",
  "libsecp256k1",
  "multihash",
@@ -2739,6 +2740,7 @@ dependencies = [
  "num-integer",
  "num-traits",
  "quickcheck",
+ "quickcheck_macros",
  "rand",
  "rand_chacha",
  "serde",
diff --git a/Makefile b/Makefile
index a7b428de0..feaae3253 100644
--- a/Makefile
+++ b/Makefile
@@ -12,7 +12,7 @@ clean:
 
 lint:
 	cargo fmt --all
-	cargo clippy --all -- -D warnings -A clippy::upper_case_acronyms
+	cargo clippy --all --all-targets -- -D warnings -A clippy::upper_case_acronyms
 
 license:
 	./scripts/add_license.sh
diff --git a/shared/Cargo.toml b/shared/Cargo.toml
index 8423cb5ce..8466b6512 100644
--- a/shared/Cargo.toml
+++ b/shared/Cargo.toml
@@ -24,7 +24,7 @@ anyhow = "1.0.51"
 fvm_ipld_encoding = { version = "0.3", path = "../ipld/encoding" }
 serde = { version = "1", default-features = false }
 serde_tuple = "0.5"
-arbitrary = { version = "1.1", optional = true, features = ["derive"]}
+arbitrary = { version = "1.1", optional = true, features = ["derive"] }
 quickcheck = { version = "1", optional = true }
 bitflags = "1.3.2"
 
@@ -40,6 +40,9 @@ rand = "0.8"
 rand_chacha = "0.3"
 serde_json = "1.0.56"
 multihash = { version = "0.16.3", default-features = false, features = ["multihash-impl", "sha2", "sha3", "ripemd"] }
+quickcheck_macros = "1"
+
+fvm_shared = { path = ".", features = ["arb"] }
 
 [features]
 default = []
diff --git a/shared/src/address/payload.rs b/shared/src/address/payload.rs
index 34af880ba..8c71c9c03 100644
--- a/shared/src/address/payload.rs
+++ b/shared/src/address/payload.rs
@@ -25,10 +25,12 @@ pub struct DelegatedAddress {
 #[cfg(feature = "arb")]
 impl quickcheck::Arbitrary for DelegatedAddress {
     fn arbitrary(g: &mut quickcheck::Gen) -> Self {
+        let length = usize::arbitrary(g) % (MAX_SUBADDRESS_LEN + 1);
+        let buffer = from_fn(|idx| if idx < length { u8::arbitrary(g) } else { 0u8 });
         Self {
             namespace: ActorID::arbitrary(g),
-            length: usize::arbitrary(g) % (MAX_SUBADDRESS_LEN + 1),
-            buffer: from_fn(|_| u8::arbitrary(g)),
+            length,
+            buffer,
         }
     }
 }
@@ -36,11 +38,17 @@ impl quickcheck::Arbitrary for DelegatedAddress {
 #[cfg(feature = "arb")]
 impl<'a> arbitrary::Arbitrary<'a> for DelegatedAddress {
     fn arbitrary(u: &mut arbitrary::Unstructured<'a>) -> arbitrary::Result<Self> {
-        Ok(DelegatedAddress {
+        let length = u.int_in_range(0usize..=MAX_SUBADDRESS_LEN)?;
+        let mut buffer = [0u8; MAX_SUBADDRESS_LEN];
+        for b in buffer.iter_mut().take(length) {
+            *b = arbitrary::Arbitrary::arbitrary(u)?;
+        }
+        let addr = DelegatedAddress {
             namespace: arbitrary::Arbitrary::arbitrary(u)?,
-            length: u.int_in_range(0usize..=MAX_SUBADDRESS_LEN)?,
-            buffer: arbitrary::Arbitrary::arbitrary(u)?,
-        })
+            length,
+            buffer,
+        };
+        Ok(addr)
     }
 }
 
diff --git a/shared/src/sector/post.rs b/shared/src/sector/post.rs
index 5126666e0..a2b77e08e 100644
--- a/shared/src/sector/post.rs
+++ b/shared/src/sector/post.rs
@@ -53,7 +53,7 @@ impl quickcheck::Arbitrary for PoStProof {
             ])
             .unwrap();
         PoStProof {
-            post_proof: (*registered_postproof).into(),
+            post_proof: *registered_postproof,
             proof_bytes: Vec::arbitrary(g),
         }
     }
diff --git a/shared/tests/address_test.rs b/shared/tests/address_test.rs
index 65f3418d8..0ce8bc492 100644
--- a/shared/tests/address_test.rs
+++ b/shared/tests/address_test.rs
@@ -10,6 +10,7 @@ use fvm_ipld_encoding::{from_slice, to_vec};
 use fvm_shared::address::{
     Address, Error, Protocol, BLS_PUB_LEN, MAX_SUBADDRESS_LEN, PAYLOAD_HASH_LEN, SECP_PUB_LEN,
 };
+use quickcheck_macros::quickcheck;
 
 #[test]
 fn bytes() {
@@ -608,3 +609,14 @@ fn invalid_strings_tests() {
         assert!(Address::from_str(st).is_err());
     }
 }
+
+#[quickcheck]
+fn prop_address_roundtrip(addr0: Address) -> Result<(), String> {
+    let bz = addr0.to_bytes();
+    let addr1 =
+        Address::from_bytes(&bz).map_err(|e| format!("error deserializing address: {e}"))?;
+    if addr1 != addr0 {
+        return Err("address differs after roundtrip".to_owned());
+    }
+    Ok(())
+}