Skip to content

fix(codecs): three pre-existing chunked_gelf bugs - #26162

Merged
pront merged 5 commits into
masterfrom
pront-chunked-gelf-preexisting-fixes
Aug 24, 2026
Merged

fix(codecs): three pre-existing chunked_gelf bugs#26162
pront merged 5 commits into
masterfrom
pront-chunked-gelf-preexisting-fixes

Conversation

@pront

@pront pront commented Aug 20, 2026

Copy link
Copy Markdown
Member

Summary

Three independent bugs in the chunked_gelf framing decoder, split out of #26137 so that PR is left with only the new memory bound it is really about. All of these affect master today.

A one-byte message panics the source. decode_message formats the first two bytes of a frame in its trace log, but the guard above only rejects an empty one, so a single-byte datagram panics with range end index 2 out of range. It needs trace logging enabled for the target, since tracing only evaluates the field expression when the level is on, but it is reachable from the same unauthenticated path as everything else the decoder handles. Introduced in #21816.

pending_messages_limit is applied to every chunk. It is checked before the table lookup, so once the table is full the decoder rejects every chunk, including chunks belonging to messages already pending. Those can then never complete and expire instead, so a burst of new message IDs stalls legitimate in-flight traffic rather than merely capping how much is buffered. Only a new message grows the table, so the check belongs on insert.

Dropping a message for exceeding max_length leaks its timeout task. The entry is removed without aborting the timer spawned for it, so the task outlives the entry it was meant to reclaim and the live-task count is not bounded by pending_messages_limit the way the entry count is.

Also here

Memory hygiene in the same file, no behaviour change:

  • BytesDecoder hands over a slice of FramedRead's buffer without copying, so storing that slice held the whole buffer for as long as the chunk was pending. The buffer is at least 8 KiB and an MTU-sized chunk is around 1388 bytes, so a pending message could hold several times what its payload needs. add_chunk now copies.
  • Reassembly reserved nothing and grew into a BytesMut, paying reallocation slack on top of the copy it already needs. It now reserves the length up front and releases each chunk as it copies.
  • HashMap stores values inline and MessageState is around 4 KiB because of its fixed chunk array, so every bucket carried one whether occupied or not and a rehash transiently allocated two copies of the table. Boxed.

References

Split out of #26137.

Vector configuration

sources:
  graylog:
    type: socket
    mode: udp
    address: 0.0.0.0:12201
    framing:
      method: chunked_gelf
    decoding:
      codec: gelf

sinks:
  out:
    type: blackhole
    inputs:
      - graylog

How did you test this PR?

Five unit tests in lib/codecs/src/decoding/framing/chunked_gelf.rs, one per fix. Each was checked against a deliberately broken build to confirm it fails, rather than assumed to work: the panic test reproduces range end index 2 out of range before the fix, and reverting the copy makes the stored chunk alias the input buffer.

make fmt, cargo clippy -p codecs --lib --tests, make check-generated-docs and the full codecs lib suite are clean.

Is this a breaking change?

  • Yes
  • No

Does this PR include user facing changes?

  • Yes. Please add a changelog fragment based on our guidelines.
  • No. A maintainer will apply the no-changelog label to this PR.

pront added 5 commits August 20, 2026 10:03
`decode_message` formats the first two bytes of a frame in its trace log, but
the guard above only rejects an empty one. A single-byte datagram therefore
panics with "range end index 2 out of range", taking the source task down. It
needs trace logging enabled for the target, since `tracing` only evaluates the
field expression when the level is on, but it is reachable from the same
unauthenticated path as everything else the decoder handles.

Slice to what is there. The branch already did the right thing otherwise: a
short frame is not a framing failure, it is a malformed payload for the
deserializer to reject.

Introduced in #21816.
… chunk

`pending_messages_limit` was checked before the table lookup, so once the table
was full the decoder rejected every chunk, including chunks belonging to
messages already pending. Those messages could then never be completed and
expired instead, so a burst of new message IDs stalled legitimate in-flight
traffic rather than merely capping how much was buffered.

Only a new message grows the table, so check it there.
…opped

Dropping a message for exceeding `max_length` removed its table entry without
aborting the timer spawned for it. The task then outlived the entry it was
meant to reclaim, so the number of live tasks was not actually bounded by
`pending_messages_limit` the way the entry count was.
`BytesDecoder` hands over a slice of `FramedRead`'s buffer without copying, so
storing that slice held the whole buffer for as long as the chunk was pending.
The buffer is at least 8 KiB, and a chunk sized for an MTU is around 1388
bytes, so a pending message could hold several times what its payload needs.
`add_chunk` now copies into an allocation of its own.

Two smaller ones alongside it:

- Reassembly reserved nothing and grew into a `BytesMut`, so it paid
  reallocation slack on top of the copy it already needs. Reserve the length up
  front and release each chunk as it is copied.
- `HashMap` stores values inline, and `MessageState` is around 4 KiB because of
  its fixed chunk array, so every bucket carried one whether occupied or not
  and a rehash transiently allocated two copies of the table. Box it.
@pront
pront requested a review from a team as a code owner August 20, 2026 14:14
pront added a commit that referenced this pull request Aug 20, 2026
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.
@pront
pront added this pull request to the merge queue Aug 24, 2026
Merged via the queue into master with commit 214996e Aug 24, 2026
82 checks passed
@pront
pront deleted the pront-chunked-gelf-preexisting-fixes branch August 24, 2026 14:35
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 24, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants