Open
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0220375130
ℹ️ 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".
📝 WalkthroughWalkthroughAdds TCP tracing to the in_ebpf plugin: new TCP event types and payloads, a TCP eBPF tracepoint program capturing listen/accept/connect, handler/encoder to emit Fluent Bit records, trace context updates to track eBPF skeletons, and runtime tests for encoding. Changes
Sequence DiagramsequenceDiagram
participant Kernel as Kernel (eBPF)
participant Maps as Maps / Events Buffer
participant Handler as TCP Handler
participant Encoder as Log Encoder
participant FB as Fluent Bit
Kernel->>Maps: save per-thread syscall args (enter)
Kernel->>Maps: reserve & submit event (exit)
Handler->>Maps: consume event
Handler->>Handler: validate & dispatch by event type
Handler->>Encoder: encode_tcp_event(ev)
Encoder->>FB: commit encoded record
FB->>FB: append to input buffer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 docstrings
🧪 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 |
cosmo0920
force-pushed
the
cosmo0920-add-tcp-ebpf-trace
branch
from
dc0d8f1 to
b83e0b9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
plugins/in_ebpf/traces/tcp/handler.h (1)
8-10: Missing include forstruct eventforward declaration.The
encode_tcp_eventfunction declaration usesconst struct event *ev, but this header does not includeevents.hor provide a forward declaration forstruct event. This could cause compilation errors if this header is included beforeevents.h.♻️ Proposed fix
`#include` <fluent-bit/flb_input_plugin.h> `#include` <fluent-bit/flb_log_event_encoder.h> + +#include "common/events.h" int trace_tcp_handler(void *ctx, void *data, size_t data_sz);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/in_ebpf/traces/tcp/handler.h` around lines 8 - 10, The header declares encode_tcp_event(...) with a parameter const struct event *ev but doesn't provide a forward declaration or include for struct event, which can cause compile errors; fix by adding a forward declaration "struct event;" at the top of this header (or include the header that defines it, e.g., events.h) so encode_tcp_event's prototype is valid when this file is included.plugins/in_ebpf/traces/tcp/handler.c (1)
11-49: Consider making theaddrparameter const.The
encode_tcp_addrfunction does not modify thetcp_addrstructure, but the parameter is not declaredconst. This creates a const-correctness issue when called with addresses from aconst struct event *(lines 98 and 113 cast away const).♻️ Proposed fix
static int encode_tcp_addr(struct flb_log_event_encoder *log_encoder, const char *key_prefix, - struct tcp_addr *addr) + const struct tcp_addr *addr) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/in_ebpf/traces/tcp/handler.c` around lines 11 - 49, Change the encode_tcp_addr signature to take a pointer-to-const (i.e., make the addr parameter "const struct tcp_addr *addr") and update its implementation accordingly; then remove the casts at call sites that currently cast away const (calls passing (struct tcp_addr *) should pass the const pointer directly). Ensure any prototype/declaration for encode_tcp_addr is updated to match (function name encode_tcp_addr) so callers (previously casting away const) compile without const casts.plugins/in_ebpf/traces/tcp/bpf.c (1)
160-173: Confirm reservedstruct eventrecords start zeroed.These exit handlers submit
sizeof(*event)but only populate the active union member. Ifgadget_reserve_buf()behaves like a normal ring-buffer reserve, unused union/padding bytes—anddetails.accept.peeron failed accepts orNULLpeer buffers—can carry stale data into user space. Clearing the record, or at least the active payload, makes this safe regardless of helper semantics.🧹 Safer fill pattern
event = gadget_reserve_buf(&events, sizeof(*event)); if (!event) { bpf_map_delete_elem(&accepts, &tid); return 0; } + __builtin_memset(event, 0, sizeof(*event)); fill_common(event, args->mntns_id);Also applies to: 219-235, 308-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/in_ebpf/traces/tcp/bpf.c` around lines 160 - 173, The reserved event record returned by gadget_reserve_buf may contain stale union/padding bytes—ensure the record is zeroed before populating only the active union member: after calling gadget_reserve_buf(&events, sizeof(*event)) and before fill_common(), explicitly zero the reserved memory for *event (or at minimum zero the specific active payload such as event->details.listen and event->details.accept.peer) so no stale data is submitted; update the same pattern around the other exit handlers that call gadget_reserve_buf and gadget_submit_buf (refer to gadget_reserve_buf, fill_common, gadget_submit_buf and the event->details.* union usage).
🤖 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_ebpf/traces/tcp/bpf.c`:
- Around line 35-40: parse_sockaddr() currently reads full sockaddr structures
without respecting the syscall-provided length and uses an
uninitialized/unchecked family value; fix by reading and saving the
user-supplied length from accept_args.upeer_addrlen at syscall entry (store it
in the per-CPU/map context used for exit handling), initialize and set 'family'
only after a successful bpf_probe_read_user() call (check return and bail/mark
failure if it fails), and then pass that saved length into parse_sockaddr() so
it bounds reads to the user buffer size; ensure every
bpf_probe_read_user()/bpf_probe_read() return is checked and handle errors by
skipping parsing or using a safe default (e.g., AF_UNSPEC) so no over-reads
occur.
---
Nitpick comments:
In `@plugins/in_ebpf/traces/tcp/bpf.c`:
- Around line 160-173: The reserved event record returned by gadget_reserve_buf
may contain stale union/padding bytes—ensure the record is zeroed before
populating only the active union member: after calling
gadget_reserve_buf(&events, sizeof(*event)) and before fill_common(), explicitly
zero the reserved memory for *event (or at minimum zero the specific active
payload such as event->details.listen and event->details.accept.peer) so no
stale data is submitted; update the same pattern around the other exit handlers
that call gadget_reserve_buf and gadget_submit_buf (refer to gadget_reserve_buf,
fill_common, gadget_submit_buf and the event->details.* union usage).
In `@plugins/in_ebpf/traces/tcp/handler.c`:
- Around line 11-49: Change the encode_tcp_addr signature to take a
pointer-to-const (i.e., make the addr parameter "const struct tcp_addr *addr")
and update its implementation accordingly; then remove the casts at call sites
that currently cast away const (calls passing (struct tcp_addr *) should pass
the const pointer directly). Ensure any prototype/declaration for
encode_tcp_addr is updated to match (function name encode_tcp_addr) so callers
(previously casting away const) compile without const casts.
In `@plugins/in_ebpf/traces/tcp/handler.h`:
- Around line 8-10: The header declares encode_tcp_event(...) with a parameter
const struct event *ev but doesn't provide a forward declaration or include for
struct event, which can cause compile errors; fix by adding a forward
declaration "struct event;" at the top of this header (or include the header
that defines it, e.g., events.h) so encode_tcp_event's prototype is valid when
this file is included.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3eab3cf1-426f-4cd1-b00b-f631f0b92bf2
📒 Files selected for processing (9)
plugins/in_ebpf/in_ebpf.cplugins/in_ebpf/traces/includes/common/encoder.hplugins/in_ebpf/traces/includes/common/events.hplugins/in_ebpf/traces/tcp/bpf.cplugins/in_ebpf/traces/tcp/handler.cplugins/in_ebpf/traces/tcp/handler.hplugins/in_ebpf/traces/traces.htests/runtime/CMakeLists.txttests/runtime/in_ebpf_tcp_handler.c
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugins/in_ebpf/traces/tcp/bpf.c (1)
88-123:⚠️ Potential issue | 🟠 Major
parse_sockaddr()still has issues flagged in prior review: uninitializedfamilyand uncheckedbpf_probe_read_userreturn.The
familyvariable at line 91 is declared but not explicitly initialized. Ifbpf_probe_read_userat line 103 fails,familycontains undefined stack data, leading to incorrect address-family detection. The return value ofbpf_probe_read_usershould be checked.Additionally, the function reads full
sockaddr_in/sockaddr_in6structures without consulting the user-provided address length, which could cause over-reads when the buffer is smaller than the structure size.🐛 Proposed fix for initialization and error checking
static __always_inline void parse_sockaddr(const struct sockaddr *addr, struct tcp_addr *out) { - sa_family_t family; + sa_family_t family = 0; + int ret; if (!out) { return; } __builtin_memset(out, 0, sizeof(*out)); if (!addr) { return; } - bpf_probe_read_user(&family, sizeof(family), &addr->sa_family); + ret = bpf_probe_read_user(&family, sizeof(family), &addr->sa_family); + if (ret < 0) { + return; /* leave out zeroed */ + } if (family == AF_INET) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/in_ebpf/traces/tcp/bpf.c` around lines 88 - 123, parse_sockaddr leaves family uninitialized and ignores bpf_probe_read_user return values and potential user buffer sizes; fix by initializing sa_family_t family = 0, checking the return of bpf_probe_read_user when reading &addr->sa_family and returning early on error, and similarly check returns when reading the full sockaddr_in/sockaddr_in6 into local in4/in6; to avoid over-reads add an argument for the user buffer length (e.g., socklen_t addr_len) or otherwise use a safe length check and only probe_read_user up to min(addr_len, sizeof(struct sockaddr_in)) / min(addr_len, sizeof(struct sockaddr_in6)) before copying into out (update call sites accordingly), keeping all changes inside parse_sockaddr and using the existing symbols family, bpf_probe_read_user, in4, in6, and tcp_addr.
🧹 Nitpick comments (2)
plugins/in_ebpf/traces/tcp/handler.c (2)
38-66: Consider validatingaddr->versionbefore encoding address fields.If
addr->versionis neither 4 nor 6 (e.g., due to corrupted data or an uninitialized struct from a failedbpf_probe_read_user), the code falls through to the IPv4 branch and encodes potentially garbage data. An explicit check could improve robustness.♻️ Suggested defensive check
- if (addr->version == 6) { + if (addr->version == 6) { int i; for (i = 0; i < 4; i++) { snprintf(key, sizeof(key), "%s_addr_v6_%d", key_prefix, i); ret = flb_log_event_encoder_append_body_cstring(log_encoder, key); if (ret != FLB_EVENT_ENCODER_SUCCESS) { return -1; } ret = flb_log_event_encoder_append_body_uint32(log_encoder, addr->addr_raw.v6[i]); if (ret != FLB_EVENT_ENCODER_SUCCESS) { return -1; } } } - else { + else if (addr->version == 4) { snprintf(key, sizeof(key), "%s_addr_v4", key_prefix); ret = flb_log_event_encoder_append_body_cstring(log_encoder, key); if (ret != FLB_EVENT_ENCODER_SUCCESS) { return -1; } ret = flb_log_event_encoder_append_body_uint32(log_encoder, addr->addr_raw.v4); if (ret != FLB_EVENT_ENCODER_SUCCESS) { return -1; } } + else { + /* Unknown IP version - skip address encoding or return error */ + return -1; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/in_ebpf/traces/tcp/handler.c` around lines 38 - 66, Validate addr->version before encoding: in the function using addr, check explicitly that addr->version is either 6 or 4 (e.g., if (addr->version == 6) { ... } else if (addr->version == 4) { ... } else { handle error }) and handle any other value by logging/returning an error (return -1) or skipping encoding to avoid emitting garbage; update the branches around addr->version, key_prefix, log_encoder, and calls to flb_log_event_encoder_append_body_uint32 / flb_log_event_encoder_append_body_cstring to rely on this explicit check.
71-73: Unused parameterins.The
insparameter is declared but never used withinencode_tcp_event(). Consider removing it or marking it as unused to avoid compiler warnings.♻️ Option 1: Remove unused parameter
-int encode_tcp_event(struct flb_input_instance *ins, - struct flb_log_event_encoder *log_encoder, +int encode_tcp_event(struct flb_log_event_encoder *log_encoder, const struct event *ev)Note: This would require updating the call site at line 176 and any test code.
♻️ Option 2: Mark as unused
int encode_tcp_event(struct flb_input_instance *ins, struct flb_log_event_encoder *log_encoder, const struct event *ev) { int ret; + (void) ins; /* unused, kept for API consistency */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/in_ebpf/traces/tcp/handler.c` around lines 71 - 73, The parameter `ins` in function encode_tcp_event is declared but unused; either remove it from the signature and update all call sites that pass that argument (including the caller that invokes encode_tcp_event) or keep the parameter and mark it explicitly unused to suppress warnings (e.g., add a cast/unused macro like (void)ins or FLB_UNUSED(ins) at the top of encode_tcp_event); choose the marking approach if you want to avoid touching callers, or remove the parameter if you prefer a cleaner API and will update all callers/tests accordingly.
🤖 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_ebpf/traces/tcp/handler.c`:
- Around line 181-188: The code returns immediately on flb_input_log_append
failure without calling flb_log_event_encoder_reset, leaving
encoder->output_buffer/state stale; update the handler to ensure
flb_log_event_encoder_reset(encoder) is invoked whether flb_input_log_append
succeeds or fails (e.g., call reset before returning on ret == -1 or use a
single exit path that resets the encoder), referencing flb_input_log_append,
flb_log_event_encoder_reset, encoder and event_ctx->ins to locate and modify the
failing branch.
---
Duplicate comments:
In `@plugins/in_ebpf/traces/tcp/bpf.c`:
- Around line 88-123: parse_sockaddr leaves family uninitialized and ignores
bpf_probe_read_user return values and potential user buffer sizes; fix by
initializing sa_family_t family = 0, checking the return of bpf_probe_read_user
when reading &addr->sa_family and returning early on error, and similarly check
returns when reading the full sockaddr_in/sockaddr_in6 into local in4/in6; to
avoid over-reads add an argument for the user buffer length (e.g., socklen_t
addr_len) or otherwise use a safe length check and only probe_read_user up to
min(addr_len, sizeof(struct sockaddr_in)) / min(addr_len, sizeof(struct
sockaddr_in6)) before copying into out (update call sites accordingly), keeping
all changes inside parse_sockaddr and using the existing symbols family,
bpf_probe_read_user, in4, in6, and tcp_addr.
---
Nitpick comments:
In `@plugins/in_ebpf/traces/tcp/handler.c`:
- Around line 38-66: Validate addr->version before encoding: in the function
using addr, check explicitly that addr->version is either 6 or 4 (e.g., if
(addr->version == 6) { ... } else if (addr->version == 4) { ... } else { handle
error }) and handle any other value by logging/returning an error (return -1) or
skipping encoding to avoid emitting garbage; update the branches around
addr->version, key_prefix, log_encoder, and calls to
flb_log_event_encoder_append_body_uint32 /
flb_log_event_encoder_append_body_cstring to rely on this explicit check.
- Around line 71-73: The parameter `ins` in function encode_tcp_event is
declared but unused; either remove it from the signature and update all call
sites that pass that argument (including the caller that invokes
encode_tcp_event) or keep the parameter and mark it explicitly unused to
suppress warnings (e.g., add a cast/unused macro like (void)ins or
FLB_UNUSED(ins) at the top of encode_tcp_event); choose the marking approach if
you want to avoid touching callers, or remove the parameter if you prefer a
cleaner API and will update all callers/tests accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 078e1c7c-f53e-41b7-bd4c-eb4167620c31
📒 Files selected for processing (9)
plugins/in_ebpf/in_ebpf.cplugins/in_ebpf/traces/includes/common/encoder.hplugins/in_ebpf/traces/includes/common/events.hplugins/in_ebpf/traces/tcp/bpf.cplugins/in_ebpf/traces/tcp/handler.cplugins/in_ebpf/traces/tcp/handler.hplugins/in_ebpf/traces/traces.htests/runtime/CMakeLists.txttests/runtime/in_ebpf_tcp_handler.c
✅ Files skipped from review due to trivial changes (1)
- plugins/in_ebpf/traces/tcp/handler.h
🚧 Files skipped from review as they are similar to previous changes (5)
- plugins/in_ebpf/traces/includes/common/encoder.h
- plugins/in_ebpf/traces/traces.h
- tests/runtime/in_ebpf_tcp_handler.c
- plugins/in_ebpf/in_ebpf.c
- plugins/in_ebpf/traces/includes/common/events.h
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
force-pushed
the
cosmo0920-add-tcp-ebpf-trace
branch
from
b83e0b9 to
8757c04
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/in_ebpf/in_ebpf.c (1)
176-180:⚠️ Potential issue | 🟡 MinorIncomplete cleanup when a trace setup fails mid-way.
If
trace_setupfails after some traces were already successfully registered, the previously registered traces (their ring buffers and skeletons) are not cleaned up before returning-1.🐛 Proposed fix
if (trace_setup(ctx, trace_name) != 0) { flb_plg_error(ctx->ins, "failed to configure trace: %s", trace_name); + for (int i = 0; i < ctx->trace_count; i++) { + ring_buffer__free(ctx->traces[i].rb); + if (ctx->traces[i].skel_destroy) { + ctx->traces[i].skel_destroy(ctx->traces[i].skel); + } + } + flb_log_event_encoder_destroy(ctx->log_encoder); + flb_free(ctx->traces); flb_free(ctx); return -1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/in_ebpf/in_ebpf.c` around lines 176 - 180, When trace_setup(ctx, trace_name) fails, currently only flb_free(ctx) is called leaving previously registered traces' resources (ring buffers, skeletons) allocated; update the error path to iterate over ctx's registered trace list (or call an existing cleanup helper) to free/unregister each trace's ring buffer and bpf skeleton before calling flb_free(ctx) and returning -1. Locate the failure block around trace_setup in in_ebpf.c and ensure functions/structures involved (trace_setup, ctx->traces or similar, and any unregister/free functions for ring buffers and skeletons) are used to perform full cleanup of all partially-initialized traces prior to returning.
🧹 Nitpick comments (2)
plugins/in_ebpf/in_ebpf.c (1)
35-39: Potential memory leak ifflb_reallocfails.If
flb_reallocfails, the originalctx->tracespointer is lost, leaking any previously allocated trace contexts. This is a pre-existing pattern, but worth noting.♻️ Suggested fix
- ctx->traces = flb_realloc(ctx->traces, sizeof(struct trace_context) * (ctx->trace_count + 1)); - if (!ctx->traces) { + struct trace_context *new_traces; + new_traces = flb_realloc(ctx->traces, sizeof(struct trace_context) * (ctx->trace_count + 1)); + if (!new_traces) { flb_plg_error(ctx->ins, "failed to allocate memory for trace handlers"); return -1; } + ctx->traces = new_traces;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/in_ebpf/in_ebpf.c` around lines 35 - 39, The current call to flb_realloc directly assigns to ctx->traces which can leak the original buffer on failure; instead, call flb_realloc into a temporary pointer (e.g., tmp), check if tmp is NULL, and only assign tmp back to ctx->traces after a successful realloc; on failure log via flb_plg_error(ctx->ins, ...) and return -1 without modifying or freeing ctx->traces so existing trace contexts are preserved; update the allocation logic around ctx->traces, ctx->trace_count and the flb_realloc call accordingly.tests/runtime/in_ebpf_tcp_handler.c (1)
42-48: Decoder is allocated but never used.The
flb_log_event_decoderis created and destroyed but not used in any test assertions. If the intent is to validate the encoded output, consider adding decode verification; otherwise, remove it to simplify the test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/runtime/in_ebpf_tcp_handler.c` around lines 42 - 48, The decoder (ctx->decoder created via flb_log_event_decoder_create) is unnecessary if you aren't asserting on encoded output; either remove the ctx->decoder allocation and its subsequent cleanup (and related flb_log_event_decoder_destroy calls) to simplify the test, or actually use it to validate encoding by retrieving the encoder buffer and running the decoder (use flb_log_event_decoder_decode or the appropriate decode API on the payload from the encoder) and add assertions against decoded records; update or remove the matching cleanup alongside flb_log_event_encoder_destroy and flb_free(ctx->ins)/flb_free(ctx) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/runtime/in_ebpf_tcp_handler.c`:
- Around line 85-87: The test currently calls init_test_context() into ctx and
uses TEST_CHECK(ctx != NULL) but continues on failure; update the test to stop
execution if ctx is NULL by checking the return value after TEST_CHECK and
returning from the test (or calling the test framework’s abort/cleanup helper).
Concretely, after the TEST_CHECK(ctx != NULL) invocation for the ctx produced by
init_test_context(), add an explicit if (!ctx) return (or call the existing test
cleanup/abort routine) to avoid dereferencing ctx in subsequent code paths that
follow in this test function.
---
Outside diff comments:
In `@plugins/in_ebpf/in_ebpf.c`:
- Around line 176-180: When trace_setup(ctx, trace_name) fails, currently only
flb_free(ctx) is called leaving previously registered traces' resources (ring
buffers, skeletons) allocated; update the error path to iterate over ctx's
registered trace list (or call an existing cleanup helper) to free/unregister
each trace's ring buffer and bpf skeleton before calling flb_free(ctx) and
returning -1. Locate the failure block around trace_setup in in_ebpf.c and
ensure functions/structures involved (trace_setup, ctx->traces or similar, and
any unregister/free functions for ring buffers and skeletons) are used to
perform full cleanup of all partially-initialized traces prior to returning.
---
Nitpick comments:
In `@plugins/in_ebpf/in_ebpf.c`:
- Around line 35-39: The current call to flb_realloc directly assigns to
ctx->traces which can leak the original buffer on failure; instead, call
flb_realloc into a temporary pointer (e.g., tmp), check if tmp is NULL, and only
assign tmp back to ctx->traces after a successful realloc; on failure log via
flb_plg_error(ctx->ins, ...) and return -1 without modifying or freeing
ctx->traces so existing trace contexts are preserved; update the allocation
logic around ctx->traces, ctx->trace_count and the flb_realloc call accordingly.
In `@tests/runtime/in_ebpf_tcp_handler.c`:
- Around line 42-48: The decoder (ctx->decoder created via
flb_log_event_decoder_create) is unnecessary if you aren't asserting on encoded
output; either remove the ctx->decoder allocation and its subsequent cleanup
(and related flb_log_event_decoder_destroy calls) to simplify the test, or
actually use it to validate encoding by retrieving the encoder buffer and
running the decoder (use flb_log_event_decoder_decode or the appropriate decode
API on the payload from the encoder) and add assertions against decoded records;
update or remove the matching cleanup alongside flb_log_event_encoder_destroy
and flb_free(ctx->ins)/flb_free(ctx) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 59038b17-65c6-4382-adfd-6db73e1ea30f
📒 Files selected for processing (9)
plugins/in_ebpf/in_ebpf.cplugins/in_ebpf/traces/includes/common/encoder.hplugins/in_ebpf/traces/includes/common/events.hplugins/in_ebpf/traces/tcp/bpf.cplugins/in_ebpf/traces/tcp/handler.cplugins/in_ebpf/traces/tcp/handler.hplugins/in_ebpf/traces/traces.htests/runtime/CMakeLists.txttests/runtime/in_ebpf_tcp_handler.c
✅ Files skipped from review due to trivial changes (2)
- plugins/in_ebpf/traces/tcp/handler.h
- plugins/in_ebpf/traces/tcp/handler.c
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/runtime/CMakeLists.txt
- plugins/in_ebpf/traces/includes/common/encoder.h
- plugins/in_ebpf/traces/includes/common/events.h
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugins/in_ebpf/in_ebpf.c (2)
198-208:⚠️ Potential issue | 🟡 MinorFree
ctx->tracesin this unwind branch too.This path now destroys each ring buffer and skeleton, but it still returns without freeing the trace-context array itself.
💡 Proposed fix
if (ctx->coll_fd < 0) { flb_plg_error(ctx->ins, "failed to set up collector"); for (int i = 0; i < ctx->trace_count; i++) { ring_buffer__free(ctx->traces[i].rb); if (ctx->traces[i].skel_destroy) { ctx->traces[i].skel_destroy(ctx->traces[i].skel); } } flb_log_event_encoder_destroy(ctx->log_encoder); + flb_free(ctx->traces); flb_free(ctx); return -1; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/in_ebpf/in_ebpf.c` around lines 198 - 208, The unwind branch that handles ctx->coll_fd < 0 frees each trace's ring buffer and skeleton and destroys the log encoder but neglects to free the trace-context array itself; after the loop that calls ring_buffer__free and skel_destroy for each ctx->traces[i], add a call to free the trace array (e.g., free or flb_free on ctx->traces) before flb_log_event_encoder_destroy and flb_free(ctx), ensuring you reference ctx->traces and ctx->trace_count so the correct buffer is released.
26-45:⚠️ Potential issue | 🔴 CriticalFix realloc failure handling in
trace_register().The code directly assigns the
flb_realloc()result toctx->traces:ctx->traces = flb_realloc(ctx->traces, sizeof(struct trace_context) * (ctx->trace_count + 1)); if (!ctx->traces) { flb_plg_error(ctx->ins, "failed to allocate memory for trace handlers"); return -1; }
flb_realloc()is a wrapper around standardrealloc(), which returns NULL on failure. When this happens,ctx->tracesis set to NULL, butctx->trace_countremains non-zero. The unwind loop inin_ebpf_init()later dereferencesctx->traces[i], causing a crash.Use a temporary pointer to preserve the old allocation on failure:
Proposed fix
int trace_register(struct flb_in_ebpf_context *ctx, const char *name, void *skel, struct bpf_object *obj, trace_skel_destroy_func_t skel_destroy, trace_event_handler_t handler) { struct trace_context *trace; + struct trace_context *tmp; struct bpf_map *map, *events_map; int map_fd; flb_plg_debug(ctx->ins, "registering trace handler for: %s", name); - ctx->traces = flb_realloc(ctx->traces, sizeof(struct trace_context) * (ctx->trace_count + 1)); - if (!ctx->traces) { + tmp = flb_realloc(ctx->traces, + sizeof(struct trace_context) * (ctx->trace_count + 1)); + if (!tmp) { flb_plg_error(ctx->ins, "failed to allocate memory for trace handlers"); return -1; } + ctx->traces = tmp;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/in_ebpf/in_ebpf.c` around lines 26 - 45, The realloc result in trace_register() is assigned directly to ctx->traces which loses the old pointer on failure; change this to use a temporary pointer (e.g., new_traces) to call flb_realloc(ctx->traces, sizeof(struct trace_context) * (ctx->trace_count + 1)), check if new_traces is NULL, log and return -1 without modifying ctx->traces if allocation fails, and only assign ctx->traces = new_traces and increment ctx->trace_count after successful allocation; this preserves the original ctx->traces used by the in_ebpf_init() unwind loop and avoids null dereferences.
🤖 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_ebpf/in_ebpf.c`:
- Around line 263-265: The help text for the "Trace" config entry in
plugins/in_ebpf/in_ebpf.c advertises trace names without the required "trace_"
prefix, causing "unknown trace name" errors; update the help string associated
with the FLB_CONFIG_MAP_STR entry for "Trace" to list the actual registered
trace names (e.g., trace_bind, trace_malloc, trace_signal, trace_vfs, trace_tcp)
so examples in --help match the trace registration used by the plugin.
---
Outside diff comments:
In `@plugins/in_ebpf/in_ebpf.c`:
- Around line 198-208: The unwind branch that handles ctx->coll_fd < 0 frees
each trace's ring buffer and skeleton and destroys the log encoder but neglects
to free the trace-context array itself; after the loop that calls
ring_buffer__free and skel_destroy for each ctx->traces[i], add a call to free
the trace array (e.g., free or flb_free on ctx->traces) before
flb_log_event_encoder_destroy and flb_free(ctx), ensuring you reference
ctx->traces and ctx->trace_count so the correct buffer is released.
- Around line 26-45: The realloc result in trace_register() is assigned directly
to ctx->traces which loses the old pointer on failure; change this to use a
temporary pointer (e.g., new_traces) to call flb_realloc(ctx->traces,
sizeof(struct trace_context) * (ctx->trace_count + 1)), check if new_traces is
NULL, log and return -1 without modifying ctx->traces if allocation fails, and
only assign ctx->traces = new_traces and increment ctx->trace_count after
successful allocation; this preserves the original ctx->traces used by the
in_ebpf_init() unwind loop and avoids null dereferences.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
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.
TCP/IP also provides eBPF entrypoints so we can provide this type of traces.
At first, we can provide connect, accept/accept4, and listen.
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
New Features
Improvements
Tests