Conversation
The type-description depth guard in from_wire(.., depth) was reset to 0 each time an Any *value* was decoded, because the inner type is read with a fresh descriptor. It therefore did not bound the value-decode call chain from_wire_field -> from_wire_full -> from_wire_field. A peer value that is a chain of nested Any (each encoded as a single 0x82 byte) drove one recursion level per byte and exhausted the thread stack. The resulting stack overflow raises SIGSEGV, which is not a std::exception and so is not caught by the message-dispatch try/catch, crashing the whole process. This is reachable before authentication via CONNECTION_VALIDATION, and on the client via any reply value. Make the nesting bound a single global invariant shared by both decoders: a named maxNestingDepth constant, threaded through from_wire_field / from_wire_full / from_wire_valid and checked where each recursive descent re-enters (from_wire for a nested type, from_wire_field for a nested value), with depth flowing across the Any/AnyA boundary so the type and value chains share one count. "Total nesting <= maxNestingDepth" then holds by construction and bounds both the recursion stack and the per-message node allocation; plain (flat) struct nesting is unaffected. Adds a testxcode regression that decodes a long run of 0x82 and asserts the decode faults instead of recursing without bound.
A peer-supplied wire count (array length or struct child count) is a 32-bit value decoded straight into a shared_array (or reserve) before any of the matching element bytes are read. A short frame can therefore claim ~4.29e9 elements: the POD array path allocates the storage (lazily reserved on some hosts, bad_alloc on others) and the StructA/UnionA/AnyA path default-constructs every Value -- committing real memory -- before the short read finally faults. This is reachable during the connection validation handshake, before authentication, so it is an unauthenticated remote memory-exhaustion vector independent of any frame-size cap. Bound every such count to the bytes that could still be decoded. size() reports only the current pulled-up window, so add Buffer::maxAvail() (the full remaining body, including bytes not yet pulled from the backing evbuffer) and fault when the count exceeds it -- every element costs at least one wire byte, sizeof(C) for a fixed-size POD. This ties each allocation to bytes actually received without false-rejecting a large but legitimate array delivered in multiple segments. testxcode: a value claiming 1000 elements over a near-empty body must leave the field empty rather than pre-sized to the claimed count.
handle_MONITOR() decoded the message payload before validating it against the operation state. The data branch takes Guard G(info->fl->lock), but info->fl stays null until the INIT reply has been processed, so a peer that has acknowledged CREATE_CHANNEL -- the op then sits in opByIOID with state==Creating and fl==nullptr -- could send a MONITOR data message before the INIT reply and dereference the null free-list, a remote-triggerable client crash. Move the operation-state validation (handle.lock(), op==CMD_MONITOR, and the subcmd-vs-state check) ahead of the payload-decode block. A data message in the Creating state now faults the buffer and the connection is dropped before any access to info->fl, so "payload is decoded only after a successful INIT" holds by construction rather than by a trailing check. The regression test drives a real client against a hand-rolled peer that completes the handshake through MONITOR INIT and then injects a pre-INIT data frame; a correct client rejects it and closes the connection.
physwkim
force-pushed
the
fix/remote-triggerable-crashes
branch
from
May 25, 2026 11:22
54c4965 to
c933e23
Compare
Member
Contributor
Author
|
Thanks for taking 1 and 3. For 2, I see it a bit more like the same class of issue as 1 than a general resource-exhaustion problem. PVA's support for arbitrary nesting and flexible data structures naturally creates these kinds of edge cases where a very small input can drive disproportionately large work. I don't think this patch meaningfully changes the overall threat model, but it does close off one of the more obvious and easily reachable cases. My impression from the TLS work is that there is some interest in improving robustness against less-trusted peers, and incrementally addressing these low-hanging cases seems consistent with that goal. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thanks for merging the earlier fixes. Three more I ran into while reading the
decode and client monitor paths, all remote-triggerable crashes. Numbers 1 and
2 are reachable during CONNECTION_VALIDATION, before auth.
1. codec: bound value-decode recursion
The depth guard in
from_wire()is reset every time anAnyvalue is decoded(the inner type is read with a fresh descriptor), so it never bounds the
value-decode chain. A peer value that is a run of nested
Any(one0x82byteeach) recurses one level per byte and overflows the stack. The SIGSEGV is not a
std::exception, so the dispatch try/catch misses it and the whole processgoes down. Fix threads a single
maxNestingDepthacross theAnyboundary sothe bound holds for both type and value decode.
2. codec: bound decode allocations to the remaining body
An array length / struct child count is a peer-supplied 32-bit value, used to
allocate (POD) or default-construct every
Value(StructA/UnionA/AnyA) beforethe element bytes are read. A short frame claiming ~4.29e9 elements commits the
memory first, then faults on the short read. Fix caps the count against the
bytes still available (new
Buffer::maxAvail()), since each element needs atleast one wire byte.
3. client: reject a MONITOR data message before the INIT reply
handle_MONITOR()decodes the payload before checking operation state, butinfo->flstays null until the INIT reply. A peer that has ackedCREATE_CHANNEL can send a data frame before INIT and the client dereferences
the null free-list. Fix moves the state check ahead of the decode.
Each has a regression test (testxcode, testclientconn). Built and run on
macOS/arm64.