out_stackdriver: fix payload parsing over-reads and null safety by erain · Pull Request #12022 · fluent/fluent-bit

erain · 2026-06-30T12:42:57Z

Summary

Small, independent correctness fixes in the out_stackdriver payload parsers, found while auditing the plugin:

try_assign_subfield_int — atoll(obj.via.str.ptr) was called on a msgpack string that is not NUL-terminated, so atoll reads past the value into adjacent memory. Copy into a bounded stack buffer and NUL-terminate before parsing.
try_assign_subfield_str — flb_sds_copy(*subfield, ...) was invoked even when *subfield is NULL; create the sds via flb_sds_create_len in that case.
pack_sds_safe — new helper that packs a possibly-NULL flb_sds_t as an empty string instead of dereferencing it; applied in the operation / sourceLocation / httpRequest packers, which could previously pack NULL fields.
init_http_request — initialize the sub-fields to NULL (with matching NULL guards) instead of allocating an empty sds per record.

These are independent of each other and of any larger refactor.

Testing

Unit tests: ctest -R flb-rt-out_stackdriver — all out_stackdriver runtime tests pass.
[N/A] Valgrind: covered by the runtime test suite.

Documentation

[N/A] No user-facing or behavioral change.

Backporting

Candidate for backport (small, self-contained correctness fixes).

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
- Improved Stackdriver string serialization to handle missing values safely and consistently during event formatting.
- Fixed latency parsing and storage by properly initializing, copying, and validating the latency field (including stricter conversion behavior).
- Enhanced operation and source location string packing to avoid malformed or incomplete output when values are null/empty.
Tests
- Added a runtime test covering sourceLocation.line rejection for malformed numeric strings (e.g., 123abc).

Signed-off-by: Yu Yi <yiyu@google.com>

coderabbitai · 2026-06-30T12:43:19Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: a28e676a-701a-402b-b0bf-d38c77f296cc

📥 Commits

Reviewing files that changed from the base of the PR and between bc0a065 and f3a110f.

📒 Files selected for processing (3)

plugins/out_stackdriver/stackdriver_helper.c
tests/runtime/data/stackdriver/stackdriver_test_source_location.h
tests/runtime/out_stackdriver.c

✅ Files skipped from review due to trivial changes (1)

tests/runtime/data/stackdriver/stackdriver_test_source_location.h

🚧 Files skipped from review as they are similar to previous changes (1)

plugins/out_stackdriver/stackdriver_helper.c

📝 Walkthrough

Walkthrough

Adds null-safe SDS handling for Stackdriver string fields, introduces pack_sds_safe for MsgPack serialization, hardens integer parsing for string inputs, and adds a runtime test for malformed sourceLocation.line values.

Changes

SDS null-safety, packing, and line parsing

Layer / File(s)	Summary
Helper parsing and packing `plugins/out_stackdriver/stackdriver_helper.h`, `plugins/out_stackdriver/stackdriver_helper.c`	Adds `pack_sds_safe`, updates string subfield assignment to handle null destinations, and changes string-to-integer parsing to reject partial or overflowing input.
http_request null-init and packing `plugins/out_stackdriver/stackdriver_http_request.c`	Initializes HTTP request string SDS fields to `NULL`, treats null latency as absent, packs request string subfields through `pack_sds_safe`, and allocates or copies latency during validation.
operation and source location packing `plugins/out_stackdriver/stackdriver_operation.c`, `plugins/out_stackdriver/stackdriver_source_location.c`	Replaces manual MsgPack string packing in `add_operation_field` and `add_source_location_field` with `pack_sds_safe` for stored SDS values.
Malformed line runtime test `tests/runtime/data/stackdriver/stackdriver_test_source_location.h`, `tests/runtime/out_stackdriver.c`	Adds a malformed `sourceLocation.line` fixture and a runtime test that checks the parsed line falls back to `0` while file and function are preserved.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: cosmo0920, braydonk, JeffLuoo

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main changes: safer payload parsing and NULL handling in out_stackdriver.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

