Panic on instructions with non-zero reserved part #737
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Dentosal
added
breaking
Dentosal
changed the title
Merged
1 task
bvrooman
reviewed
May 23, 2024
fuel-asm/src/encoding_tests.rs
Outdated
Comment on lines
57
to
77
| fn invalid_reserved_part() { | ||
| // Args: 0 | ||
| Instruction::try_from([Opcode::NOOP as u8, 0, 0, 0]).unwrap(); | ||
| Instruction::try_from([Opcode::NOOP as u8, 0, 0, 1]) | ||
| .expect_err("Reserved part is nonzero"); | ||
|
|
||
| // Args: 1 | ||
| Instruction::try_from([Opcode::RET as u8, 0, 0, 0]).unwrap(); | ||
| Instruction::try_from([Opcode::RET as u8, 0, 0, 1]) | ||
| .expect_err("Reserved part is nonzero"); | ||
|
|
||
| // Args: 2 | ||
| Instruction::try_from([Opcode::NOT as u8, 0, 0, 0]).unwrap(); | ||
| Instruction::try_from([Opcode::NOT as u8, 0, 0, 1]) | ||
| .expect_err("Reserved part is nonzero"); | ||
|
|
||
| // Args: 3 | ||
| Instruction::try_from([Opcode::XOR as u8, 0, 0, 0]).unwrap(); | ||
| Instruction::try_from([Opcode::XOR as u8, 0, 0, 1]) | ||
| .expect_err("Reserved part is nonzero"); | ||
| } |
There was a problem hiding this comment.
While this behaviour is implemented as a macro, and all opcodes inherit this behaviour, it may be worth testing this for all opcodes. From an opcode-centric perspective, we want to show that each opcode fulfills the specification (regardless of implementation).
Maybe we could have a macro-generated test(s) that tests common behaviours for a given opcode, and maintain a list that invokes this test macro for all opcodes.
But that's up to you - it may be overkill here.
bvrooman
previously approved these changes
May 23, 2024
xgreenx
reviewed
May 24, 2024
fuel-asm/src/encoding_tests.rs
Outdated
Comment on lines
63
to
76
| // Args: 1 | ||
| Instruction::try_from([Opcode::RET as u8, 0, 0, 0]).unwrap(); | ||
| Instruction::try_from([Opcode::RET as u8, 0, 0, 1]) | ||
| .expect_err("Reserved part is nonzero"); | ||
|
|
||
| // Args: 2 | ||
| Instruction::try_from([Opcode::NOT as u8, 0, 0, 0]).unwrap(); | ||
| Instruction::try_from([Opcode::NOT as u8, 0, 0, 1]) | ||
| .expect_err("Reserved part is nonzero"); | ||
|
|
||
| // Args: 3 | ||
| Instruction::try_from([Opcode::XOR as u8, 0, 0, 0]).unwrap(); | ||
| Instruction::try_from([Opcode::XOR as u8, 0, 0, 1]) | ||
| .expect_err("Reserved part is nonzero"); |
There was a problem hiding this comment.
I don't see where are we using arg 2 and arg 3=)
xgreenx
requested changes
May 24, 2024
xgreenx
added a commit
to FuelLabs/fuel-specs
that referenced
this pull request
May 27, 2024
…#583) VM PR: FuelLabs/fuel-vm#737 What is says on the tin. This is a small but breaking change. It's unlikely to cause any breakage, as no Sway code or code constructed using the fuel-asm helper functions is affected. This change is originating from an audit report that pointed out the likely unintentional behavior of the unused part of instruction is ignored. ### Before requesting review - [x] I have reviewed the code myself Co-authored-by: Green Baneling <XgreenX9999@gmail.com>
# Conflicts: # CHANGELOG.md
# Conflicts: # CHANGELOG.md
…f packed. Added tests to verify behaviour for the unreserved parts.
xgreenx
previously approved these changes
Jun 1, 2024
xgreenx
previously approved these changes
Jun 1, 2024
Dentosal
commented
Jun 1, 2024
fuel-asm/src/lib.rs
Outdated
| #[cfg_attr(feature = "typescript", wasm_bindgen::prelude::wasm_bindgen)] | ||
| pub struct RegId(u8); | ||
|
|
||
| /// Represents a 6-bit immediate value, guaranteed to be masked by construction. | ||
| #[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] | ||
| #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] |
There was a problem hiding this comment.
The derived Deserialize impl allows constructing invalid values.
…`Instruction` takes 12(with aligment) bytes instead of 4
xgreenx
approved these changes
Jun 3, 2024
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Spec PR: FuelLabs/fuel-specs#583
See spec pr for extended description.
Checklist
Before requesting review