Handle padded_len_* overflows gracefully #719

Dentosal · 2024-04-10T16:47:37Z

There was a bug in padded_len_word and padded_len_usize functions that would overflow on inputs close to max value of the type. While inputs around the max value were never sensible, at least the LDC opcode could be used to trigger the overflow.

Checklist

Breaking changes are clearly marked as such in the PR description and changelog
New behavior is reflected in tests

Before requesting review

I have reviewed the code myself

MitchTurner · 2024-04-12T21:03:14Z

fuel-tx/src/tests/bytes.rs

@@ -733,8 +733,8 @@ fn script_input_coin_data_offset() {
                            .inputs_predicate_offset_at(offset)
                            .expect("Failed to fetch offset");

-                        assert_ne!(bytes::padded_len(&predicate), predicate.len());
-                        assert_eq!(bytes::padded_len(&predicate), len);
+                        assert_ne!(bytes::padded_len(&predicate), Some(predicate.len()));


Why even include the ne case? There are many things that it's not equal to :)

No idea. It was there already, and I just kept the old tests as-is.

MitchTurner · 2024-04-12T21:03:23Z

fuel-tx/src/tests/bytes.rs

@@ -826,8 +826,8 @@ fn upgrade_input_coin_data_offset() {
                    .inputs_predicate_offset_at(offset)
                    .expect("Failed to fetch offset");

-                assert_ne!(bytes::padded_len(&predicate), predicate.len());
-                assert_eq!(bytes::padded_len(&predicate), len);
+                assert_ne!(bytes::padded_len(&predicate), Some(predicate.len()));


Same question.

Same answer.

MitchTurner · 2024-04-12T21:09:39Z

fuel-tx/src/transaction/types/chargeable_transaction.rs

+                    .zip(
+                        input
+                            .predicate_len()
+                            .map(|l| bytes::padded_len_usize(l).unwrap_or(usize::MAX)),


You can use and_then instead of map here to keep an Option and not need the unwrap_or.

The usize::MAX feels arbitrary to me.

Using and_then here would mean combining together (a) input has no predicate_len and (b) that the padding overflows. These are fundamentally different things. The idea behind using usize::MAX is that the value was already quite close to it, and impossibly large. Any actual usage of the return value will correctly cause MemoryOverflow or similar panic. The alternative would be to return Option<Result<usize, PanicReason>> , and that feels a bit excessive. Alternatively we could just Rust-panic on such sizes, but I'm not confident that we actually verify the sizes beforehands.

MitchTurner · 2024-04-12T21:29:03Z

fuel-tx/src/transaction/types/input.rs

-                    .data_offset()
-                    .map(|o| o + bytes::padded_len(data))
+                InputRepr::Message.data_offset().map(|o| {
+                    o.saturating_add(bytes::padded_len(data).unwrap_or(usize::MAX))


The usize::Max kinda makes more sense here with the saturating_add, but I haven a hard time with None for len being equivalent to usize::Max.

You could do a:

bytes::padded_len(data).map(|len| InputRepr::Message.data_offset().map(|o| o.saturating_add(len))

Not really an improvement on readability though :/ So up to you.

MitchTurner · 2024-04-12T21:29:29Z

fuel-tx/src/transaction/types/input.rs

-                .map(|o| o + bytes::padded_len(predicate)),
+            | Input::MessageDataPredicate(MessageDataPredicate { predicate, .. }) => {
+                self.predicate_offset().map(|o| {
+                    o.saturating_add(bytes::padded_len(predicate).unwrap_or(usize::MAX))


MitchTurner · 2024-04-12T21:29:39Z

fuel-tx/src/transaction/types/script.rs

@@ -233,7 +233,9 @@ mod field {
                return body.script_data_offset;
            }

-            self.script_offset() + bytes::padded_len(self.body.script.as_slice())
+            self.script_offset().saturating_add(
+                bytes::padded_len(self.body.script.as_slice()).unwrap_or(usize::MAX),


MitchTurner · 2024-04-12T21:29:50Z

fuel-tx/src/transaction/types/script.rs

-            self.script_data_offset()
-                + bytes::padded_len(self.body.script_data.as_slice())
+            self.script_data_offset().saturating_add(
+                bytes::padded_len(self.body.script_data.as_slice()).unwrap_or(usize::MAX),


MitchTurner · 2024-04-12T21:32:32Z

fuel-types/src/bytes.rs

+}
+
+#[test]
+fn padded_len_value() {


Can we name this something like padded_len_usize__gives_values_in_increments_of_8 or something like that. To say what the tested behavior is.

Makes sense. I'll rename it.

MitchTurner · 2024-04-12T21:33:04Z

fuel-types/src/bytes.rs

-    assert_eq!(WORD_SIZE * 1, padded_len(&[0; WORD_SIZE]));
-    assert_eq!(WORD_SIZE * 2, padded_len(&[0; WORD_SIZE + 1]));
-    assert_eq!(WORD_SIZE * 2, padded_len(&[0; WORD_SIZE * 2]));
+    assert_eq!(Some(WORD_SIZE * 0), padded_len(&[]));


This test could use a clearer name of what the expected behavior is.

xgreenx · 2024-04-15T07:21:17Z

fuel-vm/src/interpreter/blockchain.rs

+        .map(|len| len as Word)
+        .unwrap_or(Word::MAX);


Maybe we also want to return PanicReason::MemoryOverflow?

This Word::MAX is guaranteed to hit the check just below, and return PanicReason::ContracMaxSize. Unless we set contract_max_size == Word::MAX which shouldn't happen, and even then the grow_stack below will catch it.

Handle padded_len overflows gracefully

45283ad

Dentosal added bug Something isn't working breaking A breaking api change fuel-vm Related to the `fuel-vm` crate. fuel-tx Related to the `fuel-tx` crate. labels Apr 10, 2024

Dentosal self-assigned this Apr 10, 2024

Dentosal added 2 commits April 10, 2024 19:53

Add more test cases

1d1441d

Add changelog entry

fa5cc45

Dentosal marked this pull request as ready for review April 10, 2024 16:55

Dentosal requested a review from a team April 10, 2024 16:55

Fix test case

02cf3ad

MitchTurner reviewed Apr 12, 2024

View reviewed changes

xgreenx reviewed Apr 15, 2024

View reviewed changes

Make test naming clearer

f650862

Dentosal requested review from MitchTurner and xgreenx April 15, 2024 12:50

Merge branch 'master' into dento/ldc-padding-overflow-fix

48fec10

xgreenx previously approved these changes Apr 15, 2024

View reviewed changes

Dentosal added 2 commits April 15, 2024 17:09

Merge branch 'master' into dento/ldc-padding-overflow-fix

8cf4bf0

Fix tests after merge

537bb31

Dentosal dismissed xgreenx’s stale review via 537bb31 April 15, 2024 14:14

xgreenx approved these changes Apr 15, 2024

View reviewed changes

Dentosal added this pull request to the merge queue Apr 15, 2024

Merged via the queue into master with commit f94aa10 Apr 15, 2024
38 checks passed

Dentosal deleted the dento/ldc-padding-overflow-fix branch April 15, 2024 15:11

xgreenx mentioned this pull request Apr 15, 2024

Release v0.49.0 #723

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle padded_len_* overflows gracefully #719

Handle padded_len_* overflows gracefully #719

Dentosal commented Apr 10, 2024 •

edited

Loading

MitchTurner Apr 12, 2024

Dentosal Apr 15, 2024

MitchTurner Apr 12, 2024

Dentosal Apr 15, 2024

MitchTurner Apr 12, 2024

Dentosal Apr 15, 2024

MitchTurner Apr 12, 2024

MitchTurner Apr 12, 2024

MitchTurner Apr 12, 2024

MitchTurner Apr 12, 2024

MitchTurner Apr 12, 2024

Dentosal Apr 15, 2024

Dentosal Apr 15, 2024

MitchTurner Apr 12, 2024

Dentosal Apr 15, 2024

xgreenx Apr 15, 2024

Dentosal Apr 15, 2024

Handle padded_len_* overflows gracefully #719

Handle padded_len_* overflows gracefully #719

Conversation

Dentosal commented Apr 10, 2024 • edited Loading

Checklist

Before requesting review

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dentosal commented Apr 10, 2024 •

edited

Loading