fix(engine)!: reject engine calls made outside a template invocation

[sdbondi]

Description

The engine enters WASM outside of any invocation twice per call: once to run the template's tari_alloc when staging the CallInfo, and once to run its tari_free on the pointer the template function returned. A template that calls tari_engine from either one reaches the full engine op set from a context the runtime attributes to no invocation.

From tari_free — the effects commit and the compute is free. An EmitLog issued there lands in the finalized result, and the WASM that produced it is never charged: invoke calls end_metered_invocation() and record_wasm_execution(...) before env.free(...), so with the meter torn down the mid-call sync in tari_engine_entrypoint returns None. Measured: 200k volatile loop iterations added to tari_free left wasm_execution_points at 116 — byte-identical to the empty version.

From tari_alloc — the node dies. The host allocates each engine-call response through tari_alloc, so a tari_alloc that calls the engine recurses host→WASM→host once per response. Verified against this branch with the guard disabled: thread has overflowed its stack / fatal runtime error: stack overflow, aborting (SIGABRT). max_call_depth does not apply — it is all one call frame — and the recursion is driven by the host, so the wasmer meter does not bound it either.

There is no recursion in the tari_free case, incidentally: the engine never frees the response to an engine call (the template owns it), so free -> engine call -> free does not cycle. That call simply succeeds where it should not.

Fix

Refuse the call in tari_engine_entrypoint when no template function invocation is in flight. New error: RuntimeError::EngineCallOutsideInvocation { op }.

The window is tracked as its own state (WasmEnv::in_template_invocation), opened and closed around func.call alone, deliberately not derived from invocation_meter. The two coincide today only because end_metered_invocation() runs before the free; widening the metering window later — to charge the alloc and free the engine drives, say — must not silently re-admit these calls.

The entrypoint can only signal a refusal by returning a null pointer, and a template is free to ignore that and return normally, so the refusal is recorded in its own slot (WasmEnv::refused_engine_call) and surfaced host-side by alloc_checked / free_checked, which wrap the two entries into WASM that happen outside an invocation. The slot is kept separate from last_engine_error, which the normal dispatch path writes: sharing it would make a mid-invocation error swallowed by a template surface as though the free had made an illegal engine call, rejecting a transaction that previously succeeded.

handle's env_mut.alloc is untouched: that runs mid-invocation, where the meter is live and allocation is legitimate.

Tests

Two cases added to the buggy template suite — test_engine_call_in_tari_free and test_engine_call_in_tari_alloc. Each builds a template that calls tari_engine from that hook, ignores the null it gets back, and returns normally; both transactions are rejected with EngineCallOutsideInvocation.

The variants supply their own tari_alloc/tari_free and deliberately do not link tari_template_abi, whose tari_free would collide on the #[no_mangle] symbol. They carry a hardcoded _ABI_TEMPLATE_DEF declaring one function so the template is callable and the engine actually reaches both hooks.

cargo test -p tari_engine passes in full (36 suites).

Also in this branch

The buggy template's extern block declared debug, which matches no host import — the engine imports tari_debug. Renamed, and given #[link(wasm_import_module = "env")] so the import resolves under the current wasm32 target rules. Unused by the existing variants, so inert for them; needed by the new ones.

Not covered

The unbilled compute itself. A tari_free that burns cycles without calling the engine is still uncharged, bounded only by the instance's leftover wasmer allowance — and each instruction gets a fresh instance. Closing that means moving the accounting after the free, or opening a second metered window around it. Larger change, left out of this PR — and note the guard above is written so that change cannot reopen this hole.

Host recursion from tari_alloc inside an invocation — tracked as #2442. Pre-existing and untouched: handle allocates every engine-call response through the template's tari_alloc, so the same host→WASM→host recursion is reachable from a tari_alloc called during a legitimate invocation, where engine calls are and must remain permitted. Verified to still abort the process with this branch applied.

Breaking

Consensus-affecting: a transaction whose template calls the engine from tari_alloc/tari_free previously committed (or crashed the node) and now rejects.

[sdbondi]

Reviewed at 0cbc852.

Verified sound

• The check sits before both the meter sync and dispatch, so no op runs and no effect commits (crates/engine/src/wasm/process.rs:181).
• No false positives: all in-invocation template code runs inside func.call, between begin_metered_invocation and end_metered_invocation. Nested cross-template calls get their own WasmProcess/WasmEnv/meter, so the callee's CallInfo alloc is correctly outside-invocation while the caller's meter stays live.
• Error precedence in alloc_checked/free_checked is right — the recorded engine error wins over MemoryAllocationFailed.
• Load-time instantiation (crates/engine/src/wasm/module.rs:136) binds stub imports, so a start-section engine call can't reach the runtime there either.
• The hardcoded 28-byte ABI blob decodes as documented (prefix includes itself, matching the existing 16-byte variant and with_memory_embedded_len).

