chore(avm): calldata, returndata slices out of range padded with zeros

barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp

@@ -2138,11 +2138,7 @@ AvmError AvmTraceBuilder::op_fee_per_da_gas(uint8_t indirect, uint32_t dst_offse
  *        Simplified version with exclusively memory store operations and
  *        values from calldata passed by an array and loaded into
  *        intermediate registers.
- *        Assume that caller passes call_data_mem which is large enough so that
- *        no out-of-bound memory issues occur.
- *        TODO: error handling if dst_offset + copy_size > 2^32 which would lead to
- *              out-of-bound memory write. Similarly, if cd_offset + copy_size is larger
- *              than call_data_mem.size()
+ *        Slice calldata portion which is out-of-range will be filled with zero values.
  *
  * @param indirect A byte encoding information about indirect/direct memory access.
  * @param cd_offset_address The starting index of the region in calldata to be copied.
@@ -2177,8 +2173,17 @@ AvmError AvmTraceBuilder::op_calldata_copy(uint8_t indirect,
 
     bool is_top_level = current_ext_call_ctx.is_top_level;
 
-    // Do not take a reference as calldata might be resized below.
+    // No reference as we mutate calldata
     auto calldata = current_ext_call_ctx.calldata;
+
+    // Any out-of-range values from calldata is replaced by a zero value.
+    // We append zeros in this case to calldata.
+    // TODO: Properly constrain this use case. Currently, for top level calls we do not add any padding in
+    // calldata public columns but concatenate the calldata vectors of the top-level calls.
+    if (cd_offset + copy_size > calldata.size()) {
+        calldata.resize(cd_offset + copy_size, FF(0));
+    }
+
     if (is_ok(error)) {
         if (is_top_level) {
             if (!check_slice_mem_range(dst_offset_resolved, copy_size)) {
@@ -2190,9 +2195,11 @@ AvmError AvmTraceBuilder::op_calldata_copy(uint8_t indirect,
                     calldata, clk, call_ptr, cd_offset, copy_size, dst_offset_resolved);
             }
         } else {
-            calldata.resize(copy_size);
             // If we are not at the top level, we write to memory directly
-            error = write_slice_to_memory(dst_offset_resolved, AvmMemoryTag::FF, calldata);
+            error = write_slice_to_memory(
+                dst_offset_resolved,
+                AvmMemoryTag::FF,
+                std::vector<FF>(calldata.begin() + cd_offset, calldata.begin() + cd_offset + copy_size));
         }
     }
 
@@ -2309,9 +2316,17 @@ AvmError AvmTraceBuilder::op_returndata_copy(uint8_t indirect,
 
     if (is_ok(error)) {
         // Write the return data to memory
-        // TODO: validate bounds
-        auto returndata_slice = std::vector(current_ext_call_ctx.nested_returndata.begin() + rd_offset,
-                                            current_ext_call_ctx.nested_returndata.begin() + rd_offset + copy_size);
+
+        // No reference as we potentially mutate.
+        auto returndata = current_ext_call_ctx.nested_returndata;
+
+        // Any out-of-range values from returndata is replaced by a zero value.
+        // We append zeros in this case to returndata.
+        if (rd_offset + copy_size > returndata.size()) {
+            returndata.resize(rd_offset + copy_size, FF(0));
+        }
+
+        auto returndata_slice = std::vector(returndata.begin() + rd_offset, returndata.begin() + rd_offset + copy_size);
 
         pc += Deserialization::get_pc_increment(OpCode::RETURNDATACOPY);
 

yarn-project/simulator/src/avm/opcodes/memory.test.ts

@@ -425,18 +425,32 @@ describe('Memory instructions', () => {
     it('Should return error when memory slice calldatacopy target is out-of-range', async () => {
       const calldata = [new Fr(1n), new Fr(2n), new Fr(3n)];
       context = initContext({ env: initExecutionEnvironment({ calldata }) });
-      context.machineState.memory.set(0, new Uint32(0)); // cdoffset
-      context.machineState.memory.set(1, new Uint32(3)); // size
+      context.machineState.memory.set(0, new Uint32(0)); // cdStart = 0
+      context.machineState.memory.set(1, new Uint32(3)); // copySize = 3
 
       await expect(
         new CalldataCopy(
           /*indirect=*/ 0,
-          /*cdOffset=*/ 0,
-          /*copySize=*/ 1,
+          /*cdStartOffset=*/ 0,
+          /*copySizeOffset=*/ 1,
           /*dstOffset=*/ TaggedMemory.MAX_MEMORY_SIZE - 2,
         ).execute(context),
       ).rejects.toThrow(MemorySliceOutOfRangeError);
     });
+
+    it('Should pad with zeros when the calldata slice is out-of-range', async () => {
+      const calldata = [new Fr(1n), new Fr(2n), new Fr(3n)];
+      context = initContext({ env: initExecutionEnvironment({ calldata }) });
+      context.machineState.memory.set(0, new Uint32(2)); // cdStart = 2
+      context.machineState.memory.set(1, new Uint32(3)); // copySize = 3
+
+      await new CalldataCopy(/*indirect=*/ 0, /*cdStartOffset=*/ 0, /*copySizeOffset=*/ 1, /*dstOffset=*/ 0).execute(
+        context,
+      );
+
+      const actual = context.machineState.memory.getSlice(/*offset=*/ 0, /*size=*/ 3);
+      expect(actual).toEqual([new Field(3), new Field(0), new Field(0)]);
+    });
   });
 
   describe('RETURNDATASIZE', () => {
@@ -523,17 +537,31 @@ describe('Memory instructions', () => {
     it('Should return error when memory slice target is out-of-range', async () => {
       context = initContext();
       context.machineState.nestedReturndata = [new Fr(1n), new Fr(2n), new Fr(3n)];
-      context.machineState.memory.set(0, new Uint32(1)); // rdoffset
-      context.machineState.memory.set(1, new Uint32(2)); // size
+      context.machineState.memory.set(0, new Uint32(1)); // rdStart = 1
+      context.machineState.memory.set(1, new Uint32(2)); // copySize = 2
 
       await expect(
         new ReturndataCopy(
           /*indirect=*/ 0,
-          /*rdOffset=*/ 0,
-          /*copySize=*/ 1,
+          /*rdStartOffset=*/ 0,
+          /*copySizeOffset=*/ 1,
           /*dstOffset=*/ TaggedMemory.MAX_MEMORY_SIZE - 1,
         ).execute(context),
       ).rejects.toThrow(MemorySliceOutOfRangeError);
     });
+
+    it('Should pad with zeros when returndata slice is out-of-range', async () => {
+      context = initContext();
+      context.machineState.nestedReturndata = [new Fr(1n), new Fr(2n), new Fr(3n)];
+      context.machineState.memory.set(0, new Uint32(2)); // rdStart = 2
+      context.machineState.memory.set(1, new Uint32(3)); // copySize = 3
+
+      await new ReturndataCopy(/*indirect=*/ 0, /*rdStartOffset=*/ 0, /*copySizeOffset=*/ 1, /*dstOffset=*/ 0).execute(
+        context,
+      );
+
+      const actual = context.machineState.memory.getSlice(/*offset=*/ 0, /*size=*/ 3);
+      expect(actual).toEqual([new Field(3), new Field(0), new Field(0)]);
+    });
   });
 });

yarn-project/simulator/src/avm/opcodes/memory.ts

@@ -190,7 +190,10 @@ export class CalldataCopy extends Instruction {
     const copySize = memory.get(copySizeOffset).toNumber();
     context.machineState.consumeGas(this.gasCost(copySize));
 
-    const transformedData = context.environment.calldata.slice(cdStart, cdStart + copySize).map(f => new Field(f));
+    // Values which are out-of-range of the calldata array will be set with Field(0);
+    const slice = context.environment.calldata.slice(cdStart, cdStart + copySize).map(f => new Field(f));
+    // slice has size = MIN(copySize, calldata.length - cdStart) as TS truncates out-of-range portion
+    const transformedData = [...slice, ...Array(copySize - slice.length).fill(new Field(0))];
 
     memory.setSlice(dstOffset, transformedData);
 
@@ -253,9 +256,10 @@ export class ReturndataCopy extends Instruction {
     const copySize = memory.get(copySizeOffset).toNumber();
     context.machineState.consumeGas(this.gasCost(copySize));
 
-    const transformedData = context.machineState.nestedReturndata
-      .slice(rdStart, rdStart + copySize)
-      .map(f => new Field(f));
+    // Values which are out-of-range of the returndata array will be set with Field(0);
+    const slice = context.machineState.nestedReturndata.slice(rdStart, rdStart + copySize).map(f => new Field(f));
+    // slice has size = MIN(copySize, returndata.length - rdStart) as TS truncates out-of-range portion
+    const transformedData = [...slice, ...Array(copySize - slice.length).fill(new Field(0))];
 
     memory.setSlice(dstOffset, transformedData);