boundary: result-conversion reentrancy permits delivery after recipient poisoning #347

Summary

In the async, non-suspendable arm of createLoweredImport
(runtime/src/exec/boundary.ts:1885-1892), the discard guard that is
supposed to stop a cancelled/poisoned recipient from receiving a result is a
TOCTOU against reentrancy from the very host value it is converting. When
the resolved value is a plain object whose result field is defined via an
accessor (get x() { ... }), that accessor runs synchronously inside
convert/lowerFlatRecord, after the guard has already read false. If
the accessor poisons the instance during conversion, the guard cannot see
it: onResolve proceeds, the subtask resolves to RETURNED, and the
record's flat words are written into guest linear memory — a fully silent,
successful-looking delivery into an instance the runtime has itself just
marked poisoned.

This report covers only that poison-mode case, verified end-to-end by fresh
reproduction. A related cancel-mode variant of the same getter (the getter
cancels its own subtask instead of poisoning the instance) also writes guest
memory despite the subtask ending in CANCELLED_BEFORE_RETURNED, but that
variant's control-flow failure is already caught and routed through the
existing store.hostFailure channel (the same channel
runtime/tests/async_lower_onresolve_failure_test.ts already pins for
settle-time lowering traps) — it is not filed here as a separate incident,
to avoid conflating a known/pinned path with the novel one.

Scope note — do not conflate with a different, unverified code path:
this finding is specifically about the async non-suspendable arm at
boundary.ts:1885-1892, reached via the raw createLoweredImport /
HostSettlement seam (no DeferredHostResult, no full component
instantiation involved in the reproduction below). The sibling
suspendable arm (boundary.ts:1856-1878, DeferredHostResult-aware,
which defers conversion into produce at scheduler resume) re-checks
isInstancePoisoned(inst) immediately before its own onResolve call
(boundary.ts:1865) and looks tighter, but a getter reentering during
that onResolve/convert would face the same class of TOCTOU. That arm
was not reproduced and is not part of this report — it is a distinct
code stage from the one verified below, and any fix should be checked
against both stages rather than assumed to cover both by fixing one.

Location

runtime/src/exec/boundary.ts:1882-1892 (async, non-suspendable arm of
createLoweredImport, current baseline 1a4f5e1):

