Make $hp point to the first available byte, instead of the one before…

… it (#377)

* Make $hp point to the first available byte, instead of the one before it

* Test that we cannot allocate stack and heap on top of each other

* Clippy

* Fix comment

* Fix test

* Clippy's else if rules just because

* fmt

---------

Co-authored-by: Brandon Vrooman <brandon.vrooman@gmail.com>
Co-authored-by: Brandon Vrooman <brandon.vrooman@fuel.sh>

fuel-vm/src/interpreter/blockchain.rs

@@ -295,7 +295,7 @@ impl<'vm, S, I> LoadContractCodeCtx<'vm, S, I> {
         let memory_offset_end = checked_add_usize(memory_offset, length)?;
 
         // Validate arguments
-        if memory_offset_end > *self.hp as usize
+        if memory_offset_end >= *self.hp as usize
             || contract_id_end as Word > VM_MAX_RAM
             || length > MEM_MAX_ACCESS_SIZE as usize
             || length > self.contract_max_size as usize

fuel-vm/src/interpreter/initialization.rs

@@ -36,7 +36,7 @@ where
         self.registers[RegId::SSP] = 0;
 
         // Set heap area
-        self.registers[RegId::HP] = VM_MAX_RAM - 1;
+        self.registers[RegId::HP] = VM_MAX_RAM;
 
         self.push_stack(self.transaction().id().as_ref())
             .map_err(|e| io::Error::new(io::ErrorKind::Other, e))?;

fuel-vm/src/interpreter/memory.rs

@@ -8,6 +8,7 @@ use fuel_asm::{PanicReason, RegId};
 use fuel_types::{RegisterId, Word};
 
 use std::ops;
+use std::ops::Range;
 
 pub type Memory<const SIZE: usize> = Box<[u8; SIZE]>;
 
@@ -117,7 +118,7 @@ impl MemoryRange {
     {
         use ops::Bound::*;
 
-        let heap = vm.registers()[RegId::HP].saturating_add(1);
+        let heap = vm.registers()[RegId::HP];
 
         self.start = match self.start {
             Included(start) => Included(heap.saturating_add(start)),
@@ -272,7 +273,7 @@ where
 {
     let (result, overflow) = f(*sp, v);
 
-    if overflow || result > *hp {
+    if overflow || result >= *hp {
         Err(PanicReason::MemoryOverflow.into())
     } else {
         *sp = result;
@@ -335,8 +336,8 @@ pub(crate) fn store_byte(
     c: Word,
 ) -> Result<(), RuntimeError> {
     let (ac, overflow) = a.overflowing_add(c);
-
-    if overflow || ac >= VM_MAX_RAM || !(owner.has_ownership_stack(ac) || owner.has_ownership_heap(ac)) {
+    let range = ac..(ac + 1);
+    if overflow || ac >= VM_MAX_RAM || !(owner.has_ownership_stack(&range) || owner.has_ownership_heap(&range)) {
         Err(PanicReason::MemoryOverflow.into())
     } else {
         memory[ac as usize] = b as u8;
@@ -483,44 +484,56 @@ impl OwnershipRegisters {
         }
     }
     pub(crate) fn has_ownership_range(&self, range: &MemoryRange) -> bool {
-        let (a, ab) = range.boundaries(self);
+        let (start_incl, end_excl) = range.boundaries(self);
 
-        let a_is_stack = a < self.sp;
-        let a_is_heap = a > self.hp;
+        let range = start_incl..end_excl;
 
-        let ab_is_stack = ab <= self.sp;
-        let ab_is_heap = ab >= self.hp;
+        if range.is_empty() {
+            return false;
+        }
 
-        a < ab
-            && (a_is_stack && ab_is_stack && self.has_ownership_stack(a) && self.has_ownership_stack_exclusive(ab)
-                || a_is_heap && ab_is_heap && self.has_ownership_heap(a) && self.has_ownership_heap_exclusive(ab))
-    }
+        if (self.ssp..self.sp).contains(&start_incl) {
+            return self.has_ownership_stack(&range);
+        }
 
-    pub(crate) const fn has_ownership_stack(&self, a: Word) -> bool {
-        a <= VM_MAX_RAM && self.ssp <= a && a < self.sp
+        self.has_ownership_heap(&range)
     }
 
-    pub(crate) const fn has_ownership_stack_exclusive(&self, a: Word) -> bool {
-        a <= VM_MAX_RAM && self.ssp <= a && a <= self.sp
+    /// Zero-length range is never owned
+    pub(crate) fn has_ownership_stack(&self, range: &Range<Word>) -> bool {
+        if range.is_empty() {
+            return false;
+        }
+
+        if range.end > VM_MAX_RAM {
+            return false;
+        }
+
+        (self.ssp..self.sp).contains(&range.start) && (self.ssp..=self.sp).contains(&range.end)
     }
 
-    pub(crate) fn has_ownership_heap(&self, a: Word) -> bool {
+    /// Zero-length range is never owned
+    pub(crate) fn has_ownership_heap(&self, range: &Range<Word>) -> bool {
         // TODO implement fp->hp and (addr, size) validations
         // fp->hp
         // it means $hp from the previous context, i.e. what's saved in the
         // "Saved registers from previous context" of the call frame at
         // $fp`
-        let external = self.context.is_external();
+        if range.is_empty() {
+            return false;
+        }
 
-        self.hp < a && (external && a < VM_MAX_RAM || !external && a <= self.prev_hp)
-    }
+        if range.start < self.hp {
+            return false;
+        }
 
-    pub(crate) fn has_ownership_heap_exclusive(&self, a: Word) -> bool {
-        // TODO reflect the pending changes from `has_ownership_heap`
-        let external = self.context.is_external();
+        let heap_end = if self.context.is_external() {
+            VM_MAX_RAM
+        } else {
+            self.prev_hp
+        };
 
-        self.hp < a
-            && (external && a <= VM_MAX_RAM || !external && self.prev_hp.checked_add(1).map_or(false, |f| a <= f))
+        self.hp != heap_end && range.end <= heap_end
     }
 }
 

fuel-vm/src/interpreter/memory/tests.rs

@@ -35,33 +35,31 @@ fn memcopy() {
     vm.instruction(op::aloc(0x10)).unwrap();
 
     // r[0x20] := 128
-    vm.instruction(op::addi(0x20, 0x20, 128)).unwrap();
+    vm.instruction(op::addi(0x20, RegId::ZERO, 128)).unwrap();
 
     for i in 0..alloc {
         vm.instruction(op::addi(0x21, RegId::ZERO, i)).unwrap();
-        vm.instruction(op::sb(RegId::HP, 0x21, (i + 1) as Immediate12)).unwrap();
+        vm.instruction(op::sb(RegId::HP, 0x21, i as Immediate12)).unwrap();
     }
 
-    // r[0x23] := m[$hp, 0x20] == m[0x12, 0x20]
-    vm.instruction(op::meq(0x23, RegId::HP, 0x12, 0x20)).unwrap();
+    // r[0x23] := m[$hp, 0x20] == m[$zero, 0x20]
+    vm.instruction(op::meq(0x23, RegId::HP, RegId::ZERO, 0x20)).unwrap();
 
     assert_eq!(0, vm.registers()[0x23]);
 
     // r[0x12] := $hp + r[0x20]
     vm.instruction(op::add(0x12, RegId::HP, 0x20)).unwrap();
-    vm.instruction(op::add(0x12, RegId::ONE, 0x12)).unwrap();
 
     // Test ownership
-    vm.instruction(op::add(0x30, RegId::HP, RegId::ONE)).unwrap();
-    vm.instruction(op::mcp(0x30, 0x12, 0x20)).unwrap();
+    vm.instruction(op::mcp(RegId::HP, 0x12, 0x20)).unwrap();
 
     // r[0x23] := m[0x30, 0x20] == m[0x12, 0x20]
-    vm.instruction(op::meq(0x23, 0x30, 0x12, 0x20)).unwrap();
+    vm.instruction(op::meq(0x23, RegId::HP, 0x12, 0x20)).unwrap();
 
     assert_eq!(1, vm.registers()[0x23]);
 
     // Assert ownership
-    vm.instruction(op::subi(0x24, RegId::HP, 1)).unwrap();
+    vm.instruction(op::subi(0x24, RegId::HP, 1)).unwrap(); // TODO: look into this
     let ownership_violated = vm.instruction(op::mcp(0x24, 0x12, 0x20));
 
     assert!(ownership_violated.is_err());
@@ -87,13 +85,13 @@ fn memrange() {
         .unwrap();
     vm.instruction(op::aloc(0x10)).unwrap();
 
-    let m = MemoryRange::new(vm.registers()[RegId::HP], bytes);
+    let m = MemoryRange::new(vm.registers()[RegId::HP] - 1, bytes);
     assert!(!vm.ownership_registers().has_ownership_range(&m));
 
-    let m = MemoryRange::new(vm.registers()[RegId::HP] + 1, bytes);
+    let m = MemoryRange::new(vm.registers()[RegId::HP], bytes);
     assert!(vm.ownership_registers().has_ownership_range(&m));
 
-    let m = MemoryRange::new(vm.registers()[RegId::HP] + 1, bytes + 1);
+    let m = MemoryRange::new(vm.registers()[RegId::HP], bytes + 1);
     assert!(!vm.ownership_registers().has_ownership_range(&m));
 
     let m = MemoryRange::new(0, bytes).to_heap(&vm);
@@ -129,7 +127,7 @@ fn stack_alloc_ownership() {
     => false ; "empty stack and heap"
 )]
 #[test_case(
-    OwnershipRegisters::test(0..0, 0..0, Context::Script{ block_height: 0}), 0..1
+    OwnershipRegisters::test(0..0, VM_MAX_RAM..VM_MAX_RAM, Context::Script{ block_height: 0 }), 0..1
     => false ; "empty stack and heap (external)"
 )]
 #[test_case(
@@ -157,7 +155,7 @@ fn stack_alloc_ownership() {
     => false; "between ranges (external)"
 )]
 #[test_case(
-    OwnershipRegisters::test(0..19, 31..100, Context::Script{ block_height: 0}), 0..1
+    OwnershipRegisters::test(0..19, 31..100, Context::Script{ block_height: 0 }), 0..1
     => true; "in stack range (external)"
 )]
 fn test_ownership(reg: OwnershipRegisters, range: Range<u64>) -> bool {

fuel-vm/tests/backtrace.rs

@@ -98,11 +98,10 @@ fn backtrace() {
 
     contract_call.as_ref().iter().enumerate().for_each(|(i, b)| {
         script.push(op::movi(0x10, *b as Immediate18));
-        script.push(op::sb(RegId::HP, 0x10, 1 + i as Immediate12));
+        script.push(op::sb(RegId::HP, 0x10, i as Immediate12));
     });
 
-    script.push(op::addi(0x10, RegId::HP, 1));
-    script.push(op::call(0x10, RegId::ZERO, RegId::ZERO, RegId::CGAS));
+    script.push(op::call(RegId::HP, RegId::ZERO, RegId::ZERO, RegId::CGAS));
     script.push(op::ret(RegId::ONE));
 
     let input_undefined = Input::contract(rng.gen(), rng.gen(), rng.gen(), rng.gen(), contract_undefined);

fuel-vm/tests/blockchain.rs

@@ -70,8 +70,8 @@ fn state_read_write() {
         op::jnei(0x10, 0x31, 45),                               // (1, b) Unpack arg into 4x16 and xor into state
         op::movi(0x20, 32),                                     // r[0x20]      := 32
         op::aloc(0x20),                                         // aloc            0x20
-        op::addi(0x20, RegId::HP, 1),                       // r[0x20]      := $hp+1
-        op::srwq(0x20, SET_STATUS_REG, 0x11, RegId::ONE),   // m[0x20,32]   := s[m[b, 32], 32]
+        op::move_(0x20, RegId::HP),                             // r[0x20]      := $hp
+        op::srwq(0x20, SET_STATUS_REG, 0x11, RegId::ONE),   // m[0x20,32]       := s[m[b, 32], 32]
         op::lw(0x21, 0x11, 4),                                  // r[0x21]      := m[b+32, 8]
         op::log(0x21, 0x00, 0x00, 0x00),
         op::srli(0x22, 0x21, 48),                               // r[0x22]      := r[0x21] >> 48
@@ -348,15 +348,14 @@ fn load_external_contract_code() {
         let index = i as Immediate12;
         let value = *byte as Immediate12;
         load_contract.extend([
-            op::xor(reg_a, reg_a, reg_a),        // r[a] := 0
-            op::ori(reg_a, reg_a, value),        // r[a] := r[a] | value
-            op::sb(RegId::HP, reg_a, index + 1), // m[$hp+index+1] := r[a] (=value)
+            op::xor(reg_a, reg_a, reg_a),    // r[a] := 0
+            op::ori(reg_a, reg_a, value),    // r[a] := r[a] | value
+            op::sb(RegId::HP, reg_a, index), // m[$hp+index] := r[a] (=value)
         ]);
     }
 
     load_contract.extend([
         op::move_(reg_a, RegId::HP),                      // r[a] := $hp
-        op::addi(reg_a, reg_a, 1),                        // r[a] += 1
         op::xor(reg_b, reg_b, reg_b),                     // r[b] := 0
         op::ori(reg_b, reg_b, 12),                        // r[b] += 12 (will be padded to 16)
         op::ldc(reg_a, RegId::ZERO, reg_b),               // Load first two words from the contract
@@ -497,9 +496,9 @@ fn ldc_reason_helper(cmd: Vec<Instruction>, expected_reason: PanicReason, should
             let index = i as Immediate12;
             let value = *byte as Immediate12;
             load_contract.extend([
-                op::xor(reg_a, reg_a, reg_a),        // r[a] := 0
-                op::ori(reg_a, reg_a, value),        // r[a] := r[a] | value
-                op::sb(RegId::HP, reg_a, index + 1), // m[$hp+index+1] := r[a] (=value)
+                op::xor(reg_a, reg_a, reg_a),    // r[a] := 0
+                op::ori(reg_a, reg_a, value),    // r[a] := r[a] | value
+                op::sb(RegId::HP, reg_a, index), // m[$hp+index] := r[a] (=value)
             ]);
         }
 
@@ -628,7 +627,6 @@ fn ldc_contract_offset_over_length() {
 
     let load_contract = vec![
         op::move_(reg_a, RegId::HP),                      // r[a] := $hp
-        op::addi(reg_a, reg_a, 1),                        // r[a] += 1
         op::xor(reg_b, reg_b, reg_b),                     // r[b] := 0
         op::ori(reg_b, reg_b, 12),                        // r[b] += 12 (will be padded to 16)
         op::ldc(reg_a, reg_a, reg_b),                     // Load first two words from the contract
@@ -803,7 +801,7 @@ fn code_size_b_over_max_ram() {
 fn sww_sets_status() {
     #[rustfmt::skip]
         let program = vec![
-        op::sww(0x30,  SET_STATUS_REG, RegId::ZERO),
+        op::sww(0x30, SET_STATUS_REG, RegId::ZERO),
         op::srw(0x31, SET_STATUS_REG + 1, RegId::ZERO),
         op::log(SET_STATUS_REG, SET_STATUS_REG + 1, 0x00, 0x00),
         op::ret(RegId::ONE),
@@ -832,12 +830,12 @@ fn scwq_clears_status_for_range() {
     let program = vec![
         op::movi(0x11, 100),
         op::aloc(0x11),
-        op::addi(0x31, RegId::HP, 0x5),
+        op::addi(0x31, RegId::HP, 0x4),
         op::addi(0x32, RegId::ONE, 2),
         op::scwq(0x31, SET_STATUS_REG, 0x32),
-        op::addi(0x31, RegId::HP, 0x5),
+        op::addi(0x31, RegId::HP, 0x4),
         op::swwq(0x31, SET_STATUS_REG + 1, 0x31, 0x32),
-        op::addi(0x31, RegId::HP, 0x5),
+        op::addi(0x31, RegId::HP, 0x4),
         op::scwq(0x31, SET_STATUS_REG + 2, 0x32),
         op::log(SET_STATUS_REG, SET_STATUS_REG + 1, SET_STATUS_REG + 2, 0x00),
         op::ret(RegId::ONE),
@@ -922,11 +920,9 @@ fn swwq_sets_status_with_range() {
     let program = vec![
         op::movi(0x11, 100),
         op::aloc(0x11),
-        op::addi(0x31, RegId::HP, 0x01),
         op::movi(0x32, 0x2),
-        op::swwq(0x31, SET_STATUS_REG, 0x31, 0x32),
-        op::addi(0x31, RegId::HP, 0x01),
-        op::swwq(0x31, SET_STATUS_REG + 1, 0x31, 0x32),
+        op::swwq(RegId::HP, SET_STATUS_REG, 0x31, 0x32),
+        op::swwq(RegId::HP, SET_STATUS_REG + 1, 0x31, 0x32),
         op::log(SET_STATUS_REG, SET_STATUS_REG + 1, 0x00, 0x00),
         op::ret(RegId::ONE),
     ];
@@ -1107,11 +1103,10 @@ fn state_r_qword_c_plus_32_over() {
     let state_read_qword = vec![
         op::movi(0x11, 100),
         op::aloc(0x11),
-        op::addi(0x31, RegId::HP, 0x01),
         op::xor(reg_a, reg_a, reg_a),
         op::not(reg_a, reg_a),
         op::subi(reg_a, reg_a, 31 as Immediate12),
-        op::srwq(0x31, SET_STATUS_REG, reg_a, RegId::ONE),
+        op::srwq(RegId::HP, SET_STATUS_REG, reg_a, RegId::ONE),
     ];
 
     check_expected_reason_for_instructions(state_read_qword, MemoryOverflow);
@@ -1143,7 +1138,7 @@ fn state_r_qword_c_over_max_ram() {
     let state_read_qword = vec![
         op::movi(0x11, 100),
         op::aloc(0x11),
-        op::addi(0x31, RegId::HP, 0x01),
+        op::move_(0x31, RegId::HP),
         op::xor(reg_a, reg_a, reg_a),
         op::ori(reg_a, reg_a, 1),
         op::slli(reg_a, reg_a, MAX_MEM_SHL),

fuel-vm/tests/crypto.rs

@@ -37,7 +37,7 @@ fn ecrecover() {
         op::addi(0x22, 0x21, message.as_ref().len() as Immediate12),
         op::movi(0x10, PublicKey::LEN as Immediate18),
         op::aloc(0x10),
-        op::addi(0x11, RegId::HP, 1),
+        op::move_(0x11, RegId::HP),
         op::ecr(0x11, 0x20, 0x21),
         op::meq(0x12, 0x22, 0x11, 0x10),
         op::log(0x12, 0x00, 0x00, 0x00),
@@ -81,7 +81,7 @@ fn ecrecover_error() {
         op::addi(0x22, 0x21, message.as_ref().len() as Immediate12),
         op::movi(0x10, PublicKey::LEN as Immediate18),
         op::aloc(0x10),
-        op::addi(0x11, RegId::HP, 1),
+        op::move_(0x11, RegId::HP),
         op::ecr(0x11, 0x20, 0x21),
     ];
 
@@ -159,7 +159,7 @@ fn sha256() {
         op::addi(0x21, 0x20, message.len() as Immediate12),
         op::movi(0x10, Bytes32::LEN as Immediate18),
         op::aloc(0x10),
-        op::addi(0x11, RegId::HP, 1),
+        op::move_(0x11, RegId::HP),
         op::movi(0x12, message.len() as Immediate18),
         op::s256(0x11, 0x20, 0x12),
         op::meq(0x13, 0x11, 0x21, 0x10),
@@ -251,7 +251,7 @@ fn keccak256() {
         op::addi(0x21, 0x20, message.len() as Immediate12),
         op::movi(0x10, Bytes32::LEN as Immediate18),
         op::aloc(0x10),
-        op::addi(0x11, RegId::HP, 1),
+        op::move_(0x11, RegId::HP),
         op::movi(0x12, message.len() as Immediate18),
         op::k256(0x11, 0x20, 0x12),
         op::meq(0x13, 0x11, 0x21, 0x10),

fuel-vm/tests/flow.rs

@@ -103,7 +103,7 @@ fn code_copy() {
     let mut script_ops = vec![
         op::movi(0x10, 2048),
         op::aloc(0x10),
-        op::addi(0x10, RegId::HP, 0x01),
+        op::move_(0x10, RegId::HP),
         op::movi(0x20, 0x00),
         op::add(0x11, RegId::ZERO, 0x20),
         op::movi(0x12, contract_size as Immediate18),
@@ -887,7 +887,7 @@ fn retd_from_top_of_heap() {
     let script = vec![
         op::movi(REG_SIZE, 32),              // Allocate 32 bytes.
         op::aloc(REG_SIZE),                  // $hp -= 32.
-        op::addi(REG_PTR, RegId::HP, 1), // Pointer is $hp + 1, first byte in allocated buffer.
+        op::move_(REG_PTR, RegId::HP),       // Pointer is $hp, first byte in allocated buffer.
         op::retd(REG_PTR, REG_SIZE),         // Return the allocated buffer.
     ].into_iter()
     .collect::<Vec<u8>>();
@@ -922,7 +922,7 @@ fn logd_from_top_of_heap() {
     let script = vec![
         op::movi(REG_SIZE, 32),                                               // Allocate 32 bytes.
         op::aloc(REG_SIZE),                                                   // $hp -= 32.
-        op::addi(reg_ptr, RegId::HP, 1),                                  // Pointer is $hp + 1, first byte in allocated buffer.
+        op::move_(reg_ptr, RegId::HP),                                        // Pointer is $hp, first byte in allocated buffer.
         op::logd(RegId::ZERO, RegId::ZERO, reg_ptr, REG_SIZE), // Log the whole buffer
         op::ret(RegId::ONE),                                                     // Return
     ].into_iter()