decode: bound the memory repeated elements and map entries materialize#319
Merged
Conversation
Nothing bounded what a repeated length-delimited field decodes into. An empty message element is two bytes on the wire and a whole struct in the Vec it lands in: 256 bytes for a message of a few Vec/String fields, so 4 MiB of them materialized 2,097,152 elements and about 512 MiB, all of it inside any input limit a caller could set. max_message_size cannot help, because it bounds the bytes going in rather than what they expand into, and the two are not proportional. Empty bytes and string elements take the same route at 16x and 12x. Charge each element's footprint against a budget shared by the whole decode, defaulting to 32 MiB. Charging by footprint rather than counting elements means a budget denotes the same quantity of memory whatever the element size, which a count could not: a hundred thousand elements is 25.6 MiB of one message and 3.2 MiB of another. Sharing it across the decode rather than per field is what makes it a bound at all — per field, a message with fifty repeated fields has fifty times the ceiling, and nesting multiplies further. Packed scalars are not charged. Their worst case is a one-byte varint becoming a four-byte i32, which is not amplification, and bounding them would reject the columnar payloads that carry millions of elements by design. DecodeContext::new is public and generated code calls it, so the budget is optional: a context built that way charges nothing and behaves exactly as before, which keeps generated code that has not been regenerated working. buffa's own entry points and codegen attach it. This rejects input that previously decoded, and 32 MiB is a judgement rather than a derivation — it is the same order as what the unknown-field allowance already permits, which is about 38 MiB. Lazy views are not charged: they store byte slices at decode and materialize on access, an 8x ratio below the string case this does charge.
The budget missed the most common length-delimited container in protobuf. A map entry amplifies exactly as a repeated element does — an omitted message value still materializes a whole value, and a few bytes of key buy a distinct slot — and nothing charged it: twenty thousand map<string, Address> entries decoded into two megabytes under a budget of one kilobyte. google.protobuf.Struct.fields is a map<string, Value>, so the hole reached a well-known type. The reflective decoder was worse. DynamicMessage::merge built a context with no budget attached at all, so every repeated element and map entry decoded through it was unbounded, and it is a public entry point for untrusted bytes — Any unpacking, gRPC reflection, the via-reflect conformance path. Charge its list pushes and map inserts, and attach the budget where it builds the context. Message::decode_length_delimited was missed as well, though its own doc calls it a top-level entry point and its sibling decode was updated. The suite passing was not evidence any of this worked. It passed because no bypassed path had a test, which is the same reason the holes survived being written in the first place: the tests covered the paths I had thought about. Add the ones that would have caught it — maps, the view path, and a nested tree, which pins the sharing across depth that the design's whole claim rests on. Docs and changelog now say what is bounded rather than what was intended.
|
All contributors have signed the CLA ✍️ ✅ |
Charging map entries changed what the view generator emits, and google.protobuf.Struct.fields is a map<string, Value> — the one well-known type the change reaches. Regenerated after the codegen change rather than before it, which is what check-generated-code caught.
iainmcgin
marked this pull request as ready for review
July 17, 2026 04:42
iainmcgin
enabled auto-merge
July 17, 2026 04:42
rpb-ant
approved these changes
Jul 17, 2026
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Bounds the memory a decode materializes in the elements of length-delimited containers. Closes #301.
DecodeOptions::with_element_memory_limit(bytes), defaulting to 32 MiB (DEFAULT_ELEMENT_MEMORY_LIMIT), charging each element's footprint against one budget shared by the whole decode tree. NewDecodeError::ElementMemoryLimitExceeded.The vector, measured
Nothing bounded what a repeated length-delimited field decodes into. Numbers taken against
lazyviews::Payload, not estimated:repeated messagePayloadrepeated bytesrepeated stringmap<string, Message>int32The issue's example reproduces exactly: 4 MiB of empty repeated-message elements = 2,097,152 elements = ~512 MiB, all of it inside any input limit a caller could set.
with_max_message_sizecannot help — it bounds the bytes going in, not what they expand into, and the two are not proportional.Design
Three decisions, all deliberate:
Charge bytes, not element count. A count means different amounts of memory per message: 100K elements is 25.6 MiB of
Payloadbut 3.2 MiB of a 32-byte message. Chargingsize_of::<Element>()gives one uniform ceiling, which is the thing actually worth bounding.One budget for the whole decode tree, not per field. Per field bounds nothing in aggregate — a message with fifty repeated fields gets fifty times the ceiling, and nesting multiplies further. There is a test pinning that a nested message's elements draw from its parent's pool.
Packed scalars are never charged. Their worst case is a 1-byte varint becoming a 4-byte
i32. Charging them would reject the columnar payloads that carry millions of elements by design —ColumnBatch(#318) has 1024 per column, and real Arrow batches run 64K+. Lazy views are likewise uncharged: they borrow byte slices at decode and materialize on access.The default
32 MiB is a judgement, not a derivation, and it is anchored rather than invented:
DEFAULT_UNKNOWN_FIELD_LIMIT(1M x 40 B) already permits ~38 MiB, so buffa has made this call once already in the same units. This can reject input that previously decoded — raise the budget for trusted input that legitimately decodes into more.Backward compatibility
DecodeContext::newis public and called by generated code, so changing its signature would break every consumer with checked-in generated code until they regenerate. The budget is thereforeOption<&Cell<usize>>:newleaves it absent and charges nothing, so un-regenerated code behaves exactly as before; buffa's own entry points and codegen attach it. A test pins that a context built the old way is unaffected.What review caught
The first commit had a hole big enough to make the guard theatre: maps were not charged at all. Confirmed empirically — 20,000
map<string, Address>entries materialized ~2 MB under a 1 KB budget and decoded fine.google.protobuf.Struct.fieldsis amap<string, Value>, so it reached a WKT. Two further public entries were unbounded: the reflectiveDynamicMessagedecoder (which builds its own context — and is an entry point for untrusted bytes viaAnyunpacking and gRPC reflection) andMessage::decode_length_delimited, whose own doc calls it a top-level entry point.Worth stating plainly: the first commit passed 48 test suites and 14/14 conformance. That was not evidence it worked — every bypassed path was green because it had no test.
f545423closes the holes and adds the tests that would have caught them (maps, views, nesting). The map test fails against the first commit.Verification
DynamicMessage).cargo fmt --check,clippy --workspace --all-targets --all-features -D warnings,RUSTDOCFLAGS='-D warnings' cargo docall clean.Known scope
Vecgrows by doubling, so peak resident memory can reach roughly twice the budget — this bounds what is materialized, not what the allocator reserves. Documented on the const.