in_winevtlog: Add text format for event rendering#11448
Conversation
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
📝 WalkthroughWalkthroughAdds a text-based rendering path and associated helpers to the Windows Event Log input plugin, introduces config options and validation for text vs XML rendering, adds UTF-8/guid/sid/time helpers and a text event packer, and unifies memory cleanup across rendering branches. Changes
Sequence Diagram(s)sequenceDiagram
participant EventProvider as "Event Provider"
participant Reader as "winevtlog_read"
participant Renderer as "render_system / get_description"
participant Packer as "winevtlog_pack_text_event"
participant Encoder as "log_encoder"
EventProvider->>Reader: deliver event (system + message + inserts)
Reader->>Renderer: render system metadata & message
Renderer-->>Reader: rendered_system, message, string_inserts
Reader->>Packer: pack text event (system, message, inserts, channel, ctx)
Packer->>Encoder: append key=value payload (and optional StringInserts)
Encoder-->>Reader: ack
Reader->>Reader: free rendered buffers / cleanup
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
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: adada3d79c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@plugins/in_winevtlog/pack.c`:
- Around line 1070-1081: The code unconditionally overwrites ret when calling
flb_log_event_encoder_append_body_string for ctx->render_event_text_key,
ignoring earlier failures from flb_log_event_encoder_begin_record and
flb_log_event_encoder_set_current_timestamp; update the flow so you check ret
after flb_log_event_encoder_begin_record and again after
flb_log_event_encoder_set_current_timestamp and only call
flb_log_event_encoder_append_body_string (for ctx->render_event_text_key and
later for text/out_len) when ret == FLB_EVENT_ENCODER_SUCCESS, preserving and
propagating the first failure instead of clobbering it; reference the ret
variable and the functions flb_log_event_encoder_begin_record,
flb_log_event_encoder_set_current_timestamp, and
flb_log_event_encoder_append_body_string on ctx->log_encoder (and the keys
ctx->render_event_text_key and text/out_len) when making these guard checks.
- Around line 122-143: In append_kv_line, failing flb_sds_cat calls can set
*text to NULL and subsequent flb_sds_cat calls will dereference it; update the
function (append_kv_line) to check the return of each flb_sds_cat invocation
immediately and return -1 on NULL instead of only after the final call, i.e.,
after each assignment like "*text = flb_sds_cat(...)" verify "*text != NULL" and
bail out if it is, so you never call flb_sds_cat with a NULL *text; keep the
existing input NULL checks for text/key and preserve the final newline append
and return codes.
In `@plugins/in_winevtlog/winevtlog.c`:
- Around line 930-946: The code calls render_system_event, get_description, and
get_string_inserts but only frees message and string_inserts inside the if
(rendered_system) block; add cleanup when render_system_event fails by ensuring
message and string_inserts are freed even if rendered_system is NULL (either add
an else branch after if (rendered_system) that flb_free(string_inserts) and
flb_free(message) as needed, or move the frees for string_inserts and message
out of the conditional), referencing render_system_event, get_description,
get_string_inserts, rendered_system, string_inserts, message and
winevtlog_pack_text_event so the allocation/free pairs are balanced.
🧹 Nitpick comments (2)
plugins/in_winevtlog/pack.c (2)
404-490:filetime_to_stringlargely duplicatespack_filetime.The two functions share identical timezone / formatting logic. Consider extracting the shared core into a helper that returns the formatted string, then have both callers use it —
pack_filetimewould pass the result to the encoder, andfiletime_to_stringwould return it to the caller.
585-615: Remove unused static functionuint_to_string_u64.This function has no callers anywhere in the codebase. The text event packer formats numeric fields inline via
_snprintf_sinstead, making this function dead code.
cosmo0920
force-pushed
the
cosmo0920-add-text-rendering-format-on-in_winevtlog
branch
from
3d8b700 to
886cd25
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@plugins/in_winevtlog/pack.c`:
- Around line 873-891: The code in winevtlog_pack_text_event uses
flb_sds_create_size to allocate `text` but assumes it remains non-NULL after
calls to append_kv_line; if append_kv_line hit OOM and flb_sds_cat returned
NULL, `text` can become NULL and later flb_sds_len(text) will dereference NULL.
Fix by checking `text` for NULL after each append_kv_line call (or immediately
before any use like flb_sds_len/text concatenation near where flb_sds_len(text)
is called) and handle the OOM case by freeing/returning early (or propagating
the error). Ensure append_kv_line's contract is respected (it should return NULL
on failure) and add a guard in winevtlog_pack_text_event before using `text`
(and similarly around the block at lines 1074-1078) to avoid dereferencing a
NULL sds.
🧹 Nitpick comments (3)
plugins/in_winevtlog/pack.c (3)
413-499:filetime_to_stringlargely duplicatespack_filetime(~60 lines of identical timezone/formatting logic).The only difference is the output sink:
pack_filetimeappends to the encoder,filetime_to_stringreturns a heap-allocated string. Consider extracting the shared FILETIME→local-time→formatted-string conversion into a single helper that both can call. This would eliminate the maintenance burden of keeping two copies in sync.Also a minor nit:
lenis declaredsize_t(line 420) but receivesintfrom_snprintf_s. The cast on line 482 works in practice (the buffer is large enough that truncation won't occur), but declaringlenasintwould be cleaner and match the return type.Also applies to: 341-411
528-592:LookupAccountSidAshares a singlelenvariable for both account and domain buffer sizes.Line 558 passes
&lenfor bothcchNameandcchReferencedDomainName. The API modifies both on output, so after the calllenholds whichever was written last. This mirrors the same pattern inpack_sid(line 651–652), so it's a pre-existing quirk rather than a new bug. WithMAX_NAME=256it's unlikely to cause issues in practice, but using separate size variables would be more correct.Suggested improvement
- DWORD len = MAX_NAME, err = ERROR_SUCCESS; + DWORD name_len = MAX_NAME; + DWORD domain_len = MAX_NAME; + DWORD err = ERROR_SUCCESS; ... - if (LookupAccountSidA(NULL, sid, account, &len, domain, &len, &sid_type)) { + if (LookupAccountSidA(NULL, sid, account, &name_len, domain, &domain_len, &sid_type)) {
594-624: Remove the unuseduint_to_string_u64function.This function (lines 594–624) is never called anywhere in the codebase. All numeric conversions in
winevtlog_pack_text_eventuse inline_snprintf_swith a localnumbufbuffer, making this function dead code.
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
force-pushed
the
cosmo0920-add-text-rendering-format-on-in_winevtlog
branch
from
886cd25 to
354b663
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@plugins/in_winevtlog/pack.c`:
- Around line 528-592: The LookupAccountSidA call in sid_to_utf8 uses a single
DWORD variable (len) for both the account and domain buffer sizes which is
incorrect; create two separate DWORD variables (e.g., name_len and domain_len)
initialized to MAX_NAME, pass &name_len for the account cchName and &domain_len
for cchReferencedDomainName, and adjust any logic that reads/uses those lengths
accordingly; also apply the same fix to the similar pattern in pack_sid to
ensure both calls use distinct size variables instead of the shared len.
🧹 Nitpick comments (2)
plugins/in_winevtlog/pack.c (2)
413-499:filetime_to_stringduplicates most ofpack_filetime(lines 341–411).The two functions share nearly identical time-zone and formatting logic. Consider extracting the shared conversion into a common helper that both can call, reducing maintenance burden if the format or TZ logic changes.
594-624: Remove unuseduint_to_string_u64function.This function is defined but never called anywhere in the codebase. Consider removing it to eliminate dead code.
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
edsiper
deleted the
cosmo0920-add-text-rendering-format-on-in_winevtlog
branch
2 participants
For alignment of EventViewer format, we need to provide another format for rendering as text which means
key=valuerendered key-value pairs that are concatenate with newlines.This could be improved for the affinity of extracting the similar rendering of EventViewer.
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:
StringInserts is still handled as a separated key-arrayed-values. So, users can easily remove the latter pipeline or just removing with
StringInsert=Off.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
Improvements