From 0ce19d85fafeb857c51fa366aa5ed43cf89dc169 Mon Sep 17 00:00:00 2001
From: Cheradenine Zakalwe <vatueilo@proton.me>
Date: Tue, 22 Aug 2023 19:00:49 +0000
Subject: [PATCH] fix: large value_data length in api crashes server

---
 .../rpc_processor/coders/signed_value_data.rs |  2 +-
 veilid-core/src/storage_manager/mod.rs        |  4 +-
 veilid-core/src/veilid_api/tests/fixtures.rs  |  2 +-
 .../src/veilid_api/types/dht/value_data.rs    | 54 ++++++++++++++++---
 4 files changed, 51 insertions(+), 11 deletions(-)

diff --git a/veilid-core/src/rpc_processor/coders/signed_value_data.rs b/veilid-core/src/rpc_processor/coders/signed_value_data.rs
index a3d29932..bb866413 100644
--- a/veilid-core/src/rpc_processor/coders/signed_value_data.rs
+++ b/veilid-core/src/rpc_processor/coders/signed_value_data.rs
@@ -25,7 +25,7 @@ pub fn decode_signed_value_data(
     let signature = decode_signature512(&sr);
 
     Ok(SignedValueData::new(
-        ValueData::new_with_seq(seq, data, writer),
+        ValueData::new_with_seq(seq, data, writer).map_err(RPCError::protocol)?,
         signature,
     ))
 }
diff --git a/veilid-core/src/storage_manager/mod.rs b/veilid-core/src/storage_manager/mod.rs
index c1b30978..fe80c4ad 100644
--- a/veilid-core/src/storage_manager/mod.rs
+++ b/veilid-core/src/storage_manager/mod.rs
@@ -402,9 +402,9 @@ impl StorageManager {
                 return Ok(None);
             }
             let seq = last_signed_value_data.value_data().seq();
-            ValueData::new_with_seq(seq + 1, data, writer.key)
+            ValueData::new_with_seq(seq + 1, data, writer.key)?
         } else {
-            ValueData::new(data, writer.key)
+            ValueData::new(data, writer.key)?
         };
 
         // Validate with schema
diff --git a/veilid-core/src/veilid_api/tests/fixtures.rs b/veilid-core/src/veilid_api/tests/fixtures.rs
index cb81285d..b906ae68 100644
--- a/veilid-core/src/veilid_api/tests/fixtures.rs
+++ b/veilid-core/src/veilid_api/tests/fixtures.rs
@@ -207,6 +207,6 @@ pub fn fix_veilidvaluechange() -> VeilidValueChange {
         key: fix_typedkey(),
         subkeys: vec![1, 2, 3, 4],
         count: 5,
-        value: ValueData::new_with_seq(23, b"ValueData".to_vec(), fix_cryptokey()),
+        value: ValueData::new_with_seq(23, b"ValueData".to_vec(), fix_cryptokey()).unwrap(),
     }
 }
diff --git a/veilid-core/src/veilid_api/types/dht/value_data.rs b/veilid-core/src/veilid_api/types/dht/value_data.rs
index f15c4a4c..f5aee32e 100644
--- a/veilid-core/src/veilid_api/types/dht/value_data.rs
+++ b/veilid-core/src/veilid_api/types/dht/value_data.rs
@@ -1,4 +1,5 @@
 use super::*;
+use veilid_api::VeilidAPIResult;
 
 #[derive(Clone, Default, PartialOrd, PartialEq, Eq, Ord, Serialize, Deserialize, JsonSchema)]
 pub struct ValueData {
@@ -17,17 +18,25 @@ pub struct ValueData {
 impl ValueData {
     pub const MAX_LEN: usize = 32768;
 
-    pub fn new(data: Vec<u8>, writer: PublicKey) -> Self {
-        assert!(data.len() <= Self::MAX_LEN);
-        Self {
+    pub fn new(data: Vec<u8>, writer: PublicKey) -> VeilidAPIResult<Self> {
+        if data.len() > Self::MAX_LEN {
+            apibail_generic!("invalid size");
+        }
+        Ok(Self {
             seq: 0,
             data,
             writer,
-        }
+        })
     }
-    pub fn new_with_seq(seq: ValueSeqNum, data: Vec<u8>, writer: PublicKey) -> Self {
-        assert!(data.len() <= Self::MAX_LEN);
-        Self { seq, data, writer }
+    pub fn new_with_seq(
+        seq: ValueSeqNum,
+        data: Vec<u8>,
+        writer: PublicKey,
+    ) -> VeilidAPIResult<Self> {
+        if data.len() > Self::MAX_LEN {
+            apibail_generic!("invalid size");
+        }
+        Ok(Self { seq, data, writer })
     }
 
     pub fn seq(&self) -> ValueSeqNum {
@@ -56,3 +65,34 @@ impl fmt::Debug for ValueData {
             .finish()
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn value_data_ok() {
+        assert!(ValueData::new(vec![0; ValueData::MAX_LEN], CryptoKey { bytes: [0; 32] }).is_ok());
+        assert!(ValueData::new_with_seq(
+            0,
+            vec![0; ValueData::MAX_LEN],
+            CryptoKey { bytes: [0; 32] }
+        )
+        .is_ok());
+    }
+
+    #[test]
+    fn value_data_too_long() {
+        assert!(ValueData::new(
+            vec![0; ValueData::MAX_LEN + 1],
+            CryptoKey { bytes: [0; 32] }
+        )
+        .is_err());
+        assert!(ValueData::new_with_seq(
+            0,
+            vec![0; ValueData::MAX_LEN + 1],
+            CryptoKey { bytes: [0; 32] }
+        )
+        .is_err());
+    }
+}