From 666a7b255a8f046eeaf8fd929d59414aba908fc9 Mon Sep 17 00:00:00 2001 From: Nitjsefnie Date: Sun, 2 Aug 2026 09:35:00 +0200 Subject: [PATCH] chunk: close the opcode metadata drift; type the layout switches as OpCode with no default arm (#737) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three of the four hand-maintained opcode metadata tables had drifted: - op_verify_operands (the untrusted-chunk sandbox gate) was missing OP_TRAJECTORY_SLOT: its default arm walked the 3-byte instruction as 1 byte, so is_start[] marked the operand's bytes as instruction boundaries and a crafted jump could land mid-instruction and still pass pass 2. Added beside its slot siblings as VR_RAW (slots are runtime-guarded); no other entry, role, or acceptance changed. - op_name was missing OP_REPORT_SLOT, OP_REPORT_NAME, OP_OBSERVE_VALUE_SLOT, OP_OBSERVE_VALUE_NAME (printed as "???"). - op_has_u16 was missing 16 opcodes that carry u16 operands (9 plain, 7 superinstructions), desynchronizing chunk_disassemble on all of them. A boolean cannot express the multi-operand superinstructions, so it is now op_u16_operand_count returning 0..3, and the disassembler skips all operands. Enforcement: op_u16_operand_count, op_verify_operands, and op_stack_effect (compiler.c) now switch on OpCode with NO default arm, so -Werror=switch (Makefile:9) turns a missing case into a build error. Verified: injecting a fake enum opcode fails the build in all three functions; removing it builds clean. Gate: tools/opcode_layout_check.sh (in the style of stdlib_index_check.sh) requires every enum opcode in all four metadata sites — covering op_name, a lookup table the switch enforcement structurally cannot reach — with a --selftest that injects a fake opcode. Wired into the suite as [99i]. Regression asserts for the verifier hole added to test_vm_run_bytecode.eigs. Co-Authored-By: Kimi K3 --- src/chunk.c | 129 +++++++++++++++++++++++++++----- src/compiler.c | 28 ++++++- tests/run_all_tests.sh | 11 +++ tests/test_vm_run_bytecode.eigs | 14 ++++ tools/opcode_layout_check.sh | 79 +++++++++++++++++++ 5 files changed, 237 insertions(+), 24 deletions(-) create mode 100755 tools/opcode_layout_check.sh diff --git a/src/chunk.c b/src/chunk.c index f7cdc0bf..07c9e038 100644 --- a/src/chunk.c +++ b/src/chunk.c @@ -373,14 +373,30 @@ const char *op_name(uint8_t op) { [OP_DEFAULT_PARAM] = "DEFAULT_PARAM", [OP_DESTRUCTURE_UNPACK] = "DESTRUCTURE_UNPACK", [OP_SLICE_GET] = "SLICE_GET", + /* #737: these four drifted out of the table and disassembled as + * "???" — their siblings (REPORT_VALUE_*, OBSERVE_ASSIGN*) were + * present, which is exactly how the omission went unnoticed. */ + [OP_REPORT_SLOT] = "REPORT_SLOT", + [OP_REPORT_NAME] = "REPORT_NAME", + [OP_OBSERVE_VALUE_SLOT] = "OBSERVE_VALUE_SLOT", + [OP_OBSERVE_VALUE_NAME] = "OBSERVE_VALUE_NAME", }; if (op < OP_COUNT && names[op]) return names[op]; return "???"; } -/* Returns 1 if opcode has a 16-bit operand */ -static int op_has_u16(uint8_t op) { +/* Number of 16-bit operands an opcode carries (0..3). Mirrors the operand + * layout the VM decodes in vm.c — keep in lockstep if an opcode changes. + * (Was op_has_u16, a boolean — which structurally could not express the + * multi-operand superinstructions, so chunk_disassemble desynchronized on + * them and on the operand-carrying opcodes missing here entirely, #737.) + * + * Deliberately switched on OpCode with NO default arm: -Werror=switch + * (Makefile:9) then makes a missing case — e.g. a newly added opcode — a + * BUILD ERROR instead of a silently wrong operand width. */ +static int op_u16_operand_count(OpCode op) { switch (op) { + /* One u16 operand */ case OP_CONST: case OP_GET_LOCAL: case OP_SET_LOCAL: case OP_GET_NAME: case OP_SET_NAME: case OP_SET_NAME_LOCAL: case OP_SET_FN_NAME_LOCAL: @@ -394,16 +410,50 @@ static int op_has_u16(uint8_t op) { case OP_TRY_BEGIN: case OP_LOOP_STALL_CHECK: case OP_LOOP_CAP_CHECK: case OP_OBSERVE_ASSIGN: case OP_OBSERVE_ASSIGN_LOCAL: case OP_IMPORT: case OP_MATCH: + case OP_DESTRUCTURE_UNPACK: + case OP_REPORT_SLOT: case OP_REPORT_NAME: + case OP_OBSERVE_VALUE_SLOT: case OP_OBSERVE_VALUE_NAME: + case OP_OBSERVE_NAME_POST: case OP_REPORT_VALUE_SLOT: case OP_REPORT_VALUE_NAME: case OP_TRAJECTORY_SLOT: case OP_TRAJECTORY_NAME: return 1; case OP_INTERROGATE: case OP_PREDICATE: return 1; /* kind:8 but padded to 16 for uniformity */ + /* Two u16 operands */ + case OP_PREDICATE_SLOT: case OP_PREDICATE_NAME: + case OP_DEFAULT_PARAM: + case OP_LOCAL_DOT_GET: case OP_LOCAL_DOT_SET: + case OP_LOCAL_IDX_GET: + case OP_INTERROGATE_NAMED: case OP_INTERROGATE_NAMED_AT: + return 2; + /* Three u16 operands */ + case OP_LOCAL_IDX_DOT_GET: case OP_LOCAL_IDX_DOT_SET: + return 3; /* OP_LINE has a 32-bit operand (#630) — handled separately by the - * disassembler and verifier, never through the u16 path. */ - default: + * disassembler and verifier, never through the u16 path. OP_WIDE is an + * unimplemented placeholder, never emitted. The rest carry no operand. */ + case OP_LINE: case OP_WIDE: + case OP_NULL: case OP_NUM_ZERO: case OP_NUM_ONE: + case OP_ADD: case OP_SUB: case OP_MUL: case OP_DIV: case OP_MOD: + case OP_BAND: case OP_BOR: case OP_BXOR: case OP_SHL: case OP_SHR: + case OP_NEG: case OP_NOT: case OP_BNOT: + case OP_EQ: case OP_NE: case OP_LT: case OP_GT: case OP_LE: case OP_GE: + case OP_POP: case OP_DUP: case OP_DUP2: + case OP_RETURN: case OP_RETURN_NULL: + case OP_INDEX_GET: case OP_INDEX_SET: + case OP_ITER_SETUP: + case OP_LOOP_ENV_FRESH: case OP_LOOP_ENV_END: case OP_LOOP_ENV_CLEAR: + case OP_BREAK: case OP_CONTINUE: + case OP_TRY_END: + case OP_UNOBSERVED_BEGIN: case OP_UNOBSERVED_END: + case OP_LISTCOMP_BEGIN: case OP_LISTCOMP_APPEND: + case OP_DISPATCH: case OP_SLICE_GET: + case OP_COUNT: /* sentinel, never an instruction */ return 0; } + /* Unreachable for any valid enumerator; guards an out-of-enum byte + * (NOT a default arm — -Werror=switch still fires on a missing case). */ + return 0; } void chunk_disassemble(EigsChunk *chunk, const char *label) { @@ -423,17 +473,26 @@ void chunk_disassemble(EigsChunk *chunk, const char *label) { ((uint32_t)chunk->code[i + 3] << 24); fprintf(stderr, " %u", arg); i += 4; - } else if (op_has_u16(op) && i + 1 < chunk->code_len) { - uint16_t arg = chunk->code[i] | (chunk->code[i + 1] << 8); - fprintf(stderr, " %d", arg); - if (op == OP_CONST && arg < (uint16_t)chunk->const_count) { - Value *v = chunk->constants[arg]; - if (v->type == VAL_NUM) - fprintf(stderr, " (%.6g)", v->data.num); - else if (v->type == VAL_STR) - fprintf(stderr, " (\"%s\")", v->data.str); + } else if (op < OP_COUNT) { + /* #737: skip ALL of the opcode's u16 operands, not just one — + * the multi-operand superinstructions desynchronized every + * following byte under the old boolean op_has_u16. The op < + * OP_COUNT guard keeps a crafted out-of-range byte (which + * op_name above prints as "???") out of the enum-typed switch: + * it has no default arm, by design. */ + int nops = op_u16_operand_count(op); + for (int k = 0; k < nops && i + 1 < chunk->code_len; k++) { + uint16_t arg = chunk->code[i] | (chunk->code[i + 1] << 8); + fprintf(stderr, " %d", arg); + if (k == 0 && op == OP_CONST && arg < (uint16_t)chunk->const_count) { + Value *v = chunk->constants[arg]; + if (v->type == VAL_NUM) + fprintf(stderr, " (%.6g)", v->data.num); + else if (v->type == VAL_STR) + fprintf(stderr, " (\"%s\")", v->data.str); + } + i += 2; } - i += 2; } fprintf(stderr, "\n"); } @@ -470,8 +529,17 @@ typedef enum { } VerifyRole; /* Fill roles[] for op; return its operand count (0..3). Mirrors the operand - * layout the VM decodes in vm.c — keep in lockstep if an opcode changes. */ -static int op_verify_operands(uint8_t op, VerifyRole roles[3]) { + * layout the VM decodes in vm.c — keep in lockstep if an opcode changes. + * + * Deliberately switched on OpCode with NO default arm: -Werror=switch + * (Makefile:9) then makes a missing case a BUILD ERROR. The old + * `default: return 0` silently absorbed OP_TRAJECTORY_SLOT (#737): its + * 3-byte instruction was walked as 1 byte, so is_start[] marked the + * operand's bytes as instruction boundaries and a crafted jump could land + * mid-instruction and still pass pass 2 — in the untrusted-chunk sandbox + * gate. The caller (chunk_verify) rejects op >= OP_COUNT before calling, + * so every value reaching here is a valid enumerator. */ +static int op_verify_operands(OpCode op, VerifyRole roles[3]) { switch (op) { case OP_CONST: roles[0] = VR_CONST; return 1; @@ -494,12 +562,10 @@ static int op_verify_operands(uint8_t op, VerifyRole roles[3]) { case OP_LIST: case OP_DICT: case OP_OBSERVE_ASSIGN: case OP_OBSERVE_ASSIGN_LOCAL: case OP_REPORT_SLOT: case OP_OBSERVE_VALUE_SLOT: - case OP_REPORT_VALUE_SLOT: + case OP_REPORT_VALUE_SLOT: case OP_TRAJECTORY_SLOT: case OP_INTERROGATE: case OP_PREDICATE: case OP_MATCH: case OP_DESTRUCTURE_UNPACK: roles[0] = VR_RAW; return 1; - /* OP_LINE's operand is 32-bit (#630); chunk_verify walks it specially, - * so it never reaches this per-operand (u16-strided) role machinery. */ case OP_LOCAL_DOT_GET: case OP_LOCAL_DOT_SET: roles[0] = VR_RAW; roles[1] = VR_NAME; return 2; /* slot, name */ case OP_LOCAL_IDX_GET: @@ -514,9 +580,32 @@ static int op_verify_operands(uint8_t op, VerifyRole roles[3]) { roles[0] = VR_RAW; roles[1] = VR_JFWD; return 2; /* slot, skip */ case OP_LOCAL_IDX_DOT_GET: case OP_LOCAL_IDX_DOT_SET: roles[0] = VR_RAW; roles[1] = VR_RAW; roles[2] = VR_NAME; return 3; - default: + /* OP_LINE's operand is 32-bit (#630); chunk_verify walks it specially and + * never calls here — the case exists only to keep the switch exhaustive. + * OP_WIDE is an unimplemented placeholder, never emitted. The rest carry + * no operand. */ + case OP_LINE: case OP_WIDE: + case OP_NULL: case OP_NUM_ZERO: case OP_NUM_ONE: + case OP_ADD: case OP_SUB: case OP_MUL: case OP_DIV: case OP_MOD: + case OP_BAND: case OP_BOR: case OP_BXOR: case OP_SHL: case OP_SHR: + case OP_NEG: case OP_NOT: case OP_BNOT: + case OP_EQ: case OP_NE: case OP_LT: case OP_GT: case OP_LE: case OP_GE: + case OP_POP: case OP_DUP: case OP_DUP2: + case OP_RETURN: case OP_RETURN_NULL: + case OP_INDEX_GET: case OP_INDEX_SET: + case OP_ITER_SETUP: + case OP_LOOP_ENV_FRESH: case OP_LOOP_ENV_END: case OP_LOOP_ENV_CLEAR: + case OP_BREAK: case OP_CONTINUE: + case OP_TRY_END: + case OP_UNOBSERVED_BEGIN: case OP_UNOBSERVED_END: + case OP_LISTCOMP_BEGIN: case OP_LISTCOMP_APPEND: + case OP_DISPATCH: case OP_SLICE_GET: + case OP_COUNT: /* sentinel, never an instruction */ return 0; /* no operand */ } + /* Unreachable — chunk_verify rejects op >= OP_COUNT before calling; not + * a default arm, so -Werror=switch still fires on a missing case. */ + return 0; } int chunk_verify(EigsChunk *chunk) { diff --git a/src/compiler.c b/src/compiler.c index 7c20b5cc..3f58dea4 100644 --- a/src/compiler.c +++ b/src/compiler.c @@ -241,8 +241,12 @@ static void adjust_stack(Compiler *c, int delta) { /* ---- Emit helpers ---- */ -/* Stack effect of each opcode */ -static int op_stack_effect(uint8_t op) { +/* Stack effect of each opcode. + * + * Deliberately switched on OpCode with NO default arm: -Werror=switch + * (Makefile:9) then makes a missing case — e.g. a newly added opcode — a + * BUILD ERROR instead of a silently wrong stack depth (#737). */ +static int op_stack_effect(OpCode op) { switch (op) { /* Push 1 */ case OP_CONST: case OP_NULL: case OP_NUM_ZERO: case OP_NUM_ONE: @@ -346,10 +350,26 @@ static int op_stack_effect(uint8_t op) { /* DISPATCH: pop 3 (table, key, arg), push 1 = -2 */ case OP_DISPATCH: return -2; - /* CALL, LIST, DICT: dynamic — handled separately */ - default: + /* MATCH: vm.c decodes it as push-null (net +1), but the compiler never + * emits it (match is compiled as DUP+compare+jump) — the case exists so + * the switch stays exhaustive, not because it runs. */ + case OP_MATCH: + return 1; + /* Loop guards: iteration checks only, no stack change */ + case OP_LOOP_STALL_CHECK: case OP_LOOP_CAP_CHECK: + return 0; + /* WIDE: unimplemented placeholder, never emitted */ + case OP_WIDE: + return 0; + /* CALL, LIST, DICT: dynamic — handled separately by the emit sites */ + case OP_CALL: case OP_LIST: case OP_DICT: + return 0; + case OP_COUNT: /* sentinel, never an instruction */ return 0; } + /* Unreachable for any valid enumerator; not a default arm, so + * -Werror=switch still fires on a missing case. */ + return 0; } static void emit(Compiler *c, uint8_t op, int line) { diff --git a/tests/run_all_tests.sh b/tests/run_all_tests.sh index 5b35cb77..14f82964 100755 --- a/tests/run_all_tests.sh +++ b/tests/run_all_tests.sh @@ -4094,6 +4094,17 @@ fi rm -rf "$CONT_DIR" echo "" +echo "[99i] Opcode layout tables complete (#737)" +TOTAL=$((TOTAL + 1)) +if bash "$TESTS_DIR/../tools/opcode_layout_check.sh" && bash "$TESTS_DIR/../tools/opcode_layout_check.sh" --selftest >/dev/null; then + PASS=$((PASS + 1)) + echo " PASS: all four opcode metadata tables cover every opcode (gate self-test green)" +else + FAIL=$((FAIL + 1)) + echo " FAIL: opcode metadata table drift, or gate self-test broke (see lines above)" +fi +echo "" + # Final guard (#681): if the binary changed during the last block, results are invalid. check_binary_fingerprint diff --git a/tests/test_vm_run_bytecode.eigs b/tests/test_vm_run_bytecode.eigs index 70c88645..1a42e5a2 100644 --- a/tests/test_vm_run_bytecode.eigs +++ b/tests/test_vm_run_bytecode.eigs @@ -295,4 +295,18 @@ assert_eq of [vm_run_bytecode of [ABI, iter_neg, [[10, 20, 30], 1, "EXITED"]], 2 try9 is [TRY_BEGIN,27,0, TRY_BEGIN,24,0, TRY_BEGIN,21,0, TRY_BEGIN,18,0, TRY_BEGIN,15,0, TRY_BEGIN,12,0, TRY_BEGIN,9,0, TRY_BEGIN,6,0, TRY_BEGIN,3,0, JUMP,4,0, CONST,1,0, RETURN, CONST,0,0, RETURN] assert_eq of [vm_run_bytecode of [ABI, try9, ["NOERR", "CAUGHT8"]], "CAUGHT8", "#726: the 9th TRY_BEGIN raises instead of silently mis-pairing"] +# ---- #737: op_verify_operands was missing OP_TRAJECTORY_SLOT (91) — the +# default arm walked its 3-byte instruction as 1 byte, so the operand's bytes +# were marked instruction starts and a crafted jump could land mid-instruction +# and still pass pass 2. A legit TRAJECTORY_SLOT chunk must now verify and run +# (unobserved slot snapshots as an empty trajectory dict), and a jump into its +# operand bytes must be REJECTED. The crafted chunk is exploit-shaped: under +# the old 1-byte walk its operand bytes re-decode as a valid stream +# (NUM_ZERO; CONST 0; RETURN), so the pre-fix verifier ACCEPTED it and +# executed it (returned "x"); the post-fix verifier rejects it outright. +TRAJ_SLOT is 91 +NUM_ZERO is 2 +assert_eq of [type of (vm_run_bytecode of [ABI, [TRAJ_SLOT,0,0, RETURN], []]), "dict", "#737: TRAJECTORY_SLOT verifies as a 3-byte instruction"] +assert_eq of [vm_run_bytecode of [ABI, [JUMP,1,0, TRAJ_SLOT,NUM_ZERO,0, 0,0, RETURN], ["x"]], null, "#737: jump into TRAJECTORY_SLOT's operand is rejected"] + test_summary of null diff --git a/tools/opcode_layout_check.sh b/tools/opcode_layout_check.sh new file mode 100755 index 00000000..4dcde11b --- /dev/null +++ b/tools/opcode_layout_check.sh @@ -0,0 +1,79 @@ +#!/bin/bash +# Opcode layout-table drift gate (#737). +# +# Adding one opcode requires editing several hand-maintained metadata sites, +# and three of them had ALREADY drifted when #737 was filed: op_verify_operands +# (the untrusted-chunk sandbox gate) was missing OP_TRAJECTORY_SLOT, op_name +# printed 4 opcodes as "???", and the disassembler's operand-width table was +# missing 16 opcodes. The compile-time leg of the fix types the three layout +# switches as OpCode with no default arm so -Werror=switch (Makefile:9) +# makes a missing case a build error — but that covers only the switch-based +# tables, and only under a compiler that honors it. op_name is a lookup TABLE +# (a switch cannot enforce a designated-initializer array), so this grep gate +# is the table-agnostic backstop: every opcode in the OpCode enum must be +# named in ALL FOUR metadata sites: +# +# 1. op_name() — src/chunk.c (disassembler names) +# 2. op_u16_operand_count() — src/chunk.c (disassembler operand widths) +# 3. op_verify_operands() — src/chunk.c (untrusted-chunk verifier) +# 4. op_stack_effect() — src/compiler.c (stack-depth accounting) +# +# Usage: tools/opcode_layout_check.sh [--selftest] +# --selftest : inject a fake opcode and confirm the gate flags it (proves +# the checker isn't vacuously green). +# Exit 0 = all four tables cover every opcode; 1 = drift (or selftest failure). + +set -u +cd "$(dirname "$0")/.." + +# --- every opcode in the OpCode enum (src/vm.h), minus the OP_COUNT sentinel --- +enum_opcodes() { + awk '/^typedef enum \{/,/^\} OpCode;/' src/vm.h \ + | grep -oE '^[[:space:]]+OP_[A-Z_0-9]+' | tr -d ' ' \ + | grep -v '^OP_COUNT$' | sort -u +} + +# --- OP_ tokens mentioned inside one function body (col-0 start to col-0 }) --- +function_opcodes() { # args: file, function-start-regex + awk "/$2/,/^\\}/" "$1" | grep -oE 'OP_[A-Z_0-9]+' | grep -v '^OP_COUNT$' | sort -u +} + +check_table() { # args: label, file, function-start-regex, required-set + local label="$1" file="$2" fnre="$3" required="$4" + local have missing + have=$(function_opcodes "$file" "$fnre") + missing=$(comm -23 <(printf '%s\n' "$required") <(printf '%s\n' "$have")) + if [ -n "$missing" ]; then + echo "OPCODE TABLE DRIFT in $label ($file): opcode(s) absent from the table:" + echo "$missing" | sed 's/^/ - /' + return 1 + fi + return 0 +} + +check_all() { # arg: optional extra (fake) opcode for selftest + local extra="${1:-}" required rc=0 + required=$(enum_opcodes) + [ -n "$extra" ] && required=$(printf '%s\n%s\n' "$required" "$extra" | sort -u) + check_table "op_name" src/chunk.c '^const char \*op_name' "$required" || rc=1 + check_table "op_u16_operand_count" src/chunk.c '^static int op_u16_operand_count' "$required" || rc=1 + check_table "op_verify_operands" src/chunk.c '^static int op_verify_operands' "$required" || rc=1 + check_table "op_stack_effect" src/compiler.c '^static int op_stack_effect' "$required" || rc=1 + return $rc +} + +# --- selftest: the gate MUST catch a deliberately-injected fake opcode --- +if [ "${1:-}" = "--selftest" ]; then + if check_all "OP_ZZ_SELFTEST_PROBE" >/dev/null 2>&1; then + echo "SELFTEST FAILED: gate did not flag an injected fake opcode" + exit 1 + fi + echo "SELFTEST OK: gate flags an injected fake opcode" + exit 0 +fi + +if check_all; then + echo "opcode layout OK: $(enum_opcodes | wc -l | tr -d ' ') opcodes covered by all 4 metadata tables" + exit 0 +fi +exit 1