in_systemd: allow a parser to be specified as part of the systemd unit #11353
Conversation
Co-authored-by: Dennis Kaarsemaker <seveas@github.com> Signed-off-by: Nour Douffir <nourdouf@github.com>
📝 WalkthroughWalkthroughAdds parser-aware handling to the systemd input: MESSAGE fields can be dynamically parsed by a named parser, parsed key/value maps are re-encoded into the existing log encoder, and parsing is skipped or fallen back to raw MESSAGE when parser retrieval or parsing fails. Changes
Sequence DiagramsequenceDiagram
participant Journal as Systemd Journal
participant Enum as data_store enumerator
participant Parser as Named Parser
participant Repack as Repack Helper
participant Encoder as Log Encoder
participant Output as Output Buffer
Journal->>Enum: enumerate entry (contains MESSAGE, metadata)
activate Enum
Enum->>Enum: extract fields (skip FLUENT_BIT_PARSER metadata)
alt FLUENT_BIT_PARSER present
Enum->>Parser: lookup named parser
alt parser found
Parser->>Parser: parse MESSAGE -> kv map / error
alt parse success (parsed map)
Parser-->>Repack: return parsed map
Repack->>Encoder: unpack parsed map into encoder
else parse failed
Parser-->>Enum: error
Enum->>Encoder: use original MESSAGE
end
else parser not found
Parser-->>Enum: not found
Enum->>Encoder: use original MESSAGE
end
else no parser configured
Enum->>Encoder: use original MESSAGE
end
Encoder->>Output: emit encoded event
deactivate Enum
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
🤖 Fix all issues with AI agents
In @plugins/in_systemd/systemd.c:
- Around line 421-431: flb_strndup can return NULL; after calling it in the
FLUENT_BIT_PARSER block check if name == NULL and handle the allocation failure
by logging an error via flb_plg_error (e.g. "failed to allocate parser name")
and skip calling flb_parser_get and any further use of name; only call
flb_free(name) if name is non-NULL (or ensure early return/continue so name is
not used afterward). Ensure parser remains NULL when allocation fails so the
existing logic that depends on parser behaves correctly.
- Around line 105-121: The loop assumes source_map.data is a map; add a type
check before iterating: verify source_map.type (or source_map.data.type) equals
MSGPACK_OBJECT_MAP (or the appropriate msgpack type enum used in this codebase)
and only enter the for loop when that check passes; if the check fails, handle
the non-map case (skip iteration or return an error) so accesses to
source_map.data.via.map.size and .ptr are safe and then continue using
flb_log_event_encoder_append_body_msgpack_object as before.
🧹 Nitpick comments (1)
plugins/in_systemd/systemd.c (1)
184-191: Consider documenting the return code convention.The function now returns multiple special codes (
-1,-2,-3). Adding a brief comment documenting these return values would improve maintainability:
0: Success-1: Error-2: Skip entry (no separator found)-3: Parsed content available inout_buf/out_size
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/in_systemd/systemd.c
🧰 Additional context used
🧠 Learnings (1)
📚 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:
plugins/in_systemd/systemd.c
🧬 Code graph analysis (1)
plugins/in_systemd/systemd.c (2)
src/flb_parser.c (2)
flb_parser_do(1044-1066)flb_parser_get(1022-1042)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
🔇 Additional comments (4)
plugins/in_systemd/systemd.c (4)
24-25: LGTM!The new includes are appropriate for the added parser functionality and log event decoding.
217-233: LGTM!The logic to skip
FLUENT_BIT_PARSERmetadata and conditionally apply parsing to theMESSAGEfield is well-structured. The graceful fallback to unparsed content when parsing fails is a good resilience pattern.
513-523: LGTM!The parsed content handling is correct. Memory is properly freed after repacking, and the defensive
pbuf = NULLprevents potential double-free issues.
793-800: LGTM!The test formatter correctly passes
NULLfor parser-related parameters, maintaining backward compatibility. The short-circuit evaluation insystemd_enumerate_data_storeensures no parser logic executes whenparserisNULL.
Testing DetailsHere is valgrind output showing no memory leaks: |
![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: 637a784d62
ℹ️ 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".
This patch addresses three issues: - Add NULL check after flb_strndup to prevent potential segfault when memory allocation fails - Add type validation for msgpack object before accessing map fields to prevent undefined behavior with non-map data - Fix FLUENT_BIT_PARSER counting toward max_fields limit by returning -1 instead of 0 Signed-off-by: Nour Douffir <nourdouf@github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @plugins/in_systemd/systemd.c:
- Around line 524-534: When ret == -3 before calling flb_systemd_repack_map,
check that pbuf is not NULL; if pbuf is NULL, skip repacking (increment no
entries), continue the loop, and avoid calling flb_systemd_repack_map with a
NULL buffer. Specifically, in the branch handling ret == -3 (around variables
pbuf, plength, ret and the call to flb_systemd_repack_map), add a defensive if
(pbuf == NULL) { continue; } (or equivalent cleanup/continue) so
flb_systemd_repack_map and msgpack_unpack_next are never invoked with a NULL
pbuf.
🧹 Nitpick comments (2)
plugins/in_systemd/systemd.c (2)
347-347: Consider initializingplengthfor safety.While
plengthis only used afterflb_parser_dosets it (line 233), initializing it to0at declaration improves defensive programming and makes the code more resilient to future changes.♻️ Proposed initialization
- size_t plength; + size_t plength = 0;
431-431: Consider using a named constant for field offset.The magic number
18represents the length of"FLUENT_BIT_PARSER=". A named constant would improve maintainability.♻️ Proposed refactor using named constant
Add near the top of the file with other constants:
#define FLUENT_BIT_PARSER_FIELD "FLUENT_BIT_PARSER" #define FLUENT_BIT_PARSER_FIELD_LEN 18Then use it:
- ret = sd_journal_get_data(ctx->j, "FLUENT_BIT_PARSER", &data, &length); + ret = sd_journal_get_data(ctx->j, FLUENT_BIT_PARSER_FIELD, &data, &length); if (ret == 0) { - name = flb_strndup((const char *)(data+18), length-18); + name = flb_strndup((const char *)(data + FLUENT_BIT_PARSER_FIELD_LEN), + length - FLUENT_BIT_PARSER_FIELD_LEN);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/in_systemd/systemd.c
🧰 Additional context used
🧠 Learnings (1)
📚 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:
plugins/in_systemd/systemd.c
🧬 Code graph analysis (1)
plugins/in_systemd/systemd.c (1)
src/flb_parser.c (2)
flb_parser_do(1044-1066)flb_parser_get(1022-1042)
🔇 Additional comments (5)
plugins/in_systemd/systemd.c (5)
75-132: LGTM - Type validation is now in place.The type check at line 99 correctly validates that
source_map.data.typeisMSGPACK_OBJECT_MAPbefore the loop accesses map-specific fields. The previous issue flagged in past reviews has been addressed.
223-227: LGTM - Metadata field correctly skipped.The return value
-1ensuresFLUENT_BIT_PARSERis excluded from log content and does not count towardmax_fields(caller continues at line 535 without incrementingentries). This correctly treats the field as metadata-only.
229-239: LGTM - Parser applied conditionally to MESSAGE field.The logic correctly applies the parser only when both a parser is specified and the field is
MESSAGE. Falling through to normal processing if parsing fails is a reasonable fallback.
427-442: LGTM - NULL check now prevents crash.The NULL check at line 432 correctly handles memory allocation failure from
flb_strndup, preventing the segfault that was flagged in previous reviews. The parser gracefully remainsNULLon any error, allowing the MESSAGE field to be processed unparsed.
806-807: Test function correctly passes NULL for new parameters.The test function
cb_systemd_format_testappropriately passesNULLfor the new parser-related parameters. Sincesystemd_enumerate_data_storechecksparserat line 230 before use, this is safe.
| else if (ret == -3) { | ||
| /* Parsed content - add it to encoder as msgpack */ | ||
| ret = flb_systemd_repack_map(ctx->log_encoder, pbuf, plength); | ||
| flb_free(pbuf); | ||
| pbuf = NULL; | ||
| if (ret != FLB_EVENT_ENCODER_SUCCESS) { | ||
| continue; | ||
| } | ||
| entries++; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Add NULL check before repacking parsed content.
pbuf could theoretically be NULL even when ret == -3 if flb_parser_do succeeds but returns a NULL buffer. Passing NULL to flb_systemd_repack_map would cause msgpack_unpack_next to operate on invalid data at line 93.
🐛 Proposed fix to add defensive NULL check
else if (ret == -3) {
/* Parsed content - add it to encoder as msgpack */
+ if (pbuf != NULL) {
- ret = flb_systemd_repack_map(ctx->log_encoder, pbuf, plength);
- flb_free(pbuf);
- pbuf = NULL;
- if (ret != FLB_EVENT_ENCODER_SUCCESS) {
+ ret = flb_systemd_repack_map(ctx->log_encoder, pbuf, plength);
+ flb_free(pbuf);
+ pbuf = NULL;
+ if (ret != FLB_EVENT_ENCODER_SUCCESS) {
+ continue;
+ }
+ entries++;
+ }
+ else {
+ flb_plg_error(ctx->ins, "parser succeeded but returned NULL buffer");
continue;
}
- entries++;
continue;
}🤖 Prompt for AI Agents
In @plugins/in_systemd/systemd.c around lines 524 - 534, When ret == -3 before
calling flb_systemd_repack_map, check that pbuf is not NULL; if pbuf is NULL,
skip repacking (increment no entries), continue the loop, and avoid calling
flb_systemd_repack_map with a NULL buffer. Specifically, in the branch handling
ret == -3 (around variables pbuf, plength, ret and the call to
flb_systemd_repack_map), add a defensive if (pbuf == NULL) { continue; } (or
equivalent cleanup/continue) so flb_systemd_repack_map and msgpack_unpack_next
are never invoked with a NULL pbuf.
|
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 17625 @cosmo0920, here's a summary of the issues I flagged in my reviews of this PR: ✅ Resolved Issues (fixed in commit 1e55072)
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 11305 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 1536 🏁 Script executed: Length of output: 2758 @cosmo0920, I've reviewed my previous comments on this PR. Most of the issues have been addressed, but there is one remaining issue that needs attention: Remaining IssueLocation: Lines 524-534 in When else if (ret == -3) {
/* Parsed content - add it to encoder as msgpack */
ret = flb_systemd_repack_map(ctx->log_encoder, pbuf, plength);
flb_free(pbuf);
pbuf = NULL;Issue: Although Recommended fix: else if (ret == -3) {
/* Parsed content - add it to encoder as msgpack */
if (pbuf == NULL) {
skip_entries++;
continue;
}
ret = flb_systemd_repack_map(ctx->log_encoder, pbuf, plength);
flb_free(pbuf);
pbuf = NULL;
if (ret != FLB_EVENT_ENCODER_SUCCESS) {
continue;
}
entries++;
continue;
}This adds a safety check to prevent potential crashes if Previously Fixed Issues ✅The following issues from my earlier reviews have been successfully addressed:
|
2 participants
Summary
This PR modifies the systemd input to dynamically select the fluent bit parser based on the value of
FLUENT_BIT_PARSERon the log record. This is especially useful because it allows us to specify the parser in the systemd unit file like:This does not fix any known issues/bugs.
This change was originally introduced in this PR: #9567 by
seveas. I have since picked the patch up and fixed conflicts with the latest version. We are currently manually applying this patch when we run fluent-bit on our systems so we would love to get this upstream.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
✏️ Tip: You can customize this high-level summary in your review settings.