decoder: add MaxOutputBytes decode option#22
Conversation
tamirms
force-pushed
the
reduce-allocations
branch
2 times, most recently
from
19bd55b to
bc50d72
Compare
There was a problem hiding this comment.
Pull request overview
Adds an output-size accounting mechanism to the xdr3 decoder to mitigate memory amplification during decoding, along with stress-style tests intended to validate the fix and concurrency behavior.
Changes:
- Introduces
DecodeOptions.MaxOutputBytesand decoder-side byte tracking (TrackOutputBytes,TrackOutputBytesOf[T]) to bound decoded output allocations. - Modifies slice decoding to cap initial pre-allocation (256) and grow via append for large arrays.
- Adds a new test suite covering the preallocation amplification scenario and MaxOutputBytes behavior; bumps the module’s Go version.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
xdr3/decode.go |
Adds MaxOutputBytes tracking, new tracking helpers, and changes array/map/union/pointer allocation behavior. |
xdr3/prealloc_vulnerability_test.go |
Adds tests intended to demonstrate the prior amplification issue, plus MaxOutputBytes/concurrency coverage. |
go.mod |
Updates the go directive to 1.25. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
tamirms
force-pushed
the
reduce-allocations
branch
3 times, most recently
from
f3572dd to
112f917
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
… limits Add a MaxOutputBytes field to DecodeOptions that tracks cumulative decoded output size across a single decode operation. Before each allocation (array element, union arm, optional field, opaque data), the size is added to a running total; if it exceeds MaxOutputBytes the decode is aborted with ErrOutputBytesExceeded. Key changes: - Add TrackOutputBytes method and TrackOutputBytesOf[T] generic helper - Cap array pre-allocation at 256 elements, growing via append beyond that - Track allocations in DecodeFixedOpaque, decodeArray, decodeUnion, decodeMap, and allocPtrIfNil - Bump minimum Go version to 1.25 for generics support Co-Authored-By: Claude Opus 4.6 <[email protected]>
tamirms
force-pushed
the
reduce-allocations
branch
from
112f917 to
c677e1b
Compare
Co-Authored-By: Claude Opus 4.6 <[email protected]>
There was a problem hiding this comment.
Something of interest to me is that this ties a successful decode to the implementation of Go. Since it tracks the size of the Go objects in memory if that max is set very tight, a change in the Go compiler and how it structures memory will impact if a decode is inside or outside of the configured max. Even building on one host operating system may produce a different value to another.
Will this be configured pretty high with a lot of buffer?
The limits in soroban-rpc will be set with significant headroom — 4KB for getLedgerEntries where legitimate data is a few hundred bytes, 1MB for sendTransaction where a valid envelope is typically well under 100KB |
The old name was ambiguous — it could refer to wire output, serialized bytes, etc. MaxDecodedSize makes it clear this tracks the cumulative in-memory size (unsafe.Sizeof) of Go objects created during decoding. Also renames the internal fields and error sentinel to match: maxOutputBytes → maxDecodedSize outputBytes → decodedSize ErrOutputBytesExceeded → ErrDecodedSizeExceeded TrackOutputBytes/TrackOutputBytesOf function names are left unchanged since they are called by xdrgen-generated code. Co-Authored-By: Claude Opus 4.6 <[email protected]>
MaxMemoryBytes more clearly communicates that this limits the cumulative in-memory size of Go objects allocated during decoding. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Co-Authored-By: Claude Opus 4.6 <[email protected]>
Add an aggregate complete job that depends on all build matrix jobs. This provides a stable status check name that doesn't change when Go versions are bumped, allowing branch protection to reference complete instead of version-specific Build (X.Y) checks. Co-Authored-By: Claude Opus 4.6 <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Address CodeQL security finding by adding explicit permissions block to limit the workflow token to contents: read. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Summary
MaxOutputBytesfield toDecodeOptionsthat tracks cumulative decoded output size across a single decode operation and aborts if the limit is exceededTrackOutputBytesmethod andTrackOutputBytesOf[T]generic helper for generated code to call before each allocationDecodeFixedOpaque,decodeArray,decodeUnion,decodeMap, andallocPtrIfNilTest plan
TestMaxOutputBytescovers: unlimited (no limit), exceeded (aborts), not_reached (succeeds)🤖 Generated with Claude Code