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

aterga · 2024-11-28T10:22:49Z

If a governance error is too large (over 2 MiB), it cannot be sent back to the client, and the replica just drops the error with the following symptoms, e.g.:

Canister violated contract: ic0.msg_reply_data_append:
    application payload size (3043594) cannot be larger than 2097152.
Consider checking the response size and returning an error if it is too long.
See documentation:
    http://internetcomputer.org/docs/current/references/execution-errors#msg_reply_data_append-payload-too-large

This PR bounds the maximum size of errors that can be produced upon UpgradeSnsControlledCanister proposal invalidation. In particular, SNS Governance no longer incorporates the entire (invalidated) WASM bytes string into validation errors.

Additional measures are taken to bound the maximal byte size of common proposal fields, such as title, url, motion text, etc., while still attempting to provide useful error messages.

daniel-wong-dfinity-org · 2024-11-28T17:48:44Z

rs/sns/governance/src/proposal.rs

+    if new_canister_wasm.len() < 4
+        || new_canister_wasm[..4] != RAW_WASM_HEADER[..]
+            && new_canister_wasm[..3] != GZIPPED_WASM_HEADER[..]


This seems orthogonal to the problem(s) mentioned in the PR title+description. Perhaps, mention that there are some additional small entrained fixes?

daniel-wong-dfinity-org · 2024-11-28T17:49:28Z

rs/sns/governance/src/proposal.rs

-        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.",


align. rustfmt is great sometimes. All kneel before the robots!

daniel-wong-dfinity-org · 2024-11-28T17:52:57Z

rs/sns/governance/src/lib.rs

-/// 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


Suggested change

/// Return an Err whose inner value describes (in detail) what is wrong with a field value (should

/// Return an Err whose inner value describes (in detail) what is wrong with a field value (e.g. should

?

daniel-wong-dfinity-org · 2024-11-28T17:55:22Z

rs/sns/governance/src/lib.rs

+    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;
+        }
+    }


Uh. You are just trying to limit the length of the defect description, right? How about this: https://sourcegraph.com/github.com/dfinity/ic/-/blob/rs/nervous_system/string/src/lib.rs?L25:8-25:23

That function doesn't seem safe for arbitrary unicode strings. For example, it would panic in the following case:

clamp_string_len("\u{200D}\u{200D}\u{200D}", 2)

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=16ace03d1cfbfb292b28b2e96b9cbe1f

daniel-wong-dfinity-org · 2024-11-28T17:58:46Z

rs/sns/governance/src/lib.rs

    Err(format!(
-        "The value in field {} is {}: {:?}",
-        field_name, defect, field_value
+        "The first {} characters of the value in field `{}` are {}: `{}`",


The new wording doesn't make sense to me. It should say what bad property the field value has. E.g. "The value in foo is not prime: 42".

I'll improve the wording.

daniel-wong-dfinity-org · 2024-11-28T18:00:42Z

rs/sns/governance/src/lib.rs

+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,


unicode characters are unbounded

Whaa??

Okay, this comment is misleading. Actually there are ligatures of unbounded size, but they would be represented as multiple chars in Rust's utf-8 encoding. I'll adjust the comment.

daniel-wong-dfinity-org · 2024-11-28T18:04:55Z

rs/sns/governance/src/lib.rs

+            (input_value, observer_err)
+        };
+
+        // Scenario A: maximum size that still fits.


Suggested change

// Scenario A: maximum size that still fits.

// Value is (barely) not too large to be fully included in the defect description.

daniel-wong-dfinity-org · 2024-11-28T18:05:14Z

rs/sns/governance/src/lib.rs

+            assert!(observer_err.contains(&input_value));
+        }
+
+        // Scenario B: minimum size that no longer fits.


Suggested change

// Scenario B: minimum size that no longer fits.

// Value is too large to be fully included in the defect description.

daniel-wong-dfinity-org · 2024-11-28T18:07:34Z

rs/sns/governance/src/lib.rs

+            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();


Suggested change

let observer_err = field_err("foo", input_value.clone(), "bar").unwrap_err();

let observer_err = field_err("field_name", input_value.clone(), "ugly").unwrap_err();

Not sure I see the point to have these dummy values over others?

daniel-wong-dfinity-org · 2024-11-28T18:08:39Z

rs/sns/governance/src/lib.rs

+
+    #[test]
+    fn test_giant_field_err() {
+        let expected_upper_bound = 12_500;


Suggested change

let expected_upper_bound = 12_500;

let barely_not_too_large_len = 12_500;

first

d0b8442

github-actions bot added the fix label Nov 28, 2024

aterga marked this pull request as ready for review November 28, 2024 10:24

aterga requested a review from a team as a code owner November 28, 2024 10:24

github-actions bot added the @nns-team label Nov 28, 2024

second

eac1a2a

daniel-wong-dfinity-org approved these changes Nov 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

aterga commented Nov 28, 2024

daniel-wong-dfinity-org Nov 28, 2024

daniel-wong-dfinity-org Nov 28, 2024

daniel-wong-dfinity-org Nov 28, 2024

daniel-wong-dfinity-org Nov 28, 2024

aterga Nov 28, 2024

daniel-wong-dfinity-org Nov 28, 2024

aterga Nov 28, 2024

daniel-wong-dfinity-org Nov 28, 2024

aterga Nov 28, 2024

daniel-wong-dfinity-org Nov 28, 2024

daniel-wong-dfinity-org Nov 28, 2024

daniel-wong-dfinity-org Nov 28, 2024

aterga Nov 28, 2024

daniel-wong-dfinity-org Nov 28, 2024

	/// Return an Err whose inner value describes (in detail) what is wrong with a field value (should
	/// Return an Err whose inner value describes (in detail) what is wrong with a field value (e.g. should

	// Scenario A: maximum size that still fits.
	// Value is (barely) not too large to be fully included in the defect description.

	// Scenario B: minimum size that no longer fits.
	// Value is too large to be fully included in the defect description.

	let observer_err = field_err("foo", input_value.clone(), "bar").unwrap_err();

	let observer_err = field_err("field_name", input_value.clone(), "ugly").unwrap_err();

	let expected_upper_bound = 12_500;
	let barely_not_too_large_len = 12_500;

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

Are you sure you want to change the base?

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

Conversation

aterga commented Nov 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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