chunk: make the opcode metadata tables switch-exhaustive so drift is a build error - #807
Closed
Nitjsefnie wants to merge 1 commit into
Closed
Conversation
…pCode with no default arm (InauguralSystems#737) 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 <noreply@kimi.com>
Contributor
Author
|
Closing — your #804 already landed this (merged a few hours before I opened this; I should have re-checked the issue state first). The operand-table fix matches. One small thing I had added on top, in case it is useful: a consistency check for the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes #737. Three opcode metadata tables had drifted from the 93-opcode enum, and the drift was silent because each table's
switchcarried adefault:arm. This adds the missing entries and, more importantly, removes thedefault:arms so-Werror=switchturns any future drift into a build error.Enumerated against
vm.c's decode as ground truth:op_verify_operands) — missing exactly 1:OP_TRAJECTORY_SLOT, added asVR_RAWbeside its slot siblings. No other verifier entry, role or acceptance decision is changed.op_name— missing 4:OP_REPORT_SLOT,OP_REPORT_NAME,OP_OBSERVE_VALUE_SLOT,OP_OBSERVE_VALUE_NAME(these disassembled as???).op_has_u16— a boolean that couldn't express the 7 superinstructions, replaced byop_u16_operand_countreturning 0–3. The disassembler now skips the right number of operand words. One note on the count: the issue says 15 desyncs; enumerating them it is 16 — 6 single-u16 and 3 two-u16 non-superinstructions, plus the 7 superinstructions.All three layout functions (
op_u16_operand_count,op_verify_operands,op_stack_effect) now switch onOpCodewith nodefault:arm. Becauseop_nameis a table a switch can't enforce, a newtools/opcode_layout_check.sh(in the mould ofstdlib_index_check.sh, with a--selftestthat injects a probe opcode) covers it, wired into the suite as[99i].Scope of the security relevance, stated carefully. The verifier gap this closes is the one your own issue names: before this, a chunk could be shaped so the verifier's 1-byte walk approved one instruction stream while the VM executed operand bytes as a different one, because
OP_TRAJECTORY_SLOT's operand width was absent from the verifier's model. This PR closes that single opcode's gap and makes the class un-reintroducible via the compiler. It does not claim the untrusted-chunk path is now safe in general — this is a metadata-consistency fix, scoped to the tables, and I'm not representing it as more than that.Changes
src/chunk.c,src/compiler.c— the three tables and thedefault:-arm removals;op_has_u16→op_u16_operand_count, its one caller (the disassembler) updated.tools/opcode_layout_check.sh— new consistency gate with--selftest.tests/test_vm_run_bytecode.eigs— two regression asserts.Testing
./build.shfail with-Werror=switchat three sites; removing it restores a clean build. The reverse confirms the olddefault:arms were masking it — the same injection builds clean on pre-fixmain.f59d3f1; the absolute climbs quickly here, so that is +2 asserts over the base's 3360).make asanclean, bytecode and sandbox tests clean under asan+ubsan.[99i]verified unique against the current section numbers.A correctness note surfaced while enumerating and left out of scope:
OP_WIDE(vm.c) is an unimplemented placeholder, so if it is ever implemented it will need adding to these same tables — the new-Werror=switchenforcement will catch that automatically.OP_MATCH's oldop_stack_effectdefault masked its real+1effect, but that path has no emit site and is dead; made explicit rather than left under a default.Related Issues and Pull Requests
Fixes #737
Checklist
make testpasses locally — full suite3363/3363 passed, 0 failed;make asancleandocs/BUILTINS.md— n/a, no new builtinsdocs/STDLIB.md— n/aGenerated by Claude Opus 5 (brief, review), Kimi K3 (implementation, verification)