interchange, repr: Prevent deep Protobuf and jsonb stack overflows - #37582
Closed
def- wants to merge 7 commits into
Closed
interchange, repr: Prevent deep Protobuf and jsonb stack overflows#37582def- wants to merge 7 commits into
def- wants to merge 7 commits into
Conversation
martykulma
reviewed
Jul 13, 2026
martykulma
left a comment
martykulma
left a comment
Contributor
There was a problem hiding this comment.
It seems this has some upgrade impact. My understanding is that we would re-render CREATE SOURCE from the catalog, which may already contain protobuf schema with a depth > 128.
Contributor
Author
|
Ouch, thanks for catching that. Edit: fixed |
def-
changed the title
…flow `derive_inner_type` recurses once per message when deriving a source's relation type from a Protobuf `FileDescriptorSet`. It guarded against cyclic message types with a `seen_messages` name set, but not against a deep *non-cyclic* chain (`m0 -> m1 -> ... -> mN`). A descriptor set is a flat list of messages that reference each other by name, so such a chain encodes cheaply and, at `CREATE SOURCE ... FORMAT PROTOBUF` plan time, overflowed the coordinator stack and aborted environmentd. Bound the nesting depth (the `seen_messages` length is the current depth) and return a graceful error past the limit instead of recursing further. Closes: SQL-515 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A `jsonb` value is a single column type that can hold arbitrarily deep nesting, built at runtime (e.g. via `WITH MUTUALLY RECURSIVE` + `jsonb_build_array`). Two recursive walks over such a value ran on the environmentd/pgwire thread with no stack management and overflowed: * `Datum::is_instance_of_sql` (the type-check applied to a value before it is returned), and * `JsonbDatum::serialize` (pgwire/text output encoding). Wrap both in `mz_ore::stack::maybe_grow` so a deep value grows the stack rather than aborting the process. Regression test in scalar.rs (STACK-8). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The message-nesting guard added for SQL-515 runs in `derive_inner_type`, after `DescriptorPool::decode` has already decoded the whole `FileDescriptorSet`. That decode is itself recursive: `DescriptorProto` nests via `nested_type` and unknown group fields are skipped recursively, and the workspace builds Prost with `no-recursion-limit`, so a deeply nested (but cheap to encode) descriptor set overflows the stack during decode, before the guard runs. This is reachable from user input via `FORMAT PROTOBUF MESSAGE ... USING SCHEMA '<bytes>'`. Add an iterative wire-format pre-scan in `from_bytes` that rejects input nested deeper than a limit before decoding. It walks the wire with an explicit stack so it cannot overflow, and descends into a superset of what Prost recurses into, so any input that would drive the decoder past the limit is rejected first. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The wire pre-scan guarding `DescriptorPool::decode` against stack overflow descended into every length-delimited field, treating opaque string and bytes payloads as nested messages. An option string of `0x4b` bytes reads as a chain of `StartGroup` keys, so the scan rejected valid, shallow descriptors once such a string passed the depth limit. This broke normal CSR compilation and `FORMAT PROTOBUF ... USING SCHEMA`, and could panic a storage worker re-rendering an inline descriptor persisted by an older version. Track the `descriptor.proto` message type of each wire region and descend only into genuinely message-typed fields, mirroring the decoder's recursion. Strings, bytes, and unknown fields are skipped as opaque leaves. Groups still count toward the limit, so both overflow vectors (`nested_type` chains and nested groups) stay bounded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Deriving a relation type from a deep Protobuf message chain now succeeds,
so everything downstream that recurses over the resulting nested record
type must also survive on a fixed stack. Grow the stack on demand in
SqlScalarType/ReprScalarType clone, drop, serde, and proto conversions
(manual impls, with serde(remote) mirrors to keep the wire format
unchanged), and in the SQL<->repr type conversions.
Prost's generated encode/decode recursion cannot be instrumented per
level, so SourceData::{encode,decode}_schema and deeply nested message
decoding in the Protobuf Decoder run on a single large stack via the new
mz_ore::stack::grow.
A type with a Drop impl cannot be destructured by value or
const-promoted, so adjust the affected match and static sites.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Deriving a relation type from a deep Protobuf message chain now succeeds, so the comparison, hashing, and formatting traits that recurse over the resulting nested type must survive on a fixed stack too. Give SqlScalarType manual PartialEq/Eq/Ord/PartialOrd/Hash/Debug impls and ReprScalarType a manual Debug impl, and grow the stack on demand in their recursive arms, along with ReprScalarType's existing PartialEq/Ord/Hash and Display. Downstream types that merely contain these enums (SqlColumnType, SqlRelationType, RelationDesc) keep their derived impls, since the recursion passes through one of the guarded methods exactly once per nesting level. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
martykulma
reviewed
Jul 20, 2026
martykulma
left a comment
martykulma
left a comment
Contributor
There was a problem hiding this comment.
This PR is getting really hard to reason about! I've been trying to get through it for the better part of this afternoon, but not done.
I get the sense this is turning into whack-a-mole. Having to hand-roll PartialEq, Hash, etc. Means you'll solve for known cases today, but future cases may run into issues. I'm not even sure if all cases are covered. I got as far as SqlScalarType::eq_inner, and it looks like paths that reach it may not be protected (not sure yet).
Contributor
Author
|
Indeed, once when stack overflow is fixed, the next one pops up. First I wanted to just prevent deep protobufs, but that seems tough if someone is using them already, especially in self-managed |
def-
marked this pull request as draft
July 20, 2026 21:19
Contributor
|
I'll get back to this as part of the Stack Overflow project. There, I have a different approach for solving stack overflows in derived things like |
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
3 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.
Motivation
Materialize recursively processes Protobuf schemas when deriving source relation types and when Prost decodes wire-nested descriptor messages or unknown groups. Deep but inexpensive inputs can exhaust the environmentd stack and abort the process during
CREATE SOURCE. Deeply nestedjsonbvalues can likewise exhaust the stack during SQL type validation or serialization.A fixed limit on Protobuf message reference chains is not upgrade-safe. Materialize can re-render sources whose catalogs already contain schemas beyond a newly introduced limit. Those schemas must remain decodable.
After this change, existing deeply nested Protobuf schemas continue to work during upgrades and re-renders, deeply nested
jsonbvalues no longer crash Materialize in these paths, and pathological wire-nested descriptor sets produce an error instead of aborting the process.Description
jsonbtype validation and serialization.FileDescriptorSetiteratively beforeDescriptorPool::decodeand reject message or group wire nesting deeper than 128 levels.Verification
Adds Protobuf regression coverage for deep non-cyclic message reference chains, deeply wire-nested descriptor messages, deeply nested unknown groups, and valid option strings containing message-like bytes. Also adds a regression test for type-checking deeply nested
jsonbvalues without overflowing the stack.Closes: SS-342