Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make zero-sized value counting more fine grained #516

Closed
wants to merge 5 commits into from
Closed

make zero-sized value counting more fine grained #516

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8fb5d78
fix counting zero sized values
chenyan-dfinity Jan 25, 2024
b98ea0f
fix
chenyan-dfinity Jan 26, 2024
f61e277
fix
chenyan-dfinity Jan 26, 2024
b4efc33
spec
chenyan-dfinity Jan 26, 2024
b60b7d1
clarify about counting
chenyan-dfinity Jan 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 19 additions & 19 deletions rust/candid/src/de.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,10 +256,7 @@ impl<'de> Deserializer<'de> {
gamma: Gamma::default(),
field_name: None,
is_untyped: false,
#[cfg(not(target_arch = "wasm32"))]
zero_sized_values: 2_000_000,
#[cfg(target_arch = "wasm32")]
zero_sized_values: 0,
#[cfg(not(target_arch = "wasm32"))]
full_error_message: true,
#[cfg(target_arch = "wasm32")]
Expand Down Expand Up @@ -337,16 +334,13 @@ impl<'de> Deserializer<'de> {
}
Ok(())
}
fn is_zero_sized_type(&self, t: &Type) -> bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conceptually, we should check this condition for every (nested) value and limit all such (nested) zero-sized values at 2M in total.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above. That would be overcounting, e.g., record { null; null } would be counted as 3 instead of 2.

match t.as_ref() {
TypeInner::Null | TypeInner::Reserved => true,
TypeInner::Record(fs) => fs.iter().all(|f| {
let t = self.table.trace_type(&f.ty).unwrap();
// recursive records have been replaced with empty already, it's safe to call without memoization.
self.is_zero_sized_type(&t)
}),
_ => false,
fn dec_zero_sized_value(&mut self) -> Result<()> {
//eprintln!("{}", self.zero_sized_values);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this comment be dropped?

if self.zero_sized_values == 0 {
return Err(Error::msg("Too many zero sized values"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return Err(Error::msg("Too many zero sized values"));
return Err(Error::msg("Too many zero sized values"));
Suggested change
return Err(Error::msg("Too many zero sized values"));
return Err(Error::msg("Too many zero-sized values"));

}
self.zero_sized_values -= 1;
Ok(())
}
// Should always call set_field_name to set the field_name. After deserialize_identifier
// processed the field_name, field_name will be reset to None.
Expand Down Expand Up @@ -413,6 +407,7 @@ impl<'de> Deserializer<'de> {
where
V: Visitor<'de>,
{
self.dec_zero_sized_value()?;
let bytes = vec![3u8];
visitor.visit_byte_buf(bytes)
}
Expand Down Expand Up @@ -655,6 +650,7 @@ impl<'de, 'a> de::Deserializer<'de> for &'a mut Deserializer<'de> {
&& matches!(*self.wire_type, TypeInner::Null | TypeInner::Reserved),
"unit"
);
self.dec_zero_sized_value()?;
visitor.visit_unit()
}
fn deserialize_bool<V>(self, visitor: V) -> Result<V::Value>
Expand Down Expand Up @@ -715,7 +711,10 @@ impl<'de, 'a> de::Deserializer<'de> for &'a mut Deserializer<'de> {
{
self.unroll_type()?;
match (self.wire_type.as_ref(), self.expect_type.as_ref()) {
(TypeInner::Null | TypeInner::Reserved, TypeInner::Opt(_)) => visitor.visit_none(),
(TypeInner::Null | TypeInner::Reserved, TypeInner::Opt(_)) => {
self.dec_zero_sized_value()?;
visitor.visit_none()
}
(TypeInner::Opt(t1), TypeInner::Opt(t2)) => {
self.wire_type = t1.clone();
self.expect_type = t2.clone();
Expand Down Expand Up @@ -755,12 +754,6 @@ impl<'de, 'a> de::Deserializer<'de> for &'a mut Deserializer<'de> {
let expect = e.clone();
let wire = self.table.trace_type(w)?;
let len = Len::read(&mut self.input)?.0;
if self.is_zero_sized_type(&wire) {
if self.zero_sized_values < len {
return Err(Error::msg("vec length of zero sized values too large"));
}
self.zero_sized_values -= len;
}
visitor.visit_seq(Compound::new(self, Style::Vector { len, expect, wire }))
}
(TypeInner::Record(e), TypeInner::Record(w)) => {
Expand All @@ -773,6 +766,9 @@ impl<'de, 'a> de::Deserializer<'de> for &'a mut Deserializer<'de> {
self.wire_type
)));
}
if w.is_empty() {
self.dec_zero_sized_value()?;
}
let value =
visitor.visit_seq(Compound::new(self, Style::Struct { expect, wire }))?;
Ok(value)
Expand Down Expand Up @@ -879,6 +875,9 @@ impl<'de, 'a> de::Deserializer<'de> for &'a mut Deserializer<'de> {
(TypeInner::Record(e), TypeInner::Record(w)) => {
let expect = e.clone().into();
let wire = w.clone().into();
if w.is_empty() {
self.dec_zero_sized_value()?;
}
let value =
visitor.visit_map(Compound::new(self, Style::Struct { expect, wire }))?;
Ok(value)
Expand Down Expand Up @@ -1143,6 +1142,7 @@ impl<'de, 'a> de::VariantAccess<'de> for Compound<'a, 'de> {
*self.de.expect_type == TypeInner::Null && *self.de.wire_type == TypeInner::Null,
"unit_variant"
);
self.de.dec_zero_sized_value()?;
Ok(())
}

