pack: harden flb_msgpack_to_json buffer handling by vitaly-sinev · Pull Request #12133 · fluent/fluent-bit

vitaly-sinev · 2026-07-22T05:03:48Z

Scope: this is a small, self-contained API contract / buffer-safety fix for flb_msgpack_to_json(). It does not attempt to fix any caller's overflow recovery — see the note at the bottom.

Two boundary issues in `flb_msgpack_to_json()`

1. Overflow reported as 0 instead of a negative value.
The function is documented to return "a number of characters on success, or a negative value", but on a buffer overflow it returns 0 (the FLB_FALSE from msgpack2json()), which is indistinguishable from a successful zero-length write:

ret = msgpack2json(json_str, &off, json_size - 1, obj, escape_unicode);
json_str[off] = '\0';
return ret ? off: ret;   /* ret == 0 on overflow */

Return -1 on overflow to match the contract. I audited every caller: the _str/_sds wrappers and the AWS/CloudWatch/Kinesis paths already test ret <= 0, so they are unaffected; only out_s3/out_chronicle test ret < 0 (see note).

2. Zero-capacity buffer wraps the size computation.
json_size - 1 underflows to SIZE_MAX when a non-NULL buffer is passed with json_size == 0, allowing msgpack2json() to write out of bounds. Reject json_size == 0 up front.

Testing

tests/internal/pack.c::json_pack_truncation asserts overflow returns a negative value and that a zero-capacity buffer is rejected. flb-it-pack passes.

Note on callers (out_s3 / out_chronicle)

These two log_key extraction paths test ret < 0 and simply break — they do not grow/retry, so an oversized log_key value is still dropped (previously it was emitted empty). That is a pre-existing data-loss bug and the real fix belongs in those plugins (grow-and-retry, like flb_msgpack_to_json_str already does). It will be sent as a separate out_s3/out_chronicle PR; this PR is intentionally scoped to the core contract/safety only.

Surfaced during review of #12129.

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.

🤖 Generated with Claude Code

Summary by CodeRabbit

Bug Fixes
- Improved msgpack-to-JSON conversion by safely rejecting zero-capacity destination buffers.
- Adjusted return-value behavior so truncated or non-fit JSON outputs are consistently reported as errors (negative results), while only successful conversions are treated as positive.
Tests
- Added a new unit test covering both too-small destination truncation and zero-capacity buffer rejection for msgpack-to-JSON conversion.

coderabbitai · 2026-07-22T05:04:12Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8257afd0-be32-4907-a862-c814f818efba

📥 Commits

Reviewing files that changed from the base of the PR and between ee66240 and d025ebe.

📒 Files selected for processing (2)

src/flb_pack.c
tests/internal/pack.c

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

tests/internal/pack.c
src/flb_pack.c

📝 Walkthrough

Walkthrough

flb_msgpack_to_json() now rejects zero-sized buffers and reports fixed-buffer truncation as -1, with tests covering both behaviors.

Changes

JSON truncation handling

Layer / File(s)	Summary
Conversion contract and truncation test `src/flb_pack.c`, `tests/internal/pack.c`	`flb_msgpack_to_json()` validates buffer capacity, returns `-1` when JSON output does not fit, and tests cover truncation and zero-capacity buffers.

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

Possibly related PRs

fluent/fluent-bit#12137: Related callers retry or grow scratch buffers based on the conversion result when output does not fit.

Suggested reviewers: edsiper

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: hardening flb_msgpack_to_json buffer handling.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.}

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 `@src/flb_pack.c`:
- Around line 1169-1175: Guard zero-capacity destinations before the json_size -
1 calculation in the msgpack2json packing path, returning -1 for non-NULL
buffers with json_size == 0 while preserving existing behavior for valid
capacities and NULL destinations. Add a regression case alongside
test_json_pack_truncation() in pack tests to verify zero-size output does not
write or wrap the capacity.

🪄 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: 6eacf2f5-e7d4-4adc-b923-9c0fc040a1ba

📥 Commits

Reviewing files that changed from the base of the PR and between c830fb6 and e95d167.

📒 Files selected for processing (2)

src/flb_pack.c
tests/internal/pack.c

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e95d167e31

ℹ️ 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".

chatgpt-codex-connector · 2026-07-22T05:07:06Z

+     * distinguish truncation from a successful zero-length write, matching the
+     * documented contract above.
+     */
+    return ret ? off : -1;


Handle JSON overflow at log_key call sites

Returning -1 here sends buffer overflows down the fatal path, but the two production callers that only test < 0 do not recover from it: with log_key enabled, plugins/out_s3/s3.c:3689 and plugins/out_chronicle/chronicle.c:598 just break out of the map loop. If the selected non-string value's JSON encoding exceeds their fixed bytes + bytes / 4 scratch buffer (for example arrays of small integers or nested values that expand when escaped), S3 returns NULL/FLB_ERROR when it is the first value or silently omits that record after earlier values, and Chronicle skips the record. Please resize the scratch buffer or propagate a retryable extraction error at those call sites before changing this API result.

Useful? React with 👍 / 👎.

vitaly-sinev · 2026-07-22T05:52:08Z

Agreed on both points.

Added the json_size == 0 guard (CodeRabbit) — good catch, that was a real out-of-bounds path.
On the log_key callers: you're right that returning -1 alone doesn't make out_s3/out_chronicle recover — they'd still drop the oversized value instead of growing. I've scoped this PR to the core contract/safety only and will send the actual recovery fix (grow-and-retry, matching flb_msgpack_to_json_str) as a separate out_s3/out_chronicle PR. Updated the PR description to say so explicitly.

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 `@tests/internal/pack.c`:
- Around line 945-952: Check and assert the return value of msgpack_unpack
before calling flb_msgpack_to_json in this test setup. Ensure the converter
receives obj only after successful unpacking, while preserving the existing
buffer-size assertion and cleanup flow.

