encode: enforce the protobuf 2 GiB message-size limit - #271
Merged
Conversation
…oint Encoding was infallible while decoding rejects length-delimited payloads over 2 GiB-1: a message that organically crossed 2^31-1 bytes serialized silently into a blob no conforming decoder - including buffa's own - will read back, and sizes past u32::MAX wrapped into corrupt output. Hoist the limit into pub const MAX_MESSAGE_BYTES (deduping the decode-side literals), inhabit the reserved EncodeError enum with MessageTooLarge, and check the computed size at every provided Message / ViewEncode / SizeCachePool encode entry point: the existing methods now panic on over-limit messages before writing anything, and new try_encode, try_encode_with_cache, try_encoded_len, try_encode_length_delimited, try_encode_to_vec, and try_encode_to_bytes twins return the error instead (pool: try_encode / try_encode_view / try_encoded_len). saturate_size, checked_encode_size, and assert_encode_size are exported so generated code and manual implementations share one funnel. Belts: encode_to_vec/encode_to_bytes gain a debug-build assertion that write_to produced exactly the byte count compute_size declared, and SizeCache::consume_next rejects over-limit slots - a backstop for callers driving compute_size/write_to directly.
Generated compute_size previously accumulated in u32: three 1.5 GiB sub-trees wrap to an under-limit value the new entry-point check cannot distinguish from a legitimate size, and a single huge bytes field wraps in its usize-to-u32 cast. Accumulate in a u64 local (which cannot overflow for any in-memory message) and saturate to u32 once at each node's return via buffa::saturate_size - the trait signature and SizeCache stay u32, and the byte-exact size survives the 2-4 GiB window so the entry-point guard sees the true value. The same discipline applies everywhere size arithmetic lives: write_to's transient packed-payload and map-entry lengths, the map_codec runtime helpers (MapCodec::encoded_len, field_len, message_field_len), the lazy-fragment emitter, and tag lengths. put_len_delimited_header takes a plain u64 length. Lazy views' inherent encode entry points route through the shared buffa funnel, gain try_encode / try_encoded_len / try_encode_to_vec / try_encode_to_bytes twins, and the debug two-pass byte-count assertion. A new codegen test locks the discipline by asserting the emitted compute_size/write_to bodies contain no u32 size arithmetic. Regenerates the checked-in WKT and bootstrap descriptor types.
DynamicMessage gains the same 2 GiB guard as generated types: encode and encode_to_vec panic on over-limit sizes (nested recursion uses an unchecked path so the check is one size walk per top-level encode), with try_encode / try_encode_to_vec twins returning EncodeError::MessageTooLarge. The library's own Result-returning pipelines that re-encode a parsed message stop panicking and surface the condition through their existing error channels instead: the JSON/text Any and extension converters (any_from_json, any_merge_text, message_merge_text, group_merge_text, message_from_json, the WKT from_json closures, reflective deserialize_any), pack_any (new AnyError::MessageTooLarge variant), to_message (DecodeError::MessageTooLarge), and from_options. set_extension documents that message-typed values are encoded on set and inherit the panic.
The checked-in logging example still carried u32 compute_size arithmetic from several codegen generations ago; regenerating also surfaced that main.rs predated the buffa::Map field type (std HashMap literals no longer infer), so the map literals now collect() into the generated type. bsr-quickstart's generated code needs BSR authentication to regenerate and is left as-is. Also strips trailing whitespace in SECURITY.md that failed markdownlint.
|
All contributors have signed the CLA ✍️ ✅ |
…allible extension/Any paths Follow-up to the 2 GiB encode guard, addressing review findings: - Every panicking encode entry point (Message, ViewEncode, SizeCachePool, DynamicMessage, generated lazy views) now delegates to its try_* twin and routes the Err through one #[cold] encode_size_overflow shim, so the panicking and fallible paths cannot diverge. The thin wrappers are #[inline]; assert_encode_size (added earlier on this branch, now zero-caller) is removed. The shared panic message names the try_* escape generically since the shim now serves more than the encode family. - SizeCache::consume_next's over-limit slot check becomes a debug_assert: it is unreachable through every provided entry point (each child compute_size saturates, so an over-limit slot forces the checked total over the limit first), and it sat on the hottest write-pass loop. Matches the debug-only two-pass ledger philosophy. - OwnedView::from_owned maps an over-limit encode to Err(DecodeError::MessageTooLarge) instead of panicking inside a Result-returning API; DecodeError::MessageTooLarge's display text now covers both the protobuf maximum and configured decode limits. - The eager re-encode paths gain fallible twins: Any::try_pack and ExtensionSet::try_set_extension (staged, so Err leaves the extendee untouched - which also makes set_extension's panic path atomic). ExtensionCodec/SingularCodec swap their required method: try_encode / try_encode_one are required, the panicking encode / encode_one are provided wrappers, so a fallible codec cannot accidentally leave the try_ path panicking. - DynamicMessage's private guard adapts onto the shared checked_encode_size/saturate_size helpers (one comparison, one panic wording); pack_any documents its MessageTooLarge error; encoded_len documents that it is exact and unchecked; DynamicMessage::encode documents the size-walk cost. - The three per-file size-reporting test doubles consolidate into #[cfg(test)] test_doubles::SizedMsg; buffa-types keeps its own copy (cfg(test) items do not cross crates) with a cross-reference. - Docs: guide gains the extension/Any panic-vs-try section; both migration guides stop calling encode infallible. A Breaking changes fragment records the u32->u64 signature changes, the regenerate-generated-code requirement, and the codec-trait swap. Regenerated WKT/descriptor generated code (from_owned doc update only - no checked-in code emits lazy views).
Restore direct bodies for encode_to_vec / encode_to_bytes (Message, ViewEncode, generated lazy views) instead of delegating to the try_ twins: LLVM does not fold the Result<Vec<u8>>/Result<Bytes> niche away even under full LTO inlining, so the delegating form re-checked the capacity sentinel and round-tripped the payload through a Result temp in every caller. Measured on a quieted c7i.metal spot box with layout-normalized builds (lto, codegen-units=1, -align-all-nofallthru-blocks=6): - combined bench, delegating form: google_message1 encode +7.5%, api_response encode +2.1% vs the pre-delegation baseline; disassembly showed +6 instructions, +1 branch, and a second 16-byte stack round-trip of the Vec on the serialized chain in the bench loop, while write_to itself shrank (the removed consume_next branch). - with this change: api_response encode +0.26%; google_message1 encode -0.19% in the isolated per-shape harness (the residual combined-bench delta is the documented code-placement lottery of all-shapes binaries; the untouched decode control was flat). The unit- and scalar-returning entry points keep the try_ delegation - their Results stay in registers and fold cleanly. A comment on Message::encode_to_vec records the rule.
This was referenced Jul 6, 2026
Conflict resolution keeps both contracts intact: - Every encode entry point (Message, ViewEncode, SizeCachePool, generated lazy views, DynamicMessage) retains the guard funnel — panicking variant delegating to a try_* twin, direct bodies on the measured fat-payload points — with main's `impl EncodeSink` sink bound replacing `BufMut` throughout, including on the try_* twins main never saw. - put_len_delimited_header is now (field_number: u32, len: u64, buf: &mut impl EncodeSink): the u64 size discipline layered over main's sink retyping. - reflect/dynamic.rs: main's per-arm tag writes and value-shape validation compose with this branch's encode_unchecked nested fast path; the merged doc comments carry both sides' contracts. - Generated code (WKTs, bootstrap descriptor types, logging example) regenerated from the merged codegen. Generated lazy-view encode twins now also document the debug-build two-pass panic, and the guide's error-handling section notes EncodeError is non_exhaustive. Verified: workspace tests, clippy -D warnings, no_std checks, conformance (std, no_std, via-view, via-lazy, view-json, via-reflect), and a consumer crate driving the merged surface end-to-end: 2 GiB-1 encodes and decodes back, one byte over errors with nothing written (contiguous and Rope sinks), encode_to_vec panics with the documented message, and a Bytes-heavy message encodes through a Rope zero-copy and decodes back identical. The via-vtable conformance run fails identically on main (DynamicMessage::set pool-identity validation vs cross-crate WKT views, introduced by #272; that run is not part of CI) — tracked separately. :house: Remote-Dev: homespace
chrisfinazzo
pushed a commit
to chrisfinazzo/buffa
that referenced
this pull request
Jul 9, 2026
…ics#284) ## Summary Adds vectored ("rope") encode: a message whose dominant content is one large `bytes` field can now be encoded without copying the payload at all. Draft for design review — targeted at the 0.9.0 breaking bundle. Today every encode is contiguous: `Message::write_to` and all the encode helpers take `&mut impl BufMut`, so a large payload is always memcpy'd into the output buffer, even when the field already holds a refcounted `bytes::Bytes` and the consumer (hyper/h2, any vectored writer) would happily take segments. ## Design - **`EncodeSink`** replaces the `BufMut` bound on every encode path. A blanket impl over `BufMut` keeps all existing callers source-compatible (`encode_to_vec`, `encode_to_bytes`, `msg.encode(&mut vec)` unchanged), and `const IS_SEGMENTED = false` const-folds the zero-copy branch away, so contiguous sinks compile to the exact pre-change code — verified at ±noise on the benchmark. The trait deliberately exposes only the `BufMut` subset the encoders use. - **`Rope`** is the segmented sink: tags/varints/small fields coalesce into a tail buffer; `bytes::Bytes` fields at or above `min_segment` (default 4 KiB) become refcount-shared segments via the new provided `ProtoBytes::as_shared` hook. `Rope::with_backing` extends this to **views**: large borrowed `&[u8]` fields that lie inside the decode source buffer are captured zero-copy through a containment-checked `slice_ref` (shared with the existing `bytes_from_source` helper), making decode→re-encode relays fully zero-copy. - **`RopeBuf`** adapts a finished rope to `bytes::Buf` with a `chunks_vectored` override, so the whole rope reaches a vectored writer in one call; consumed segments are released eagerly. - **Codegen** emits `put_shared_bytes_field` for `bytes` fields in every context (singular, optional, repeated, oneof, and map values through the shared-aware codecs). `Vec<u8>` fields and view slices keep the copy path — `as_shared` is a compile-time `None` there. ## Why breaking Manual `Message`/`ViewEncode` implementations must change their `write_to` bound (`impl BufMut` → `impl EncodeSink`), and generated code must be regenerated — the same class of churn as the other queued 0.9.0 items, hence one bundle. `cargo semver-checks` passes (196/196), but note its known blind spot: trait-method bound changes on implementors are exactly what it does not model; the changelog fragment declares the break explicitly. ## Numbers (dev machine, chunk-shaped message: 32 B digest + u64 + one `bytes` payload) | payload | contiguous `encode_to_bytes` | through `Rope` | |---|---|---| | 64 KiB | 1.43 µs | 150 ns | | 512 KiB | 10.6 µs | 150 ns | | 2 MiB | 128 µs | 151 ns | Rope encode is payload-size-independent — the ~150 ns is the two-pass size computation plus the tail buffer and one refcount. Contiguous `encode_to_bytes` is unchanged within run-to-run noise. Worth re-confirming on the quiesced benchmark host and against the full benchmark suite before merge. ## Testing - 2,307 workspace tests pass; 10 new unit tests on `Rope`/`RopeBuf` (segment ordering, backing containment, Buf contract, Default parity, reborrow forwarding, eager segment release) and 7 integration tests through real generated code (pointer-identity for singular/repeated/oneof/map fields, byte-identical wire output vs contiguous, zero-copy view re-encode, `Vec<u8>` fallback). - `check-nostd` clean (`chunks_vectored` is std-gated); clippy clean; MSRV-1.75 compatible. ## Notes for review - The generated-code diff (buffa-descriptor, buffa-types) is the mechanical signature swap plus the `put_shared_bytes_field` call sites; the reviewable core is `buffa/src/encode_sink.rs`, the `types.rs` additions, and the two codegen files. - `examples/logging` and `examples/bsr-quickstart` got the signature swap in their checked-in generated code, but both examples were already broken against current main for unrelated reasons (map hasher drift; `MessageView` trait drift vs the pinned BSR plugin) — examples aren't built in CI. Worth a separate cleanup. - Sequencing: this touches the same encode entry points as anthropics#271 (encode-size-guard); whichever lands second rebases.
Collaborator
|
[claude code] Thanks @rpb-ant — the core of this (the guard at the entry points, the u64 size discipline in codegen, the fallible reflect/registry paths) landed as you wrote it. Recording what changed on top of your original four commits, since the PR description has been rewritten to describe the merged design rather than the initial submission.
Delegating
#284 retyped every encode path from One thing found along the way that is not from this PR: the via-vtable conformance mode fails 20 tests on |
iainmcgin
approved these changes
Jul 12, 2026
iainmcgin
left a comment
iainmcgin
left a comment
Collaborator
There was a problem hiding this comment.
[claude code] Approving on behalf of @iainmcgin. Verified on the merge head f5b7662: all 9 checks green; workspace tests, clippy -D warnings, no_std and 32-bit checks, and the full local conformance suite (std, no_std, via-view, via-lazy, view-json, via-reflect) pass. Additionally drove the merged surface end-to-end from a consumer crate — a message at exactly 2 GiB-1 encodes and decodes back, one byte over returns MessageTooLarge with nothing written (contiguous and Rope sinks alike), encode_to_vec panics with the documented message naming its try_ escape, and a Bytes-heavy message encodes through a Rope zero-copy and decodes back identical.
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.
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.
Encoding was infallible while decode has always rejected length-delimited payloads over 2 GiB−1 (
DecodeError::MessageTooLarge): a writer whose message crossed 2^31−1 bytes silently serialized a blob that no conforming decoder (including buffa's own) will read back, and pastu32::MAXthe size arithmetic wrapped outright, producing corrupt output. In production the failure surfaces far from the cause — the writer keeps succeeding while every reader starts failing. buffa now refuses to produce bytes it wouldn't accept.Design
"Wide inside, checked at the door" (protobuf C++'s shape:
size_t ByteSizeLong()+ anINT_MAXcheck at the serialize entry):compute_sizeaccumulates in au64local (which can't overflow for any in-memory message) and saturates tou32once at each node's return (buffa::saturate_size). The trait signature andSizeCachestayu32; the 2–4 GiB window stays byte-exact so the guard sees true values.compute_sizeis bottom-up and monotone, so one check at the top-level entry points provably catches aggregation overflow, sub-message overflow, and the single-huge-field case. Existing entry points (encode,encode_to_vec,encode_to_bytes,encode_length_delimited,encoded_len,encode_with_cache) panic before writing anything;try_*twins returnErr(EncodeError::MessageTooLarge), the first variant of the previously uninhabited enum.write_to's transient packed/map lengths, themap_codecruntime helpers, lazy-fragment sizing,DynamicMessage(usize-exact, own guard),SizeCachePool. A codegen test locks the rule by asserting emittedcompute_size/write_tobodies contain no u32 arithmetic.write_toproduced exactly the byte countcompute_sizedeclared (the write pass is wrap-free ground truth, so this converts future two-pass codegen bugs into CI failures), andSizeCache::consume_nextrejects over-limit slots for callers driving the two passes directly.Result-returning pipelines that re-encode parsed messages (JSON/textAnyand extension converters,pack_any,to_message,from_options) surface the condition through their existing error channels rather than panicking.Structure
Four commits: (1) runtime guard +
try_*surface onMessage/ViewEncode/SizeCachePool; (2) u64 size arithmetic in codegen +map_codec+ lazy views, with regenerated WKT/descriptor code and the codegen lock test; (3)DynamicMessage+ fallible-pipeline migrations; (4) logging-example regen (which surfaced pre-existingbuffa::Mapdrift in itsmain.rs) and a SECURITY.md markdownlint fix.Notes for review
try_*escape. prost has no encode cap at all; protobuf C++ and Go both error here.put_len_delimited_headernow takeslen: u64(wasu32) — bare literals still infer;u32callers widen withu64::from.encoded_len()panics rather than returning a saturated lie;try_encoded_len()is the non-panicking probe.usizesizing is exact on 64-bit; the 32-bit theoretical wrap (sharedBytesclones summing pastusize::MAX) is documented in-code and unreachable in practice.examples/bsr-quickstartremains stale (regen requires BSR auth), same as on main.Verified: full workspace tests, clippy
-D warnings, no_std (host + thumbv7em) and MSRV 1.75 checks, conformance 14/14, and a consumer-crate drive: a version-envelope message at exactly 2 GiB−1 encodes and decodes back; one byte over returnsErr(MessageTooLarge)with nothing written (not even the length prefix);encode_to_vecpanics with the documented message; a 4.4 GB payload (the old u32-wrap class) errors instead of wrapping.