out_stackdriver: fix multi-worker resource-metadata data race (SIGSEGV) - #12023
Conversation
When the output runs with workers >= 2, multiple flush threads call stackdriver_format() concurrently on the shared plugin context and overwrite the same ctx->pod_name / namespace_name / container_name / node_name / local_resource_id strings (flb_sds_destroy + flb_sds_create), causing use-after-free / double-free crashes (SIGSEGV) under load. Move these per-flush fields into a stack-local struct stackdriver_format_ctx owned by stackdriver_format(), thread the resource helpers through it, and release them in a single cleanup path. The shared context no longer holds mutable per-record resource state, so the format path is safe for any worker count without locking. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Yu Yi <yiyu@google.com>
Add resource_k8s_container_concurrency: two output workers across five input streams formatting k8s_container records concurrently, exercising the thread-local format context and guarding against the resource-string data race. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Yu Yi <yiyu@google.com>
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Stackdriver output now extracts Kubernetes labels into a per-call format context, removes those fields from shared plugin state, routes formatting failures through consolidated cleanup, switches msgpack key matching to case-insensitive comparison, and adds a runtime concurrency test. ChangesStackdriver per-call format context refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
- Add research/findings/REPORT.md as the authoritative final report: experiments & results, and two action-item sets (OSS changes, GKE config changes) with measured benefits and evidence pointers. - Fix TECHNICAL_REPORT exec summary: drop the "100% secure / maximum egress throughput" overclaim; state the honest findings (workers=1; L3 is quota-bound; GKE forgery already defended by parser.lua). - STATE.json -> done; record upstream PRs fluent#12022 (PR-A) and fluent#12023 (PR-B) and the remaining PR-C cluster-fallback item. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
plugins/out_stackdriver/stackdriver.c (1)
729-730: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the changed C style with this repository.
Line 730 puts a function opening brace on the signature line, and Line 1080 uses a
//comment.Proposed style cleanup
static int extract_local_resource_id(const void *data, size_t bytes, - struct stackdriver_format_ctx *fmt_ctx, const char *tag) { + struct stackdriver_format_ctx *fmt_ctx, + const char *tag) +{ @@ - // parsing local_resource_id from tag takes higher priority + /* Parsing local_resource_id from tag takes higher priority. */As per coding guidelines, C files should put function opening braces on the next line and use
/* ... */comments.Also applies to: 1080-1080
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/out_stackdriver/stackdriver.c` around lines 729 - 730, Update the C style in stackdriver.c to match the repository conventions: move the opening brace of extract_local_resource_id onto the next line instead of keeping it on the function signature line, and replace the single-line // comment near the referenced logging/comment block with a /* ... */ style comment. Keep the same behavior while adjusting only the formatting in the affected function and comment site.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/out_stackdriver/stackdriver.c`:
- Around line 1020-1022: Guard the `local_resource_id` slicing in the
Stackdriver matching logic before using `ctx->tag_prefix` as an offset. In the
block that assigns `prefix_len`, `str_to_be_matcheds`, and `len_to_be_matched`,
add a length check on `fmt_ctx->local_resource_id` against `prefix_len` so
shorter values are rejected or skipped before pointer arithmetic and regex
matching. Use the existing `ctx->tag_prefix` and `fmt_ctx->local_resource_id`
handling in this section to keep the fix localized and prevent advancing past
the SDS buffer.
In `@tests/runtime/out_stackdriver.c`:
- Around line 3687-3698: The stackdriver runtime test setup is bypassing the
formatter hook, so it reaches the real flush/auth path instead of exercising
stackdriver_format() in-process. Update the test block around
flb_output()/flb_output_set()/flb_start() to call flb_output_set_test() before
starting the engine, using the same stackdriver output instance so the formatter
path is covered and the unused label assertions remain meaningful.
---
Nitpick comments:
In `@plugins/out_stackdriver/stackdriver.c`:
- Around line 729-730: Update the C style in stackdriver.c to match the
repository conventions: move the opening brace of extract_local_resource_id onto
the next line instead of keeping it on the function signature line, and replace
the single-line // comment near the referenced logging/comment block with a /*
... */ style comment. Keep the same behavior while adjusting only the formatting
in the affected function and comment site.
🪄 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: b095ec6f-5b28-48f5-9cd9-5eadf633baf7
📒 Files selected for processing (4)
plugins/out_stackdriver/stackdriver.cplugins/out_stackdriver/stackdriver.hplugins/out_stackdriver/stackdriver_conf.ctests/runtime/out_stackdriver.c
💤 Files with no reviewable changes (1)
- plugins/out_stackdriver/stackdriver.h
is_local_resource_id_match_regex() sliced local_resource_id at flb_sds_len(tag_prefix) without checking the id was at least that long. A payload-supplied local_resource_id shorter than tag_prefix advanced the pointer past the SDS buffer and produced a negative match length, which was then passed to the regex engine as a size_t. Return no-match when local_resource_id is shorter than tag_prefix. Signed-off-by: Yu Yi <yiyu@google.com>
Summary
Fixes a use-after-free / double-free crash (SIGSEGV) in the Stackdriver output when it runs with
workers >= 2.stackdriver_format()extracts per-record Kubernetes resource metadata (pod_name,namespace_name,container_name,node_name,local_resource_id) and stored it on the shared plugin context (struct flb_stackdriver), repeatedlyflb_sds_destroy()+flb_sds_create()-ing those fields. With multiple flush worker threads, two concurrent flushes race on the same context strings — freeing and reading the same buffer at once — a data race that manifests as heap corruption / SIGSEGV under load.This moves those fields into a per-flush stack-local
struct stackdriver_format_ctxowned bystackdriver_format(), threads the resource helpers through it, and frees them in a single cleanup path. The shared context no longer holds mutable per-record state, so the format path is correct for any worker count without locking.Testing
resource_k8s_container_concurrency(2 workers × 5 concurrent input streams);ctest -R out_stackdriverpasses.workers=2before this change are eliminated.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
k8s_containermonitored resource formatting.