🪄 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: d142575b-3237-4e10-9ae5-dbe95bbec3dd

📥 Commits

Reviewing files that changed from the base of the PR and between e95d167 and ee66240.

📒 Files selected for processing (2)

src/flb_pack.c
tests/internal/pack.c

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

src/flb_pack.c

vitaly-sinev · 2026-07-22T06:14:59Z

The caller-recovery fix is now up as #12137 (out_s3/out_chronicle grow-and-retry on log_key overflow). It's independent of this PR — it checks ret <= 0, so it works whether the helper returns 0 or -1.

Two buffer-boundary issues in flb_msgpack_to_json(): - On a buffer overflow the function returned 0 (the FLB_FALSE from msgpack2json()), indistinguishable from a successful zero-length write. Return -1 instead, matching the documented contract, so fixed-buffer callers can detect truncation. Callers already testing ret <= 0 are unaffected. - A zero-capacity (non-NULL) buffer made 'json_size - 1' wrap to SIZE_MAX, letting msgpack2json() write past the buffer. Reject json_size == 0 up front. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: v.sinev <vit.phantom@gmail.com>

Verify that a value whose JSON form does not fit returns a negative value (not 0), and that a zero-capacity buffer is rejected rather than wrapping the size computation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: v.sinev <vit.phantom@gmail.com>

vitaly-sinev · 2026-07-22T09:23:58Z

Addressed — test_json_pack_truncation now checks the msgpack_unpack() return and only proceeds (with cleanup) on MSGPACK_UNPACK_SUCCESS. Pushed.

vitaly-sinev · 2026-07-24T19:05:19Z

The unit-test CI failure here is unrelated to this change. The failing test is flb-it-oauth2 (oauth2.c:1067 — mock-token-1 / server.token_requests == 1), which exercises the mock OAuth2 HTTP server and is a known flake (timing/port). This PR only touches src/flb_pack.c and tests/internal/pack.c; the relevant test flb-it-pack passed (0.66s) in the same run, and there are no oauth2-related changes here.

Could a maintainer re-run the failed jobs? I don't have permission to re-run CI on the upstream repo. Thanks!

vitaly-sinev requested review from cosmo0920 and edsiper as code owners July 22, 2026 05:03

github-actions Bot added the docs-required label Jul 22, 2026

vitaly-sinev mentioned this pull request Jul 22, 2026

pack: format JSON floats using shortest round-trip representation #12129

Open

6 tasks

coderabbitai Bot reviewed Jul 22, 2026

View reviewed changes

Comment thread src/flb_pack.c

chatgpt-codex-connector Bot reviewed Jul 22, 2026

View reviewed changes

vitaly-sinev force-pushed the pack-json-truncation-signal branch from e95d167 to ee66240 Compare July 22, 2026 05:51

vitaly-sinev changed the title ~~pack: report JSON buffer overflow as a negative value~~ pack: harden flb_msgpack_to_json buffer handling Jul 22, 2026

coderabbitai Bot reviewed Jul 22, 2026

View reviewed changes

Comment thread tests/internal/pack.c

vitaly-sinev mentioned this pull request Jul 22, 2026

out_s3, out_chronicle: grow log_key buffer to avoid dropping oversized values #12137

Open

1 task

vitaly-sinev temporarily deployed to pr July 22, 2026 07:16 — with GitHub Actions Inactive

vitaly-sinev temporarily deployed to pr July 22, 2026 07:47 — with GitHub Actions Inactive

vitaly-sinev temporarily deployed to pr July 22, 2026 07:48 — with GitHub Actions Inactive

vitaly-sinev and others added 2 commits July 22, 2026 12:02

vitaly-sinev force-pushed the pack-json-truncation-signal branch from ee66240 to d025ebe Compare July 22, 2026 09:23

vitaly-sinev temporarily deployed to pr July 23, 2026 02:27 — with GitHub Actions Inactive

vitaly-sinev temporarily deployed to pr July 23, 2026 03:02 — with GitHub Actions Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

pack: harden flb_msgpack_to_json buffer handling#12133

pack: harden flb_msgpack_to_json buffer handling#12133
vitaly-sinev wants to merge 2 commits into
fluent:masterfrom
vitaly-sinev:pack-json-truncation-signal

vitaly-sinev commented Jul 22, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 22, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Jul 22, 2026

Uh oh!

vitaly-sinev commented Jul 22, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

vitaly-sinev commented Jul 22, 2026

Uh oh!

vitaly-sinev commented Jul 22, 2026

Uh oh!

vitaly-sinev commented Jul 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

vitaly-sinev commented Jul 22, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Two boundary issues in flb_msgpack_to_json()

Testing

Note on callers (out_s3 / out_chronicle)

Summary by CodeRabbit

Uh oh!

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

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jul 22, 2026

Choose a reason for hiding this comment

Uh oh!

vitaly-sinev commented Jul 22, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vitaly-sinev commented Jul 22, 2026

Uh oh!

vitaly-sinev commented Jul 22, 2026

Uh oh!

vitaly-sinev commented Jul 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

vitaly-sinev commented Jul 22, 2026 •

edited by coderabbitai Bot

Loading

Two boundary issues in `flb_msgpack_to_json()`

coderabbitai Bot commented Jul 22, 2026 •

edited

Loading