log_event_decoder: time: plug 2038 problem on timestamp#11549
log_event_decoder: time: plug 2038 problem on timestamp#11549
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdate timestamp handling to use 64-bit seconds across the codebase: header/type changes, decoder timestamp parsing adjusted for separate sec/nsec with 64-bit semantics and group-marker handling, stdout formatting updated, and tests modified to validate post-2038 timestamps. (≤50 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use Clang for C/C++ static analysis and code quality checks.Clang provides comprehensive static analysis for C and C++ code, including syntax checking, type checking, and various warning diagnostics. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3eacc86bd3
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
a6873cd to
3eacc86
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/fluent-bit/flb_output.h`:
- Around line 580-597: flb_output_flush_destroy currently frees the flush struct
but doesn't free the processed_event_chunk allocated by flb_output_flush_create,
causing a leak when flb_output_return isn't called (e.g., flb_pipe_w failure).
Update flb_output_flush_destroy to check for and free
out_flush->processed_event_chunk (or the actual field name used for that
allocation) before flb_free(out_flush); ensure you null the pointer after
freeing; keep existing coro cleanup intact. This mirrors the cleanup performed
in flb_output_return and prevents the leak on the early-destroy path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 67816f29-a9eb-41f4-866c-c8154b6b5608
📒 Files selected for processing (2)
include/fluent-bit/flb_output.hsrc/flb_output.c
3c3e938 to
3eacc86
Compare
0bb730c to
ebcba8a
Compare
5614e09 to
d0388ec
Compare
|
@cosmo0920 I'd probably say let's do a build of all targets just to confirm no problems anywhere but I think it's fine. |
|
This branch is still underdevelopment so I removed ok-package-test for now. |
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Fixes #11532.
On Fluent Bit core, we still have 2038 issues.
second part of flb_tm struct was processed as int32_t type and out_stdout has int32_t assumed part of printing timestamps.
So, those of operations can be truncate after 2038 cut-off of timestamps issue.
We need to fix and process after surpassed of 2038 timestamps.
After patching this, we can process until 2106 A.D logs.
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:
It's not valgrind but it ran under leaks command on macOS:
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.
Summary by CodeRabbit