When pushing new element onto array, do not free existing data (#928)

This data will be stale and may lead to corruption.

Signed-off-by: Sean Young <sean@mess.org>

src/emit/mod.rs

@@ -675,6 +675,7 @@ pub trait TargetRuntime<'a> {
         &self,
         bin: &Binary<'a>,
         ty: &Type,
+        _existing: bool,
         slot: &mut IntValue<'a>,
         dest: BasicValueEnum<'a>,
         function: FunctionValue<'a>,
@@ -3499,7 +3500,7 @@ pub trait TargetRuntime<'a> {
                             .expression(bin, storage, &w.vars, function, ns)
                             .into_int_value();
 
-                        self.storage_store(bin, ty, &mut slot, value, function, ns);
+                        self.storage_store(bin, ty, true, &mut slot, value, function, ns);
                     }
                     Instr::SetStorageBytes {
                         storage,

src/emit/solana.rs

@@ -240,6 +240,24 @@ impl SolanaTarget {
             .as_global_value()
             .set_unnamed_address(UnnamedAddress::Local);
 
+        let function = binary.module.add_function(
+            "sol_log_64_",
+            void_ty.fn_type(
+                &[
+                    u64_ty.into(),
+                    u64_ty.into(),
+                    u64_ty.into(),
+                    u64_ty.into(),
+                    u64_ty.into(),
+                ],
+                false,
+            ),
+            None,
+        );
+        function
+            .as_global_value()
+            .set_unnamed_address(UnnamedAddress::Local);
+
         let function = binary.module.add_function(
             "sol_sha256",
             void_ty.fn_type(&[sol_bytes.into(), u32_ty.into(), u8_ptr.into()], false),
@@ -786,6 +804,8 @@ impl SolanaTarget {
                 "offset_ptr",
             );
 
+            let mut free_array = None;
+
             if elem_ty.is_dynamic(ns) || zero {
                 let length = if let Some(length) = dim.last().unwrap().as_ref() {
                     binary
@@ -798,6 +818,8 @@ impl SolanaTarget {
                         .build_load(offset_ptr, "offset")
                         .into_int_value();
 
+                    free_array = Some(elem_slot);
+
                     self.storage_array_length(binary, function, slot, elem_ty, ns)
                 };
 
@@ -840,22 +862,19 @@ impl SolanaTarget {
             }
 
             // if the array was dynamic, free the array itself
-            if dim.last().unwrap().is_none() {
-                let slot = binary
-                    .builder
-                    .build_load(offset_ptr, "offset")
-                    .into_int_value();
-
+            if let Some(offset) = free_array {
                 binary.builder.build_call(
                     binary.module.get_function("account_data_free").unwrap(),
-                    &[data.into(), slot.into()],
+                    &[data.into(), offset.into()],
                     "",
                 );
 
-                // account_data_alloc will return 0 if the string is length 0
-                let new_offset = binary.context.i32_type().const_zero();
+                if zero {
+                    // account_data_alloc will return 0 if the string is length 0
+                    let new_offset = binary.context.i32_type().const_zero();
 
-                binary.builder.build_store(offset_ptr, new_offset);
+                    binary.builder.build_store(offset_ptr, new_offset);
+                }
             }
         } else if let ast::Type::Struct(struct_no) = ty {
             for (i, field) in ns.structs[*struct_no].fields.iter().enumerate() {
@@ -1938,7 +1957,7 @@ impl<'a> TargetRuntime<'a> for SolanaTarget {
         );
 
         if let Some(val) = val {
-            self.storage_store(binary, ty, &mut new_offset, val, function, ns);
+            self.storage_store(binary, ty, false, &mut new_offset, val, function, ns);
         }
 
         if ty.is_reference_type(ns) {
@@ -2349,7 +2368,8 @@ impl<'a> TargetRuntime<'a> for SolanaTarget {
         &self,
         binary: &Binary<'a>,
         ty: &ast::Type,
-        slot: &mut IntValue<'a>,
+        existing: bool,
+        offset: &mut IntValue<'a>,
         val: BasicValueEnum<'a>,
         function: FunctionValue<'a>,
         ns: &ast::Namespace,
@@ -2358,7 +2378,7 @@ impl<'a> TargetRuntime<'a> for SolanaTarget {
         let account = self.contract_storage_account(binary);
 
         // the slot is simply the offset after the magic
-        let member = unsafe { binary.builder.build_gep(data, &[*slot], "data") };
+        let member = unsafe { binary.builder.build_gep(data, &[*offset], "data") };
 
         if *ty == ast::Type::String || *ty == ast::Type::DynamicBytes {
             let offset_ptr = binary.builder.build_pointer_cast(
@@ -2367,104 +2387,149 @@ impl<'a> TargetRuntime<'a> for SolanaTarget {
                 "offset_ptr",
             );
 
-            let offset = binary
-                .builder
-                .build_load(offset_ptr, "offset")
-                .into_int_value();
+            let new_string_length = binary.vector_len(val);
 
-            let existing_string_length = binary
-                .builder
-                .build_call(
-                    binary.module.get_function("account_data_len").unwrap(),
+            let offset = if existing {
+                let offset = binary
+                    .builder
+                    .build_load(offset_ptr, "offset")
+                    .into_int_value();
+
+                // get the length of the existing string in storage
+                let existing_string_length = binary
+                    .builder
+                    .build_call(
+                        binary.module.get_function("account_data_len").unwrap(),
+                        &[data.into(), offset.into()],
+                        "length",
+                    )
+                    .try_as_basic_value()
+                    .left()
+                    .unwrap()
+                    .into_int_value();
+
+                // do we need to reallocate?
+                let allocation_necessary = binary.builder.build_int_compare(
+                    IntPredicate::NE,
+                    existing_string_length,
+                    new_string_length,
+                    "allocation_necessary",
+                );
+
+                let entry = binary.builder.get_insert_block().unwrap();
+
+                let realloc = binary.context.append_basic_block(function, "realloc");
+                let memcpy = binary.context.append_basic_block(function, "memcpy");
+
+                binary
+                    .builder
+                    .build_conditional_branch(allocation_necessary, realloc, memcpy);
+
+                binary.builder.position_at_end(realloc);
+
+                // do not realloc since we're copying everything
+                binary.builder.build_call(
+                    binary.module.get_function("account_data_free").unwrap(),
                     &[data.into(), offset.into()],
-                    "length",
-                )
-                .try_as_basic_value()
-                .left()
-                .unwrap()
-                .into_int_value();
+                    "free",
+                );
 
-            let new_string_length = binary.vector_len(val);
+                // account_data_alloc will return offset = 0 if the string is length 0
+                let rc = binary
+                    .builder
+                    .build_call(
+                        binary.module.get_function("account_data_alloc").unwrap(),
+                        &[account.into(), new_string_length.into(), offset_ptr.into()],
+                        "alloc",
+                    )
+                    .try_as_basic_value()
+                    .left()
+                    .unwrap()
+                    .into_int_value();
 
-            let allocation_necessary = binary.builder.build_int_compare(
-                IntPredicate::NE,
-                existing_string_length,
-                new_string_length,
-                "allocation_necessary",
-            );
+                let is_rc_zero = binary.builder.build_int_compare(
+                    IntPredicate::EQ,
+                    rc,
+                    binary.context.i64_type().const_zero(),
+                    "is_rc_zero",
+                );
 
-            let entry = binary.builder.get_insert_block().unwrap();
+                let rc_not_zero = binary.context.append_basic_block(function, "rc_not_zero");
+                let rc_zero = binary.context.append_basic_block(function, "rc_zero");
 
-            let realloc = binary.context.append_basic_block(function, "realloc");
-            let memcpy = binary.context.append_basic_block(function, "memcpy");
+                binary
+                    .builder
+                    .build_conditional_branch(is_rc_zero, rc_zero, rc_not_zero);
 
-            binary
-                .builder
-                .build_conditional_branch(allocation_necessary, realloc, memcpy);
+                binary.builder.position_at_end(rc_not_zero);
 
-            binary.builder.position_at_end(realloc);
+                self.return_code(
+                    binary,
+                    binary.context.i64_type().const_int(5u64 << 32, false),
+                );
 
-            // do not realloc since we're copying everything
-            binary.builder.build_call(
-                binary.module.get_function("account_data_free").unwrap(),
-                &[data.into(), offset.into()],
-                "free",
-            );
+                binary.builder.position_at_end(rc_zero);
 
-            // account_data_alloc will return offset = 0 if the string is length 0
-            let rc = binary
-                .builder
-                .build_call(
-                    binary.module.get_function("account_data_alloc").unwrap(),
-                    &[account.into(), new_string_length.into(), offset_ptr.into()],
-                    "alloc",
-                )
-                .try_as_basic_value()
-                .left()
-                .unwrap()
-                .into_int_value();
+                let new_offset = binary.builder.build_load(offset_ptr, "new_offset");
 
-            let is_rc_zero = binary.builder.build_int_compare(
-                IntPredicate::EQ,
-                rc,
-                binary.context.i64_type().const_zero(),
-                "is_rc_zero",
-            );
+                binary.builder.build_unconditional_branch(memcpy);
 
-            let rc_not_zero = binary.context.append_basic_block(function, "rc_not_zero");
-            let rc_zero = binary.context.append_basic_block(function, "rc_zero");
+                binary.builder.position_at_end(memcpy);
 
-            binary
-                .builder
-                .build_conditional_branch(is_rc_zero, rc_zero, rc_not_zero);
+                let offset_phi = binary
+                    .builder
+                    .build_phi(binary.context.i32_type(), "offset");
 
-            binary.builder.position_at_end(rc_not_zero);
+                offset_phi.add_incoming(&[(&new_offset, rc_zero), (&offset, entry)]);
 
-            self.return_code(
-                binary,
-                binary.context.i64_type().const_int(5u64 << 32, false),
-            );
+                offset_phi.as_basic_value().into_int_value()
+            } else {
+                // account_data_alloc will return offset = 0 if the string is length 0
+                let rc = binary
+                    .builder
+                    .build_call(
+                        binary.module.get_function("account_data_alloc").unwrap(),
+                        &[account.into(), new_string_length.into(), offset_ptr.into()],
+                        "alloc",
+                    )
+                    .try_as_basic_value()
+                    .left()
+                    .unwrap()
+                    .into_int_value();
 
-            binary.builder.position_at_end(rc_zero);
+                let is_rc_zero = binary.builder.build_int_compare(
+                    IntPredicate::EQ,
+                    rc,
+                    binary.context.i64_type().const_zero(),
+                    "is_rc_zero",
+                );
 
-            let new_offset = binary.builder.build_load(offset_ptr, "new_offset");
+                let rc_not_zero = binary.context.append_basic_block(function, "rc_not_zero");
+                let rc_zero = binary.context.append_basic_block(function, "rc_zero");
 
-            binary.builder.build_unconditional_branch(memcpy);
+                binary
+                    .builder
+                    .build_conditional_branch(is_rc_zero, rc_zero, rc_not_zero);
 
-            binary.builder.position_at_end(memcpy);
+                binary.builder.position_at_end(rc_not_zero);
 
-            let offset_phi = binary
-                .builder
-                .build_phi(binary.context.i32_type(), "offset");
+                self.return_code(
+                    binary,
+                    binary.context.i64_type().const_int(5u64 << 32, false),
+                );
 
-            offset_phi.add_incoming(&[(&new_offset, rc_zero), (&offset, entry)]);
+                binary.builder.position_at_end(rc_zero);
+
+                binary
+                    .builder
+                    .build_load(offset_ptr, "new_offset")
+                    .into_int_value()
+            };
 
             let dest_string_data = unsafe {
-                binary.builder.build_gep(
-                    data,
-                    &[offset_phi.as_basic_value().into_int_value()],
-                    "dest_string_data",
-                )
+                binary
+                    .builder
+                    .build_gep(data, &[offset], "dest_string_data")
             };
 
             binary.builder.build_call(
@@ -2478,7 +2543,9 @@ impl<'a> TargetRuntime<'a> for SolanaTarget {
             );
         } else if let ast::Type::Array(elem_ty, dim) = ty {
             // make sure any pointers are freed
-            self.storage_free(binary, ty, data, *slot, function, false, ns);
+            if existing {
+                self.storage_free(binary, ty, data, *offset, function, false, ns);
+            }
 
             let offset_ptr = binary.builder.build_pointer_cast(
                 member,
@@ -2495,7 +2562,7 @@ impl<'a> TargetRuntime<'a> for SolanaTarget {
                 binary.vector_len(val)
             };
 
-            let mut elem_slot = *slot;
+            let mut elem_slot = *offset;
 
             if dim.last().unwrap().is_none() {
                 // reallocate to the right size
@@ -2564,7 +2631,7 @@ impl<'a> TargetRuntime<'a> for SolanaTarget {
 
             // we need a phi for the offset
             let offset_phi =
-                builder.add_loop_phi(binary, "offset", slot.get_type(), elem_slot.into());
+                builder.add_loop_phi(binary, "offset", offset.get_type(), elem_slot.into());
 
             let index = builder.over(binary, binary.context.i32_type().const_zero(), length);
 
@@ -2577,6 +2644,7 @@ impl<'a> TargetRuntime<'a> for SolanaTarget {
             self.storage_store(
                 binary,
                 elem_ty.deref_any(),
+                false, // storage already freed with storage_free
                 &mut offset_val,
                 if elem_ty.deref_memory().is_fixed_reference_type() {
                     elem.into()
@@ -2603,7 +2671,7 @@ impl<'a> TargetRuntime<'a> for SolanaTarget {
                 let field_offset = ns.structs[*struct_no].storage_offsets[i].to_u64().unwrap();
 
                 let mut offset = binary.builder.build_int_add(
-                    *slot,
+                    *offset,
                     binary.context.i32_type().const_int(field_offset, false),
                     "field_offset",
                 );
@@ -2620,11 +2688,14 @@ impl<'a> TargetRuntime<'a> for SolanaTarget {
                 };
 
                 // free any existing dynamic storage
-                self.storage_free(binary, &field.ty, data, offset, function, false, ns);
+                if existing {
+                    self.storage_free(binary, &field.ty, data, offset, function, false, ns);
+                }
 
                 self.storage_store(
                     binary,
                     &field.ty,
+                    existing,
                     &mut offset,
                     if field.ty.is_fixed_reference_type() {
                         elem.into()

tests/solana.rs

@@ -481,6 +481,41 @@ impl<'a> SyscallObject<UserError> for SolLogPubKey<'a> {
     }
 }
 
+struct SolLogU64<'a> {
+    context: SyscallContext<'a>,
+}
+impl<'a> SolLogU64<'a> {
+    /// new
+    pub fn init(context: SyscallContext<'a>) -> Box<(dyn SyscallObject<UserError> + 'a)> {
+        Box::new(Self { context })
+    }
+}
+
+impl<'a> SyscallObject<UserError> for SolLogU64<'a> {
+    fn call(
+        &mut self,
+        arg1: u64,
+        arg2: u64,
+        arg3: u64,
+        arg4: u64,
+        arg5: u64,
+        _memory_mapping: &mut MemoryMapping,
+        result: &mut Result<u64, EbpfError<UserError>>,
+    ) {
+        let message = format!(
+            "{:#x}, {:#x}, {:#x}, {:#x}, {:#x}",
+            arg1, arg2, arg3, arg4, arg5
+        );
+
+        println!("log64: {}", message);
+
+        if let Ok(mut vm) = self.context.vm.try_borrow_mut() {
+            vm.logs.push_str(&message);
+        }
+        *result = Ok(0)
+    }
+}
+
 struct SolSha256();
 impl SolSha256 {
     /// new
@@ -1243,6 +1278,10 @@ impl VirtualMachine {
             .register_syscall_by_name(b"sol_log_pubkey", SolLogPubKey::init, SolLogPubKey::call)
             .unwrap();
 
+        syscall_registry
+            .register_syscall_by_name(b"sol_log_64_", SolLogU64::init, SolLogU64::call)
+            .unwrap();
+
         syscall_registry
             .register_syscall_by_name(
                 b"sol_sha256",
@@ -1333,9 +1372,7 @@ impl VirtualMachine {
 
         deserialize_parameters(&parameter_bytes, &refs, &mut self.account_data);
 
-        let output = &self.account_data[&self.stack[0].data].data;
-
-        VirtualMachine::validate_heap(output);
+        self.validate_account_data_heap();
 
         if let Some((_, return_data)) = &self.return_data {
             println!("return: {}", hex::encode(&return_data));
@@ -1392,7 +1429,7 @@ impl VirtualMachine {
         println!("input: {}", hex::encode(&calldata));
 
         let res = self.execute(&calldata, seeds);
-        assert!(matches!(res, Ok(0)));
+        assert_eq!(res, Ok(0));
 
         if let Some((_, return_data)) = &self.return_data {
             println!("return: {}", hex::encode(&return_data));
@@ -1518,21 +1555,24 @@ impl VirtualMachine {
         (account, seed.to_vec())
     }
 
-    fn validate_heap(data: &[u8]) {
+    fn validate_account_data_heap(&self) -> usize {
+        let data = &self.account_data[&self.stack[0].data].data;
+        let mut count = 0;
+
         let mut prev_offset = 0;
         let return_len = LittleEndian::read_u32(&data[4..]) as usize;
         let return_offset = LittleEndian::read_u32(&data[8..]) as usize;
         let mut offset = LittleEndian::read_u32(&data[12..]) as usize;
 
-        // println!("data:{}", hex::encode(&data));
-        println!("returndata:{}", return_offset);
-        let real_return_len = if return_offset == 0 {
-            0
-        } else {
-            LittleEndian::read_u32(&data[return_offset - 8..]) as usize
-        };
+        // The return_offset/len fields are no longer used (we should remove them at some point)
+        assert_eq!(return_len, 0);
+        assert_eq!(return_offset, 0);
 
-        assert_eq!(real_return_len, return_len);
+        println!(
+            "static: length:{:x} {}",
+            offset - 16,
+            hex::encode(&data[16..offset])
+        );
 
         loop {
             let next = LittleEndian::read_u32(&data[offset..]) as usize;
@@ -1540,11 +1580,15 @@ impl VirtualMachine {
             let length = LittleEndian::read_u32(&data[offset + 8..]) as usize;
             let allocate = LittleEndian::read_u32(&data[offset + 12..]) as usize;
 
+            if allocate == 1 {
+                count += 1;
+            }
+
             println!(
-                "offset:{} prev:{} next:{} length:{} allocated:{} {}",
-                offset,
-                prev,
-                next,
+                "offset:{:x} prev:{:x} next:{:x} length:{} allocated:{} {}",
+                offset + 16,
+                prev + 16,
+                next + 16,
                 length,
                 allocate,
                 hex::encode(&data[offset + 16..offset + 16 + length])
@@ -1565,6 +1609,8 @@ impl VirtualMachine {
 
             offset = next;
         }
+
+        count
     }
 
     pub fn events(&self) -> Vec<RawLog> {

tests/solana_tests/arrays.rs

@@ -892,3 +892,71 @@ fn array_literal() {
         ])]
     );
 }
+
+#[test]
+fn storage_pop_push() {
+    let mut vm = build_solidity(
+        r#"
+    contract Testing {
+        struct NonConstantStruct {
+            string[] b;
+        }
+
+        string[] vec_2;
+        NonConstantStruct[] public complex_array;
+
+        function fn1() public {
+            vec_2.push("tea");
+        }
+
+        function fn2() public {
+            vec_2.push("coffee");
+        }
+
+        function fn3() public {
+            NonConstantStruct memory ss = NonConstantStruct(vec_2);
+            complex_array.push(ss);
+        }
+
+        function fn4() public {
+            vec_2.pop();
+        }
+
+        function fn5() public {
+            vec_2.pop();
+        }
+
+        function fn6() public {
+            vec_2.push("cortado");
+        }
+
+        function fn7() public {
+            vec_2.push("cappuccino");
+        }
+
+        function fn8() public {
+            NonConstantStruct memory sr = NonConstantStruct(vec_2);
+            complex_array.push(sr);
+        }
+
+        function clear() public {
+            vec_2 = new string[](0);
+            complex_array = new NonConstantStruct[](0);
+        }
+    }"#,
+    );
+
+    vm.constructor("Testing", &[]);
+    vm.function("fn1", &[], &[], None);
+    vm.function("fn2", &[], &[], None);
+    vm.function("fn3", &[], &[], None);
+    vm.function("fn4", &[], &[], None);
+    vm.function("fn5", &[], &[], None);
+    vm.function("fn6", &[], &[], None);
+    vm.function("fn7", &[], &[], None);
+    vm.function("fn8", &[], &[], None);
+    vm.function("clear", &[], &[], None);
+
+    // make sure every thing has been freed
+    assert_eq!(vm.validate_account_data_heap(), 0);
+}