…ure) Final plan to finish the out_stackdriver research: - PR-A opened upstream (fluent#12022). - E1: run the attribution reachability probe (still not done; report cites a missing file) to decide PR-C and whether the GKE trust_payload setting is safe. - E2: journal honest workers=1 vs 2 perf (>=3 replicates, real BigQuery delivery, ODR); reclassify L3 as quota-bound. - F1: make PR-B mergeable (revert buffer=0, split plugin/test commits, blank lines). - F2: re-do PR-C after E1 (cluster-resource fallback per fluent#1186, not "unknown"). - Phase R: final report with two action-item sets (OSS changes, GKE config changes) each with measured benefit + evidence; drop the secure/max-throughput overclaims. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

coderabbitai

Actionable comments posted: 1

🤖 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_helper.c`:
- Around line 63-67: Reject malformed or truncated numeric strings before
writing to the target field in the Stackdriver helper’s numeric parsing logic.
In the code path that copies into buf and assigns through *subfield, stop
accepting partially parsed values from atoll(); instead validate the full input
with strtoll() using endptr and errno, and return early if the string is
oversized or not a complete integer so httpRequest and sourceLocation.line only
update on fully valid input.

🪄 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: 0b6956e2-2d1f-4def-9b09-5ac91c52e8a1

📥 Commits

Reviewing files that changed from the base of the PR and between b81c252 and bc0a065.

📒 Files selected for processing (5)

plugins/out_stackdriver/stackdriver_helper.c
plugins/out_stackdriver/stackdriver_helper.h
plugins/out_stackdriver/stackdriver_http_request.c
plugins/out_stackdriver/stackdriver_operation.c
plugins/out_stackdriver/stackdriver_source_location.c

- 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>

try_assign_subfield_int() used atoll() on the copied payload string, which silently accepts partially-parsed input ("123abc" -> 123), treats an empty string as 0, and would parse a truncated value from an oversized string. Numeric httpRequest fields and sourceLocation.line could therefore be set from malformed input. Validate the full string with strtoll() instead: reject empty or oversized strings up front, and only assign when the entire buffer parsed as one complete integer (endptr at NUL) without overflow (errno). On invalid input the field is left at its previous value. Add a runtime test (sourceLocation_line_invalid_string) asserting that a "123abc" line string is rejected and left at 0. Signed-off-by: Yu Yi <yiyu@google.com>

erain · 2026-07-02T03:20:22Z

Good catch — addressed in f3a110f.

try_assign_subfield_int() now validates the whole string with strtoll() instead of atoll(): it rejects empty or oversized strings up front, and only assigns when the entire buffer parses as one complete integer (endptr at the terminating NUL) with no overflow (errno). Malformed input like "123abc", and truncated/oversized strings, no longer produce a partial value — the field is left at its previous value. This covers both the numeric httpRequest fields and sourceLocation.line.

Added a runtime test (sourceLocation_line_invalid_string) that asserts a "123abc" line string is rejected and left at 0; the full flb-rt-out_stackdriver suite passes locally.

…trings record_get_field_uint64() and record_get_field_time() called strtoul()/flb_strptime() directly on msgpack_object.via.str.ptr. msgpack strings are raw, length-prefixed bytes into the decode buffer, not NUL-terminated, so these C-string functions could read past the field's true boundary. record_get_field_ptr()'s strncmp() key match had the same latent issue (a key that is a prefix of fieldname could false-match, and a short key could still be read past its bounds by strncmp with a longer fieldname length). A spec-compliant Kubernetes Event field (e.g. resourceVersion as a digit-only JSON string) placed at the edge of the decode buffer is enough to trigger an out-of-bounds read; confirmed via a guard-page harness that reproduces EXC_BAD_ACCESS inside strtoul_l, called from record_get_field_uint64. This is the same bug class fixed same-day for the sibling out_stackdriver plugin (fluent#12022, backported in fluent#12170), and the same class that produced GHSA-5rjf-prwh-pp7q in this project before. Applies the same fix pattern here: copy the field into a bounded, NUL-terminated stack buffer before parsing, and require an exact length match before the key strncmp. A prior contributor flagged the same underlying issue in fluent#12073, but it was self-closed without a fix landing; the vulnerable code is still present at HEAD. Signed-off-by: zanarelli <zanarelli.dev@gmail.com>

…trings record_get_field_uint64() and record_get_field_time() called strtoul()/flb_strptime() directly on msgpack_object.via.str.ptr. msgpack strings are raw, length-prefixed bytes into the decode buffer, not NUL-terminated, so these C-string functions could read past the field's true boundary. record_get_field_ptr()'s strncmp() key match had the same latent issue (a key that is a prefix of fieldname could false-match, and a short key could still be read past its bounds by strncmp with a longer fieldname length). A spec-compliant Kubernetes Event field (e.g. resourceVersion as a digit-only JSON string) placed at the edge of the decode buffer is enough to trigger an out-of-bounds read; confirmed via a guard-page harness that reproduces EXC_BAD_ACCESS inside strtoul_l, called from record_get_field_uint64. This is the same bug class fixed same-day for the sibling out_stackdriver plugin (#12022, backported in #12170), and the same class that produced GHSA-5rjf-prwh-pp7q in this project before. Applies the same fix pattern here: copy the field into a bounded, NUL-terminated stack buffer before parsing, and require an exact length match before the key strncmp. A prior contributor flagged the same underlying issue in #12073, but it was self-closed without a fix landing; the vulnerable code is still present at HEAD. Signed-off-by: zanarelli <zanarelli.dev@gmail.com>

out_stackdriver: fix payload parsing over-reads and null safety

bc0a065

Signed-off-by: Yu Yi <yiyu@google.com>

github-actions Bot added the docs-required label Jun 30, 2026

erain temporarily deployed to pr June 30, 2026 12:43 — with GitHub Actions Inactive

coderabbitai Bot reviewed Jun 30, 2026

View reviewed changes

Comment thread plugins/out_stackdriver/stackdriver_helper.c Outdated

erain temporarily deployed to pr June 30, 2026 13:15 — with GitHub Actions Inactive

erain temporarily deployed to pr July 2, 2026 03:20 — with GitHub Actions Inactive

erain temporarily deployed to pr July 2, 2026 03:51 — with GitHub Actions Inactive

edsiper merged commit db8c436 into fluent:master Jul 4, 2026
62 of 64 checks passed

edsiper added this to the Fluent Bit v5.0.9 milestone Jul 4, 2026

BrewTestBot mentioned this pull request Jul 4, 2026

fluent-bit 5.0.9 Homebrew/homebrew-core#291342

Merged

baizhenyu mentioned this pull request Jul 28, 2026

out_stackdriver: fix payload parsing over-reads and null safety [Backport to 4.2] #12170

Merged

1 task

zanarellidev mentioned this pull request Jul 31, 2026

in_kubernetes_events: fix OOB reads from non-NUL-terminated msgpack strings #12180

Closed

3 tasks

zanarellidev mentioned this pull request Jul 31, 2026

in_kubernetes_events: fix OOB reads from non-NUL-terminated msgpack strings #12187

Closed

4 tasks

Navigation Menu

Uh oh!

out_stackdriver: fix payload parsing over-reads and null safety - #12022

out_stackdriver: fix payload parsing over-reads and null safety#12022
edsiper merged 2 commits into
fluent:masterfrom
erain:pr-a-bugfixes

erain commented Jun 30, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jun 30, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

erain commented Jul 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

erain commented Jun 30, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Documentation

Backporting

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erain commented Jul 2, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

erain commented Jun 30, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 30, 2026 •

edited

Loading