out_stackdriver: fix multi-worker resource-metadata data race (SIGSEGV) [Backport to 4.2] - #12228
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> (cherry picked from commit ec738ee)
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> (cherry picked from commit 67a7ad1)
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> (cherry picked from commit 414458e)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
![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: e4d1a2f6d4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| char *operation_producer; | ||
| char *source_location_file; | ||
| char *source_location_function; | ||
| char path[1024]; |
There was a problem hiding this comment.
When a record's tag or logging.googleapis.com/logName override formats to more than this buffer can hold, snprintf() truncates path but returns the would-be length; the later msgpack_pack_str(&mp_pck, len) / msgpack_pack_str_body(&mp_pck, path, len) uses that larger length and can copy uninitialized or out-of-bounds stack data into the payload. This refactor shrinks the buffer from PATH_MAX, so long Fluent Bit tags that previously fit now regress; clamp len to the bytes actually stored or build the log name in an SDS.
Useful? React with 👍 / 👎.
3 participants
Backporting #12023
Backports all three commits from #12023 to 4.2:
ec738ee0out_stackdriver: make resource metadata extraction thread-safe67a7ad16tests: cover out_stackdriver multi-worker format-context safety414458e8out_stackdriver: guard local_resource_id shorter than tag_prefixstackdriver_format()extracted 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. Withworkers >= 2, two concurrent flushes race on the same context strings -- freeing and reading the same buffer at once -- which manifests as heap corruption / SIGSEGV under load.The fix 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 singlecleanup:path. The shared context no longer holds mutable per-record state, so the format path is correct for any worker count without locking.4.2 is the current stable line and ships
workerssupport forout_stackdriver, so it is exposed to the same crash. The fix is presently only in the 5.0 line.Conflict resolution
ec738ee0did not apply cleanly because 4.2 is missing two unrelated master commits that touch the same declaration block instackdriver_format():7fb66436out_stackdriver: fix grouped-log counter parity -- adds theint *formatted_recordsout-parameter33c9a52cout_stackdriver: use int64_t for source_location_lineResolved by taking the upstream side of both conflict hunks and then adapting:
formatted_recordsout-parameter and its*formatted_records = records_count;assignment, since 4.2'sstackdriver_format()has no such parameter andcb_stackdriver_flush()still usesevent_chunk->total_eventsfor metrics;int64_t source_location_linerather than thelongthatec738ee0used, to match 4.2'sextract_source_location()/add_source_location_field()signatures (this is what33c9a52clater restored on master).67a7ad16and414458e8applied cleanly. After this changeplugins/out_stackdriver/stackdriver.his byte-identical to master, and the only remainingstackdriver.cdelta versus master insidestackdriver_format()is theformatted_recordsplumbing from7fb66436above.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:
Covered by the runtime test
resource_k8s_container_concurrencyadded in67a7ad16(2 workers x 5 concurrent input streams formattingk8s_containerrecords). No user-facing or behavioral change; relying on CI for the fullctest -R out_stackdriverrun.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.