Skip to content

fix(vm): fail closed on malformed pull/send bytecode - #217

Open
ascandone wants to merge 1 commit into
mainfrom
fix/vm-malformed-pull-send
Open

ascandone wants to merge 1 commit into
mainfrom
fix/vm-malformed-pull-send

Conversation

@ascandone

Copy link
Copy Markdown
Contributor

Problem

Two pull/send shapes pass vm.Verify and produce wrong postings with no error:

  • A pull_account whose destination register is also its cap or overdraft. funds.Pull writes the destination before it reads the bounds on the bounded path, so an aliased cap stops bounding the pull and an aliased overdraft doubles the balance. Balance 30, cap 100, overdraft 0 pulls 60, leaving the source at -30 with a zero overdraft.
  • A set_current_asset while funds are still queued. The debit was taken under the previous asset; the posting is emitted under the new one.

Two more are silent but less severe: a run that ends with funds still queued drops the debit, and an unverified program ending in the first word of a two-word instruction panics on the ext-word read (the old TODO in vm.go).

Compiled numscript never emits any of these: the allocator never aliases a destination with an operand of the same instruction, and every send statement either pulls exactly what it sends (check_enough_funds) or sends everything it pulled. They matter for bytecode that did not come out of this process's compiler, which is what the ledger replays.

Fix

Exec returns an InternalError for all four:

  • errTruncatedInstruction on every two-word opcode (one compare before the ext-word read).
  • errPullDestAliasesInput on Op_PullAccount (two byte compares).
  • errSetAssetWithQueuedFunds and errEndWithQueuedFunds, via a new funds.RunState.HasQueuedFunds(). Zero-amount queue entries do not count: send [USD 0] lowers to a zero-cap pull that queues one and a zero-cap send that does not consume it.

Verify rejects the aliasing statically. The queue checks are not statically decidable (whether a capped send drains the queue depends on the balances), so they stay at run time; bytecode-verifier.md documents that under "What it does not check". instruction-encoding.md and the opcode comments carry the new traps.

Tests

  • White-box bytecode tests for each check plus the zero-amount case; a verifier test for the aliasing rule (and that cap and overdraft may share a register); IR-driven tests so the shapes sit in the "sequences the compiler never emits" corpus.
  • go test ./..., the difftest module, -race on the touched packages, TestCompiledCorpusPassesVerify, and 20s each of FuzzExec and FuzzMutatedBytecode are clean.

Performance

BenchmarkCompiledVM*, 10 runs each, comparing minimums: identical allocations, timings within noise except the smallest script (BenchmarkCompiledVM, 630 → 648 ns/op), which is the two HasQueuedFunds calls on an empty queue.

Not changed

  • An unverified program naming a register past its bank still panics; verify-first remains the contract.
  • ir.Typecheck does not know the aliasing rule. Hand-written IR with it assembles, and is caught by Verify one level down.

Two pull/send shapes passed vm.Verify and produced wrong postings with
no error: a pull whose destination register aliases its cap or overdraft
(funds.Pull writes the destination before it reads the bounds, so the
cap stops bounding and the overdraft doubles the balance), and a
set_current_asset with funds still queued (the posting comes out under
the new asset). A run ending with queued funds dropped the debit, and
an unverified program ending in the first word of a two-word
instruction panicked.

Exec now returns an InternalError for all four; the verifier rejects
the aliasing statically. Queue emptiness depends on the balances, so
the two queue checks stay at run time.
@NumaryBot NumaryBot added risk: medium NumaryBot classified this pull request as medium risk. bot-reviewed NumaryBot completed its review workflow for the current head. review-approved The NumaryBot review gate is satisfied for the current head. labels Oct 12, 2026

@NumaryBot NumaryBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The required automated review completed with no remaining findings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot-reviewed NumaryBot completed its review workflow for the current head. review-approved The NumaryBot review gate is satisfied for the current head. risk: medium NumaryBot classified this pull request as medium risk.

Development

Successfully merging this pull request may close these issues.

2 participants