in_winevtlog: fix crash when all channels are missing and ignore_missing_channels is enabled - #12318
in_winevtlog: fix crash when all channels are missing and ignore_missing_channels is enabled#12318lecaros wants to merge 2 commits into
Conversation
When ignore_missing_channels is enabled and every channel of an input instance fails to subscribe (e.g. a single configured channel that does not exist on the host), winevtlog_open_all() freed the channel list and returned NULL. The plugin tolerated the NULL on init and kept running with ctx->active_channel == NULL, so the first collection cycle dereferenced a NULL pointer inside mk_list_foreach() and crashed the whole process with no log output. The same NULL list was also passed to winevtlog_close_all() on exit/reload. Return the empty list instead when missing channels are tolerated, so collect/exit iterate it safely, and add NULL guards to in_winevtlog_collect() and winevtlog_close_all() as defense in depth. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: lecaros <lecaros@chronosphere.io>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe winevtlog input now safely handles instances where all configured channels are missing. Collection and shutdown guard empty or null channel lists. Windows runtime tests cover tolerated missing channels, mixed channels, repeated collection, shutdown, and strict failure handling. ChangesWinevtlog empty-channel handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change safely handles missing winevtlog channels without changing documented behavior for other configurations; no actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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: ad6cc24389
ℹ️ 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".
| if (!ctx->active_channel) { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
When ignore_missing_channels is true and winevtlog_open_all() returns NULL for a reason other than an absent channel—for example, query_for_channel() returns -1 for a malformed structured event_query—the unchanged initialization path still accepts that NULL. This guard then makes the collector return forever, so Fluent Bit reports a successful startup while silently ingesting no events. Since the tolerated all-missing case is now represented by a non-NULL empty list, treat a NULL result as fatal during initialization rather than swallowing it here.
Useful? React with 👍 / 👎.
ad6cc24 to
e6cb985
Compare
…annels
Add runtime tests for the winevtlog input (Windows-only build):
- all channels of the instance missing with ignore_missing_channels
The engine must start, survive collection cycles and stop
cleanly (regression test for the NULL active_channel crash)
- multiple missing channels in one instance
- mixed existing ('Application') and missing channels
- missing channel without ignore_missing_channels keeps failing
initialization
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: lecaros <lecaros@chronosphere.io>
e6cb985 to
7134302
Compare
1 participant
When a
winevtloginput hasignore_missing_channels: trueand everyconfigured channel of the instance fails to subscribe (e.g. a single-channel
instance whose channel does not exist on the host),
winevtlog_open_all()frees the channel list and returns NULL.
in_winevtlog_init()tolerates theNULL when the option is set but still registers the collector, so the first
collection cycle iterates a NULL list (
mk_list_foreachdereferenceshead->next) and the whole process crashes with an access violation. Theshutdown path (
in_winevtlog_exit()→winevtlog_close_all(NULL)) crashesthe same way. Running as a Windows service with recovery enabled turns this
into a silent crash/restart loop.
Note: This fix and part of the analysis was done using Fable.
Fixes #12315
Changes:
winevtlog_open_all(): whenignore_missing_channelsis enabled and nochannel could be subscribed, return the empty list instead of NULL so
collect/exit iterate it safely (collecting nothing), which is what the
option promises.
winevtlog_close_all(): NULL guard (protects the exit path).in_winevtlog_collect(): NULL guard onctx->active_channel(defense indepth).
tests/runtime/in_winevtlog.c(registered underFLB_IN_WINEVTLOG, so they only build on Windows):all_channels_missing_ignored— regression test for this crash: startwith only a missing channel, survive collect cycles, stop cleanly.
Crashes the test binary without the fix.
multiple_missing_channels_ignored— several missing channels.mixed_channels_ignored— existing (Application) + missing channelkeeps working (previously-working case, no regression).
missing_channel_fails_without_ignore— without the option, init stillfails as documented.
Behavior without the option is unchanged; the mixed existing+missing case is
unchanged. Only the "option enabled and all channels of the instance
missing" case changes: from a process crash to an idle input.
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:
Without this patch the process dies with an access violation (exit code
0xC0000005) on the first collection cycle (~1s after start) and also onshutdown/hot-reload. With the patch it keeps running, collecting nothing
from that instance, and shuts down cleanly. Control case
(
channels: Application,NonExistentChannel) behaves the same before andafter the patch.
debug
[N/A] Attached Valgrind output that shows no leaks or memory corruption was found
Valgrind is not available on Windows and
in_winevtlogis a Windows-onlyplugin (
FLB_IN_WINEVTLOGis set only incmake/windows-setup.cmake).The change removes a NULL dereference and frees nothing new; the empty
list returned by
winevtlog_open_all()is released by the existingwinevtlog_close_all()call inin_winevtlog_exit().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
[N/A] Documentation required for this feature
No new options or behavior beyond making
ignore_missing_channelsworkas already documented when all channels of an instance are missing.
Backporting
Backport to latest stable release.
Requested: this is a crash-loop fix for a released, documented option;
a separate PR against the current stable branch can follow once this is
approved.
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
Bug Fixes
Tests