const promise = settlement.then(
  (done) => {
    store.pendingHostCalls.delete(promise);
    // Discard cancelled or poisoned recipients before lowering can
    // write guest memory or re-enter through realloc.
    if (subtask.resolved() || isInstancePoisoned(opts.instance)) return;
    try {
      onResolve(toResults(convert(fulfilled(done))));
    } catch (e) {
      store.hostFailure = e;
    }
  },
  ...

convert/toResults for a record-typed result flows through
lowerFlatRecord (runtime/src/cabi/lower.ts:130-140, reached from the
type-dispatch at lower.ts:66):

for (const f of fields) flat.push(...lowerFlat(cx, v[f.label], f.type));

v[f.label] is an ordinary property read. If the host's resolved value
defines f.label as an accessor, that accessor runs synchronously inside
the already-passed discard check, with the opportunity to reenter the
runtime and flip exactly the condition the check just verified.

Authority

Per contracts/intrinsics.md and the pinned Component Model spec's
async intrinsics (third_party/component-model/design/mvp/canonical-abi/definitions.py,
on_resolve / task-cancellation semantics), a subtask the runtime has
recorded as poisoned/cancelled must not have its result delivered to guest
memory afterward — the comment directly above the guard
(runtime/src/exec/boundary.ts:1885-1886) states this intent explicitly
("Discard cancelled or poisoned recipients before lowering can write guest
memory or re-enter through realloc"). This finding shows the guard's own
implementation defeats that stated intent.

Reproduction

Self-contained probe built directly on the public internal
createLoweredImport entry point (runtime/src/exec/boundary.ts) using the
same harness pattern as the existing
runtime/tests/poisoned_host_call_retire_test.ts and
runtime/tests/host_import_cancel_test.ts. An async host import with result
type record { x: u32, y: u32 } resolves to
{ get x() { notifyInstancePoisoned(inst, ...); return 111 }, y: 222 }.

git checkout 1a4f5e1   # or any tree at this baseline; reproduced fresh here
deno run --config deno.json --allow-read --allow-env=POLYENGINE_SCHED_SEED \
  repro-boundary-poison-getter.ts

// repro-boundary-poison-getter.ts
import { assertEq } from "./runtime/tests/support/asserts.ts";
const root = "./runtime/src/";
const boundary = await import(root + "exec/boundary.ts");
const taskMod = await import(root + "task/mod.ts");
const { createLoweredImport, newStats } = boundary;
const {
  ComponentInstanceState,
  popCurrentThread,
  pushCurrentThread,
  Store,
  Task,
  Thread,
  notifyInstancePoisoned,
  isInstancePoisoned,
} = taskMod;

const FT = {
  params: [],
  results: [{
    kind: "record",
    fields: [
      { label: "x", type: { kind: "u32" } },
      { label: "y", type: { kind: "u32" } },
    ],
  }],
  async: true,
};
const TASK_OPTS = {
  async_: true,
  callback: true,
  stringEncoding: "utf8",
  memory: null,
};

const store = new Store();
const inst = new ComponentInstanceState(0, store);
const memory = new WebAssembly.Memory({ initial: 1 });
const view = {
  addrType: "i32" as const,
  get bytes() { return new Uint8Array(memory.buffer); },
  get view() { return new DataView(memory.buffer); },
  get length() { return memory.buffer.byteLength; },
  ptrType: () => "i32" as const,
  ptrSize: () => 4 as const,
};
const opts = {
  stringEncoding: "utf8",
  memory: view as never,
  realloc: null,
  postReturn: null,
  callback: null,
  async: true,
  cancellable: false,
  coreType: { params: ["i32"], results: ["i32"] },
  instance: inst,
};
const value = {
  get x() {
    // Reentrant poison, fired synchronously from inside the field read
    // that lowerFlatRecord performs during conversion.
    notifyInstancePoisoned(inst, new Error("reentrant poison from getter"));
    return 111;
  },
  y: 222,
};
const call = createLoweredImport({
  name: "host-fn-record-result",
  ft: FT,
  opts,
  hostFn: () => Promise.resolve(value),
  stats: newStats(),
  mode: "plain",
  suspendable: false,
  deferCancel: false,
  abortable: false,
}) as (...args: number[]) => unknown;

const task = new Task(FT, TASK_OPTS, inst, () => [], () => {});
const thread = new Thread(task, (function* () {})());
pushCurrentThread(thread);
let packed: unknown;
try {
  packed = call(64); // retptr = 64
} finally {
  popCurrentThread(thread);
}
const [, subtaski] = taskMod.unpackSubtaskResult(packed as number);
const subtask = inst.handles.get(subtaski);

await new Promise((r) => setTimeout(r, 0));
await new Promise((r) => setTimeout(r, 0));

const dv = new DataView(memory.buffer);
console.log(JSON.stringify({
  subtaskState: subtask.state,               // expect RETURNED (2)
  instancePoisoned: isInstancePoisoned(inst), // expect true
  memoryX: dv.getUint32(64, true),            // expect 111 (delivered anyway)
  memoryY: dv.getUint32(68, true),            // expect 222 (delivered anyway)
}));

Observed output (fresh, this session, baseline 1a4f5e1, clean tree)

{"subtaskState":2,"instancePoisoned":true,"memoryX":111,"memoryY":222}

(subtaskState: 2 = RETURNED.)

Expected vs actual

• Expected: once isInstancePoisoned(inst) becomes true, no further
  result for that instance should reach guest memory — the guard's own
  comment states this as the intent, and it is what happens when the
  instance is already poisoned before this callback runs at all.
• Actual: because the poisoning happens during the guard-protected
  conversion step, the guard reads false before the getter fires, and
  onResolve completes normally: the subtask resolves to RETURNED and
  both flat words of the record are written into guest linear memory, with
  store.hostFailure left undefined — no error, no trap, nothing
  distinguishes this from an ordinary successful delivery, even though
  isInstancePoisoned(inst) is true by the time control returns to the
  caller.

Severity

P1. The guard exists specifically to stop this delivery; reentrancy from
the value being converted defeats it by construction, silently. Any host
import returning a record (or other field-accessed composite type) whose
value contains an accessor that can reach notifyInstancePoisoned — direct
host-authored objects, proxies, or any value from less-trusted
plugin/embedder code — can smuggle a result into memory of an instance the
runtime has simultaneously declared dead, with no diagnostic anywhere.

Suggested fix shape

Re-check subtask.resolved() || isInstancePoisoned(opts.instance) after
convert/toResults complete, before calling onResolve/writing guest
memory — or perform the CABI conversion into a private staging value first,
then re-check discard eligibility immediately before delivery. This matches
the reference model's intent that a discard decided before conversion must
also still hold after it, i.e. the conversion step itself must be treated as
attacker-controlled/reentrant-capable rather than assumed inert. The
sibling suspendable arm (boundary.ts:1856-1878, noted above as out of
scope for this report) should be checked against the same class of issue
once this stage's fix shape is agreed, since it shares the "check right
before onResolve, but not right after" pattern.