security(codecs): bound chunked_gelf decoder memory - #26137
Conversation
597e488 to
2db2e2b
Compare
c4244b3 to
76018e3
Compare
76018e3 to
556f752
Compare
556f752 to
2960a0a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2960a0a922
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
0abddf6 to
7e26e03
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e26e0346f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
7e26e03 to
e5723c1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5723c11dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
e5723c1 to
2ac4a65
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ac4a6562e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
2ac4a65 to
8a27780
Compare
8a27780 to
1f093ad
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f093ad744
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1f093ad to
71fbd8a
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
71fbd8a to
b208ebf
Compare
b208ebf to
045c360
Compare
Merge commits are not allowed on this repository
The decoder keeps a table of half-finished messages keyed by the message ID in
each chunk header. Nothing bounded that table, so an unauthenticated sender
could exhaust memory by naming message IDs it never completes, each holding
buffer state until it times out. Most exposed on the `socket` source in UDP
mode, where datagrams need no handshake.
Memory here is `count x per-message overhead + payload`, a sum, so bound each
term rather than trying to measure allocations:
MAX_PENDING_MESSAGES = 4096 messages awaiting completion
MAX_BUFFERED_PAYLOAD = 128 MiB chunk payload across all of them
Capping the count covers every per-message cost at once (state, map entry,
timeout task, table slack) without pricing any of them. Capping payload in
aggregate rather than per message keeps the two terms from multiplying, which
is what `pending_messages_limit` x `max_length` cannot avoid and why those
options cannot express this bound themselves. Both survive as overrides,
clamped so they can only tighten the caps.
`buffered_payload` counts chunk payload and nothing else, so its one increment
and one decrement are symmetric by inspection. No single peak figure is
advertised: reassembly adds a transient copy, allocator size classes round each
chunk up, and decompression is bounded separately by `CappedDecoder`, so any
such figure would need re-deriving whenever one of those changed.
Rejection is asymmetric:
- A full budget refuses the chunk but keeps the message, since the condition is
not that message's fault and discarding would let a sender parking the budget
pick off everything in flight.
- A chunk that finishes its message is weighed against that message alone,
because completing is what returns budget and refusing it would wedge the
decoder. It is never exempt from the bound itself, since one chunk can be any
size on a message-based source.
- A lone chunk (`total_chunks == 1`) completes in the same call, so it is
exempt from occupancy but not from size.
- Exceeding `max_length` does discard: that message can never become valid.
Everything a new message must satisfy is settled before it gets a state and a
timer, so no rejection path leaves either behind.
The limits apply to chunked messages, the only ones buffered. An unchunked
frame passes straight through, bounded by what the source accepts as a frame.
Builds on #26162, which carries the pre-existing fixes this work uncovered.
045c360 to
567083a
Compare
Summary
The
chunked_gelfdecoder keeps a table of half-finished messages keyed by the message ID in each chunk header. Nothing bounded that table, so an unauthenticated sender could exhaust memory by naming message IDs it never completes, each holding buffer state until it times out. Most exposed on thesocketsource in UDP mode, where datagrams need no handshake.Why the existing options can't express the bound
pending_messages_limitcaps how many messages may be pending andmax_lengthcaps how large one may be, but memory is their product. Any pair permissive enough for real traffic multiplies out past what a host has.Memory is actually a sum, so bound each term separately:
Capping the count covers every per-message cost at once (state struct, map entry, timeout task, table slack) without having to price any of them individually. Capping payload in aggregate rather than per message is what keeps the two terms from multiplying.
Both existing options survive as overrides, clamped so they can only tighten the caps.
Headroom
Only chunked messages are buffered at all, so this affects senders emitting messages too large for one datagram. Pending count is roughly
rate x loss x 5s timeout, so reaching 4096 needs ~82k chunked messages/sec at 1% packet loss. The payload cap allows ~1,340 pending 100 KB messages, so the count is the binding limit for anything under ~32 KB.Rejection behaviour
total_chunks == 1) completes in the same call, so it is exempt from occupancy but not from size.max_lengthdoes discard, because that message can never become valid.Also fixed
BytesDecoderslices it without copying.add_chunknow copies.max_lengthleaked its timeout task.MessageStateis boxed to keep ~4 KiB out of everyHashMapbucket.The limits apply to chunked messages, the only ones buffered. An unchunked frame passes straight through, bounded by what the source accepts as a frame.
References
N/A
Vector configuration
How did you test this PR?
Unit tests in
lib/codecs/src/decoding/framing/chunked_gelf.rs. The bound had no coverage before this, so the new tests cover the accounting invariant across every exit path, both caps, each rejection asymmetry above, and that a stored chunk does not alias the decoder's input buffer.Each new test was checked against a deliberately broken build to confirm it fails, rather than assumed to work.
make fmt,cargo clippy -p codecs --lib --tests,make check-generated-docsand the fullcodecslib suite are clean.Is this a breaking change?
No configuration becomes invalid and nothing fails to start. Reaching either cap requires tens of thousands of multi-chunk messages per second with packet loss, since only chunked messages are buffered. Anyone hitting them was already holding hundreds of MB of half-finished messages, where the alternative to a dropped message was an OOM. Refusals surface as decoding errors with the usual component error metrics.
Does this PR include user facing changes?
no-changeloglabel to this PR.Measured
Debug build on a laptop, so absolute rates run well below release; the shapes are the point.
Well-formed 4-chunk messages:
Stall flood, first chunk only with unique IDs, 88k datagrams/s for 20s (1.76M abandoned messages):
Unbounded that is roughly 1.76M x 4 KiB = 7 GB, so the bound holds under the case it exists for.
Two honest caveats. Once datagram loss makes messages permanently incomplete, they hold slots for the full
timeout_secs, so the table can saturate and start refusing healthy messages too: at 11.6% loss above, delivery fell to 24.8% where independent loss alone predicts ~61%. And during a stall flood, legitimate traffic gets roughly its proportional share of the table (500 msg/s alongside an 88k/s flood landed 1.2%), which is inherent to an unauthenticated shared buffer rather than specific to these limits.