Two things I'd change before merge.

1. The invariant is coupled to the metering lifecycle, and the stated follow-up breaks it

has_invocation_in_flight() is invocation_meter.is_some(). That is true today only because end_metered_invocation() runs before env.free(...). The "Not covered" item — "moving the accounting after the free, or opening a second metered window around it" — would make the meter live during tari_free and silently re-open exactly this hole. test_engine_call_in_tari_free would start failing, but a future variant of that refactor that only widens the window around the alloc would not be caught at all.

Make the invariant its own state (an explicit entry_context / in_template_invocation flag set around func.call), or at minimum put a load-bearing comment on begin_metered_invocation/end_metered_invocation saying the free must stay outside the window. Deriving a consensus-critical security check from a billing side-effect is the kind of thing that regresses without a compile error.

2. take_rejected_engine_call drains more than it claims

It takes the shared last_engine_error slot, which the normal dispatch error path also writes (crates/engine/src/wasm/process.rs:307-309). So on the success path, a RuntimeError recorded mid-invocation and swallowed by a template that ignored the null now surfaces out of free_checked — reported as though the free made an illegal engine call.

Two consequences:

• Misattributed diagnostics for an unrelated error.
• A second consensus-affecting change not in the Breaking section: a transaction that previously succeeded after swallowing an engine error now rejects. Templates built with tari_template_abi panic on null (crates/template_abi/src/abi/mod.rs:68) so they hit the Err path, but hand-rolled WASM is precisely the threat model here.

Surfacing it is probably the behaviour we want — but do it deliberately: drain the slot explicitly right after func.call returns Ok, and narrow take_rejected_engine_call to match only EngineCallOutsideInvocation (or give the rejection its own field).

3. Add the tari_alloc test

Flagged as uncovered in the description. It is the more dangerous half — it runs before the meter has ever been installed, and it is the first thing invoke does. A second feature variant is cheap and this is consensus code.

Follow-up (pre-existing, not introduced here)

handle's env_mut.alloc (crates/engine/src/wasm/process.rs:337) is deliberately unchecked, and correctly so — but it means a template whose tari_alloc calls tari_engine recurses host→wasm→host once per response allocation. max_call_depth: 10 does not apply (same call frame), and only EmitLog is count-capped via max_logs; something like GenerateUniqueId is not. Bounded only by metering and the native stack. Worth confirming a node cannot be stack-overflowed this way — separate issue.

Nits

• Adding #[link(wasm_import_module = "env")] and renaming debug → tari_debug changes the extern block for every buggy variant. Correct fix (the old debug matched no host import), but it is an unrelated drive-by — worth a line in the description.
• engine_call_in_free is a non-additive feature: --all-features duplicates the #[no_mangle] _ABI_TEMPLATE_DEF symbol. Same pattern as the existing return_null_abi/return_empty_abi flags, so pre-existing, but the variant count is growing.
• internal_alloc does not guard size + USIZE_SIZE overflow or a null return from alloc. Test-only; fine.

[sdbondi]

Reviewed fbd9dba and 6e29d17. Both earlier points are addressed correctly.

Addressed

• fbd9dba — in_template_invocation is now its own flag, opened and closed around func.call alone (crates/engine/src/wasm/process.rs:440-442), with a doc comment explaining why it is not derived from the meter. refused_engine_call is a separate slot carrying only EngineOp, so take_refused_engine_call can no longer drain an unrelated mid-invocation RuntimeError — the second consensus change is gone. test_engine_call_in_tari_alloc covers the alloc half; the shared execute_buggy_main helper and the call_engine() split read well.
• 6e29d17 — dropping the WasmEnv clone is semantics-preserving: Runtime is a NonNull<Box<dyn RuntimeInterface>> (crates/engine/src/runtime/mod.rs:290), so both copies always pointed at the same interface. The on_panic_handler rewrite is faithful too — the invalid-UTF8 branch returns None and records nothing, matching the old early return.

Two new findings.

1. The recursion is still open, and it is a node crash

fbd9dba's message notes that an engine call from tari_alloc "recursed host to WASM and back once per response allocation and aborted the process with a stack overflow". The guard closes that for tari_alloc driven outside an invocation. It does not close it for handle's response allocation at crates/engine/src/wasm/process.rs:340, which runs with in_template_invocation == true:

Buggy_main → tari_engine (allowed) → handle → env_mut.alloc → template tari_alloc → tari_engine (still allowed) → handle → …

