in_tcp: Prevent truncation on incoming payloads#11531
Open
in_tcp: Prevent truncation on incoming payloads#11531
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughTightened size/type safety and payload validation in TCP connection parsing and consumption; guarded log emission when output is empty; added a thread-safe large-record test for the "format none" path to verify handling of very large payloads. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/in_tcp/tcp_conn.c`:
- Around line 378-385: The JSON consume-length error handling branch currently
clears conn->buf_len but leaves conn->pack_state unchanged; update the branch
handling ret_payload < 0 || ret_payload > conn->buf_len to also reset the parser
state (the same way the ret_payload == -1 path does) so stale parser state from
parse_payload_json() isn't reused on the next read; locate the block around the
check for ret_payload and add a reset/initialization of conn->pack_state (or
call the existing pack_state reset helper used elsewhere) before setting ret =
-1 and goto cleanup to mirror the other error path.
In `@tests/runtime/in_tcp.c`:
- Around line 652-706: The test only uses cb_count_msgpack and get_output_num to
assert one output, which misses payload truncation; change the test to inspect
the emitted record(s) payload content and length instead of only counting.
Replace or augment cb_count_msgpack usage by capturing the actual emitted
msgpack in the test callback (or use an existing callback that returns the
record data), then after flb_time_msleep(1500) decode the single output and
assert the "log" field length equals payload_size+1 and its bytes match buf (use
ctx->cb_data or the callback storage created via test_ctx_create/cb_data to
retrieve the raw emitted message); keep the existing connectivity and send loop
but assert content/length equality rather than num == 1.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef9198d0-e4ba-4944-ba0f-f78dc92dbf13
📒 Files selected for processing (2)
plugins/in_tcp/tcp_conn.ctests/runtime/in_tcp.c
cosmo0920
force-pushed
the
cosmo0920-prevent-truncation-on-in_tcp
branch
from
04594cc to
5bcdc52
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/in_tcp/tcp_conn.c`:
- Around line 383-387: The recovery path reinitializes JSON parser state but
ignores flb_pack_state_init() failure; update the block that runs when
ctx->format == FLB_TCP_FMT_JSON to check the return value of
flb_pack_state_init(&conn->pack_state) and handle allocation failure (e.g., log
error via the plugin logger, free/reset any partially initialized state, mark
the connection for close or return an error so it won't be reused). Ensure you
still set conn->pack_state.multiple = FLB_TRUE only after a successful init and
keep the flb_pack_state_reset(&conn->pack_state) call as part of cleanup on
failure to avoid leaving a broken pack_state.
- Around line 185-193: process_pack() failures are not propagated: check the
return value of process_pack(conn, pack, (size_t) out_size) and if it indicates
an error, free pack and return -1 immediately instead of proceeding to use
conn->pack_state.last_byte; only set/use processed = conn->pack_state.last_byte
and validate it after a successful process_pack call so tcp_conn_event() does
not discard payloads when encoding/appending failed.
In `@tests/runtime/in_tcp.c`:
- Around line 833-835: verifier.expected points into buf and the lib output
callback is asynchronous, so do not free buf before tearing down the engine;
move the flb_free(buf) call to after test_ctx_destroy(ctx) (you can still close
the socket with flb_socket_close(fd) before teardown), ensuring buf remains
valid until test_ctx_destroy completes and then free it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e89165a-8bc2-4ab4-8f0b-3ca29f1f2705
📒 Files selected for processing (2)
plugins/in_tcp/tcp_conn.ctests/runtime/in_tcp.c
cosmo0920
force-pushed
the
cosmo0920-prevent-truncation-on-in_tcp
branch
from
5bcdc52 to
6fbb0a0
Compare
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
force-pushed
the
cosmo0920-prevent-truncation-on-in_tcp
branch
from
6fbb0a0 to
5e89f38
Compare
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
There's type glitches for int, size_t, ssize_t on in_tcp.
So, we need to tidy up the usages and tighten the boundary of incoming payloads.
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
Bug Fixes
Tests