diff --git a/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp b/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp index 01a8237db68..d1407928a19 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm/trace/trace.cpp +++ b/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(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); diff --git a/yarn-project/simulator/src/avm/opcodes/memory.test.ts b/yarn-project/simulator/src/avm/opcodes/memory.test.ts index 6cd8b7ff7a9..1ba27d2b0d9 100644 --- a/yarn-project/simulator/src/avm/opcodes/memory.test.ts +++ b/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)]); + }); }); }); diff --git a/yarn-project/simulator/src/avm/opcodes/memory.ts b/yarn-project/simulator/src/avm/opcodes/memory.ts index 1a0c6c03ca3..e1e1cf9b2fe 100644 --- a/yarn-project/simulator/src/avm/opcodes/memory.ts +++ b/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);