out_s3, out_chronicle: grow log_key buffer to avoid dropping oversized values#12137
out_s3, out_chronicle: grow log_key buffer to avoid dropping oversized values#12137vitaly-sinev wants to merge 2 commits into
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChronicle and S3 now dynamically resize scratch buffers and retry JSON serialization when configured log-key values exceed the initial capacity. Allocation failures clean up the buffer and abort extraction. ChangesLog-key serialization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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 (1)
plugins/out_chronicle/chronicle.c (1)
604-623: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the required control-block brace style consistently.
plugins/out_chronicle/chronicle.c#L604-L623: move opening braces for the newwhileandifblocks onto the following line.plugins/out_s3/s3.c#L3695-L3715: make the same change for the new retry-loop blocks.🤖 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_chronicle/chronicle.c` around lines 604 - 623, Apply the project’s brace style to the new control blocks: in plugins/out_chronicle/chronicle.c lines 604-623, update the while and if blocks; in plugins/out_s3/s3.c lines 3695-3715, make the same adjustment for each new retry-loop block. Move each opening brace onto the following line without changing control flow.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_s3/s3.c`:
- Around line 3707-3715: Update the allocation-failure handling in the decoder
path around alloc_error so any failure after records have been appended cannot
return a partial payload. After destroying the decoder, free val_buf and return
NULL whenever alloc_error is set, regardless of val_offset or previously decoded
data.
---
Nitpick comments:
In `@plugins/out_chronicle/chronicle.c`:
- Around line 604-623: Apply the project’s brace style to the new control
blocks: in plugins/out_chronicle/chronicle.c lines 604-623, update the while and
if blocks; in plugins/out_s3/s3.c lines 3695-3715, make the same adjustment for
each new retry-loop block. Move each opening brace onto the following line
without changing control 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: 67c22f54-452d-47a4-a28e-2dad486fe111
📒 Files selected for processing (2)
plugins/out_chronicle/chronicle.cplugins/out_s3/s3.c
![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: f25bf7808d
ℹ️ 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".
| alloc_error = 1; | ||
| break; |
There was a problem hiding this comment.
For S3 chunks using log_key, if a later non-string value needs this buffer growth and flb_realloc() fails after earlier records have already advanced val_offset, setting alloc_error only exits the decoder loop; the function then falls through and creates an SDS from the partial val_buf whenever val_offset != 0. The flush path treats that non-NULL chunk as data to upload, so the current and remaining records can be dropped while the output reports success; free the buffer and return NULL on this allocation failure instead of falling through with partial data.
Useful? React with 👍 / 👎.
The log_key extraction buffer was sized bytes + bytes/4 and never grown. A non-string log_key value whose JSON form exceeded the remaining space made flb_msgpack_to_json() fail; the code only broke out of the loop, so the oversized value was silently dropped (and, before that, could be emitted truncated). Grow the scratch buffer and retry the conversion until the value fits, so the value is neither truncated nor dropped. The guard on the remaining size also avoids calling flb_msgpack_to_json() with a zero-length buffer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: v.sinev <vit.phantom@gmail.com>
The log_key extraction buffer was sized bytes + bytes/4 and never grown. A non-string log_key value whose JSON form exceeded the remaining space made flb_msgpack_to_json() fail; the code only broke out of the loop, so the oversized value was silently dropped. Grow the scratch buffer and retry the conversion until the value fits, so the value is neither truncated nor dropped. The guard on the remaining size also avoids calling flb_msgpack_to_json() with a zero-length buffer. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: v.sinev <vit.phantom@gmail.com>
f25bf78 to
032f864
Compare
|
Good catch (both @coderabbitai and @chatgpt-codex-connector) — fixed: on |
|
To use Codex here, create a Codex account and connect to github. |
|
Tip For best results, initiate chat on the files or code changes.
🐇✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
1 participant
Problem
The
log_keyextraction paths inout_s3andout_chronicleallocate a scratch buffer ofbytes + bytes / 4once and never grow it. When the selectedlog_keyvalue is a non-string (number/array/object) whose JSON encoding does not fit in the remaining space,flb_msgpack_to_json()fails and the code simplybreaks out of the loop:As a result the oversized value is silently dropped (and, depending on timing, could previously be emitted truncated). This is a pre-existing data-loss bug independent of any formatting change.
Fix
Grow the scratch buffer and retry the conversion until the value fits, so an oversized
log_keyvalue is neither truncated nor dropped. The retry also guards the remaining size soflb_msgpack_to_json()is never called with a zero-length buffer.out_s3: on allocation failure it uses the function's existingalloc_errorpath.out_chronicle: on allocation failure it frees and returnsNULL.Testing
-DFLB_OUT_S3=On -DFLB_OUT_CHRONICLE=On(fullfluent-bitbinary).Context
Surfaced during review of #12129 / #12133. #12133 makes
flb_msgpack_to_json()signal truncation as a negative value (matching its contract); this PR makes these two callers actually recover from it. The two PRs are independent — this fix works whether the helper returns0or-1on overflow (it checksret <= 0).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