Summary
readLoop decodes every inbound frame under a single session-wide error handler, so any one frame that fails to decode takes down the whole session: every outstanding import is rejected and every export disposed. A per-frame boundary would make most of these failures cost one call instead of the session.
Observed on 0.12.0.
Where
readLoop (dist/index.js:2236) is a bare while loop around a switch on the frame type, and the only handler is at the call site:
this.readLoop().catch((err) => this.abort(err)) // :1929
Anything any arm throws reaches abort(). Concretely:
- A bad export id in a
push argument: no such entry on exports table (:1586, :1618, :1628).
- A duplicate or underflowing
release: no such export ID (:1969, :1983) and refcount would go negative (:1970).
- An over-limit value:
maxDepth (:1437) and maxBigIntDigits (:1451).
Repro
Any peer bug that emits an undecodable frame will do. The one we hit is #265, a distinct defect in getImport(); its transcript shows the shape, where a single ["pipeline",1] naming a released id ends the session:
client -> ["push",["pipeline",0,["use"],[["pipeline",1]]]]
client -> ["pull",3]
server -> ["abort",["error","Error","no such entry on exports table: 1"]]
SESSION DEAD: no such entry on exports table: 1
A limit violation behaves the same way, which is easier to reproduce standalone:
const srv = new RpcSession(s.t, new Api(), { limits: { maxDepth: 8 } })
const cli = new RpcSession(c.t)
const api = cli.getRemoteMain()
await api.getThing() // fine
let deep = {}, cur = deep
for (let i = 0; i < 40; i++) { cur.a = {}; cur = cur.a }
await api.use(deep) // TypeError: Deserialization exceeded maximum allowed message depth of 8.
await api.getThing() // SESSION DEAD, same error
We understand the limits case is deliberate: RpcSessionOptions.limits documents it as "A message that exceeds a limit is rejected, aborting the session" and describes the limits as guarding "against resource-exhaustion attacks from untrusted peers" (dist/index.d.ts:279-285). It is included here only because it demonstrates the single handler, not because we think the documented behaviour is wrong. See the last section for why that makes this an opt-in question.
Suggested fix
A boundary around each switch arm, so a frame that fails to decode fails only that frame, settling the named import with an error stub rather than aborting. Two things make this safer than it may sound:
- Nothing leaks.
evaluateWithDepth (:1422) already does catch (err) { payload.dispose(); throw err }, so the hooks and imports a failed decode created are released. That is existing behaviour, not something a boundary would need to add.
- Positional ids stay aligned if the arm still allocates. A contained
push or stream frame must still push its slot, or every subsequent id is off by one, which is a considerably worse failure than the abort. Any fix wants a test that fails if the allocation is dropped, and that asserts subsequent calls still resolve, not merely that the bad call rejects.
Arms differ in what containment should mean, which is the part worth agreeing on before writing code:
push / stream: allocate the slot, install an error stub in it.
pull: answer the peer with ["reject", exportId, ...] rather than swallow. That frame is already in the vocabulary (emitted at :2013 and :2021, handled at :2294), so an unpatched peer needs nothing new. Swallowing instead would leave a peer awaiting a resolution that never arrives.
release: drop the error. Both throws precede any mutation (entry.refcount -= refcount is :1971), so containment leaves refcounts exactly as they were. The cost is one leaked export until the session ends, and no id-space disturbance, since releaseExport never appends.
resolve / reject: settle the named import with an error stub, guarded with if (imp). The no-import branch is reachable, and an unguarded settle there would be a TypeError inside the catch.
abort: still abort, but with the peer's undecodable reason as cause, instead of masking why the peer gave up with our own decode error.
- Everything outside the arm switch (transport
receive() failures, the top-level maxMessageSize check at :2246) stays fatal.
The design question
Softening decode failures from session-fatal to call-fatal removes the coarse limits as a backstop, which is exactly the posture the limits doc comment describes for untrusted peers. Our own peers are all first-party processes we launch, so per-call rejection is strictly better for us, but that is not true for everyone, and maxMessageSize is only checked when encodingLevel === "string" (:2246) so it is not a backstop on the structured-clonable transports anyway.
So this may be better as opt-in, for instance a flag on RpcSessionOptions alongside onSendError and limits, leaving the default abort-on-decode-failure behaviour untouched. Happy to open a PR against whichever shape you prefer.
Summary
readLoopdecodes every inbound frame under a single session-wide error handler, so any one frame that fails to decode takes down the whole session: every outstanding import is rejected and every export disposed. A per-frame boundary would make most of these failures cost one call instead of the session.Observed on
0.12.0.Where
readLoop(dist/index.js:2236) is a barewhileloop around aswitchon the frame type, and the only handler is at the call site:Anything any arm throws reaches
abort(). Concretely:pushargument:no such entry on exports table(:1586,:1618,:1628).release:no such export ID(:1969,:1983) andrefcount would go negative(:1970).maxDepth(:1437) andmaxBigIntDigits(:1451).Repro
Any peer bug that emits an undecodable frame will do. The one we hit is #265, a distinct defect in
getImport(); its transcript shows the shape, where a single["pipeline",1]naming a released id ends the session:A limit violation behaves the same way, which is easier to reproduce standalone:
We understand the limits case is deliberate:
RpcSessionOptions.limitsdocuments it as "A message that exceeds a limit is rejected, aborting the session" and describes the limits as guarding "against resource-exhaustion attacks from untrusted peers" (dist/index.d.ts:279-285). It is included here only because it demonstrates the single handler, not because we think the documented behaviour is wrong. See the last section for why that makes this an opt-in question.Suggested fix
A boundary around each
switcharm, so a frame that fails to decode fails only that frame, settling the named import with an error stub rather than aborting. Two things make this safer than it may sound:evaluateWithDepth(:1422) already doescatch (err) { payload.dispose(); throw err }, so the hooks and imports a failed decode created are released. That is existing behaviour, not something a boundary would need to add.pushorstreamframe must still push its slot, or every subsequent id is off by one, which is a considerably worse failure than the abort. Any fix wants a test that fails if the allocation is dropped, and that asserts subsequent calls still resolve, not merely that the bad call rejects.Arms differ in what containment should mean, which is the part worth agreeing on before writing code:
push/stream: allocate the slot, install an error stub in it.pull: answer the peer with["reject", exportId, ...]rather than swallow. That frame is already in the vocabulary (emitted at:2013and:2021, handled at:2294), so an unpatched peer needs nothing new. Swallowing instead would leave a peer awaiting a resolution that never arrives.release: drop the error. Both throws precede any mutation (entry.refcount -= refcountis:1971), so containment leaves refcounts exactly as they were. The cost is one leaked export until the session ends, and no id-space disturbance, sincereleaseExportnever appends.resolve/reject: settle the named import with an error stub, guarded withif (imp). The no-import branch is reachable, and an unguarded settle there would be aTypeErrorinside the catch.abort: still abort, but with the peer's undecodable reason ascause, instead of masking why the peer gave up with our own decode error.receive()failures, the top-levelmaxMessageSizecheck at:2246) stays fatal.The design question
Softening decode failures from session-fatal to call-fatal removes the coarse limits as a backstop, which is exactly the posture the
limitsdoc comment describes for untrusted peers. Our own peers are all first-party processes we launch, so per-call rejection is strictly better for us, but that is not true for everyone, andmaxMessageSizeis only checked whenencodingLevel === "string"(:2246) so it is not a backstop on the structured-clonable transports anyway.So this may be better as opt-in, for instance a flag on
RpcSessionOptionsalongsideonSendErrorandlimits, leaving the default abort-on-decode-failure behaviour untouched. Happy to open a PR against whichever shape you prefer.