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.

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

Avoid possible truncation of higher bits #619

Merged
merged 5 commits into from
Oct 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
- [#607](https://github.com/FuelLabs/fuel-vm/pull/607): The `Interpreter` expects the third generic argument during type definition that specifies the implementer of the `EcalHandler` trait for `ecal` opcode.
- [#609](https://github.com/FuelLabs/fuel-vm/pull/609): Checked transactions (`Create`, `Script`, and `Mint`) now enforce a maximum size. The maximum size is specified by `MAX_TRANSACTION_SIZE` in the transaction parameters, under consensus parameters. Checking a transaction above this size raises `CheckError::TransactionSizeLimitExceeded`.
- [#617](https://github.com/FuelLabs/fuel-vm/pull/617): Makes memory outside `$is..$ssp` range not executable. Separates `ErrorFlag` into `InvalidFlags`, `MemoryNotExecutable` and `InvalidInstruction`. Fixes related tests.
- [#619](https://github.com/FuelLabs/fuel-vm/pull/619): Avoid possible truncation of higher bits. It may invalidate the code that truncated higher bits causing different behavior on 32-bit vs. 64-bit systems.

## [Version 0.39.0]

Expand Down
1 change: 1 addition & 0 deletions fuel-asm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#![cfg_attr(docsrs, feature(doc_auto_cfg))]
#![cfg_attr(not(feature = "std"), no_std)]
#![cfg_attr(feature = "std", doc = include_str!("../README.md"))]
#![deny(clippy::cast_possible_truncation)]
#![deny(clippy::string_slice)]
#![deny(missing_docs)]
#![deny(unsafe_code)]
Expand Down
1 change: 1 addition & 0 deletions fuel-asm/src/panic_instruction.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ impl From<PanicInstruction> for Word {
}

impl From<Word> for PanicInstruction {
#[allow(clippy::cast_possible_truncation)]
fn from(val: Word) -> Self {
// Safe to cast as we've shifted the 8 MSB.
let reason_u8 = (val >> REASON_OFFSET) as u8;
Expand Down
6 changes: 6 additions & 0 deletions fuel-asm/src/unpack.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,26 +62,32 @@ pub(super) fn ra_imm18_from_bytes(bs: [u8; 3]) -> (RegId, Imm18) {
(ra_from_bytes(bs), imm18_from_bytes(bs))
}

#[allow(clippy::cast_possible_truncation)]
fn ra_from_u32(u: u32) -> RegId {
RegId::new((u >> 18) as u8)
}

#[allow(clippy::cast_possible_truncation)]
fn rb_from_u32(u: u32) -> RegId {
RegId::new((u >> 12) as u8)
}

#[allow(clippy::cast_possible_truncation)]
fn rc_from_u32(u: u32) -> RegId {
RegId::new((u >> 6) as u8)
}

#[allow(clippy::cast_possible_truncation)]
fn rd_from_u32(u: u32) -> RegId {
RegId::new(u as u8)
}

#[allow(clippy::cast_possible_truncation)]
fn imm06_from_u32(u: u32) -> Imm06 {
Imm06::new(u as u8)
}

#[allow(clippy::cast_possible_truncation)]
fn imm12_from_u32(u: u32) -> Imm12 {
Imm12::new(u as u16)
}
Expand Down
1 change: 1 addition & 0 deletions fuel-crypto/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#![warn(missing_docs)]
#![deny(unsafe_code)]
#![deny(unused_crate_dependencies)]
#![deny(clippy::cast_possible_truncation)]

// Satisfy unused_crate_dependencies lint for self-dependency enabling test features
#[cfg(test)]
Expand Down
25 changes: 13 additions & 12 deletions fuel-merkle/src/common/msb.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@ pub enum Bit {
}

trait GetBit {
fn get_bit(&self, bit_index: usize) -> Option<Bit>;
fn get_bit(&self, bit_index: u32) -> Option<Bit>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these changes to fuel-merkle necessary? It feels like we're changing library logic to fit business logic. A library should promise correctness under all targets it supports, including all 32 bit and 64 bit targets. If it has internal problems with truncation, that should be fixed in the library. But this change appears to be changing the library interface to accommodate changes to external libraries.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is a private trait, so it is an internal detail of how Merkle tree works=)

}

impl GetBit for u8 {
fn get_bit(&self, bit_index: usize) -> Option<Bit> {
fn get_bit(&self, bit_index: u32) -> Option<Bit> {
if bit_index < 8 {
let mask = 1 << (7 - bit_index);
let bit = self & mask;
Expand All @@ -24,21 +24,21 @@ impl GetBit for u8 {
}

pub trait Msb {
fn get_bit_at_index_from_msb(&self, index: usize) -> Option<Bit>;
fn common_prefix_count(&self, other: &Self) -> usize;
fn get_bit_at_index_from_msb(&self, index: u32) -> Option<Bit>;
fn common_prefix_count(&self, other: &Self) -> u32;
}

impl<const N: usize> Msb for [u8; N] {
fn get_bit_at_index_from_msb(&self, index: usize) -> Option<Bit> {
fn get_bit_at_index_from_msb(&self, index: u32) -> Option<Bit> {
// The byte that contains the bit
let byte_index = index / 8;
// The bit within the containing byte
let byte_bit_index = index % 8;
self.get(byte_index)
self.get(byte_index as usize)
.and_then(|byte| byte.get_bit(byte_bit_index))
}

fn common_prefix_count(&self, other: &Self) -> usize {
fn common_prefix_count(&self, other: &Self) -> u32 {
let mut count = 0;
for (byte1, byte2) in self.iter().zip(other.iter()) {
// For each pair of bytes, compute the similarity of each byte using
Expand All @@ -49,10 +49,11 @@ impl<const N: usize> Msb for [u8; N] {
break
}
}
count as usize
count
}
}

#[allow(clippy::cast_possible_truncation)]
#[cfg(test)]
mod test {
use crate::common::{
Expand All @@ -66,7 +67,7 @@ mod test {

#[test]
fn test_msb_for_bytes_1() {
const NUM_BITS: usize = size_of::<Bytes1>() * 8;
Copy link
Member

Choose a reason for hiding this comment

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

I like the size_of approach as it's more obvious to the code reader how this value is constructed. Maybe we should split this into two constants like NUM_BYTES vs NUM_BITS?

const NUM_BITS: u32 = size_of::<Bytes1>() as u32 * 8;

let bytes: Bytes1 = [0b10101010];
let expected_n = u8::from_be_bytes(bytes);
Expand All @@ -83,7 +84,7 @@ mod test {

#[test]
fn test_msb_for_bytes_2() {
const NUM_BITS: usize = size_of::<Bytes2>() * 8;
const NUM_BITS: u32 = size_of::<Bytes2>() as u32 * 8;

let bytes: Bytes2 = [0b10101010, 0b10101010];
let expected_n = u16::from_be_bytes(bytes);
Expand All @@ -100,7 +101,7 @@ mod test {

#[test]
fn test_msb_for_bytes_4() {
const NUM_BITS: usize = size_of::<Bytes4>() * 8;
const NUM_BITS: u32 = size_of::<Bytes4>() as u32 * 8;

let bytes: Bytes4 = [0b10101010, 0b10101010, 0b10101010, 0b10101010];
let expected_n = u32::from_be_bytes(bytes);
Expand All @@ -117,7 +118,7 @@ mod test {

#[test]
fn test_msb_for_bytes_8() {
const NUM_BITS: usize = size_of::<Bytes8>() * 8;
const NUM_BITS: u32 = size_of::<Bytes8>() as u32 * 8;

let bytes: Bytes8 = [
0b10101010, 0b10101010, 0b10101010, 0b10101010, 0b10101010, 0b10101010,
Expand Down
5 changes: 3 additions & 2 deletions fuel-merkle/src/common/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@ pub trait KeyFormatting {
pub trait Node {
type Key: KeyFormatting;

fn key_size_in_bits() -> usize {
mem::size_of::<Self::Key>() * 8
fn key_size_in_bits() -> u32 {
u32::try_from(mem::size_of::<Self::Key>() * 8)
.expect("The key usually is several bytes")
}

fn height(&self) -> u32;
Expand Down
8 changes: 4 additions & 4 deletions fuel-merkle/src/common/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,18 @@ impl From<Bit> for Instruction {
}

pub trait Path {
fn get_instruction(&self, index: usize) -> Option<Instruction>;
fn get_instruction(&self, index: u32) -> Option<Instruction>;
}

pub trait ComparablePath {
fn common_path_length(&self, other: &Self) -> usize;
fn common_path_length(&self, other: &Self) -> u32;
}

impl<T> Path for T
where
T: Msb,
{
fn get_instruction(&self, index: usize) -> Option<Instruction> {
fn get_instruction(&self, index: u32) -> Option<Instruction> {
self.get_bit_at_index_from_msb(index).map(Into::into)
}
}
Expand All @@ -38,7 +38,7 @@ impl<T> ComparablePath for T
where
T: Msb,
{
fn common_path_length(&self, other: &Self) -> usize {
fn common_path_length(&self, other: &Self) -> u32 {
self.common_prefix_count(other)
}
}
4 changes: 2 additions & 2 deletions fuel-merkle/src/common/path_iterator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ use crate::common::{
pub struct PathIter<T: ParentNode> {
leaf_key: T::Key,
current: Option<(ChildResult<T>, ChildResult<T>)>,
current_offset: usize,
current_offset: u32,
}

impl<T> PathIter<T>
Expand Down Expand Up @@ -137,7 +137,7 @@ where
// 0 7 00 02 04 06 08 10 12 14 252 254
// 00 01 02 03 04 05 06 07 126 127
//
let initial_offset = T::key_size_in_bits() - root.height() as usize;
let initial_offset = T::key_size_in_bits() - root.height();
Self {
leaf_key,
current: Some(initial),
Expand Down
1 change: 1 addition & 0 deletions fuel-merkle/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#![cfg_attr(not(feature = "std"), no_std)]
#![allow(clippy::bool_assert_comparison, clippy::identity_op)]
#![deny(unused_crate_dependencies)]
#![deny(clippy::cast_possible_truncation)]

#[cfg_attr(test, macro_use)]
extern crate alloc;
Expand Down
19 changes: 9 additions & 10 deletions fuel-merkle/src/sparse/branch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ where
{
let branch = if left_branch.node.is_leaf() && right_branch.node.is_leaf() {
let parent_depth = left_branch.node.common_path_length(&right_branch.node);
let parent_height = (Node::max_height() - parent_depth) as u32;
let parent_height = Node::max_height() - parent_depth;
let node =
Node::create_node(&left_branch.node, &right_branch.node, parent_height);
Branch {
Expand All @@ -53,9 +53,10 @@ where
if right_branch.node.is_node() {
let mut current_node = right_branch.node;
let path = right_branch.bits;
let parent_height = current_node.height() as usize + 1;
let parent_height = current_node.height() + 1;
let stale_depth = ancestor_height - parent_height;
let placeholders = iter::repeat(Node::create_placeholder()).take(stale_depth);
let placeholders =
iter::repeat(Node::create_placeholder()).take(stale_depth as usize);
for placeholder in placeholders {
current_node =
Node::create_node_on_path(&path, &current_node, &placeholder);
Expand All @@ -66,21 +67,19 @@ where
if left_branch.node.is_node() {
let mut current_node = left_branch.node;
let path = left_branch.bits;
let parent_height = current_node.height() as usize + 1;
let parent_height = current_node.height() + 1;
let stale_depth = ancestor_height - parent_height;
let placeholders = iter::repeat(Node::create_placeholder()).take(stale_depth);
let placeholders =
iter::repeat(Node::create_placeholder()).take(stale_depth as usize);
for placeholder in placeholders {
current_node =
Node::create_node_on_path(&path, &current_node, &placeholder);
storage.insert(current_node.hash(), &current_node.as_ref().into())?;
}
left_branch.node = current_node;
}
let node = Node::create_node(
&left_branch.node,
&right_branch.node,
ancestor_height as u32,
);
let node =
Node::create_node(&left_branch.node, &right_branch.node, ancestor_height);
Branch {
bits: left_branch.bits,
node,
Expand Down
10 changes: 5 additions & 5 deletions fuel-merkle/src/sparse/merkle_tree.rs
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ impl<TableType, StorageType> MerkleTree<TableType, StorageType> {
}

fn set_root_node(&mut self, node: Node) {
debug_assert!(node.is_leaf() || node.height() == Node::max_height() as u32);
debug_assert!(node.is_leaf() || node.height() == Node::max_height());
self.root_node = node;
}
}
Expand Down Expand Up @@ -252,7 +252,7 @@ where
}

let mut nodes = Vec::<Branch>::with_capacity(branches.len());
let mut proximities = Vec::<usize>::with_capacity(branches.len());
let mut proximities = Vec::<u32>::with_capacity(branches.len());

// Building the tree starts by merging all leaf nodes where possible.
// Given a set of leaf nodes sorted left to right (i.e., keys are sorted
Expand Down Expand Up @@ -336,9 +336,9 @@ where
// node has the final height and forms the root node.
let mut node = top.node;
let path = top.bits;
let height = node.height() as usize;
let height = node.height();
let depth = Node::max_height() - height;
let placeholders = iter::repeat(Node::create_placeholder()).take(depth);
let placeholders = iter::repeat(Node::create_placeholder()).take(depth as usize);
for placeholder in placeholders {
node = Node::create_node_on_path(&path, &node, &placeholder);
storage.insert(node.hash(), &node.as_ref().into())?;
Expand Down Expand Up @@ -455,7 +455,7 @@ where

// Merge placeholders
let ancestor_depth = requested_leaf_node.common_path_length(actual_leaf_node);
let stale_depth = cmp::max(side_nodes.len(), ancestor_depth);
let stale_depth = cmp::max(side_nodes.len(), ancestor_depth as usize);
let placeholders_count = stale_depth - side_nodes.len();
let placeholders =
iter::repeat(Node::create_placeholder()).take(placeholders_count);
Expand Down
8 changes: 4 additions & 4 deletions fuel-merkle/src/sparse/node.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ impl Node {
hash.finalize().try_into().unwrap()
}

pub fn max_height() -> usize {
pub fn max_height() -> u32 {
Node::key_size_in_bits()
}

Expand Down Expand Up @@ -112,7 +112,7 @@ impl Node {
// leaves.
// N.B.: A leaf can be a placeholder.
let parent_depth = path_node.common_path_length(side_node);
let parent_height = (Node::max_height() - parent_depth) as u32;
let parent_height = Node::max_height() - parent_depth;
match path.get_instruction(parent_depth).unwrap() {
Instruction::Left => {
Node::create_node(path_node, side_node, parent_height)
Expand All @@ -127,7 +127,7 @@ impl Node {
// ancestor of the node with the lesser height.
// N.B.: A leaf can be a placeholder.
let parent_height = cmp::max(path_node.height(), side_node.height()) + 1;
let parent_depth = Node::max_height() - parent_height as usize;
let parent_depth = Node::max_height() - parent_height;
match path.get_instruction(parent_depth).unwrap() {
Instruction::Left => {
Node::create_node(path_node, side_node, parent_height)
Expand All @@ -143,7 +143,7 @@ impl Node {
Self::Placeholder
}

pub fn common_path_length(&self, other: &Node) -> usize {
pub fn common_path_length(&self, other: &Node) -> u32 {
debug_assert!(self.is_leaf());
debug_assert!(other.is_leaf());

Expand Down
1 change: 1 addition & 0 deletions fuel-storage/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#![no_std]
#![deny(clippy::cast_possible_truncation)]
#![deny(unsafe_code)]
#![deny(unused_crate_dependencies)]

Expand Down
5 changes: 3 additions & 2 deletions fuel-tx/src/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ impl TransactionBuilder<Create> {
};

*tx.bytecode_length_mut() = (bytecode.as_ref().len() / 4) as Word;
*tx.bytecode_witness_index_mut() = tx.witnesses().len() as u8;
*tx.bytecode_witness_index_mut() = 0;

tx.witnesses_mut().push(bytecode);

Expand Down Expand Up @@ -424,7 +424,8 @@ impl<Tx: Buildable> TransactionBuilder<Tx> {

/// Adds a secret to the builder, and adds a corresponding witness if it's a new entry
fn upsert_secret(&mut self, secret_key: SecretKey) -> u8 {
let witness_len = self.witnesses().len() as u8;
let witness_len = u8::try_from(self.witnesses().len())
.expect("The number of witnesses can't exceed `u8::MAX`");

let witness_index = self.sign_keys.entry(secret_key).or_insert_with(|| {
// if this private key hasn't been used before,
Expand Down
1 change: 1 addition & 0 deletions fuel-tx/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
// Wrong clippy convention; check
// https://rust-lang.github.io/api-guidelines/naming.html
#![allow(clippy::wrong_self_convention)]
#![deny(clippy::cast_possible_truncation)]
#![deny(clippy::string_slice)]
#![deny(unused_crate_dependencies)]
#![deny(unsafe_code)]
Expand Down
1 change: 1 addition & 0 deletions fuel-tx/src/tests/valid_cases/input.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#![allow(clippy::cast_possible_truncation)]
use super::PREDICATE_PARAMS;

use fuel_crypto::{
Expand Down
Loading
Loading