Expand Down
6 changes: 5 additions & 1 deletion rust/candid/tests/serde.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,11 @@ fn test_vector() {
let bytes = hex("4449444c036c01d6fca702016d026c00010080ade204");
check_error(
|| test_decode(&bytes, &candid::Reserved),
"zero sized values too large",
"Too many zero sized values",
);
check_error(
|| test_decode(&hex("4449444c016d7f01008094ebdc03"), &vec![None::<u128>]),
"Too many zero sized values",
);
}

Expand Down
6 changes: 3 additions & 3 deletions spec/Candid.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
# Candid Specification

Version: 0.1.7
Version: 0.1.8

Date: Dec 12, 2024
Date: Jan 25, 2024

## Motivation

Expand Down Expand Up @@ -1255,7 +1255,7 @@ M(id(v*) : principal) = i8(1) M(v* : vec nat8)

Note:

* Since `null`, `reserved`, `record {}`, and records of such values, take no space, to prevent unbounded sized message, we limit the total vector length of such zero-sized values in a messagev (on the wire) to be 2,000,000 elements. For example, if a message contains two vectors, one at type `vec null` and one at type `vec record {}`, then the length of both vectors combined cannot exceed 2,000,000 elements.
* Since `null`, `reserved` and `record {}` take no space, to prevent unbounded sized message, we limit the total number of such zero-sized values in a message (on the wire) to be 2,000,000 elements. For example, a value of type `record { null; reserved; record { record {} } }` contains 3 zero-sized values; a value of type `vec opt record { null; null }` with length 100 contains at most 200 zero-sized values (it is 200 when the vector contains no `null` values); if a message contains two vectors, one at type `vec null` and one at type `vec record {}`, then the length of both vectors combined cannot exceed 2,000,000 elements.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a value of type record { null; reserved; record { record {} } } contains 3 zero-sized values
Could you please list the zero-sized values explicitly? I believe to have counter 4:

  • null
  • reserved
  • record {}
  • record { record {} }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And in fact also the entire record { null; reserved; record { record {} } } is a 5th zero-sized value there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try to rephrase the spec. We only count the occurrences of null, reserved and record {} to simulate the effect that if these values were not zero sized. record { record {} } would not be zero sized, if record {} weren't zero sized.


#### References

Expand Down
11 changes: 7 additions & 4 deletions test/spacebomb.test.did
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,15 +2,15 @@
// Space bomb tests


// Messages with more than 2_000_000 zero-length elements in vectors should be rejected
// Messages with more than 2_000_000 zero-length elements should be rejected

// \80\94\eb\dc\03 is 1000_000_000
// \80\ad\e2\04 is 10_000_000
// \80\89\7a is 2_000_000
// \ff\ff\3f is 1_048_575
// \aa\d8\28 is 666_666
// \80\b5\18 is 400_000


// Plain decoding (unused arguments)
assert blob "DIDL\01\6d\7f\01\00\80\94\eb\dc\03" !: () "vec null (extra argument)";
assert blob "DIDL\01\6d\70\01\00\80\94\eb\dc\03" !: () "vec reserved (extra argument)";
Expand All @@ -21,16 +21,18 @@ assert blob "DIDL\03\6c\01\d6\fc\a7\02\01\6d\02\6c\00\01\00\80\ad\e2\04"
// Messages with exactly 2_000_000 zero-length elements should succeed
assert blob "DIDL\01\6d\7f\01\00\80\89\7a" : () "vec null (exactly 2000000)";
assert blob "DIDL\01\6d\70\01\00\80\89\7a" : () "vec reserved (exactly 2000000)";
assert blob "DIDL\04\6c\03\01\7f\02\01\03\02\6c\01\01\70\6c\00\6d\00\01\03\80\89\7a" : () "zero-sized record (exactly 2000000)";
assert blob "DIDL\04\6c\03\01\7f\02\01\03\02\6c\01\01\70\6c\00\6d\00\03\03\7f\7f\aa\d8\28" : () "zero-sized record (exactly 2000000)";
assert blob "DIDL\02\6d\01\6d\7f\01\00\05\80\b5\18\80\b5\18\80\b5\18\80\b5\18\80\b5\18" : () "vec vec null (exactly 2000000)";
assert blob "DIDL\03\6c\01\d6\fc\a7\02\01\6d\02\6c\00\01\00\80\89\7a" : () "vec record {} (exactly 2000000)";
//assert blob "DIDL\17\6c\02\01\7f\02\7f\6c\02\01\00\02\00\6c\02\00\01\01\01\6c\02\00\02\01\02\6c\02\00\03\01\03\6c\02\00\04\01\04\6c\02\00\05\01\05\6c\02\00\06\01\06\6c\02\00\07\01\07\6c\02\00\08\01\08\6c\02\00\09\01\09\6c\02\00\0a\01\0a\6c\02\00\0b\01\0b\6c\02\00\0c\01\0c\6c\02\00\0d\02\0d\6c\02\00\0e\01\0e\6c\02\00\0f\01\0f\6c\02\00\10\01\10\6c\02\00\11\01\11\6c\02\00\12\01\12\6c\02\00\13\01\13\6e\14\6d\15\01\16\01\01" : () "vec opt record with 2^20 null (exactly 1048576)";