max_call_depth: 10 does not apply — it is the same call frame. max_logs caps EmitLog only; GenerateUniqueId and CallerContextInvoke have no count cap and always succeed. Metering will not get there first: 100M points against a ~100-point tari_alloc body is millions of rounds, and the native stack blows at a few thousand. A stack overflow is a SIGSEGV, not a catchable trap — so this aborts a consensus node from a template anyone can publish.

test_engine_call_in_tari_alloc does not reach it because the CallInfo alloc fails first. A tari_alloc that only calls the engine once a static counter passes 1 gets past that and into the loop.

The machinery added in fbd9dba closes it — bracket the response allocation the same way:

env_mut.exit_template_invocation();
let ptr = env_mut.alloc(&mut store, len as u32);
env_mut.enter_template_invocation();
take_refused_engine_call(env_mut)?;
let ptr = ptr?;

That also makes the rule uniform and matches what the PR title already claims: no engine call from tari_alloc/tari_free, ever. It is pre-existing, so a separate PR is defensible — but it is a live remote DoS and the fix belongs next to this one.

2. 6e29d17 introduces &mut aliasing across the WASM boundary

alloc_checked (crates/engine/src/wasm/process.rs:135-141) holds env: &mut WasmEnv<Runtime> from data_and_store_mut() across env.alloc(&mut store, len), which re-enters WASM. If the template's tari_alloc calls tari_engine, wasmer hands tari_engine_entrypoint a second &mut to that same WasmEnv, and it writes refused_engine_call through it — which the outer, still-live &mut then reads. Not hypothetical: it is the exact path test_engine_call_in_tari_alloc exercises. Same shape in free_checked and with_alloc_and_mem_writer.

Before this commit the host side used a separate WasmEnv object with Arc<Mutex<_>> fields, so the two &muts never pointed at the same allocation — the interior mutability was load-bearing, not just lock ceremony. data_and_store_mut obtains its &mut T through a raw-pointer split so this compiles, and Runtime already leans on the same trick by design. But it is a Stacked Borrows violation on plain, now non-atomic fields, and the kind of thing an optimiser is entitled to break later.

Cheap fix: do not hold the borrow across the call. TypedFunction is Clone, so pull it out, drop the borrow, call, then re-borrow to drain:

let alloc_fn = self.fn_env.as_ref(store).mem_alloc_func()?.clone();
let result = alloc_fn.call(store, len);
take_refused_engine_call(self.fn_env.as_mut(store))?;

handle has the same shape at line 340 and predates this commit — the bracketing in finding 1 wants the same treatment.

Minor

• The Err arm of invoke checks take_last_engine_error and take_last_panic_message but not take_refused_engine_call. Harmless today — every refusal is drained by the alloc or free that caused it — but if the guard extends to handle, a refusal followed by a template trap would be silently dropped and the trap reported instead. Worth draining there pre-emptively.
• enter_template_invocation/exit_template_invocation are manually paired with no ? between them, which is correct but fragile to future edits. A short comment noting that nothing may return early between them would help.

[sdbondi]

Reviewed b6e23d2 and 5f5bff6, plus the rebase. LGTM.

b6e23d2 closes the aliasing on both host-driven paths: the exported function is cloned out, the borrow dropped, the call made, then a fresh borrow drains the refusal. Refusal still takes precedence over the call's own error and over MemoryAllocationFailed, and moving the null check into alloc_checked is behaviour-preserving (WasmPtr::is_null() is offset == 0). The two remaining split borrows only touch linear memory and never re-enter WASM.

Rebase — compared each rewritten commit's patch against its original: identical modulo hunk offsets, except the one real conflict in 078a94cc where upstream had swapped wasm_point_allowance() for compute_allowance() -> ComputeAllowance and split the out-of-gas error. Resolution keeps upstream's shape; nothing lost.

5f5bff6 is a pure refactor. metering_allowance reproduces the extracted block exactly — same reads, same min chain, same binding_allowance filter; the reordering is all pure reads off one borrow, and get_remaining_points still runs first. consumed is correctly carried rather than re-read, since record_wasm_execution mutates the total before the Err arm reads it. match allowance_remaining doesn't move (_ never binds, u64 is Copy), so the later filter/map still sees it. The env/env_mut/env_and_store rename is applied consistently, and alloc_checked/free_checked keep the borrow released.

Nit: metering_allowance takes a concrete &mut Store while its siblings are generic over S: AsStoreMut. Only one caller, so it does not matter.

Two things before merge, neither about the code:

• The description still frames the fix around WasmEnv::invocation_meter being Some only between begin_metered_invocation and end_metered_invocation. b1ca1e6 deliberately replaced that with an explicit window. Worth updating, since it becomes the squashed commit message.
• Branch is BEHIND again, and test (3/3) / integration test are still running.

The remaining handle recursion and its aliasing are tracked in #2442 and correctly out of scope here.