From 85af7856a2c7b8c9c1f388ee644740bd77feac3f Mon Sep 17 00:00:00 2001 From: jeanmon Date: Wed, 26 Mar 2025 10:05:33 +0000 Subject: [PATCH 1/4] 12934: AVM TS - move tag validation outside of instruction constructors --- .../src/public/avm/avm_memory_types.ts | 6 ++--- .../src/public/avm/opcodes/memory.ts | 18 ++++++------- .../instruction_serialization.ts | 25 ++++++++++++++++--- 3 files changed, 32 insertions(+), 17 deletions(-) diff --git a/yarn-project/simulator/src/public/avm/avm_memory_types.ts b/yarn-project/simulator/src/public/avm/avm_memory_types.ts index 7590b2bdaceb..db2d286fc8d9 100644 --- a/yarn-project/simulator/src/public/avm/avm_memory_types.ts +++ b/yarn-project/simulator/src/public/avm/avm_memory_types.ts @@ -329,10 +329,8 @@ export class TaggedMemory implements TaggedMemoryInterface { } } - public static checkIsValidTag(tagNumber: number) { - if (!VALID_TAGS.has(tagNumber)) { - throw new InvalidTagValueError(tagNumber); - } + public static checkIsValidTag(tagNumber: number): boolean { + return VALID_TAGS.has(tagNumber); } /** diff --git a/yarn-project/simulator/src/public/avm/opcodes/memory.ts b/yarn-project/simulator/src/public/avm/opcodes/memory.ts index 9420c03b4884..c82898cbde98 100644 --- a/yarn-project/simulator/src/public/avm/opcodes/memory.ts +++ b/yarn-project/simulator/src/public/avm/opcodes/memory.ts @@ -13,42 +13,42 @@ export class Set extends Instruction { OperandType.UINT8, // opcode OperandType.UINT8, // indirect OperandType.UINT8, // dstOffset - OperandType.UINT8, // tag + OperandType.TAG, // tag OperandType.UINT8, // const (value) ]; public static readonly wireFormat16: OperandType[] = [ OperandType.UINT8, // opcode OperandType.UINT8, // indirect OperandType.UINT16, // dstOffset - OperandType.UINT8, // tag + OperandType.TAG, // tag OperandType.UINT16, // const (value) ]; public static readonly wireFormat32: OperandType[] = [ OperandType.UINT8, // opcode OperandType.UINT8, // indirect OperandType.UINT16, // dstOffset - OperandType.UINT8, // tag + OperandType.TAG, // tag OperandType.UINT32, // const (value) ]; public static readonly wireFormat64: OperandType[] = [ OperandType.UINT8, // opcode OperandType.UINT8, // indirect OperandType.UINT16, // dstOffset - OperandType.UINT8, // tag + OperandType.TAG, // tag OperandType.UINT64, // const (value) ]; public static readonly wireFormat128: OperandType[] = [ OperandType.UINT8, // opcode OperandType.UINT8, // indirect OperandType.UINT16, // dstOffset - OperandType.UINT8, // tag + OperandType.TAG, // tag OperandType.UINT128, // const (value) ]; public static readonly wireFormatFF: OperandType[] = [ OperandType.UINT8, // opcode OperandType.UINT8, // indirect OperandType.UINT16, // dstOffset - OperandType.UINT8, // tag + OperandType.TAG, // tag OperandType.FF, // const (value) ]; @@ -59,7 +59,6 @@ export class Set extends Instruction { private value: bigint | number, ) { super(); - TaggedMemory.checkIsValidTag(inTag); } public async execute(context: AvmContext): Promise { @@ -85,19 +84,18 @@ export class Cast extends Instruction { OperandType.UINT8, OperandType.UINT8, OperandType.UINT8, - OperandType.UINT8, + OperandType.TAG, ]; static readonly wireFormat16 = [ OperandType.UINT8, OperandType.UINT8, OperandType.UINT16, OperandType.UINT16, - OperandType.UINT8, + OperandType.TAG, ]; constructor(private indirect: number, private srcOffset: number, private dstOffset: number, private dstTag: number) { super(); - TaggedMemory.checkIsValidTag(dstTag); } public async execute(context: AvmContext): Promise { diff --git a/yarn-project/simulator/src/public/avm/serialization/instruction_serialization.ts b/yarn-project/simulator/src/public/avm/serialization/instruction_serialization.ts index f1287c63f013..bf1b933d7c26 100644 --- a/yarn-project/simulator/src/public/avm/serialization/instruction_serialization.ts +++ b/yarn-project/simulator/src/public/avm/serialization/instruction_serialization.ts @@ -1,5 +1,7 @@ import { strict as assert } from 'assert'; +import { TaggedMemory } from '../avm_memory_types.js'; +import { InvalidTagValueError } from '../errors.js'; import { BufferCursor } from './buffer_cursor.js'; /** @@ -103,6 +105,7 @@ export enum OperandType { UINT64, UINT128, FF, + TAG, } type OperandNativeType = number | bigint; @@ -116,6 +119,7 @@ const OPERAND_SPEC = new Map OperandNa [OperandType.UINT64, [8, Buffer.prototype.readBigInt64BE, Buffer.prototype.writeBigInt64BE]], [OperandType.UINT128, [16, readBigInt128BE, writeBigInt128BE]], [OperandType.FF, [32, readBigInt254BE, writeBigInt254BE]], + [OperandType.TAG, [1, Buffer.prototype.readUint8, Buffer.prototype.writeUint8]], ]); function readBigInt254BE(this: Buffer, offset: number): bigint { @@ -166,11 +170,26 @@ export function deserialize(cursor: BufferCursor | Buffer, operands: OperandType cursor = new BufferCursor(cursor); } - for (const op of operands) { - const opType = op; + let isTagValid: boolean = true; + let invalidTagValue: number = 0; // Only used when a tag value is invalid. + + for (const opType of operands) { const [sizeBytes, reader, _writer] = OPERAND_SPEC.get(opType)!; - argValues.push(reader.call(cursor.buffer(), cursor.position())); + const value = reader.call(cursor.buffer(), cursor.position()); + argValues.push(value); cursor.advance(sizeBytes); + + // We do not throw here as we first want to detect other parsing errors (e.g., instruction size + // is longer than remaining bytes) first. Order of errors need to match with circuit. + if (opType == OperandType.TAG) { + // We parsed a single byte (readUInt8()) and therefore value is of number type (not bigint) + isTagValid &&= TaggedMemory.checkIsValidTag(value as number); + invalidTagValue = value as number; + } + } + + if (!isTagValid) { + throw new InvalidTagValueError(invalidTagValue); } return argValues; From 3d1e8a8cc8fbbc409ae70dacac2914e03d93868c Mon Sep 17 00:00:00 2001 From: jeanmon Date: Wed, 26 Mar 2025 10:20:40 +0000 Subject: [PATCH 2/4] Add unit test --- .../serialization/bytecode_serialization.test.ts | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/yarn-project/simulator/src/public/avm/serialization/bytecode_serialization.test.ts b/yarn-project/simulator/src/public/avm/serialization/bytecode_serialization.test.ts index 7704c2ec6698..0dd6af56263f 100644 --- a/yarn-project/simulator/src/public/avm/serialization/bytecode_serialization.test.ts +++ b/yarn-project/simulator/src/public/avm/serialization/bytecode_serialization.test.ts @@ -179,6 +179,22 @@ describe('Bytecode Serialization', () => { } }); + it('Should throw an AvmParsingError while deserializing an incomplete instruction and a wrong tag value', () => { + const decodeIncomplete = (truncated: Buffer) => { + return () => decodeFromBytecode(truncated); + }; + + const bufSet16Truncated = Buffer.from([ + Opcode.SET_16, //opcode + 0x02, // indirect + ...Buffer.from('3456', 'hex'), // dstOffset + 0x09, //tag (invalid) + // We should have a 16-bit value here (truncation) + ]); + + expect(decodeIncomplete(bufSet16Truncated)).toThrow(AvmParsingError); + }); + it('Should throw an InvalidTagValueError while deserializing a tag value out of range', () => { const decodeInvalidTag = (buf: Buffer) => { return () => decodeFromBytecode(buf); From 7a7aec0e86d34f9b7cc36b2aa72872790d177d28 Mon Sep 17 00:00:00 2001 From: jeanmon Date: Wed, 26 Mar 2025 11:16:34 +0000 Subject: [PATCH 3/4] Addressing review feedback --- .../src/public/avm/avm_memory_types.ts | 6 ++-- .../instruction_serialization.ts | 28 +++++++++---------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/yarn-project/simulator/src/public/avm/avm_memory_types.ts b/yarn-project/simulator/src/public/avm/avm_memory_types.ts index db2d286fc8d9..7590b2bdaceb 100644 --- a/yarn-project/simulator/src/public/avm/avm_memory_types.ts +++ b/yarn-project/simulator/src/public/avm/avm_memory_types.ts @@ -329,8 +329,10 @@ export class TaggedMemory implements TaggedMemoryInterface { } } - public static checkIsValidTag(tagNumber: number): boolean { - return VALID_TAGS.has(tagNumber); + public static checkIsValidTag(tagNumber: number) { + if (!VALID_TAGS.has(tagNumber)) { + throw new InvalidTagValueError(tagNumber); + } } /** diff --git a/yarn-project/simulator/src/public/avm/serialization/instruction_serialization.ts b/yarn-project/simulator/src/public/avm/serialization/instruction_serialization.ts index bf1b933d7c26..972b831aeb50 100644 --- a/yarn-project/simulator/src/public/avm/serialization/instruction_serialization.ts +++ b/yarn-project/simulator/src/public/avm/serialization/instruction_serialization.ts @@ -1,7 +1,6 @@ import { strict as assert } from 'assert'; import { TaggedMemory } from '../avm_memory_types.js'; -import { InvalidTagValueError } from '../errors.js'; import { BufferCursor } from './buffer_cursor.js'; /** @@ -108,6 +107,9 @@ export enum OperandType { TAG, } +// Define a type that represents the possible types of the deserialized values. +export type DeserializedValue = number | bigint; + type OperandNativeType = number | bigint; type OperandWriter = (value: any) => void; @@ -164,34 +166,30 @@ function writeBigInt128BE(this: Buffer, value: bigint): void { * @param operands Specification of the operand types. * @returns An array as big as {@code operands}, with the converted TS values. */ -export function deserialize(cursor: BufferCursor | Buffer, operands: OperandType[]): (number | bigint)[] { - const argValues = []; +export function deserialize(cursor: BufferCursor | Buffer, operands: OperandType[]): DeserializedValue[] { + const argValues: DeserializedValue[] = []; if (Buffer.isBuffer(cursor)) { cursor = new BufferCursor(cursor); } - let isTagValid: boolean = true; - let invalidTagValue: number = 0; // Only used when a tag value is invalid. - for (const opType of operands) { const [sizeBytes, reader, _writer] = OPERAND_SPEC.get(opType)!; const value = reader.call(cursor.buffer(), cursor.position()); argValues.push(value); cursor.advance(sizeBytes); + } - // We do not throw here as we first want to detect other parsing errors (e.g., instruction size - // is longer than remaining bytes) first. Order of errors need to match with circuit. - if (opType == OperandType.TAG) { + // We first want to detect other parsing errors (e.g., instruction size + // is longer than remaining bytes) first and therefore tag validation is done after completion + // of parsing above. Order of errors need to match with circuit. + for (let i = 0; i < operands.length; i++) { + if (operands[i] === OperandType.TAG) { // We parsed a single byte (readUInt8()) and therefore value is of number type (not bigint) - isTagValid &&= TaggedMemory.checkIsValidTag(value as number); - invalidTagValue = value as number; + // We need to cast to number because checkIsValidTag expects a number. + TaggedMemory.checkIsValidTag(argValues[i] as number); } } - if (!isTagValid) { - throw new InvalidTagValueError(invalidTagValue); - } - return argValues; } From 31dd49df47e56dcfc7f6614ef4ef5f5b5db609e0 Mon Sep 17 00:00:00 2001 From: jeanmon Date: Wed, 26 Mar 2025 11:48:30 +0000 Subject: [PATCH 4/4] Addressing review feedback --- .../src/public/avm/serialization/instruction_serialization.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/yarn-project/simulator/src/public/avm/serialization/instruction_serialization.ts b/yarn-project/simulator/src/public/avm/serialization/instruction_serialization.ts index 972b831aeb50..14e0a7812127 100644 --- a/yarn-project/simulator/src/public/avm/serialization/instruction_serialization.ts +++ b/yarn-project/simulator/src/public/avm/serialization/instruction_serialization.ts @@ -108,7 +108,7 @@ export enum OperandType { } // Define a type that represents the possible types of the deserialized values. -export type DeserializedValue = number | bigint; +type DeserializedValue = number | bigint; type OperandNativeType = number | bigint; type OperandWriter = (value: any) => void; @@ -186,7 +186,7 @@ export function deserialize(cursor: BufferCursor | Buffer, operands: OperandType if (operands[i] === OperandType.TAG) { // We parsed a single byte (readUInt8()) and therefore value is of number type (not bigint) // We need to cast to number because checkIsValidTag expects a number. - TaggedMemory.checkIsValidTag(argValues[i] as number); + TaggedMemory.checkIsValidTag(Number(argValues[i]) ?? 0); } }