Repository navigation
Conversation
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
left a comment
There was a problem hiding this comment.
The required automated review completed with no remaining findings.
| // TODO crashes if this is the last instruction (the ext word is | ||
| // missing): instrs[pc] reads past the end. e.g. a program ending in a | ||
| // lone Op_PullAccount word. | ||
| instrExt := instrs[pc] |
There was a problem hiding this comment.
🟠 [major] Guard extension-word reads before indexing instructions
An unverified program ending with Op_PullAccount still executes instrs[pc] after pc has advanced past the final word, causing an index-out-of-range panic rather than the promised InternalError. Op_MetaMonetary retains the same unchecked read at line 437. The change removes the TODOs without implementing the stated fail-closed behavior.
Suggestion: Check that an extension word exists before each two-word instruction reads it and return an InternalError for truncation. Add direct Exec tests for each truncated opcode.
| // TODO crashes if this is the last instruction (the ext word is | ||
| // missing): instrs[pc] reads past the end. e.g. a program ending in a | ||
| // lone Op_PullAccount word. | ||
| instrExt := instrs[pc] |
There was a problem hiding this comment.
🟠 [major] Reject aliased pull destinations at runtime
Exec still passes the destination and cap/overdraft register pointers directly to the funds engine without comparing their indices. An unverified pull whose destination aliases a bound therefore retains the malformed-bytecode behavior this PR says Exec rejects: the destination write can change the bound and allow an excessive debit. The new verifier test does not protect callers executing unverified bytecode.
Suggestion: Before invoking Pull or PullUncapped, reject a destination equal to either non-nil cap or overdraft register with InternalError, and add direct Exec regression tests for both aliases.
Problem
Two pull/send shapes pass
vm.Verifyand produce wrong postings with no error:pull_accountwhose destination register is also its cap or overdraft.funds.Pullwrites 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.set_current_assetwhile 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
Execreturns anInternalErrorfor all four:errTruncatedInstructionon every two-word opcode (one compare before the ext-word read).errPullDestAliasesInputonOp_PullAccount(two byte compares).errSetAssetWithQueuedFundsanderrEndWithQueuedFunds, via a newfunds.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.Verifyrejects 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.mddocuments that under "What it does not check".instruction-encoding.mdand the opcode comments carry the new traps.Tests
go test ./..., the difftest module,-raceon the touched packages,TestCompiledCorpusPassesVerify, and 20s each ofFuzzExecandFuzzMutatedBytecodeare 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 twoHasQueuedFundscalls on an empty queue.Not changed
ir.Typecheckdoes not know the aliasing rule. Hand-written IR with it assembles, and is caught byVerifyone level down.