Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Commit

Permalink
xcmp-queue: Fix handling of encoded blobs
Browse files Browse the repository at this point in the history
With #701 we tried to fix some infinite loop related to encoded blobs, however that lead actually to
not being able to process encoded blobs at all. The reason for this is that `decode_all` doesn't
consume the given input. The point of this function is that it returns an error if the data couldn't
be decoded or there is still data left. However, this means that the check
`remaining_fragments.len() < last_remaining_fragments.len()` would always fail.

We remove the while loop, because we decode the entire fragment anyway or it fails. Aka, we don't
need to loop here. Next we remove the broken check and we directly reset the
`remaining_fragments` (because `decode_all` doesn't consume anything).
  • Loading branch information
bkchr committed Jan 5, 2022
1 parent 38ae71f commit 2c54ba7
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 24 deletions.
42 changes: 21 additions & 21 deletions pallets/xcmp-queue/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -536,28 +536,28 @@ impl<T: Config> Pallet<T> {
}
},
XcmpMessageFormat::ConcatenatedEncodedBlob => {
while !remaining_fragments.is_empty() {
if !remaining_fragments.is_empty() {
// `decode_all` doesn't consume the input. Thus, we directly reset
// `remaining_fragments` to an empty slice and only reset it when
// the message didn't get processed because of being too heavy.
last_remaining_fragments = remaining_fragments;
match <Vec<u8>>::decode_all(&mut remaining_fragments) {
Ok(blob) if remaining_fragments.len() < last_remaining_fragments.len() => {
let weight = max_weight - weight_used;
match Self::handle_blob_message(sender, sent_at, blob, weight) {
Ok(used) => weight_used = weight_used.saturating_add(used),
Err(true) => {
// That message didn't get processed this time because of being
// too heavy. We leave it around for next time and bail.
remaining_fragments = last_remaining_fragments;
break
},
Err(false) => {
// Message invalid; don't attempt to retry
},
}
},
_ => {
debug_assert!(false, "Invalid incoming blob message data");
remaining_fragments = &b""[..];
},
remaining_fragments = &b""[..];

if let Ok(blob) = <Vec<u8>>::decode_all(last_remaining_fragments) {
let weight = max_weight - weight_used;
match Self::handle_blob_message(sender, sent_at, blob, weight) {
Ok(used) => weight_used = weight_used.saturating_add(used),
Err(true) => {
// That message didn't get processed this time because of being
// too heavy. We leave it around for next time and bail.
remaining_fragments = last_remaining_fragments;
},
Err(false) => {
// Message invalid; don't attempt to retry
},
}
} else {
debug_assert!(false, "Invalid incoming blob message data");
}
}
},
Expand Down
7 changes: 4 additions & 3 deletions pallets/xcmp-queue/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,12 @@ fn bad_message_is_handled() {
});
}

/// Tests that a blob message is handled. Currently this isn't implemented and panics when debug assertions
/// are enabled. When this feature is enabled, this test should be rewritten properly.
#[test]
#[should_panic = "Invalid incoming blob message data"]
#[should_panic = "Blob messages not handled."]
#[cfg(debug_assertions)]
fn other_bad_message_is_handled() {
fn handle_blob_message() {
new_test_ext().execute_with(|| {
let bad_data = vec![
1, 1, 1, 1, 3, 0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 64, 239,
Expand All @@ -58,7 +60,6 @@ fn other_bad_message_is_handled() {
];
InboundXcmpMessages::<Test>::insert(ParaId::from(1000), 1, bad_data);
let format = XcmpMessageFormat::ConcatenatedEncodedBlob;
// This should exit with an error.
XcmpQueue::process_xcmp_message(1000.into(), (1, format), 10_000_000_000, 10_000_000_000);
});
}
Expand Down

0 comments on commit 2c54ba7

Please sign in to comment.