// Messages with exactly 2_000_001 zero-length elements should fail
assert blob "DIDL\01\6d\7f\01\00\80\89\7b" !: () "vec null (exactly 2000001)";
assert blob "DIDL\01\6d\70\01\00\80\89\7b" !: () "vec reserved (exactly 2000001)";
assert blob "DIDL\04\6c\03\01\7f\02\01\03\02\6c\01\01\70\6c\00\6d\00\01\03\80\89\7b" !: () "zero-sized record (exactly 2000001)";
assert blob "DIDL\04\6c\03\01\7f\02\01\03\02\6c\01\01\70\6c\00\6d\00\01\03\aa\d8\29" !: () "zero-sized record (exactly 2000001)";
assert blob "DIDL\02\6d\01\6d\7f\01\00\05\80\b5\18\80\b5\18\80\b5\18\80\b5\18\80\b5\19" !: () "vec vec null (exactly 2000001)";
assert blob "DIDL\03\6c\01\d6\fc\a7\02\01\6d\02\6c\00\01\00\80\89\7b" !: () "vec record {} (exactly 2000001)";
assert blob "DIDL\17\6c\02\01\7f\02\7f\6c\02\01\00\02\00\6c\02\00\01\01\01\6c\02\00\02\01\02\6c\02\00\03\01\03\6c\02\00\04\01\04\6c\02\00\05\01\05\6c\02\00\06\01\06\6c\02\00\07\01\07\6c\02\00\08\01\08\6c\02\00\09\01\09\6c\02\00\0a\01\0a\6c\02\00\0b\01\0b\6c\02\00\0c\01\0c\6c\02\00\0d\02\0d\6c\02\00\0e\01\0e\6c\02\00\0f\01\0f\6c\02\00\10\01\10\6c\02\00\11\01\11\6c\02\00\12\01\12\6c\02\00\13\01\13\6e\14\6d\15\01\16\02\01\01" !: () "vec opt record with 2^20 null (over 2000000)";

// Decoding to actual type
assert blob "DIDL\01\6d\7f\01\00\80\94\eb\dc\03" !: (vec opt nat) "vec null (not ignored)";
Expand All @@ -45,3 +47,4 @@ assert blob "DIDL\01\6d\70\01\00\80\94\eb\dc\03" !: (o
assert blob "DIDL\04\6c\03\01\7f\02\01\03\02\6c\01\01\70\6c\00\6d\00\01\03\80\94\eb\dc\03" !: (opt nat) "zero-sized record (subtyping)";
assert blob "DIDL\02\6d\01\6d\7f\01\00\05\ff\ff\3f\ff\ff\3f\ff\ff\3f\ff\ff\3f\ff\ff\3f" !: (vec opt nat) "vec vec null (subtyping)";
assert blob "DIDL\03\6c\01\d6\fc\a7\02\01\6d\02\6c\00\01\00\80\ad\e2\04" !: (opt nat) "vec record {} (subtyping)";
assert blob "DIDL\17\6c\02\01\7f\02\7f\6c\02\01\00\02\00\6c\02\00\01\01\01\6c\02\00\02\01\02\6c\02\00\03\01\03\6c\02\00\04\01\04\6c\02\00\05\01\05\6c\02\00\06\01\06\6c\02\00\07\01\07\6c\02\00\08\01\08\6c\02\00\09\01\09\6c\02\00\0a\01\0a\6c\02\00\0b\01\0b\6c\02\00\0c\01\0c\6c\02\00\0d\02\0d\6c\02\00\0e\01\0e\6c\02\00\0f\01\0f\6c\02\00\10\01\10\6c\02\00\11\01\11\6c\02\00\12\01\12\6c\02\00\13\01\13\6e\14\6d\15\01\16\05\01\01\01\01\01" !: (vec opt record {}) "vec opt record with 2^20 null (subtyping)";
Loading