out_logrotate: add new out plugin #11110
Conversation
SagiROosto
requested review from
celalettin1286,
cosmo0920,
edsiper,
fujimotos,
niedbalski and
patrick-stephens
as code owners
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new out_logrotate output plugin with implementation, header, CMake registration and build options, example configuration, and comprehensive runtime tests; the plugin provides size-based rotation, optional gzip, retention, multiple formats, and directory creation. Changes
Sequence Diagram(s)sequenceDiagram
rect rgb(250,250,245)
participant Input as Input Plugin
participant Logrot as out_logrotate
participant FS as File System
participant Gzip as Gzip (optional)
end
Input->>Logrot: flush event chunk
Logrot->>FS: ensure directory / open current file
Logrot->>Logrot: check current file size (atomic)
alt size >= Max_Size
Logrot->>FS: rename current -> rotated.<ts>
alt Gzip enabled
Logrot->>Gzip: stream-compress rotated file
Gzip-->>FS: write rotated file.gz
FS->>FS: remove uncompressed rotated file
end
Logrot->>FS: prune old rotated files (Max_Files)
Logrot->>FS: create new current file
end
Logrot->>Logrot: format records (JSON/CSV/LTSV/PLAIN/MSGPACK/TEMPLATE)
Logrot->>FS: append formatted records to current file
Logrot-->>Input: acknowledge flush
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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.
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CMakeLists.txt(1 hunks)cmake/plugins_options.cmake(1 hunks)cmake/windows-setup.cmake(1 hunks)conf/fluent-bit-logrotate.conf(1 hunks)plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)plugins/out_logrotate/logrotate.h(1 hunks)tests/runtime/CMakeLists.txt(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
tests/runtime/CMakeLists.txtcmake/windows-setup.cmakeplugins/out_logrotate/CMakeLists.txtCMakeLists.txt
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Applied to files:
CMakeLists.txt
🧬 Code graph analysis (2)
tests/runtime/out_logrotate.c (1)
src/flb_lib.c (9)
flb_create(143-225)flb_input(266-276)flb_input_set(305-335)flb_output(279-289)flb_output_set(520-551)flb_start(983-994)flb_lib_push(843-870)flb_stop(1011-1055)flb_destroy(228-263)
plugins/out_logrotate/logrotate.c (6)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1298-1321)src/flb_output.c (2)
flb_output_get_property(1096-1099)flb_output_set_context(1524-1527)src/flb_pack.c (1)
flb_msgpack_to_json_str(1459-1500)lib/cmetrics/src/cmt_decode_msgpack.c (1)
cmt_decode_msgpack_create(1450-1496)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)flb_log_event_decoder_destroy(151-178)
🔇 Additional comments (8)
conf/fluent-bit-logrotate.conf (1)
1-21: LGTM! Configuration demonstrates plugin features well.The example configuration effectively showcases the logrotate plugin's key features: size-based rotation (Max_Size 10M), file retention (Max_Files 5), gzip compression, and automatic directory creation.
Note: The path
/tmp/logsis Linux-specific. While this is acceptable for an example, consider documenting that users on Windows would need to adjust the path (e.g.,C:\logs).plugins/out_logrotate/CMakeLists.txt (1)
1-5: LGTM! Build configuration is correct.The CMake configuration properly declares the ZLIB dependency (required for gzip compression) and follows the standard plugin registration pattern used throughout Fluent Bit.
plugins/CMakeLists.txt (1)
366-367: LGTM! Plugin registration is correct.The
out_logrotateplugin is properly registered following the same pattern as other output plugins.cmake/plugins_options.cmake (1)
120-120: LGTM! Build option properly defined.The plugin option is correctly defined with an appropriate description and default value (ON), consistent with other output plugins in the codebase.
CMakeLists.txt (1)
301-301: LGTM! Plugin enabled in all-features build.The
FLB_OUT_LOGROTATEflag is correctly set within theFLB_ALLblock, ensuring the plugin is included when building with all features enabled.cmake/windows-setup.cmake (1)
103-103: LGTM! Windows support enabled.The plugin is enabled for Windows builds following the standard pattern used for other output plugins in this file.
Note: Runtime tests for this plugin are currently only registered for non-Windows systems (see
tests/runtime/CMakeLists.txtline 237). If Windows support is intended, consider adding Windows-specific tests or verifying the existing tests work on Windows.plugins/out_logrotate/logrotate.h (1)
1-32: LGTM! Header structure is clean and correct.The header properly defines output format constants for the logrotate plugin with appropriate include guards and licensing. The enum provides clear format options (JSON, CSV, LTSV, PLAIN, MSGPACK, TEMPLATE) that align with the PR objectives for multi-format support.
tests/runtime/CMakeLists.txt (1)
235-238: Discrepancy confirmed: out_logrotate is enabled on Windows in cmake/windows-setup.cmake but excluded from runtime tests.Verification shows the review comment's concern is valid. The out_logrotate plugin:
- Is set to enabled (
Yes) on Windows in cmake/windows-setup.cmake line 103- Contains extensive Windows-specific code: Windows headers (Shlobj.h, Shlwapi.h), Windows API calls (strncpy_s, localtime_s, PathRemoveFileSpecA), and platform-specific macros
- Has no Windows-specific build exclusions in its own CMakeLists.txt
Yet the runtime test is excluded on Windows (tests/runtime/CMakeLists.txt line 236-238) under a comment stating "These plugins work only on Linux"—contradicting the Windows code present in the source.
The same pattern applies to out_file, which also has Windows-specific code but excluded tests.
Action needed: Manually verify whether Windows support is production-ready and either extend tests to cover Windows or disable the plugin on Windows if support is incomplete.
|
Once the coderabbit comments are addressed we should trigger a full package build but @SagiROosto can you confirm it builds for all targets? |
|
@patrick-stephens UT failed for typo issue I Fixed it along with coderabbit suggestions. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/runtime/out_logrotate.c (1)
294-354: Address the past review concern and clarify expected file count.The addition of
TEST_ASSERT(file_count >= 0)at lines 340 and 349 correctly addresses the past review concern about directory listing failures being silently ignored. Good fix!However, there's an inconsistency in the comment at line 350: it states "Current file + 2 rotated files" but checks
file_count <= 4. Withmax_files=3configured at line 324, the expected count should be 1 current file + 3 rotated files = 4 total, not "2 rotated files" as the comment suggests.Consider updating the comment to match the actual logic:
- TEST_CHECK(file_count <= 4); /* Current file + 2 rotated files */ + TEST_CHECK(file_count <= 4); /* Current file + 3 rotated files (max_files=3) */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
🔇 Additional comments (11)
plugins/CMakeLists.txt (1)
366-366: LGTM!The plugin registration follows the established pattern and is correctly positioned alphabetically between
out_fileandout_forward.tests/runtime/out_logrotate.c (3)
36-154: LGTM!The helper functions are well-structured with appropriate error handling and resource cleanup. The recursive directory deletion, file counting, and pattern matching logic are all correct.
156-229: LGTM!The basic rotation test correctly configures the plugin, triggers rotation by writing sufficient data, and validates that both the original file and rotated files exist.
231-292: LGTM!The gzip compression test properly validates that rotated files are compressed by waiting for
.gzfiles to appear, accounting for potential async compression.plugins/out_logrotate/logrotate.c (7)
106-206: LGTM!The initialization function correctly sets up the plugin context, creates the hash table for per-file size tracking, parses configuration options, and includes proper error handling with resource cleanup.
531-576: LGTM!The file size tracking correctly uses a hash table to maintain per-file size state, addressing the previous concern about a shared global counter. The
should_rotate_filefunction retrieves size by filename, andupdate_file_size_counterusesfstatto read actual file size before updating the hash table.
625-745: LGTM!The streaming gzip compression implementation is memory-efficient and correct:
- Processes files in 64KB chunks to avoid loading entire files into memory
- Properly initializes deflate with raw deflate mode (negative window bits)
- Writes RFC 1952 compliant gzip header and footer with CRC32
- Verifies all input is consumed and compression completes with
Z_STREAM_END- Includes proper error handling and resource cleanup
860-944: LGTM!The cleanup logic correctly:
- Counts rotated files matching the timestamp pattern
- Returns early if within the
max_fileslimit- Sorts files by modification time (oldest first) using a simple bubble sort
- Removes excess files to maintain the
max_fileslimitThe approach ensures that rotated files are cleaned up while preserving the current active file.
1067-1081: LGTM!The metrics handling path now correctly calls
update_file_size_counter(ctx, out_file, fp)at line 1078 before closing the file, which addresses the previous review concern about stale file size counters preventing rotation for metrics-only streams.
946-1182: LGTM!The flush callback implements the complete workflow correctly:
- Checks rotation condition using per-file size from hash table
- Performs rotation and cleanup when needed
- Removes rotated file's entry from hash table to reset tracking
- Initializes file size from disk for new files using
fstat- Updates file size counter after all write operations (logs, metrics, msgpack)
The per-file size tracking ensures correct behavior when multiple files are managed by a single plugin instance.
1184-1274: LGTM!The exit function properly cleans up resources by destroying the hash table and freeing the context. The configuration map is comprehensive with sensible defaults: 100MB max size, 7 max files retained, and gzip compression enabled by default.
|
You'll have to fix the commits that don't match the guidelines btw, i.e. you need a plugin/module prefix like |
SagiROosto
force-pushed
the
out_logrotate
branch
2 times, most recently
from
0dd42f9 to
99917b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
plugins/out_logrotate/logrotate.c (8)
129-134: Consider a larger hash table size for multi-file scenarios.The hash table is created with only 64 buckets. If the plugin writes to many different files (when
fileis not set and tags are used as filenames), this could lead to excessive hash collisions and degraded performance. Consider using a larger size (e.g., 256 or 512) or making it configurable for high-throughput, multi-file deployments.
605-623: Document the 4GB limitation for gzip compression.The gzip footer stores the original file size in 32 bits (4 bytes), which limits support to files under 4GB. While this is a standard gzip limitation, files larger than 4GB will have their size truncated modulo 2^32, which could cause decompression tools to report incorrect sizes or fail validation. Consider logging a warning when compressing files approaching or exceeding this limit, or documenting this limitation in the plugin configuration.
783-795: Check unlink() return values for error logging.Both
unlink()calls (lines 789 and 793) don't check return values. If deletion fails (e.g., permission denied), the files remain on disk but the function returns success or failure based solely on compression status. While the gzip failure path returns -1, the success path (line 790) doesn't verify that cleanup succeeded. Consider logging warnings ifunlink()fails.if (ret == 0) { /* Remove the uncompressed file */ - unlink(rotated_filename); + if (unlink(rotated_filename) != 0) { + flb_plg_warn(ctx->ins, "failed to remove uncompressed file %s", rotated_filename); + } flb_plg_debug(ctx->ins, "rotated and compressed file: %s", gzip_filename); } else { /* Remove the failed gzip file */ - unlink(gzip_filename); + if (unlink(gzip_filename) != 0) { + flb_plg_warn(ctx->ins, "failed to remove incomplete gzip file %s", gzip_filename); + } return -1; }
911-923: Bubble sort may be inefficient for many rotated files.The bubble sort has O(n²) complexity. If
max_filesis high and many files accumulate (e.g., hundreds of rotated logs), this could introduce noticeable latency during cleanup. For production environments with large retention counts, consider usingqsort()with a comparison function for O(n log n) performance.
216-242: No validation for non-map objects in CSV output.The function checks
obj->type == MSGPACK_OBJECT_MAP && obj->via.map.size > 0(line 216) but doesn't handle the case where the object is not a map or is empty. If a non-map object is passed, the function returns 0 (success) without writing anything. While this may be intentional, it could lead to silent data loss. Consider logging a warning when non-map objects are encountered.static int csv_output(FILE *fp, int column_names, struct flb_time *tm, msgpack_object *obj, struct flb_logrotate_conf *ctx) { int i; int map_size; msgpack_object_kv *kv = NULL; - if (obj->type == MSGPACK_OBJECT_MAP && obj->via.map.size > 0) { + if (obj->type != MSGPACK_OBJECT_MAP) { + flb_plg_warn(ctx->ins, "csv_output: expected map, got type %d", obj->type); + return -1; + } + + if (obj->via.map.size > 0) { kv = obj->via.map.ptr; map_size = obj->via.map.size; ... + } else { + flb_plg_debug(ctx->ins, "csv_output: empty map, skipping"); } return 0; }
252-272: No validation for non-map objects in LTSV output.Similar to
csv_output, this function silently returns success if the object is not a map or is empty (line 252). Consider adding validation and logging for consistency with error handling elsewhere in the plugin.
370-381: Silent failure in plain_output when JSON conversion fails.If
flb_msgpack_to_json_str()returns NULL, the function silently skips writing and returns 0 (success). This could lead to data loss without any indication to the caller. Consider returning -1 or logging a warning when conversion fails.static int plain_output(FILE *fp, msgpack_object *obj, size_t alloc_size, int escape_unicode) { char *buf; buf = flb_msgpack_to_json_str(alloc_size, obj, escape_unicode); if (buf) { fprintf(fp, "%s" NEWLINE, buf); flb_free(buf); + } else { + return -1; } return 0; }
383-407: print_metrics_text doesn't return error status.The function is
voidand doesn't return error status to the caller. Ifcmt_decode_msgpack_create()fails (line 394), an error is logged but the caller (line 1075) has no way to know the write failed. Consider changing the return type tointand propagating the error so the flush operation can retry or handle the failure appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/runtime/out_logrotate.c
- plugins/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
🧬 Code graph analysis (1)
plugins/out_logrotate/logrotate.c (7)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_hash_table.c (5)
flb_hash_table_create(99-137)flb_hash_table_destroy(197-215)flb_hash_table_get(496-522)flb_hash_table_add(401-494)flb_hash_table_del(605-674)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1298-1321)src/flb_output.c (2)
flb_output_get_property(1096-1099)flb_output_set_context(1524-1527)src/flb_pack.c (1)
flb_msgpack_to_json_str(1459-1500)lib/cmetrics/src/cmt_decode_msgpack.c (1)
cmt_decode_msgpack_create(1450-1496)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)flb_log_event_decoder_destroy(151-178)
🔇 Additional comments (1)
plugins/out_logrotate/logrotate.c (1)
1073-1081: Metrics file size update now included - excellent!The past review noted that
update_file_size_counter()was missing after writing metrics, causing rotation to never trigger for metrics-only streams. This has been correctly addressed at line 1078.
SagiROosto
force-pushed
the
out_logrotate
branch
2 times, most recently
from
2a77707 to
329cba4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CMakeLists.txt(1 hunks)cmake/plugins_options.cmake(1 hunks)cmake/windows-setup.cmake(1 hunks)conf/fluent-bit-logrotate.conf(1 hunks)plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)plugins/out_logrotate/logrotate.h(1 hunks)tests/runtime/CMakeLists.txt(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- cmake/plugins_options.cmake
- tests/runtime/out_logrotate.c
- conf/fluent-bit-logrotate.conf
- plugins/out_logrotate/CMakeLists.txt
- plugins/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
cmake/windows-setup.cmakeCMakeLists.txttests/runtime/CMakeLists.txt
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Applied to files:
cmake/windows-setup.cmakeCMakeLists.txt
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
SagiROosto
force-pushed
the
out_logrotate
branch
from
329cba4 to
9d1e142
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
plugins/out_logrotate/logrotate.c (2)
578-582: Optional: Remove redundant hash table delete.Lines 579–581 delete the existing key before re-adding it, but
flb_hash_table_add()already handles replacement internally (seesrc/flb_hash_table.clines 430–442). The delete-then-add pattern adds unnecessary overhead—though it's safe, it doubles the hash lookups.Apply this diff to streamline the update:
- /* Delete existing entry if it exists to ensure we can update the value */ - flb_hash_table_del(ctx->file_sizes, filename); /* Store or update file size in hash table */ ret = flb_hash_table_add(ctx->file_sizes, filename, strlen(filename), &file_size, sizeof(size_t));The
flb_hash_table_addfunction already performs an in-place update for existing keys, so the explicit delete is not required.
783-795: Optional: Add path-length validation before constructing rotated filenames.Lines 784 and 795 use
snprintfwithPATH_MAX - 1, which will silently truncate iffilenameis too long. WhilePATH_MAXis typically large (4096 on Linux), a truncated filename would cause rotated files to be unreachable byis_valid_rotation_filename()andcleanup_old_files(), breaking rotation cleanup.Add a length check before constructing the rotated filename:
+ /* Validate that we have room for timestamp suffix */ + if (strlen(filename) + 16 >= PATH_MAX) { /* ".YYYYMMDD_HHMMSS" = 16 chars */ + flb_plg_error(ctx->ins, "filename too long for rotation: %s", filename); + return -1; + } + /* Create rotated filename with timestamp */ snprintf(rotated_filename, PATH_MAX - 1, "%s.%s", filename, timestamp);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/out_logrotate/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
🧬 Code graph analysis (2)
plugins/out_logrotate/logrotate.c (7)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)src/flb_hash_table.c (5)
flb_hash_table_create(99-137)flb_hash_table_destroy(197-215)flb_hash_table_get(496-522)flb_hash_table_del(605-674)flb_hash_table_add(401-494)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1298-1321)src/flb_output.c (2)
flb_output_get_property(1096-1099)flb_output_set_context(1524-1527)src/flb_pack.c (1)
flb_msgpack_to_json_str(1459-1500)lib/cmetrics/src/cmt_decode_msgpack.c (1)
cmt_decode_msgpack_create(1450-1496)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)flb_log_event_decoder_destroy(151-178)
tests/runtime/out_logrotate.c (1)
src/flb_lib.c (10)
flb_create(143-225)flb_service_set(652-678)flb_input(266-276)flb_input_set(305-335)flb_output(279-289)flb_output_set(520-551)flb_start(983-994)flb_lib_push(843-870)flb_stop(1011-1055)flb_destroy(228-263)
🔇 Additional comments (4)
plugins/CMakeLists.txt (1)
366-366: LGTM: Plugin registration is correct.The
out_logrotateplugin is registered following the same pattern as other output plugins, and the placement maintains alphabetical order.tests/runtime/out_logrotate.c (3)
157-235: LGTM: Basic rotation test is well-structured.The test properly:
- Sets up and tears down the test directory
- Configures rotation with
max_size=5Kandgzip=false- Writes enough data (~16KB) to trigger rotation
- Verifies both the current file and at least one rotated file exist
The sleep intervals (lines 210, 218) are necessary to allow async flush operations to complete.
237-300: LGTM: Gzip compression test validates the compression feature.The test correctly:
- Enables gzip compression with
gzip=true- Triggers rotation by writing sufficient data
- Uses
wait_for_file_pattern()to poll for rotated files with.gzsuffix- Verifies that gzip compression was applied to rotated files
302-366: LGTM: Max files cleanup test validates the retention policy.The test properly:
- Configures
max_files=3to limit retained rotated files- Triggers multiple rotations by writing data in 5 iterations
- Validates that file count never exceeds 4 (current file + 3 rotated files)
- Uses
TEST_ASSERT(file_count >= 0)to catch directory listing failures (addressing the past review comment)- Verifies the final count after all rotations complete
The iterative checking (lines 349–351) ensures cleanup happens progressively during rotation, not just at the end.
* add logrotate out plugin * fix gzip * fix test * update max_size * update gzip * add logs Signed-off-by: SagiROosto <[email protected]>
Signed-off-by: SagiROosto <[email protected]>
Signed-off-by: SagiROosto <[email protected]>
Signed-off-by: SagiROosto <[email protected]>
Signed-off-by: SagiROosto <[email protected]>
Signed-off-by: SagiROosto <[email protected]>
Signed-off-by: SagiROosto <[email protected]>
…d fix UT Signed-off-by: SagiROosto <[email protected]>
…ad of hash table This change replaces the hash table with a linked list for storing file sizes, improving memory management and simplifying the code. Additionally, helper functions for managing file size entries have been added, enhancing the overall structure and readability of the logrotate plugin. Signed-off-by: SagiROosto <[email protected]>
SagiROosto
force-pushed
the
out_logrotate
branch
from
9d1e142 to
277aea3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugins/out_logrotate/logrotate.c (1)
751-796: Critical: Z_FINISH still not sent for certain file sizes, causing corrupt gzip output.The compression loop still has the logic error flagged in previous reviews. When the input file size is an exact multiple of
GZIP_CHUNK_SIZE,feof(src_fp)returns false immediately after the last successful read (line 764), soflush = Z_NO_FLUSH. On the next iteration,freadreturns 0, but theif (bytes_read > 0)guard on line 754 prevents the compression block from executing, soZ_FINISHis never sent. The loop exits withstatus != Z_STREAM_END, and rotated gzip files are corrupted.Apply this fix to ensure the final chunk always sends
Z_FINISH:/* Process file in chunks (ensure Z_FINISH is always issued) */ do { bytes_read = fread(input_buffer, 1, GZIP_CHUNK_SIZE, src_fp); + /* Check EOF status after read to determine if this is the last chunk */ + int is_eof = feof(src_fp) || bytes_read == 0; + if (bytes_read > 0) { /* Update CRC and total size */ crc = mz_crc32(crc, (const unsigned char *)input_buffer, bytes_read); total_input_size += bytes_read; /* Set up deflate input */ strm.next_in = (Bytef *)input_buffer; strm.avail_in = bytes_read; - /* Determine flush mode based on EOF after this read */ - flush = feof(src_fp) ? Z_FINISH : Z_NO_FLUSH; + /* Use the EOF status captured after the read */ + flush = is_eof ? Z_FINISH : Z_NO_FLUSH; /* Compress chunk */ do {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CMakeLists.txt(1 hunks)cmake/plugins_options.cmake(1 hunks)cmake/windows-setup.cmake(1 hunks)conf/fluent-bit-logrotate.conf(1 hunks)plugins/CMakeLists.txt(1 hunks)plugins/out_logrotate/CMakeLists.txt(1 hunks)plugins/out_logrotate/logrotate.c(1 hunks)plugins/out_logrotate/logrotate.h(1 hunks)tests/runtime/CMakeLists.txt(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- plugins/out_logrotate/CMakeLists.txt
- plugins/out_logrotate/logrotate.h
- CMakeLists.txt
- plugins/CMakeLists.txt
- conf/fluent-bit-logrotate.conf
- cmake/plugins_options.cmake
- tests/runtime/out_logrotate.c
- cmake/windows-setup.cmake
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
tests/runtime/CMakeLists.txt
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
🧬 Code graph analysis (1)
plugins/out_logrotate/logrotate.c (7)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1303-1326)src/flb_output.c (2)
flb_output_get_property(1108-1111)flb_output_set_context(1546-1549)src/flb_pack.c (1)
flb_msgpack_to_json_str(1459-1500)lib/cmetrics/src/cmt_decode_msgpack.c (1)
cmt_decode_msgpack_create(1450-1496)src/flb_sds.c (2)
flb_sds_create(78-90)flb_sds_destroy(389-399)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)flb_log_event_decoder_destroy(151-178)
🔇 Additional comments (7)
tests/runtime/CMakeLists.txt (1)
237-237: LGTM!The runtime test registration for
out_logrotateis correctly placed within the Linux-only block, consistent without_fileon the previous line.plugins/out_logrotate/logrotate.c (6)
527-648: Per-file size tracking correctly implemented.The file size tracking functions properly maintain a separate size counter for each output file using a linked list, resolving the previous issue where a single global counter caused incorrect rotation decisions. The linear search through the list is acceptable for typical workloads with a small number of output files.
1141-1148: LGTM! File size counter now updated for metrics.The metrics handling path now correctly updates the file size counter (line 1145) before closing the file, ensuring rotation works properly for metrics-only streams. This addresses the previous critical issue.
1179-1188: LGTM! Error path now updates file size counter.The decoder initialization error path now correctly updates the file size counter (line 1185) before closing the file, keeping internal state consistent even when errors occur. This addresses the previous issue.
1280-1365: Configuration defaults are sensible.The default values (100M max_size, 7 max_files, gzip enabled) provide reasonable defaults for typical log rotation scenarios. The
workers = 1setting correctly prevents concurrent flushes within a single instance.
873-927: LGTM! Rotation filename validation is thorough.The validation function correctly identifies rotated files by checking the timestamp pattern (YYYYMMDD_HHMMSS) and optional
.gzsuffix, preventing false positives during cleanup.
929-1014: Cleanup logic correctly retains newest files.The cleanup function properly sorts rotated files by modification time and removes the oldest entries beyond
max_files. The bubble sort (lines 982-993) is O(n²) but acceptable for the typical small number of rotated files.
Signed-off-by: SagiROosto <[email protected]>
…ng support This commit introduces several new test functions for various log formats including CSV, LTSV, plain text, MsgPack, and template formats. Additionally, it enhances the multithreading capabilities of the logrotate tests, ensuring that data is correctly handled and written in a concurrent environment. The changes also include helper functions for reading file content and verifying the output format. Signed-off-by: SagiROosto <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugins/out_logrotate/logrotate.c (1)
817-872: Verify Z_FINISH is always sent at end of compression.The compression loop has a potential issue where Z_FINISH may not be sent for files whose size is an exact multiple of
GZIP_CHUNK_SIZE(64KB). When the lastfreadconsumes exactlyGZIP_CHUNK_SIZEbytes,feof()returns false (line 830). On the next iteration,freadreturns 0,feof()is now true, but theif (bytes_read > 0)guard (line 820) prevents the compression block from executing, soZ_FINISHis never sent.This was flagged in a past review comment and marked as addressed. However, examining the current code, the issue still appears to be present. The fix would require either:
- Moving the flush mode determination outside the
bytes_read > 0check, or- Always performing a final deflate call with
Z_FINISHafter the loop whenfeof()is true.Please verify this issue was actually fixed or if it needs to be addressed.
#!/bin/bash # Verify gzip compression correctness by checking if Z_FINISH logic handles exact multiples rg -n -A5 -B5 'Z_FINISH.*feof|feof.*Z_FINISH' plugins/out_logrotate/logrotate.c
🧹 Nitpick comments (3)
plugins/out_logrotate/logrotate.c (3)
167-179: Consider validating the template parameter when format is template.When
formatis set to"template", the code doesn't verify that thetemplateparameter is configured. If a user sets format to template but doesn't provide a template string,ctx->templatewill be NULL (line 134) or the default value from config_map (line 1539:"{time} {message}"), which may not be the intended behavior. Consider adding validation to ensure the template parameter is set when using template format, or document the default template behavior.
1148-1474: Verify all error paths properly release resources.The flush callback has complex lock handling with multiple acquire/release points. While tracing through the code, all error paths appear to properly:
- Release locks if acquired
- Update file size counters before closing
- Close file pointers before returning
However, the complexity makes this error-prone for future maintenance. Consider extracting helper functions or using RAII-style patterns (if possible in C with cleanup attributes) to reduce the chance of resource leaks during future modifications.
1557-1572: Consider documenting size format for max_size parameter.The
max_sizeparameter usesFLB_CONFIG_MAP_SIZEwhich likely supports human-readable formats like "100M", "1G", etc. The default value "100000000" (line 1557) is 100MB but would be more readable as "100M" or "100MB". Consider updating the default to use the human-readable format for consistency with the description.Apply this diff to improve readability:
{ FLB_CONFIG_MAP_SIZE, "max_size", "100000000", + FLB_CONFIG_MAP_SIZE, "max_size", "100M", 0, FLB_TRUE, offsetof(struct flb_logrotate_conf, max_size), "Maximum size of file before rotation (default: 100M)" },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugins/out_logrotate/logrotate.c(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
🧬 Code graph analysis (2)
plugins/out_logrotate/logrotate.c (8)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1303-1326)src/flb_output.c (2)
flb_output_get_property(1108-1111)flb_output_set_context(1546-1549)src/flb_pack.c (1)
flb_msgpack_to_json_str(1459-1500)lib/cmetrics/src/cmt_decode_msgpack.c (1)
cmt_decode_msgpack_create(1450-1496)src/flb_sds.c (2)
flb_sds_create(78-90)flb_sds_destroy(389-399)src/flb_lock.c (4)
flb_lock_init(6-17)flb_lock_destroy(19-30)flb_lock_acquire(32-65)flb_lock_release(67-100)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)flb_log_event_decoder_destroy(151-178)
tests/runtime/out_logrotate.c (2)
include/fluent-bit/flb_mem.h (1)
flb_free(126-128)src/flb_lib.c (10)
flb_create(143-225)flb_service_set(652-678)flb_input(266-276)flb_input_set(305-335)flb_output(279-289)flb_output_set(520-551)flb_start(983-994)flb_lib_push(843-870)flb_stop(1011-1055)flb_destroy(228-263)
🪛 ast-grep (0.40.0)
tests/runtime/out_logrotate.c
[warning] 462-462: A check is done with stat and then the file is used. There is no guarantee that the status of the file has not changed since the call to stat which may allow attackers to bypass permission checks.
Context: fopen
Note: [CWE-367]: Time-of-check Time-of-use (TOCTOU) Race Condition [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/FIO45-C.+Avoid+TOCTOU+race+conditions+while+accessing+files
(file-stat-before-action-c)
🔇 Additional comments (6)
tests/runtime/out_logrotate.c (2)
182-215: TOCTOU in test helper is acceptable for test code.The static analysis tool flags a TOCTOU race between
statandfopenat line 194. In test code running in isolation, this is not a security concern and the risk window is negligible. The pattern is idiomatic for checking file size before allocation.
1035-1244: Rotation tests comprehensively validate core plugin functionality.The three rotation tests verify size-based rotation, gzip compression, and max_files cleanup. The past review issue regarding directory listing failures has been properly addressed with
TEST_ASSERT(file_count >= 0)checks before the upper-bound validations (lines 1228, 1239).plugins/out_logrotate/logrotate.c (4)
529-669: File size tracking with per-file locking is well-designed.The use of a linked list with per-file locks allows concurrent operations on different files while protecting per-file state. Lock acquisition uses retry logic with appropriate defaults, and all lock acquisitions are paired with releases.
1065-1076: Bubble sort acceptable for small file counts.The bubble sort used here is O(n²) but acceptable since the number of rotated files is typically small (default max_files is 7). For much larger max_files values, a more efficient sort could be considered, but this is unlikely to be a bottleneck.
1207-1243: Rotation logic temporarily releases lock to avoid holding during slow operations.The code releases the lock before calling
rotate_file(lines 1210-1214), then re-acquires it afterward (lines 1228-1242). This is intentional to avoid holding the lock during the potentially slow rotation and compression operations. The plugin setsworkers = 1(line 1585) which prevents concurrent flushes within a single instance, reducing the risk of races during this window. However, if multiple plugin instances target the same file (which is documented as unsupported), races could still occur.The past review comments note this limitation is documented, so this is acceptable.
1476-1501: Exit callback properly cleans up all resources.The cleanup uses
mk_list_foreach_safefor safe deletion during iteration and properly destroys mutexes before freeing memory. Resource cleanup order is correct.
|
@patrick-stephens I made a couple of changes hopefully it will pass CI this time. can you run it? |
Signed-off-by: SagiROosto <[email protected]>
Signed-off-by: SagiROosto <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
plugins/out_logrotate/logrotate.c (1)
708-823: Z_FINISH still not guaranteed for some file sizes → corrupt gzip outputThe compression loop can still skip a final
deflate(..., Z_FINISH)when the input size is an exact multiple ofGZIP_CHUNK_SIZE:
feof(src_fp)becomes true only after an attempted read past EOF.- For
N * GZIP_CHUNK_SIZE-byte files, the last successfulfreadreturnsbytes_read > 0withfeof(src_fp) == 0, so you calldeflate(..., Z_NO_FLUSH).- On the next iteration
bytes_read == 0,feof(src_fp)is now true, but theif (bytes_read > 0)guard skips the compression block, soZ_FINISHis never sent.- The loop exits with
status != Z_STREAM_END, and line 819 logs an error, but a truncated/corrupt gzip file has already been written.You need a final
deflatewithflush = Z_FINISHeven when the lastfreadreturns 0.A minimal fix is to determine
flushbefore thebytes_read > 0guard and allow one last iteration withbytes_read == 0andavail_in == 0:@@ - /* Process file in chunks (ensure Z_FINISH is always issued) */ - do { - bytes_read = fread(input_buffer, 1, GZIP_CHUNK_SIZE, src_fp); - if (bytes_read > 0) { + /* Process file in chunks (ensure Z_FINISH is always issued) */ + do { + bytes_read = fread(input_buffer, 1, GZIP_CHUNK_SIZE, src_fp); + + /* Decide flush mode for this iteration */ + if (feof(src_fp) || ferror(src_fp) || bytes_read == 0) { + flush = Z_FINISH; + } + else { + flush = Z_NO_FLUSH; + } + + if (bytes_read > 0 || flush == Z_FINISH) { @@ - /* Determine flush mode based on EOF after this read */ - flush = feof(src_fp) ? Z_FINISH : Z_NO_FLUSH; - /* Compress chunk */ do { strm.next_out = (Bytef *)output_buffer; @@ - } while (bytes_read > 0 && status != Z_STREAM_END); + } while (status != Z_STREAM_END);Any equivalent structure that guarantees a final
deflate(..., Z_FINISH)after the read loop (even whenbytes_read == 0) works; without it, some rotated.gzfiles will remain corrupt.
🧹 Nitpick comments (1)
tests/runtime/out_logrotate.c (1)
1072-1077: Unusedtimestampin basic_rotation test
strftime(timestamp, ...)computes a timestamp that is never used inflb_test_logrotate_basic_rotation(). This looks like leftover code and can be safely removed to keep the test focused and avoid confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugins/out_logrotate/logrotate.c(1 hunks)tests/runtime/out_logrotate.c(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-23T07:43:16.216Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 11059
File: plugins/in_tail/tail_file.c:1618-1640
Timestamp: 2025-10-23T07:43:16.216Z
Learning: In plugins/in_tail/tail_file.c, when truncate_long_lines is enabled and the buffer is full, the early truncation path uses `lines > 0` as the validation pattern to confirm whether process_content successfully processed content. This is intentional to track occurrences of line processing rather than byte consumption, and consuming bytes based on `processed_bytes > 0` would be overkill for this validation purpose.
Applied to files:
plugins/out_logrotate/logrotate.c
🧬 Code graph analysis (2)
plugins/out_logrotate/logrotate.c (8)
include/fluent-bit/flb_mem.h (2)
flb_calloc(84-96)flb_free(126-128)include/fluent-bit/flb_output.h (1)
flb_output_config_map_set(1303-1326)src/flb_output.c (2)
flb_output_get_property(1108-1111)flb_output_set_context(1546-1549)src/flb_pack.c (1)
flb_msgpack_to_json_str(1459-1500)lib/cmetrics/src/cmt_decode_msgpack.c (1)
cmt_decode_msgpack_create(1450-1496)src/flb_sds.c (2)
flb_sds_create(78-90)flb_sds_destroy(389-399)src/flb_lock.c (4)
flb_lock_init(6-17)flb_lock_destroy(19-30)flb_lock_acquire(32-65)flb_lock_release(67-100)src/flb_log_event_decoder.c (3)
flb_log_event_decoder_init(103-120)flb_log_event_decoder_next(309-456)flb_log_event_decoder_destroy(151-178)
tests/runtime/out_logrotate.c (2)
include/fluent-bit/flb_mem.h (1)
flb_free(126-128)src/flb_lib.c (10)
flb_create(143-225)flb_service_set(652-678)flb_input(266-276)flb_input_set(305-335)flb_output(279-289)flb_output_set(520-551)flb_start(983-994)flb_lib_push(843-870)flb_stop(1011-1055)flb_destroy(228-263)
🪛 ast-grep (0.40.0)
tests/runtime/out_logrotate.c
[warning] 473-473: A check is done with stat and then the file is used. There is no guarantee that the status of the file has not changed since the call to stat which may allow attackers to bypass permission checks.
Context: fopen
Note: [CWE-367]: Time-of-check Time-of-use (TOCTOU) Race Condition [REFERENCES]
- https://wiki.sei.cmu.edu/confluence/display/c/FIO45-C.+Avoid+TOCTOU+race+conditions+while+accessing+files
(file-stat-before-action-c)
🔇 Additional comments (1)
plugins/out_logrotate/logrotate.c (1)
73-94: Per-file size tracking and locking look soundThe move to a per-filename size structure plus per-entry mutexes (the
logrotate_file_sizelist andupdate_file_size_counter) fixes the earlier single-counter bug and makes rotation decisions per output file, with clear ownership and cleanup incb_logrotate_exit. This design looks correct and maintainable as-is.Also applies to: 531-660
| /* Function to clean up old rotated files */ | ||
| static int cleanup_old_files(struct flb_logrotate_conf *ctx, | ||
| const char *directory, | ||
| const char *base_filename) | ||
| { | ||
| char pattern[PATH_MAX]; | ||
| char full_path[PATH_MAX]; | ||
| char search_path[PATH_MAX]; | ||
| char **files = NULL; | ||
| int file_count = 0; | ||
| int max_files = ctx->max_files; | ||
| int i, j; | ||
|
|
||
| /* Create pattern to match rotated files */ | ||
| snprintf(pattern, PATH_MAX - 1, "%s.", base_filename); | ||
|
|
||
| #ifdef FLB_SYSTEM_WINDOWS | ||
| HANDLE hFind; | ||
| WIN32_FIND_DATA findData; | ||
|
|
||
| /* Create search path: directory\* */ | ||
| snprintf(search_path, PATH_MAX - 1, "%s" FLB_PATH_SEPARATOR "*", directory); | ||
|
|
||
| hFind = FindFirstFileA(search_path, &findData); | ||
| if (hFind == INVALID_HANDLE_VALUE) { | ||
| return 0; /* Directory doesn't exist or can't be opened */ | ||
| } | ||
|
|
||
| /* Count matching files */ | ||
| do { | ||
| /* Skip . and .. */ | ||
| if (strcmp(findData.cFileName, ".") == 0 || | ||
| strcmp(findData.cFileName, "..") == 0) { | ||
| continue; | ||
| } | ||
| /* Skip directories */ | ||
| if (findData.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) { | ||
| continue; | ||
| } | ||
| if (is_valid_rotation_filename(findData.cFileName, pattern)) { | ||
| file_count++; | ||
| } | ||
| } while (FindNextFileA(hFind, &findData) != 0); | ||
|
|
||
| if (file_count <= max_files) { | ||
| FindClose(hFind); | ||
| return 0; | ||
| } | ||
|
|
||
| /* Allocate array for file names */ | ||
| files = flb_calloc(file_count, sizeof(char *)); | ||
| if (!files) { | ||
| FindClose(hFind); | ||
| return -1; | ||
| } | ||
|
|
||
| /* Collect file names - restart search */ | ||
| FindClose(hFind); | ||
| hFind = FindFirstFileA(search_path, &findData); | ||
| if (hFind == INVALID_HANDLE_VALUE) { | ||
| flb_free(files); | ||
| return -1; | ||
| } | ||
|
|
||
| i = 0; | ||
| do { | ||
| /* Skip . and .. */ | ||
| if (strcmp(findData.cFileName, ".") == 0 || | ||
| strcmp(findData.cFileName, "..") == 0) { | ||
| continue; | ||
| } | ||
| /* Skip directories */ | ||
| if (findData.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) { | ||
| continue; | ||
| } | ||
| if (is_valid_rotation_filename(findData.cFileName, pattern) && | ||
| i < file_count) { | ||
| snprintf(full_path, PATH_MAX - 1, "%s" FLB_PATH_SEPARATOR "%s", | ||
| directory, findData.cFileName); | ||
| files[i] = flb_strdup(full_path); | ||
| i++; | ||
| } | ||
| } while (FindNextFileA(hFind, &findData) != 0 && i < file_count); | ||
|
|
||
| FindClose(hFind); | ||
| #else | ||
| DIR *dir; | ||
| struct dirent *entry; | ||
|
|
||
| dir = opendir(directory); | ||
| if (!dir) { | ||
| return 0; /* Directory doesn't exist or can't be opened */ | ||
| } | ||
|
|
||
| /* Count matching files */ | ||
| while ((entry = readdir(dir)) != NULL) { | ||
| if (is_valid_rotation_filename(entry->d_name, pattern)) { | ||
| file_count++; | ||
| } | ||
| } | ||
|
|
||
| if (file_count <= max_files) { | ||
| closedir(dir); | ||
| return 0; | ||
| } | ||
|
|
||
| /* Allocate array for file names */ | ||
| files = flb_calloc(file_count, sizeof(char *)); | ||
| if (!files) { | ||
| closedir(dir); | ||
| return -1; | ||
| } | ||
|
|
||
| /* Collect file names */ | ||
| rewinddir(dir); | ||
| i = 0; | ||
| while ((entry = readdir(dir)) != NULL && i < file_count) { | ||
| if (is_valid_rotation_filename(entry->d_name, pattern)) { | ||
| snprintf(full_path, PATH_MAX - 1, "%s" FLB_PATH_SEPARATOR "%s", | ||
| directory, entry->d_name); | ||
| files[i] = flb_strdup(full_path); | ||
| i++; | ||
| } | ||
| } | ||
| closedir(dir); | ||
| #endif | ||
|
|
||
| /* Sort files by modification time (oldest first) */ | ||
| for (i = 0; i < file_count - 1; i++) { | ||
| for (j = i + 1; j < file_count; j++) { | ||
| struct stat st1; | ||
| struct stat st2; | ||
| if (stat(files[i], &st1) == 0 && stat(files[j], &st2) == 0) { | ||
| if (st1.st_mtime > st2.st_mtime) { | ||
| char *temp = files[i]; | ||
| files[i] = files[j]; | ||
| files[j] = temp; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /* Remove oldest files */ | ||
| if (file_count > max_files) { | ||
| flb_plg_info(ctx->ins, | ||
| "cleaning up old rotated files: removing %d files (keeping %d)", | ||
| file_count - max_files, max_files); | ||
| } | ||
| for (i = 0; i < file_count - max_files; i++) { | ||
| if (unlink(files[i]) == 0) { | ||
| flb_plg_debug(ctx->ins, "removed old rotated file: %s", files[i]); | ||
| } | ||
| flb_free(files[i]); | ||
| } | ||
|
|
||
| /* Free remaining file names */ | ||
| for (i = file_count - max_files; i < file_count; i++) { | ||
| flb_free(files[i]); | ||
| } | ||
|
|
||
| flb_free(files); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Validate max_files to avoid out-of-bounds access and confusing semantics
cleanup_old_files() assumes ctx->max_files is non‑negative, but it comes directly from configuration (FLB_CONFIG_MAP_INT "max_files"). If a user misconfigures it as a negative value, you can hit undefined behavior:
file_countis>= 0.- For
max_files < 0,file_count <= max_filesis always false, so you go into the cleanup path. - The removal loop uses
for (i = 0; i < file_count - max_files; i++), andfile_count - max_filesbecomes greater thanfile_count, sofiles[i]is indexed out of bounds and freed, both on POSIX and Windows branches.
Even max_files = 0 is a bit surprising: it causes all rotated files to be deleted immediately (and the current file is not counted in file_count), which may not match user expectations.
Consider validating max_files once (e.g., in cb_logrotate_init) and enforcing a sane minimum, or treating non‑positive values as “no cleanup”:
@@ static int cb_logrotate_init(...)
- ctx->ins = ins;
+ ctx->ins = ins;
@@
- ret = flb_output_config_map_set(ins, (void *) ctx);
+ ret = flb_output_config_map_set(ins, (void *) ctx);
if (ret == -1) {
flb_free(ctx);
return -1;
}
+
+ /* Validate rotation limits */
+ if (ctx->max_files <= 0) {
+ flb_plg_error(ctx->ins,
+ "invalid max_files=%d; must be >= 1", ctx->max_files);
+ flb_free(ctx);
+ return -1;
+ }Alternatively, clamp max_files to 1 (or treat <= 0 as “no limit”) before using it in cleanup_old_files(). Either way, this prevents out‑of‑bounds access and makes behavior well‑defined for misconfigurations.
I'll verify this critical issue by examining the codebase structure and validating the concerns raised.
🏁 Script executed:
#!/bin/bash
# Locate the logrotate plugin file and examine configuration setup
find . -name "logrotate.c" -type f 2>/dev/null | head -5Length of output: 35
🏁 Script executed:
#!/bin/bash
# Search for cb_logrotate_init function definition
rg -n "cb_logrotate_init" --type=c -A 20Length of output: 1942
🏁 Script executed:
#!/bin/bash
# Search for max_files configuration definition
rg -n "max_files" --type=c -C 3Length of output: 10481
🏁 Script executed:
#!/bin/bash
# Search for FLB_CONFIG_MAP_INT references related to max_files
rg -n "FLB_CONFIG_MAP_INT.*max_files" --type=c -A 2 -B 2Length of output: 370
🏁 Script executed:
#!/bin/bash
# Examine the cleanup_old_files function more broadly for context
rg -n "cleanup_old_files" --type=c -B 5 -A 50Length of output: 7979
🏁 Script executed:
#!/bin/bash
# Look for any validation checks on max_files after config is loaded
rg -n "max_files.*<|>|<=|>=|==|-1|0" --type=c -C 2 | head -100Length of output: 4390
🏁 Script executed:
#!/bin/bash
# Search for configuration structure definition to understand max_files field
rg -n "struct flb_logrotate_conf" --type=c -A 30Length of output: 33397
🏁 Script executed:
#!/bin/bash
# Check for any guards or conditions on max_files usage
rg -n "if.*max_files|while.*max_files|for.*max_files" --type=c -C 2Length of output: 1741
🏁 Script executed:
#!/bin/bash
# Look for flb_output_config_map_set calls in logrotate context
rg -n "flb_output_config_map_set" --type=c -B 10 -A 5 | grep -A 15 "logrotate"Length of output: 1867
🏁 Script executed:
#!/bin/bash
# Search for default values or initialization of max_files
rg -n "max_files\s*=" --type=c -B 2 -A 2Length of output: 1164
🏁 Script executed:
#!/bin/bash
# Check for FLB_CONFIG_MAP definitions in the file
rg -n "FLB_CONFIG_MAP" --type=c -A 1 -B 1Length of output: 271448
🏁 Script executed:
#!/bin/bash
# Verify the exact line range of cleanup_old_files as cited
sed -n '990,1152p' plugins/out_logrotate/logrotate.c | head -80Length of output: 2437
🏁 Script executed:
#!/bin/bash
# Check the removal loop logic specifically
sed -n '1110,1130p' plugins/out_logrotate/logrotate.cLength of output: 604
🏁 Script executed:
#!/bin/bash
# Look for any post-config validation in init callbacks
rg -n "flb_plg_error.*max" --type=c -C 3Length of output: 10329
🏁 Script executed:
#!/bin/bash
# Search for minimum value enforcement
rg -n "max_files.*<.*1|max_files.*==.*0" --type=c -C 2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Verify if there are integration tests that might show expected behavior
find . -name "*test*logrotate*" -o -name "*logrotate*test*" 2>/dev/null | head -10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for documentation or comments about max_files expectations
rg -n "max_files" --type=c,h,md -B 2 -A 2 | grep -i "doc\|comment\|default\|expect\|valid" | head -30Length of output: 30
🏁 Script executed:
#!/bin/bash
# Check the actual configuration map setup
rg -n "config_map.*=" --type=c -A 50 | grep -B 10 -A 10 "max_files"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Examine the entire flb_logrotate_conf structure
ast-grep --pattern 'struct flb_logrotate_conf {
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Trace where ctx->max_files is assigned or modified
rg -n "ctx->max_files" --type=c -B 3 -A 3Length of output: 1045
🏁 Script executed:
#!/bin/bash
# Get file count to understand scope
wc -l plugins/out_logrotate/logrotate.cLength of output: 38
🏁 Script executed:
#!/bin/bash
# Look specifically at the problematic loop structure
sed -n '1105,1145p' plugins/out_logrotate/logrotate.cLength of output: 1361
🏁 Script executed:
#!/bin/bash
# Final check: confirm file exists and is accessible
ls -lh plugins/out_logrotate/logrotate.c && echo "---" && head -20 plugins/out_logrotate/logrotate.cLength of output: 859
🏁 Script executed:
#!/bin/bash
# Review the entire block around line 1120-1135 for the removal loop
awk 'NR>=1115 && NR<=1140 {print NR": "$0}' plugins/out_logrotate/logrotate.cLength of output: 1074
🏁 Script executed:
#!/bin/bash
# Confirm max_files type in the config structure
rg -n "int.*max_files|max_files.*int" --type=c -B 1 -A 1Length of output: 1037
🏁 Script executed:
#!/bin/bash
# Check if min/max constraints are enforced via config map
rg -n "\.min\s*=|\.max\s*=|\.flags\s*=" --type=c -B 2 -A 2 | grep -B 10 -A 5 "max_files"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Summarize all max_files related code in one comprehensive view
rg -n "max_files" --type=c plugins/out_logrotate/logrotate.cLength of output: 700
🏁 Script executed:
#!/bin/bash
# Check for default value in configuration
grep -n "max_files" plugins/out_logrotate/logrotate.c | grep -i "default\|="Length of output: 355
🏁 Script executed:
#!/bin/bash
# Get the full cleanup_old_files function to verify the complete logic
awk '/^static int cleanup_old_files/,/^}/' plugins/out_logrotate/logrotate.c | tail -100Length of output: 2853
🏁 Script executed:
#!/bin/bash
# Verify the loop bounds calculation
awk '/Remove oldest files/,/Free remaining file names/' plugins/out_logrotate/logrotate.cLength of output: 491
🏁 Script executed:
#!/bin/bash
# Look for the error handling or validation in init
rg -n "max_files.*-1|max_files.*<=.*0" --type=c -B 3 -A 3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check how max_files is read from configuration
rg -n "\.key.*=.*\"max_files\"|FLB_CONFIG_MAP.*max_files" --type=c -B 2 -A 5Length of output: 501
🏁 Script executed:
#!/bin/bash
# Final validation: display removal loop with line numbers
sed -n '1115,1140p' plugins/out_logrotate/logrotate.c | cat -nLength of output: 1100
🏁 Script executed:
#!/bin/bash
# Check cb_logrotate_init for any validation after config load
rg -n "cb_logrotate_init" --type=c -A 50 | head -70Length of output: 4072
🏁 Script executed:
#!/bin/bash
# Look for return -1 or error handling related to max_files
rg -n "max_files" --type=c -A 2 plugins/out_logrotate/logrotate.c | grep -B 5 "error\|return -1\|invalid"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Verify array access patterns in cleanup_old_files
sed -n '990,1152p' plugins/out_logrotate/logrotate.c | grep -n "files\[" Length of output: 497
🏁 Script executed:
#!/bin/bash
# Review the entire removal loop section
sed -n '1120,1145p' plugins/out_logrotate/logrotate.cLength of output: 844
🏁 Script executed:
#!/bin/bash
# Check what happens with max_files value of 0 or negative
rg -n "file_count.*max_files|max_files.*file_count" --type=c -C 3Length of output: 2711
🏁 Script executed:
#!/bin/bash
# Look at the exact point where max_files is used in loop conditions
awk 'NR>=1100 && NR<=1150 {print NR": "$0}' plugins/out_logrotate/logrotate.c | grep -E "for|while|max_files|file_count"Length of output: 396
🏁 Script executed:
#!/bin/bash
# Check the scope and type of max_files in struct definition
grep -A 100 "struct flb_logrotate_conf" plugins/out_logrotate/logrotate.c | grep "max_files" -B 5 -A 2Length of output: 1684
🏁 Script executed:
#!/bin/bash
# Identify all configuration parameter definitions
rg -n "FLB_CONFIG_MAP" plugins/out_logrotate/logrotate.c -A 3Length of output: 2235
🏁 Script executed:
#!/bin/bash
# Final comprehensive check: does any validation exist?
grep -n "if.*max_files" plugins/out_logrotate/logrotate.cLength of output: 118
🏁 Script executed:
#!/bin/bash
# Summarize the configuration setup entry point
rg -n "struct flb_config_map.*logrotate\|static.*flb_config_map" plugins/out_logrotate/logrotate.c -A 30 | head -100Length of output: 0
🏁 Script executed:
#!/bin/bash
# List the entire function signature and start to understand context
sed -n '990,1010p' plugins/out_logrotate/logrotate.cLength of output: 636
🏁 Script executed:
#!/bin/bash
# Display the critical removal loop that performs out-of-bounds access
sed -n '1128,1138p' plugins/out_logrotate/logrotate.cLength of output: 325
🏁 Script executed:
#!/bin/bash
# Check if max_files has any default value set anywhere
grep -rn "max_files" plugins/out_logrotate/ | grep -i "default"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Verify function parameters and return expectations
rg -n "flb_output_plugin_fluentbit_register\|flb_plugin_register" plugins/out_logrotate/logrotate.c -A 20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Final check for critical insight: is max_files ever validated?
grep -E "max_files.*<|max_files.*>|if.*max_files" plugins/out_logrotate/logrotate.cLength of output: 199
🏁 Script executed:
#!/bin/bash
# Look at the complete context where max_files is read from config
rg -n "flb_output_config_map_set" plugins/out_logrotate/logrotate.c -B 10 -A 15Length of output: 882
🏁 Script executed:
#!/bin/bash
# Analyze the exact removal loop logic to confirm the mathematical claim
awk 'NR>=1126 && NR<=1135 {print NR": "$0}' plugins/out_logrotate/logrotate.cLength of output: 326
🏁 Script executed:
#!/bin/bash
# Verify if this is indeed the Windows and POSIX branch implementations
grep -n "#ifdef FLB_SYSTEM_WINDOWS\|#else\|#endif" plugins/out_logrotate/logrotate.c | head -20Length of output: 340
🏁 Script executed:
#!/bin/bash
# Look at initialization defaults
grep -n "max_files.*=" plugins/out_logrotate/logrotate.c | grep -v "//"Length of output: 154
🏁 Script executed:
#!/bin/bash
# Check if there are any safeguards in calling code
rg -n "cleanup_old_files" plugins/out_logrotate/logrotate.c -B 5 -A 5Length of output: 832
🏁 Script executed:
#!/bin/bash
# Comprehensive final verification: print lines 1120-1145 which contain the critical logic
cat plugins/out_logrotate/logrotate.c | sed -n '1120,1145p'Length of output: 844
🏁 Script executed:
#!/bin/bash
# Identify maximum line number in file to verify provided range is valid
tail -20 plugins/out_logrotate/logrotate.c | cat -nLength of output: 724
🏁 Script executed:
#!/bin/bash
# Verify the snippet matches actual code at claimed lines
sed -n '1000,1020p' plugins/out_logrotate/logrotate.c | head -15Length of output: 458
🏁 Script executed:
#!/bin/bash
# Complete search for all instances of max_files with their context
grep -n "max_files" plugins/out_logrotate/logrotate.cLength of output: 700
Validate max_files in init — prevent OOB in cleanup_old_files
ctx->max_files is not validated after config parsing; a negative value makes file_count - max_files > file_count and causes out‑of‑bounds indexing/frees in cleanup_old_files (plugins/out_logrotate/logrotate.c — cleanup_old_files ≈ lines 990–1152). Add a validation in cb_logrotate_init (after flb_output_config_map_set, cb_logrotate_init ≈ lines 115–145). Example fix:
/* Validate rotation limits */
if (ctx->max_files <= 0) {
flb_plg_error(ctx->ins, "invalid max_files=%d; must be >= 1", ctx->max_files);
flb_free(ctx);
return -1;
}Alternatively clamp ctx->max_files to a sane minimum (e.g. 1) or treat <= 0 as “no cleanup” — but do not leave negative values unhandled.
🤖 Prompt for AI Agents
In plugins/out_logrotate/logrotate.c around cleanup_old_files (lines 990–1152)
and cb_logrotate_init (after flb_output_config_map_set, approximately lines
115–145), validate ctx->max_files after parsing the config to prevent negative
or zero values causing out‑of‑bounds indexing in cleanup_old_files; add a check
that if ctx->max_files <= 0 you either log an error and free ctx/return -1, or
clamp it to a sane minimum (e.g. set to 1) or document/treat <=0 as “no cleanup”
by disabling cleanup logic — ensure you emit a flb_plg_error or flb_plg_info
with the chosen action so the user sees the configuration fix.
| static void cb_logrotate_flush(struct flb_event_chunk *event_chunk, | ||
| struct flb_output_flush *out_flush, | ||
| struct flb_input_instance *ins, | ||
| void *out_context, | ||
| struct flb_config *config) | ||
| { | ||
| int ret; | ||
| int column_names; | ||
| FILE *fp; | ||
| size_t off = 0; | ||
| size_t last_off = 0; | ||
| size_t alloc_size = 0; | ||
| size_t total; | ||
| size_t file_size = 0; | ||
| char out_file[PATH_MAX]; | ||
| char *buf; | ||
| char *out_file_copy; | ||
| char directory[PATH_MAX]; | ||
| char base_filename[PATH_MAX]; | ||
| long file_pos; | ||
| bool have_directory; | ||
| struct flb_logrotate_conf *ctx = out_context; | ||
| struct flb_log_event_decoder log_decoder; | ||
| struct flb_log_event log_event; | ||
| struct logrotate_file_size *entry = NULL; | ||
| struct stat st; | ||
| int lock_acquired = 0; | ||
|
|
||
| (void) config; | ||
|
|
||
| /* Set the right output file */ | ||
| if (ctx->out_path) { | ||
| if (ctx->out_file) { | ||
| snprintf(out_file, PATH_MAX - 1, "%s" FLB_PATH_SEPARATOR "%s", | ||
| ctx->out_path, ctx->out_file); | ||
| } | ||
| else { | ||
| snprintf(out_file, PATH_MAX - 1, "%s" FLB_PATH_SEPARATOR "%s", | ||
| ctx->out_path, event_chunk->tag); | ||
| } | ||
| } | ||
| else { | ||
| if (ctx->out_file) { | ||
| snprintf(out_file, PATH_MAX - 1, "%s", ctx->out_file); | ||
| } | ||
| else { | ||
| snprintf(out_file, PATH_MAX - 1, "%s", event_chunk->tag); | ||
| } | ||
| } | ||
|
|
||
| /* Find or create file size entry and acquire lock */ | ||
| entry = find_file_size_entry(ctx, out_file); | ||
| if (entry == NULL) { | ||
| /* Entry doesn't exist yet, create it with initial size 0 */ | ||
| if (update_file_size(ctx, out_file, 0) == 0) { | ||
| entry = find_file_size_entry(ctx, out_file); | ||
| } | ||
| } | ||
|
|
||
| if (entry != NULL) { | ||
| /* Acquire lock before any file operations */ | ||
| if (flb_lock_acquire(&entry->lock, FLB_LOCK_DEFAULT_RETRY_LIMIT, | ||
| FLB_LOCK_DEFAULT_RETRY_DELAY) != 0) { | ||
| flb_plg_error(ctx->ins, "failed to acquire lock for file %s", | ||
| out_file); | ||
| FLB_OUTPUT_RETURN(FLB_ERROR); | ||
| } | ||
| lock_acquired = 1; | ||
| } | ||
|
|
||
| /* Check if file needs rotation based on current size counter */ | ||
| if (entry != NULL) { | ||
| file_size = entry->size; | ||
| } | ||
| else { | ||
| /* Entry doesn't exist, check using stat */ | ||
| if (stat(out_file, &st) == 0 && st.st_size >= 0) { | ||
| file_size = (size_t) st.st_size; | ||
| } | ||
| } | ||
|
|
||
| if (file_size >= ctx->max_size) { | ||
| have_directory = false; | ||
| directory[0] = '\0'; | ||
| /* Extract directory and base filename for cleanup */ | ||
| out_file_copy = flb_strdup(out_file); | ||
| if (out_file_copy) { | ||
| #ifdef FLB_SYSTEM_WINDOWS | ||
| PathRemoveFileSpecA(out_file_copy); | ||
| strncpy(directory, out_file_copy, PATH_MAX - 1); | ||
| directory[PATH_MAX - 1] = '\0'; | ||
| #else | ||
| strncpy(directory, dirname(out_file_copy), PATH_MAX - 1); | ||
| directory[PATH_MAX - 1] = '\0'; | ||
| #endif | ||
| flb_free(out_file_copy); | ||
| have_directory = true; | ||
| } | ||
|
|
||
| /* Get base filename for cleanup */ | ||
| { | ||
| char *last_sep = strrchr(out_file, FLB_PATH_SEPARATOR[0]); | ||
| if (last_sep) { | ||
| strncpy(base_filename, last_sep + 1, PATH_MAX - 1); | ||
| } | ||
| else { | ||
| strncpy(base_filename, out_file, PATH_MAX - 1); | ||
| } | ||
| base_filename[PATH_MAX - 1] = '\0'; | ||
| } | ||
|
|
||
| /* Release lock before rotation (rotate_file will acquire its own) */ | ||
| if (lock_acquired && entry != NULL) { | ||
| flb_lock_release(&entry->lock, FLB_LOCK_DEFAULT_RETRY_LIMIT, | ||
| FLB_LOCK_DEFAULT_RETRY_DELAY); | ||
| lock_acquired = 0; | ||
| } | ||
|
|
||
| /* Rotate the file */ | ||
| if (rotate_file(ctx, out_file) == 0) { | ||
| /* Remove file size entry from list after rotation */ | ||
| remove_file_size(ctx, out_file); | ||
| entry = NULL; /* Entry was removed */ | ||
| /* Clean up old rotated files */ | ||
| if (have_directory) { | ||
| cleanup_old_files(ctx, directory, base_filename); | ||
| } | ||
| } | ||
|
|
||
| /* Re-acquire lock after rotation */ | ||
| entry = find_file_size_entry(ctx, out_file); | ||
| if (entry == NULL) { | ||
| /* Create new entry after rotation */ | ||
| if (update_file_size(ctx, out_file, 0) == 0) { | ||
| entry = find_file_size_entry(ctx, out_file); | ||
| } | ||
| } | ||
| if (entry != NULL) { | ||
| if (flb_lock_acquire(&entry->lock, FLB_LOCK_DEFAULT_RETRY_LIMIT, | ||
| FLB_LOCK_DEFAULT_RETRY_DELAY) != 0) { | ||
| flb_plg_error(ctx->ins, | ||
| "failed to re-acquire lock for file %s", out_file); | ||
| FLB_OUTPUT_RETURN(FLB_ERROR); | ||
| } | ||
| lock_acquired = 1; | ||
| } | ||
| } | ||
|
|
||
| /* Open output file with default name as the Tag */ | ||
| fp = fopen(out_file, "ab+"); | ||
| if (ctx->mkdir == FLB_TRUE && fp == NULL && errno == ENOENT) { | ||
| out_file_copy = flb_strdup(out_file); | ||
| if (out_file_copy) { | ||
| #ifdef FLB_SYSTEM_WINDOWS | ||
| PathRemoveFileSpecA(out_file_copy); | ||
| ret = mkpath(ctx->ins, out_file_copy); | ||
| #else | ||
| ret = mkpath(ctx->ins, dirname(out_file_copy)); | ||
| #endif | ||
| flb_free(out_file_copy); | ||
| if (ret == 0) { | ||
| fp = fopen(out_file, "ab+"); | ||
| } | ||
| } | ||
| } | ||
| if (fp == NULL) { | ||
| flb_errno(); | ||
| flb_plg_error(ctx->ins, "error opening: %s", out_file); | ||
| /* Release lock before returning */ | ||
| if (lock_acquired && entry != NULL) { | ||
| flb_lock_release(&entry->lock, FLB_LOCK_DEFAULT_RETRY_LIMIT, | ||
| FLB_LOCK_DEFAULT_RETRY_DELAY); | ||
| } | ||
| FLB_OUTPUT_RETURN(FLB_ERROR); | ||
| } | ||
|
|
||
| /* Initialize file size counter if this is a new file */ | ||
| if (entry == NULL) { | ||
| /* File not in list, initialize it */ | ||
| update_file_size_counter(ctx, out_file, fp); | ||
| /* Re-find entry after initialization */ | ||
| entry = find_file_size_entry(ctx, out_file); | ||
| if (entry != NULL && !lock_acquired) { | ||
| /* Acquire lock if we didn't have it before */ | ||
| if (flb_lock_acquire(&entry->lock, FLB_LOCK_DEFAULT_RETRY_LIMIT, | ||
| FLB_LOCK_DEFAULT_RETRY_DELAY) == 0) { | ||
| lock_acquired = 1; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /* | ||
| * Get current file stream position, we gather this in case 'csv' format | ||
| * needs to write the column names. | ||
| */ | ||
| file_pos = ftell(fp); | ||
|
|
||
| /* Check if the event type is metrics, handle the payload differently */ | ||
| if (event_chunk->type == FLB_INPUT_METRICS) { | ||
| print_metrics_text(ctx->ins, fp, | ||
| event_chunk->data, event_chunk->size); | ||
| /* Update file size counter - we already hold the lock */ | ||
| if (lock_acquired && entry != NULL) { | ||
| if (fstat(fileno(fp), &st) == 0 && st.st_size >= 0) { | ||
| entry->size = (size_t) st.st_size; | ||
| } | ||
| } | ||
| else { | ||
| update_file_size_counter(ctx, out_file, fp); | ||
| } | ||
| fclose(fp); | ||
| /* Release lock before returning */ | ||
| if (lock_acquired && entry != NULL) { | ||
| flb_lock_release(&entry->lock, FLB_LOCK_DEFAULT_RETRY_LIMIT, | ||
| FLB_LOCK_DEFAULT_RETRY_DELAY); | ||
| } | ||
| FLB_OUTPUT_RETURN(FLB_OK); | ||
| } | ||
|
|
||
| /* | ||
| * Msgpack output format used to create unit tests files, useful for | ||
| * Fluent Bit developers. | ||
| */ | ||
| if (ctx->format == FLB_OUT_LOGROTATE_FMT_MSGPACK) { | ||
| off = 0; | ||
| total = 0; | ||
|
|
||
| do { | ||
| ret = fwrite((char *) event_chunk->data + off, 1, | ||
| event_chunk->size - off, fp); | ||
| if (ret < 0) { | ||
| flb_errno(); | ||
| fclose(fp); | ||
| /* Release lock before returning */ | ||
| if (lock_acquired && entry != NULL) { | ||
| flb_lock_release(&entry->lock, FLB_LOCK_DEFAULT_RETRY_LIMIT, | ||
| FLB_LOCK_DEFAULT_RETRY_DELAY); | ||
| } | ||
| FLB_OUTPUT_RETURN(FLB_RETRY); | ||
| } | ||
| total += ret; | ||
| } while (total < event_chunk->size); | ||
|
|
||
| /* Update file size counter - we already hold the lock */ | ||
| if (lock_acquired && entry != NULL) { | ||
| if (fstat(fileno(fp), &st) == 0 && st.st_size >= 0) { | ||
| entry->size = (size_t) st.st_size; | ||
| } | ||
| } | ||
| else { | ||
| update_file_size_counter(ctx, out_file, fp); | ||
| } | ||
| fclose(fp); | ||
| /* Release lock before returning */ | ||
| if (lock_acquired && entry != NULL) { | ||
| flb_lock_release(&entry->lock, FLB_LOCK_DEFAULT_RETRY_LIMIT, | ||
| FLB_LOCK_DEFAULT_RETRY_DELAY); | ||
| } | ||
| FLB_OUTPUT_RETURN(FLB_OK); | ||
| } | ||
|
|
||
| ret = flb_log_event_decoder_init(&log_decoder, | ||
| (char *) event_chunk->data, | ||
| event_chunk->size); | ||
|
|
||
| if (ret != FLB_EVENT_DECODER_SUCCESS) { | ||
| flb_plg_error(ctx->ins, | ||
| "Log event decoder initialization error : %d", ret); | ||
|
|
||
| /* Update file size counter before closing - we already hold the lock */ | ||
| if (lock_acquired && entry != NULL) { | ||
| if (fstat(fileno(fp), &st) == 0 && st.st_size >= 0) { | ||
| entry->size = (size_t) st.st_size; | ||
| } | ||
| } | ||
| else { | ||
| update_file_size_counter(ctx, out_file, fp); | ||
| } | ||
| fclose(fp); | ||
| /* Release lock before returning */ | ||
| if (lock_acquired && entry != NULL) { | ||
| flb_lock_release(&entry->lock, FLB_LOCK_DEFAULT_RETRY_LIMIT, | ||
| FLB_LOCK_DEFAULT_RETRY_DELAY); | ||
| } | ||
| FLB_OUTPUT_RETURN(FLB_ERROR); | ||
| } | ||
|
|
||
| /* | ||
| * Upon flush, for each array, lookup the time and the first field | ||
| * of the map to use as a data point. | ||
| */ | ||
| while ((ret = flb_log_event_decoder_next( | ||
| &log_decoder, | ||
| &log_event)) == FLB_EVENT_DECODER_SUCCESS) { | ||
| alloc_size = (off - last_off) + 128; /* JSON is larger than msgpack */ | ||
| last_off = off; | ||
|
|
||
| switch (ctx->format){ | ||
| case FLB_OUT_LOGROTATE_FMT_JSON: | ||
| buf = flb_msgpack_to_json_str(alloc_size, log_event.body, | ||
| config->json_escape_unicode); | ||
| if (buf) { | ||
| fprintf(fp, "%s: [%"PRIu64".%09lu, %s]" NEWLINE, | ||
| event_chunk->tag, | ||
| (uint64_t) log_event.timestamp.tm.tv_sec, | ||
| log_event.timestamp.tm.tv_nsec, buf); | ||
| flb_free(buf); | ||
| } | ||
| else { | ||
| /* Update file size counter - we already hold the lock */ | ||
| if (lock_acquired && entry != NULL) { | ||
| if (fstat(fileno(fp), &st) == 0 && st.st_size >= 0) { | ||
| entry->size = (size_t) st.st_size; | ||
| } | ||
| } | ||
| else { | ||
| update_file_size_counter(ctx, out_file, fp); | ||
| } | ||
| flb_log_event_decoder_destroy(&log_decoder); | ||
| fclose(fp); | ||
| /* Release lock before returning */ | ||
| if (lock_acquired && entry != NULL) { | ||
| flb_lock_release(&entry->lock, FLB_LOCK_DEFAULT_RETRY_LIMIT, | ||
| FLB_LOCK_DEFAULT_RETRY_DELAY); | ||
| } | ||
| FLB_OUTPUT_RETURN(FLB_RETRY); | ||
| } | ||
| break; | ||
| case FLB_OUT_LOGROTATE_FMT_CSV: | ||
| if (ctx->csv_column_names == FLB_TRUE && file_pos == 0) { | ||
| column_names = FLB_TRUE; | ||
| file_pos = 1; | ||
| } | ||
| else { | ||
| column_names = FLB_FALSE; | ||
| } | ||
| csv_output(fp, column_names, | ||
| &log_event.timestamp, | ||
| log_event.body, ctx); | ||
| break; | ||
| case FLB_OUT_LOGROTATE_FMT_LTSV: | ||
| ltsv_output(fp, | ||
| &log_event.timestamp, | ||
| log_event.body, ctx); | ||
| break; | ||
| case FLB_OUT_LOGROTATE_FMT_PLAIN: | ||
| plain_output(fp, log_event.body, alloc_size, config->json_escape_unicode); | ||
| break; | ||
| case FLB_OUT_LOGROTATE_FMT_TEMPLATE: | ||
| template_output(fp, | ||
| &log_event.timestamp, | ||
| log_event.body, ctx); | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| flb_log_event_decoder_destroy(&log_decoder); | ||
|
|
||
| /* Update file size counter - we already hold the lock */ | ||
| if (lock_acquired && entry != NULL) { | ||
| if (fstat(fileno(fp), &st) == 0 && st.st_size >= 0) { | ||
| entry->size = (size_t) st.st_size; | ||
| } | ||
| } | ||
| else { | ||
| update_file_size_counter(ctx, out_file, fp); | ||
| } | ||
| fclose(fp); | ||
|
|
||
| /* Release lock before returning */ | ||
| if (lock_acquired && entry != NULL) { | ||
| flb_lock_release(&entry->lock, FLB_LOCK_DEFAULT_RETRY_LIMIT, | ||
| FLB_LOCK_DEFAULT_RETRY_DELAY); | ||
| } | ||
|
|
||
| FLB_OUTPUT_RETURN(FLB_OK); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Consider basic validation of max_size to avoid pathological rotation behavior
cb_logrotate_flush() rotates when file_size >= ctx->max_size, and max_size is read from configuration via FLB_CONFIG_MAP_SIZE. If a user sets max_size to 0, every flush will attempt rotation before any write, leading to repeated failed rename() calls (file may not exist yet) and effectively disabling normal output.
It might be worth rejecting or clamping non‑positive max_size values during init, similar to max_files:
- Enforce
max_size > 0and fail plugin init otherwise, or - Treat
max_size == 0as “no rotation” and short‑circuit the rotation block.
This is not a correctness bug for valid configs, but tightening validation would prevent surprising behavior from simple typos.
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for max_size initialization, configuration, and validation in logrotate plugin
rg -n "max_size" plugins/out_logrotate/ -A 2 -B 2
Length of output: 1691
🏁 Script executed:
#!/bin/bash
# Search for max_files to understand the validation pattern being referenced
rg -n "max_files" plugins/out_logrotate/ -A 2 -B 2Length of output: 3472
🏁 Script executed:
#!/bin/bash
# Find the configuration initialization function and check for existing validation
rg -n "FLB_CONFIG_MAP" plugins/out_logrotate/logrotate.c -A 1 -B 1Length of output: 1353
🏁 Script executed:
#!/bin/bash
# Search for plugin init function to see where validation should occur
rg -n "flb_output_set_context|cb_logrotate_init" plugins/out_logrotate/logrotate.c -A 10 -B 2Length of output: 1201
---
🏁 Script executed:
```shell
#!/bin/bash
# Check if Fluent Bit config parsing enforces constraints on SIZE type
rg -n "FLB_CONFIG_MAP_SIZE" fluent-bit/ -A 2 | head -30
Length of output: 51
🏁 Script executed:
#!/bin/bash
# Search for validation patterns in init functions across output plugins
find plugins/out_* -name "*.c" -exec grep -l "cb_.*_init" {} \; | head -5 | xargs -I {} sh -c 'echo "File: {}" && grep -A 20 "cb_.*_init" {} | grep -E "(max_|config_map|return|if.*[<>=])" | head -10'Length of output: 2283
🏁 Script executed:
#!/bin/bash
# Look at rotate_file function to understand what happens when rotation is attempted
rg -n "rotate_file" plugins/out_logrotate/logrotate.c -A 15 | head -50Length of output: 1577
🏁 Script executed:
#!/bin/bash
# Check if there's any guard that prevents max_size from being used when 0
rg -n "max_size.*0|max_size.*==|if.*max_size" plugins/out_logrotate/logrotate.c -B 1 -A 1Length of output: 229
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and examine the rotate_file function implementation
rg -n "^static.*rotate_file" plugins/out_logrotate/logrotate.c -A 30
Length of output: 2216
🏁 Script executed:
#!/bin/bash
# Check what happens with rename/rotation when file doesn't exist
rg -n "rename\|unlink" plugins/out_logrotate/logrotate.c -B 3 -A 3Length of output: 0
🏁 Script executed:
#!/bin/bash
# Verify the actual behavior: does size_t max_size get initialized properly from FLB_CONFIG_MAP_SIZE
rg -n "flb_config_map_set|flb_config_get" plugins/out_logrotate/logrotate.c -B 2 -A 2Length of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the complete rotate_file implementation
rg -n "^static int rotate_file" plugins/out_logrotate/logrotate.c -A 100 | head -120
Length of output: 3751
🏁 Script executed:
#!/bin/bash
# Check if there are any file operations (rename/move) in rotate_file
rg -n "rename|truncate|unlink" plugins/out_logrotate/logrotate.cLength of output: 252
🏁 Script executed:
#!/bin/bash
# Look for how FLB_CONFIG_MAP_SIZE values are parsed and if constraints are applied
rg -n "FLB_CONFIG_MAP_SIZE|size_t.*=" plugins/out_logrotate/logrotate.c | grep -A2 -B2 "max_size"Length of output: 118
Add max_size validation during plugin initialization to prevent spurious rotation attempts
When max_size is set to 0, the rotation check file_size >= ctx->max_size always evaluates true, triggering rotate_file() on every flush. While rename() fails on non-existent files and the function gracefully handles the error, this causes unnecessary error logs and repeated failed operations before the file is created and written.
Add validation in cb_logrotate_init() to either:
- Reject
max_size <= 0and fail plugin initialization, or - Treat
max_size == 0as "no rotation" and skip the rotation block entirely
This prevents unexpected behavior from configuration typos, similar to defensive checks for other critical parameters.
🤖 Prompt for AI Agents
In plugins/out_logrotate/logrotate.c around lines 1154-1530, the rotation check
uses ctx->max_size directly so a configured max_size of 0 makes the condition
always true and triggers rotate_file() on every flush; add validation in
cb_logrotate_init() to handle invalid/zero values: either (A) fail
initialization when ctx->max_size <= 0 by logging an error and returning
FLB_ERROR, or (B) treat ctx->max_size == 0 as "rotation disabled" by setting a
new flag in the context (e.g., rotation_disabled = true) or assigning a sentinel
(e.g., SIZE_MAX) and update the flush path to skip the rotation block when
rotation is disabled; implement one of these behaviors, ensure you log a clear
message in init, and update cb_logrotate_flush to check the flag/sentinel before
performing any size-based rotation logic.
2 participants
This PR present a new out plugin
out_logrotate. This Will answer #7572 question and solve #7541 suggestion.This feature is highly required in bare-metal or physical machines where you have finite and limited storage.
The plugin is basically a copy of
out_filewith a rotation feature. The reason to do that in additional plugin and not on theout_fileitself is to avoid accidentally turning this feature on and having redundant info/allocs/processing where it is not needed (so normal users can still use the regular file plugin without issue).(this replaces closed #10824 PR)
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:
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
New Features
Tests