diff --git a/bytecode-verifier.md b/bytecode-verifier.md index facc9760..a3554628 100644 --- a/bytecode-verifier.md +++ b/bytecode-verifier.md @@ -37,6 +37,7 @@ that passes vars should use the second form. | Flag operands are 0 or 1 | `Op_MarkEnd` tests `A == 1`, so a 2 silently commits a region that meant to rewind | | Definite assignment | Reading a register not written on every path reaching the instruction | | The current asset is set before it is used | A `send` or `pull` before any `set_current_asset` | +| A pull's destination does not alias its cap or overdraft | `funds.Pull` writes the destination before it reads the bounds on the bounded path: aliased, the cap no longer bounds the pull and the overdraft doubles the balance | | Mark discipline (see below) | An `Op_MarkEnd` with no open mark, a send/save/asset change inside a region, a run ending with a region open | | (`VerifyWithVars`) Var-pool indices are in range | `Op_LoadVar*` indexing past the pool, or dereferencing a nil `*Vars` | @@ -68,7 +69,9 @@ only one side of a branch"). The VM still enforces the same rules at execution time, via `runstate.HasOpenMark()` and the `errSendWhileMarkOpen` / `errSetAssetWhileMarkOpen` / `errSaveWhileMarkOpen` sentinels in `vm.go`: -`Exec` does not require a verified program. +`Exec` does not require a verified program. The same goes for a truncated +two-word instruction (`errTruncatedInstruction`) and an aliased pull destination +(`errPullDestAliasesInput`). **`internal/funds` must not be relaxed along with it.** The tree-walking interpreter shares `RunState.MarkEnd` (see `interpreter.go`), and it is not @@ -82,6 +85,18 @@ verified, so `ErrNoOpenMark` and the INVARIANT documented on `MarkEnd` stay. it is a legitimate user-facing error (`InvalidUncappedSource`, "unbounded source is not allowed here"), not a malformed program. Deliberately left to run time. +### Funds left in the queue + +A pull debits its source at once; the posting only exists once a send drains the +queue. Funds still queued when `Op_SetCurrentAsset` runs would be posted under +the new asset, and funds still queued when the run ends are a debit that never +became a posting. Whether a capped send empties the queue depends on the +balances, so neither is decidable from the instruction stream: `Exec` checks +both at run time (`errSetAssetWithQueuedFunds`, `errEndWithQueuedFunds`, via +`runstate.HasQueuedFunds()`). Compiled numscript never trips them: every send +statement either pulls exactly what it sends (`Op_CheckEnoughFunds`) or sends +everything it pulled. + ### Anything semantic A verified program can still produce nonsense: allotment portions that don't sum diff --git a/instruction-encoding.md b/instruction-encoding.md index 3763a212..e1121627 100644 --- a/instruction-encoding.md +++ b/instruction-encoding.md @@ -27,7 +27,7 @@ Instructions are **4 bytes** wide: `[Opcode: 8] [A: 8] [B: 8] [C: 8]`. 00x00SET_CURRENT_ASSET asset-- - Sets the current asset (used by PULL_ACCOUNT / SEND_TO_ACCOUNT) from str_regs[A] + Sets the current asset (used by PULL_ACCOUNT / SEND_TO_ACCOUNT) from str_regs[A]. Traps while a mark is open, or while the source queue still holds funds pulled under the previous asset 10x01ASSERT_SAME_ASSET @@ -355,7 +355,7 @@ Instructions are **4 bytes** wide: `[Opcode: 8] [A: 8] [B: 8] [C: 8]`. ​​↳ cont. overdraftcolorscope - Overdraft bound int_regs[A] (0xFF = unbounded), color str_regs[B] and scope str_regs[C] (0xFF = none). Cap and overdraft both 0xFF traps (unbounded source) + Overdraft bound int_regs[A] (0xFF = unbounded), color str_regs[B] and scope str_regs[C] (0xFF = none). Cap and overdraft both 0xFF traps (unbounded source). The destination register may not be the cap or the overdraft register: rejected by the verifier, and traps 810x51SEND_TO_ACCOUNT diff --git a/internal/funds/funds.go b/internal/funds/funds.go index ab26f93c..2cd68b80 100644 --- a/internal/funds/funds.go +++ b/internal/funds/funds.go @@ -244,6 +244,18 @@ func (s *RunState) GetAccountBalance(account, scope, asset, color string) (*big. return new(big.Int).Set(bal), nil } +// HasQueuedFunds reports whether a queued source still holds a positive amount. +// Zero-amount entries do not count: a Pull of a zero cap queues one, and a Send +// of a zero cap does not consume it. +func (s *RunState) HasQueuedFunds() bool { + for i := range s.sources { + if s.sources[i].amount.Sign() > 0 { + return true + } + } + return false +} + // Pull debits up to cap from src's (currentAsset, color) balance, queues the // pulled amount as a funding source tagged with color, and writes the amount made // available into out: diff --git a/internal/vm/export_test.go b/internal/vm/export_test.go index 77388479..335fdc0c 100644 --- a/internal/vm/export_test.go +++ b/internal/vm/export_test.go @@ -1,3 +1,9 @@ package vm -var ErrSendWhileMarkOpen = errSendWhileMarkOpen +var ( + ErrSendWhileMarkOpen = errSendWhileMarkOpen + ErrTruncatedInstruction = errTruncatedInstruction + ErrPullDestAliasesInput = errPullDestAliasesInput + ErrSetAssetWithQueuedFunds = errSetAssetWithQueuedFunds + ErrEndWithQueuedFunds = errEndWithQueuedFunds +) diff --git a/internal/vm/fuzz_test.go b/internal/vm/fuzz_test.go index 647b894c..2fd6c002 100644 --- a/internal/vm/fuzz_test.go +++ b/internal/vm/fuzz_test.go @@ -23,6 +23,7 @@ func FuzzExec(f *testing.F) { f.Add([]byte{byte(Op_LoadStr), 0, 0, 0, byte(Op_Jmp), 0, 1, 0}) // jump past the end f.Add([]byte{byte(Op_ConstTrue), 0, 0, 0, byte(Op_JmpIfTrue), 0, 0, 0}) f.Add([]byte{byte(Op_MarkPush), 0, 0, 0, byte(Op_MarkEnd), 1, 0, 0}) + f.Add([]byte{byte(Op_PullAccount), 1, 0, 1, 0xFF, nilReg, nilReg, nilReg}) // dest aliases cap stringsPool := []string{"world", "dest", "USD/2"} intsPool := []big.Int{*big.NewInt(0), *big.NewInt(7)} diff --git a/internal/vm/instruction.go b/internal/vm/instruction.go index c46f280c..a95f1bd7 100644 --- a/internal/vm/instruction.go +++ b/internal/vm/instruction.go @@ -36,6 +36,8 @@ type Opcode byte // a category without renumbering. See instruction-encoding.md. const ( // --- state & assertions (0x00) --- + // InternalError while a mark is open or while the source queue still holds + // funds pulled under the previous asset Op_SetCurrentAsset Opcode = 0x00 Op_AssertSameAsset Opcode = 0x01 @@ -168,6 +170,8 @@ const ( // The most general form: account,cap,overdraft,color,scope // The 0xFF special register means NULL for cap,overdraft,color and scope // ext.A = overdraft (int reg), ext.B = color (str reg), ext.C = scope (str reg) + // A (dest) may not be the same register as C or ext.A: verifier-rejected, + // and an InternalError at execution time Op_PullAccount Opcode = 0x50 // account?, cap?, scope? diff --git a/internal/vm/ir_test.go b/internal/vm/ir_test.go index 80c36a5f..28d513ea 100644 --- a/internal/vm/ir_test.go +++ b/internal/vm/ir_test.go @@ -1369,3 +1369,68 @@ func TestIRErrorsDoNotAliasRegisters(t *testing.T) { require.Equal(t, vm.NegativeAmountError{Amount: *big.NewInt(-20)}, second) }) } + +// A pull whose destination is also its cap or overdraft reads the bound after +// having overwritten it. The verifier rejects it; unverified, Exec does too. +func TestIRPullDestAliasingItsBoundIsRejected(t *testing.T) { + store := balances(map[string]int64{"src": 30}) + + t.Run("cap", func(t *testing.T) { + execErr := runUnverifiedIRExpectingError(t, ` + $asset = "USD/2" + set_current_asset($asset) + $src = "src" + $dest = "dest" + $overdraft = 0 + $cap = 100 + $cap = pull_account(account: $src, cap: $cap, overdraft: $overdraft) + send_to_account(account: $dest) +`, "aliases its cap or overdraft", store, nil) + require.Equal(t, vm.InternalError{Err: vm.ErrPullDestAliasesInput}, execErr) + }) + + t.Run("overdraft", func(t *testing.T) { + execErr := runUnverifiedIRExpectingError(t, ` + $asset = "USD/2" + set_current_asset($asset) + $src = "src" + $dest = "dest" + $overdraft = 0 + $cap = 100 + $overdraft = pull_account(account: $src, cap: $cap, overdraft: $overdraft) + send_to_account(account: $dest) +`, "aliases its cap or overdraft", store, nil) + require.Equal(t, vm.InternalError{Err: vm.ErrPullDestAliasesInput}, execErr) + }) +} + +// Funds pulled under one asset and still queued when the asset changes would be +// posted under the new asset. Not statically decidable (a capped send drains the +// queue or not depending on the balances), so this is a run-time check only. +func TestIRSetCurrentAssetWithQueuedFundsIsAnInternalError(t *testing.T) { + execErr := runIRExpectingError(t, ` + $usd = "USD/2" + set_current_asset($usd) + $src = "src" + $dest = "dest" + $overdraft = 0 + $cap = 100 + $pulled = pull_account(account: $src, cap: $cap, overdraft: $overdraft) + $eur = "EUR/2" + set_current_asset($eur) + send_to_account(account: $dest) +`, balances(map[string]int64{"src": 100}), nil) + require.Equal(t, vm.InternalError{Err: vm.ErrSetAssetWithQueuedFunds}, execErr) +} + +func TestIREndWithQueuedFundsIsAnInternalError(t *testing.T) { + execErr := runIRExpectingError(t, ` + $asset = "USD/2" + set_current_asset($asset) + $src = "src" + $overdraft = 0 + $cap = 100 + $pulled = pull_account(account: $src, cap: $cap, overdraft: $overdraft) +`, balances(map[string]int64{"src": 100}), nil) + require.Equal(t, vm.InternalError{Err: vm.ErrEndWithQueuedFunds}, execErr) +} diff --git a/internal/vm/verify.go b/internal/vm/verify.go index 11b4bdcb..20903c0c 100644 --- a/internal/vm/verify.go +++ b/internal/vm/verify.go @@ -580,6 +580,10 @@ func decodeInstr(instr, ext Instruction) (decoded, error) { // --- funds & postings case Op_PullAccount: + // Exec's arm rejects this too; see errPullDestAliasesInput + if instr.A == instr.C || instr.A == ext.A { + err = fmt.Errorf("pull destination int register %d aliases its cap or overdraft", instr.A) + } read(bankStr, instr.B) // account readOpt(bankInt, instr.C) // cap readOpt(bankInt, ext.A) // overdraft diff --git a/internal/vm/verify_test.go b/internal/vm/verify_test.go index b5f77d37..dd2cd42d 100644 --- a/internal/vm/verify_test.go +++ b/internal/vm/verify_test.go @@ -258,3 +258,25 @@ func TestVerifyWithVarsAllowsNilWhenNoneAreRead(t *testing.T) { func TestVerifyEmptyProgram(t *testing.T) { mustAccept(t, Program{}) } + +func TestVerifyPullDestAliasesCapOrOverdraft(t *testing.T) { + pull := func(dest, cap, overdraft byte) Program { + return Program{ + Instructions: []Instruction{ + bc(Op_LoadStr, 0, 0), + bc(Op_LoadInt, 0, 0), + bc(Op_LoadInt, 1, 0), + abc(Op_SetCurrentAsset, 0, nilReg, nilReg), + abc(Op_PullAccount, dest, 0, cap), + abc(0, overdraft, nilReg, nilReg), // ext + }, + StringsPool: []string{"acc"}, + IntsPool: []big.Int{*big.NewInt(1)}, + } + } + requireRejectedWith(t, pull(0, 0, 1), "aliases its cap or overdraft") + requireRejectedWith(t, pull(1, 0, 1), "aliases its cap or overdraft") + mustAccept(t, pull(2, 0, 1)) + // the two bounds are read-only, so they may share a register + mustAccept(t, pull(2, 0, 0)) +} diff --git a/internal/vm/vm.go b/internal/vm/vm.go index 2001ffa8..323b84ea 100644 --- a/internal/vm/vm.go +++ b/internal/vm/vm.go @@ -30,6 +30,16 @@ var ( errSaveWhileMarkOpen = errors.New("save while a mark is open") ) +// Malformed pull/send sequences the verifier does not fully cover, rejected by +// the arms below. The queue checks are not statically decidable: whether a send +// drains the queue depends on the balances. +var ( + errTruncatedInstruction = errors.New("truncated instruction: missing ext word") + errPullDestAliasesInput = errors.New("pull destination aliases its cap or overdraft") + errSetAssetWithQueuedFunds = errors.New("set_current_asset with funds still queued") + errEndWithQueuedFunds = errors.New("run ended with funds still queued") +) + type Vm struct { Program Program runstate *funds.RunState @@ -156,12 +166,19 @@ func Exec[S Store]( switch Opcode(instr.Opcode) { // --- Domain-specific ops case Op_PullAccount: - // 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. + if pc == instructionsLen { + return funds.ExecutionResult{}, InternalError{Err: errTruncatedInstruction} + } instrExt := instrs[pc] pc++ + // funds.Pull writes the destination before it reads the bounds on the + // bounded path, so an aliased cap is ignored and an aliased overdraft + // doubles the balance + if instr.A == instr.C || instr.A == instrExt.A { + return funds.ExecutionResult{}, InternalError{Err: errPullDestAliasesInput} + } + account := stringsRegs[instr.B] var cap *big.Int @@ -251,6 +268,9 @@ func Exec[S Store]( return funds.ExecutionResult{}, InternalError{Err: errSaveWhileMarkOpen} } + if pc == instructionsLen { + return funds.ExecutionResult{}, InternalError{Err: errTruncatedInstruction} + } instrExt := instrs[pc] pc++ @@ -292,6 +312,10 @@ func Exec[S Store]( if runstate.HasOpenMark() { return funds.ExecutionResult{}, InternalError{Err: errSetAssetWhileMarkOpen} } + // funds queued under the previous asset would be posted under the new one + if runstate.HasQueuedFunds() { + return funds.ExecutionResult{}, InternalError{Err: errSetAssetWithQueuedFunds} + } currentAsset = stringsRegs[instr.A] runstate.SetCurrentAsset(currentAsset) @@ -356,6 +380,9 @@ func Exec[S Store]( txMeta[stringsRegs[instr.A]] = stringsRegs[instr.B] case Op_SetAccountMeta: + if pc == instructionsLen { + return funds.ExecutionResult{}, InternalError{Err: errTruncatedInstruction} + } instrExt := instrs[pc] pc++ @@ -374,6 +401,9 @@ func Exec[S Store]( accountsMeta[key] = stringsRegs[instr.C] case Op_MetaStr: + if pc == instructionsLen { + return funds.ExecutionResult{}, InternalError{Err: errTruncatedInstruction} + } instrExt := instrs[pc] pc++ @@ -388,6 +418,9 @@ func Exec[S Store]( stringsRegs[instr.A] = v case Op_MetaInt: + if pc == instructionsLen { + return funds.ExecutionResult{}, InternalError{Err: errTruncatedInstruction} + } instrExt := instrs[pc] pc++ @@ -407,6 +440,9 @@ func Exec[S Store]( intsRegs[instr.A].Set(n) case Op_MetaPortion: + if pc == instructionsLen { + return funds.ExecutionResult{}, InternalError{Err: errTruncatedInstruction} + } instrExt := instrs[pc] pc++ @@ -426,8 +462,9 @@ func Exec[S Store]( portionsRegs[instr.A].Set(r) case Op_MetaMonetary: - // TODO crashes if this is the last instruction (the ext word carrying the - // amount destination is missing), same as Op_PullAccount. + if pc == instructionsLen { + return funds.ExecutionResult{}, InternalError{Err: errTruncatedInstruction} + } instrExt := instrs[pc] pc++ @@ -553,6 +590,9 @@ func Exec[S Store]( portionsRegs[instr.A].SetFrac(num, den) case Op_Balance: + if pc == instructionsLen { + return funds.ExecutionResult{}, InternalError{Err: errTruncatedInstruction} + } instrExt := instrs[pc] pc++ @@ -635,6 +675,11 @@ func Exec[S Store]( }) } + // a debit that never became a posting + if runstate.HasQueuedFunds() { + return funds.ExecutionResult{}, InternalError{Err: errEndWithQueuedFunds} + } + postings := runstate.GetPostings() for _, p := range postings { if !funds.ValidatePosting(p) { diff --git a/internal/vm/vm_test.go b/internal/vm/vm_test.go index 6d8922e7..ef95cdef 100644 --- a/internal/vm/vm_test.go +++ b/internal/vm/vm_test.go @@ -537,3 +537,137 @@ func TestExecutionErrorsUnwrap(t *testing.T) { require.ErrorIs(t, InternalError{Err: cause}, cause) require.ErrorIs(t, StoreError{Wrapped: cause}, cause) } + +// --- malformed pull/send sequences ----------------------------------------- + +func TestTruncatedTwoWordInstructionIsAnInternalError(t *testing.T) { + for _, op := range []Opcode{ + Op_PullAccount, + Op_Save, + Op_SetAccountMeta, + Op_MetaStr, + Op_MetaInt, + Op_MetaPortion, + Op_MetaMonetary, + Op_Balance, + } { + prog := Program{Instructions: []Instruction{abc(op, 0, 0, 0)}} + _, err := Exec(context.Background(), newTestVm(prog), nil, mockStore{}) + require.Equal(t, InternalError{Err: errTruncatedInstruction}, err, "opcode 0x%02X", byte(op)) + } +} + +// pull @src cap=ints[cap] overdraft=ints[overdraft] into ints[dest], then send +// everything to @dest. ints[1] = 100, ints[2] = 0. +func pullSendProgram(dest, cap, overdraft byte) Program { + return Program{ + Instructions: []Instruction{ + bc(Op_LoadStr, 0, 0), + bc(Op_LoadStr, 1, 1), + bc(Op_LoadStr, 2, 2), + bc(Op_LoadInt, 1, 0), + bc(Op_LoadInt, 2, 1), + abc(Op_SetCurrentAsset, 0, nilReg, nilReg), + abc(Op_PullAccount, dest, 1, cap), + abc(0, overdraft, nilReg, nilReg), // ext + abc(Op_SendToAccount, 2, nilReg, nilReg), + }, + StringsPool: []string{"USD/2", "src", "dest"}, + IntsPool: []big.Int{*big.NewInt(100), *big.NewInt(0)}, + } +} + +func TestPullDestAliasingCapOrOverdraftIsAnInternalError(t *testing.T) { + store := mockStore{bal: map[funds.PairKey]int64{{Account: "src", Asset: "USD/2"}: 30}} + + res, err := Exec(context.Background(), newTestVm(pullSendProgram(0, 1, 2)), nil, store) + require.Nil(t, err) + require.Len(t, res.Postings, 1) + require.Zero(t, res.Postings[0].Amount.Cmp(big.NewInt(30))) + + // without the check, the cap is overwritten by the balance before it is + // compared, so it no longer bounds the pull + _, err = Exec(context.Background(), newTestVm(pullSendProgram(1, 1, 2)), nil, store) + require.Equal(t, InternalError{Err: errPullDestAliasesInput}, err) + + // without the check, the overdraft is read after the balance was written into + // it, so the pull makes twice the balance available + _, err = Exec(context.Background(), newTestVm(pullSendProgram(2, 1, 2)), nil, store) + require.Equal(t, InternalError{Err: errPullDestAliasesInput}, err) +} + +func TestSetCurrentAssetWithQueuedFundsIsAnInternalError(t *testing.T) { + // pull 100 USD/2 from @src, switch to EUR/2, send: the posting would be in + // EUR/2 while the debit was in USD/2 + prog := Program{ + Instructions: []Instruction{ + bc(Op_LoadStr, 0, 0), + bc(Op_LoadStr, 1, 1), + bc(Op_LoadStr, 2, 2), + bc(Op_LoadStr, 3, 3), + bc(Op_LoadInt, 1, 0), + bc(Op_LoadInt, 2, 1), + abc(Op_SetCurrentAsset, 0, nilReg, nilReg), + abc(Op_PullAccount, 0, 1, 1), + abc(0, 2, nilReg, nilReg), // ext + abc(Op_SetCurrentAsset, 3, nilReg, nilReg), + abc(Op_SendToAccount, 2, nilReg, nilReg), + }, + StringsPool: []string{"USD/2", "src", "dest", "EUR/2"}, + IntsPool: []big.Int{*big.NewInt(100), *big.NewInt(0)}, + } + store := mockStore{bal: map[funds.PairKey]int64{{Account: "src", Asset: "USD/2"}: 100}} + + require.NoError(t, Verify(fullBanks(prog))) + _, err := Exec(context.Background(), newTestVm(prog), nil, store) + require.Equal(t, InternalError{Err: errSetAssetWithQueuedFunds}, err) +} + +func TestEndWithQueuedFundsIsAnInternalError(t *testing.T) { + prog := Program{ + Instructions: []Instruction{ + bc(Op_LoadStr, 0, 0), + bc(Op_LoadStr, 1, 1), + bc(Op_LoadInt, 1, 0), + bc(Op_LoadInt, 2, 1), + abc(Op_SetCurrentAsset, 0, nilReg, nilReg), + abc(Op_PullAccount, 0, 1, 1), + abc(0, 2, nilReg, nilReg), // ext + }, + StringsPool: []string{"USD/2", "src"}, + IntsPool: []big.Int{*big.NewInt(100), *big.NewInt(0)}, + } + store := mockStore{bal: map[funds.PairKey]int64{{Account: "src", Asset: "USD/2"}: 100}} + + require.NoError(t, Verify(fullBanks(prog))) + _, err := Exec(context.Background(), newTestVm(prog), nil, store) + require.Equal(t, InternalError{Err: errEndWithQueuedFunds}, err) +} + +// `send [USD/2 0]` lowers to a zero-cap pull (which queues a zero-amount source) +// followed by a zero-cap send (which does not consume it): the queued zero must +// not count as funds, at the next asset switch or at the end of the run. +func TestQueuedZeroAmountIsNotQueuedFunds(t *testing.T) { + prog := Program{ + Instructions: []Instruction{ + bc(Op_LoadStr, 0, 0), + bc(Op_LoadStr, 1, 1), + bc(Op_LoadStr, 2, 2), + bc(Op_LoadInt, 0, 0), // 0 + abc(Op_SetCurrentAsset, 0, nilReg, nilReg), + abc(Op_PullAccount, 1, 1, 0), + abc(0, nilReg, nilReg, nilReg), // ext: unbounded overdraft + abc(Op_SendToAccount, 2, 0, nilReg), + abc(Op_SetCurrentAsset, 0, nilReg, nilReg), + abc(Op_PullAccount, 1, 1, 0), + abc(0, nilReg, nilReg, nilReg), // ext: unbounded overdraft + }, + StringsPool: []string{"USD/2", "world", "dest"}, + IntsPool: []big.Int{*big.NewInt(0)}, + } + + require.NoError(t, Verify(fullBanks(prog))) + res, err := Exec(context.Background(), newTestVm(prog), nil, mockStore{}) + require.Nil(t, err) + require.Empty(t, res.Postings) +}