fix(sns): Avoid producing overly large errors upon UpgradeSnsControlledCanister proposal invalidation

rs/sns/governance/src/lib.rs

@@ -1,5 +1,5 @@
 use crate::pb::v1::Subaccount as SubaccountProto;
-use std::{convert::TryInto, fmt::Debug};
+use std::convert::TryInto;
 
 mod cached_upgrade_steps;
 pub mod canister_control;
@@ -20,29 +20,37 @@ trait Len {
     fn len(&self) -> usize;
 }
 
+/// Maximum size, in bytes, of a scalar field (e.g., of type `String` or numeric types) that.
+/// Scalar values greater than this will be truncated during error reporting.
+pub const MAX_SCALAR_FIELD_LEN_BYTES: usize = 50_000;
+
 /// Warning: the len method on str and String is in bytes, not characters. If
 /// you want to constrain the number of characters, look at
 /// validate_chars_count.
 fn validate_len<V>(field_name: &str, field_value: &V, min: usize, max: usize) -> Result<(), String>
 where
-    V: Len + Debug,
+    V: Len + ToString,
 {
     let len = field_value.len();
 
     if len < min {
-        return field_err(
-            field_name,
-            field_value,
-            &format!("too short (min = {} vs. observed = {})", min, len),
-        );
+        let defect = &format!("too short (min = {} vs. observed = {})", min, len);
+
+        let bounded_field_value = field_value.to_string();
+
+        return field_err(field_name, bounded_field_value, defect);
     }
 
     if len > max {
-        return field_err(
-            field_name,
-            field_value,
-            &format!("too long (min = {} vs. observed = {})", min, len),
-        );
+        let defect = &format!("too long (max = {} vs. observed = {})", max, len);
+
+        let bounded_field_value = field_value
+            .to_string()
+            .chars()
+            .take(max)
+            .collect::<String>();
+
+        return field_err(field_name, bounded_field_value, defect);
     }
 
     Ok(())
@@ -94,19 +102,16 @@ fn validate_chars_count(
     let len = field_value.chars().count();
 
     if len < min {
-        return field_err(
-            field_name,
-            field_value,
-            &format!("too short (min = {} vs. observed = {})", min, len),
-        );
+        let defect = &format!("too short (min = {} vs. observed = {})", min, len);
+
+        return field_err(field_name, field_value.to_string(), defect);
     }
 
     if len > max {
-        return field_err(
-            field_name,
-            field_value,
-            &format!("too long (max = {} vs. observed = {})", max, len),
-        );
+        let defect = &format!("too long (max = {} vs. observed = {})", max, len);
+        let bounded_field_value = field_value.chars().take(max).collect::<String>();
+
+        return field_err(field_name, bounded_field_value, defect);
     }
 
     Ok(())
@@ -121,12 +126,35 @@ fn validate_required_field<'a, Inner>(
         .ok_or_else(|| format!("The {} field must be populated.", field_name))
 }
 
-/// Return an Err whose inner value describes (in detail) what is wrong with a
-/// field value, and where within some (Protocol Buffers message) struct.
-fn field_err(field_name: &str, field_value: impl Debug, defect: &str) -> Result<(), String> {
+/// Return an Err whose inner value describes (in detail) what is wrong with a field value (should
+/// be bounded), and where within some (Protocol Buffers message) struct.
+///
+/// Only up to the first `MAX_SCALAR_FIELD_LEN_BYTES` bytes will be taken from `field_value`.
+fn field_err(field_name: &str, field_value: String, defect: &str) -> Result<(), String> {
+    let mut bounded_field_value = String::new();
+    // Searching for the longest byte prefix that is a valid string is expensive in the worst case.
+    // This is because unicode characters are unbounded. Instead, concatenate characters one-by-one,
+    // until we either run out of characters or exceed the limit (in which case we pop the last
+    // character to still comply with the limits). Note that a `char` is always 4 bytes, but
+    // pushing it to a string does not always increase a string's byte size by 4 bytes.
+    // For example:
+    // ```
+    // println!("bytes = {}", std::mem::size_of_val(&'\u{200D}')); // bytes = 4
+    // println!("bytes = {}", std::mem::size_of_val("\u{200D}"));  // bytes = 3
+    // ```
+    for c in field_value.chars() {
+        bounded_field_value.push(c);
+        if bounded_field_value.len() > MAX_SCALAR_FIELD_LEN_BYTES {
+            bounded_field_value.pop();
+            break;
+        }
+    }
     Err(format!(
-        "The value in field {} is {}: {:?}",
-        field_name, defect, field_value
+        "The first {} characters of the value in field `{}` are {}: `{}`",
+        bounded_field_value.chars().count(),
+        field_name,
+        defect,
+        bounded_field_value
     ))
 }
 
@@ -201,6 +229,14 @@ mod tests {
         assert_is_ok(validate(&"abcde"));
 
         assert_is_err(validate(&"abcd\u{1F389}"));
+        assert_eq!(
+            validate(&"abcdefg"),
+            Err(
+                "The first 5 characters of the value in field `field_name` are too long \
+                 (max = 5 vs. observed = 7): `abcde`"
+                    .to_string()
+            ),
+        );
     }
 
     #[test]
@@ -261,7 +297,7 @@ mod tests {
 
     #[test]
     fn test_field_err() {
-        let result = field_err("my_field", 41, "not the meaning of life");
+        let result = field_err("my_field", 41.to_string(), "not the meaning of life");
         match result {
             Ok(()) => panic!("field_err is supposed to always return an Err."),
             Err(err) => {
@@ -271,4 +307,36 @@ mod tests {
             }
         }
     }
+
+    #[test]
+    fn test_giant_field_err() {
+        let expected_upper_bound = 12_500;
+
+        let run_test_for_value_of_size = |value_size| {
+            let input_value: String = (0..value_size).map(|_| '🤝').collect();
+            // Sanity check: We construct a string in which each character is encoded as 4 bytes.
+            assert_eq!(input_value.len(), 4 * input_value.chars().count());
+            let observer_err = field_err("foo", input_value.clone(), "bar").unwrap_err();
+            (input_value, observer_err)
+        };
+
+        // Scenario A: maximum size that still fits.
+        {
+            let (input_value, observer_err) = run_test_for_value_of_size(expected_upper_bound);
+            assert!(observer_err.contains(&input_value));
+        }
+
+        // Scenario B: minimum size that no longer fits.
+        {
+            let (input_value, observer_err) = run_test_for_value_of_size(expected_upper_bound + 1);
+            assert!(
+                !observer_err.contains(&input_value),
+                "Expected ```{}``` not to contain ```{}```.",
+                observer_err,
+                input_value
+            );
+            // Only the last character was dropped.
+            assert!(observer_err.contains(&input_value[..(input_value.chars().count() - 1)]));
+        }
+    }
 }

rs/sns/governance/src/proposal.rs

@@ -1068,30 +1068,26 @@ fn validate_and_render_upgrade_sns_controlled_canister(
     const RAW_WASM_HEADER: [u8; 4] = [0, 0x61, 0x73, 0x6d];
     // see https://ic-interface-spec.netlify.app/#canister-module-format
     const GZIPPED_WASM_HEADER: [u8; 3] = [0x1f, 0x8b, 0x08];
-    // Minimum length of raw WASM is 8 bytes (4 magic bytes and 4 bytes encoding version).
-    // Minimum length of gzipped WASM is 10 bytes (2 magic bytes, 1 byte encoding compression method, and 7 additional gzip header bytes).
-    const MIN_WASM_LEN: usize = 8;
-    if let Err(err) = validate_len(
-        "new_canister_wasm",
-        new_canister_wasm,
-        MIN_WASM_LEN,
-        usize::MAX,
-    ) {
-        defects.push(err);
-    } else if new_canister_wasm[..4] != RAW_WASM_HEADER[..]
-        && new_canister_wasm[..3] != GZIPPED_WASM_HEADER[..]
+
+    if new_canister_wasm.len() < 4
+        || new_canister_wasm[..4] != RAW_WASM_HEADER[..]
+            && new_canister_wasm[..3] != GZIPPED_WASM_HEADER[..]
     {
         defects.push("new_canister_wasm lacks the magic value in its header.".into());
     }
 
-    if new_canister_wasm.len()
-        + canister_upgrade_arg
+    if new_canister_wasm.len().saturating_add(
+        canister_upgrade_arg
             .as_ref()
             .map(|arg| arg.len())
-            .unwrap_or_default()
-        >= MAX_INSTALL_CODE_WASM_AND_ARG_SIZE
+            .unwrap_or_default(),
+    ) >= MAX_INSTALL_CODE_WASM_AND_ARG_SIZE
     {
-        defects.push(format!("the maximum canister WASM and argument size for UpgradeSnsControlledCanister is {} bytes.", MAX_INSTALL_CODE_WASM_AND_ARG_SIZE));
+        defects.push(format!(
+            "the maximum canister WASM and argument size \
+                 for UpgradeSnsControlledCanister is {} bytes.",
+            MAX_INSTALL_CODE_WASM_AND_ARG_SIZE
+        ));
     }
 
     // Generate final report.