in_winevtlog: Plug possible SEGV occurrence paths#10849
Conversation
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
requested review from
edsiper,
fujimotos and
koleini
as code owners
WalkthroughCentralizes string encoding with a new pack_str_codepage, adds pack_astr for ANSI→wide conversion, adds null guards for GUIDs, updates pack_string_inserts to use pack_astr for ANSI types, and makes winevtlog_pack_event Type-aware when packing GUIDs, emitting null strings when Type indicates absence. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant W as winevtlog_pack_event
participant S as pack_string_inserts
participant P as pack_guid
participant C as pack_str_codepage
Note over W: Event packing entry
W->>S: iterate event values
alt value.Type == EVTVarTypeAnsiString
S->>C: pack_astr -> converts ANSI to wide
C-->>S: encoded bytes (via pack_str_codepage)
else value.Type == EVTVarTypeString
S->>C: pack_wstr -> pack_str_codepage(CP_UTF8 or ANSI)
C-->>S: encoded bytes
end
Note over W: GUID fields handling
alt GUID Type != EVTVarTypeNull
W->>P: pack_guid(guid)
alt success
P-->>W: packed GUID string
else failure
P-->>W: indicate failure -> W emits null string
end
else Type == EVTVarTypeNull
W-->>W: emit null string (skip pack_guid)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/in_winevtlog/pack.c (1)
395-397: Fix: EvtVarTypeAnsiString passes char to pack_wstr (expects wchar_t) — potential SEGV**
pack_wstrexpects UTF-16 (wchar_t*). Feedingvalues[i].AnsiStringVal(char*) can misalign reads and crash. Convert ANSI -> UTF-16 (CP_ACP) and then reusepack_wstr, or encode directly to UTF-8.Apply this diff here:
- case EvtVarTypeAnsiString: - if (pack_wstr(ctx, values[i].AnsiStringVal)) { + case EvtVarTypeAnsiString: + if (pack_astr(ctx, values[i].AnsiStringVal)) { pack_nullstr(ctx); } break;Add this helper (place near pack_wstr):
/* Convert ANSI (CP_ACP) char* to UTF-16 and reuse pack_wstr */ static int pack_astr(struct winevtlog_config *ctx, const char *astr) { int wsize; wchar_t *wbuf; if (!astr) { return -1; } wsize = MultiByteToWideChar(CP_ACP, 0, astr, -1, NULL, 0); if (wsize == 0) { return -1; } wbuf = flb_malloc(sizeof(wchar_t) * wsize); if (!wbuf) { flb_errno(); return -1; } if (MultiByteToWideChar(CP_ACP, 0, astr, -1, wbuf, wsize) == 0) { flb_free(wbuf); return -1; } /* pack_wstr handles UTF-16 -> UTF-8/ACP per ctx->use_ansi */ if (pack_wstr(ctx, wbuf)) { flb_free(wbuf); return -1; } flb_free(wbuf); return 0; }
🧹 Nitpick comments (1)
plugins/in_winevtlog/pack.c (1)
45-49: Good null-guard and correct defaultChar type; consider safer fallbackNull check prevents SEGV; switching to
const char*fixes the API type. Minor: replacing unrepresentable chars with a space can hide data. Prefer?orNULL(especially for CP_UTF8 where all code points are representable).Option A (minimal):
- const char *defaultChar = " "; + const char *defaultChar = "?";Option B (avoid substitution for UTF-8):
/* after deciding code_page */ if (code_page == CP_UTF8) { defaultChar = NULL; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/in_winevtlog/pack.c(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-centos-7
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: PR - fuzzing test
🔇 Additional comments (3)
plugins/in_winevtlog/pack.c (3)
117-119: LGTM: Defensive null check in pack_guidPrevents passing NULL to StringFromCLSID. Matches new call-site guards.
649-656: LGTM: Guard ActivityID by variant type before packing GUIDAvoids dereferencing absent GUIDs and aligns with pack_guid’s contract.
661-668: LGTM: Guard RelatedActivityID similarlySymmetric handling prevents invalid GUID packing on null values.
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
plugins/in_winevtlog/pack.c (2)
40-72: Centralized encoder: good; consider propagating encoder errors and avoiding best-fit conversions.
- Wide→multi-byte path looks correct and frees on all error branches.
- Two small improvements:
- Propagate the return value of flb_log_event_encoder_append_body_string so callers can react.
- When emitting CP_ACP, add WC_NO_BEST_FIT_CHARS to prevent silent “?” substitutions.
static int pack_str_codepage(struct winevtlog_config *ctx, const wchar_t *wstr, UINT code_page, BOOL use_ansi) { - UINT cp = use_ansi ? CP_ACP : code_page; - DWORD flags = (cp == CP_UTF8) ? WC_ERR_INVALID_CHARS : 0; + UINT cp = use_ansi ? CP_ACP : code_page; + DWORD flags = (cp == CP_UTF8) ? WC_ERR_INVALID_CHARS : WC_NO_BEST_FIT_CHARS; int size = 0; char *buf = NULL; + int rc = 0; @@ - flb_log_event_encoder_append_body_string(ctx->log_encoder, buf, size - 1); + rc = flb_log_event_encoder_append_body_string(ctx->log_encoder, buf, size - 1); flb_free(buf); - return 0; + return (rc == FLB_EVENT_ENCODER_SUCCESS) ? 0 : -1; }
79-108: ANSI→wide→encoded path looks solid; add debug on conversion failures.Great cleanup and ownership handling. Add minimal diagnostics to ease field triage when MultiByteToWideChar fails.
- wlen = MultiByteToWideChar(CP_ACP, MB_ERR_INVALID_CHARS, astr, -1, NULL, 0); + wlen = MultiByteToWideChar(CP_ACP, MB_ERR_INVALID_CHARS, astr, -1, NULL, 0); if (wlen == 0) { - return -1; + flb_plg_debug(ctx->ins, "pack_astr: preflight conversion failed, err=%lu", GetLastError()); + return -1; } @@ - if (MultiByteToWideChar(CP_ACP, MB_ERR_INVALID_CHARS, astr, -1, wbuf, wlen) == 0) { - flb_free(wbuf); - return -1; - } + if (MultiByteToWideChar(CP_ACP, MB_ERR_INVALID_CHARS, astr, -1, wbuf, wlen) == 0) { + DWORD err = GetLastError(); + flb_free(wbuf); + flb_plg_debug(ctx->ins, "pack_astr: conversion failed, err=%lu", err); + return -1; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugins/in_winevtlog/pack.c(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugins/in_winevtlog/pack.c (1)
include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit, x64, x64-windows-static, 3.31.6)
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 64bit (Arm64), amd64_arm64, -DCMAKE_SYSTEM_NAME=Windows -DCMA...
- GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_COVERAGE=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=On, 3.31.6, clang, clang++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, clang, clang++)
- GitHub Check: pr-compile-centos-7
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-22.04, clang-12)
- GitHub Check: PR - fuzzing test
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, clang, clang++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-22.04, clang-12)
🔇 Additional comments (5)
plugins/in_winevtlog/pack.c (5)
146-149: Good: guard against NULL GUIDs.Prevents StringFromCLSID(NULL) crashes along rare event paths.
424-427: Correct handling of EvtVarTypeAnsiString.Switching to pack_astr removes the prior wchar_t misuse and fixes potential SEGVs on ANSI payloads.
678-685: ActivityID: Type-aware and NULL-safe.Packing only when Type != Null and falling back to empty string avoids derefs on absent GUIDs.
690-697: RelatedActivityID: mirrors the safe pattern.Matches ActivityID handling; consistent and prevents NULL deref.
74-77: No action needed – defaultuse_ansiremainsfalse, sopack_wstrcontinues to emit UTF-8 by default.
| static int pack_astr(struct winevtlog_config *ctx, const char *astr) | ||
| { | ||
| wchar_t *wbuf = NULL; | ||
| int wlen = 0; | ||
| int ret; | ||
|
|
||
| if (!astr) { | ||
| return -1; | ||
| } | ||
|
|
||
| wlen = MultiByteToWideChar(CP_ACP, MB_ERR_INVALID_CHARS, astr, -1, NULL, 0); | ||
| if (wlen == 0) { | ||
| return -1; | ||
| } | ||
|
|
||
| wbuf = flb_malloc(sizeof(wchar_t) * wlen); | ||
| if (!wbuf) { | ||
| flb_errno(); | ||
| return -1; | ||
| } | ||
|
|
||
| if (MultiByteToWideChar(CP_ACP, MB_ERR_INVALID_CHARS, astr, -1, wbuf, wlen) == 0) { | ||
| flb_free(wbuf); | ||
| return -1; | ||
| } | ||
|
|
||
| ret = pack_wstr(ctx, wbuf); | ||
| flb_free(wbuf); | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
NOTE: The Windows API does not provide a function to directly convert from CP_ACP to CP_UTF8.
In practice, the standard procedure is a two-step conversion:
MultiByteToWideChar(CP_ACP, …) → WideCharToMultiByte(CP_UTF8, …).
So, we need to implement nested function like pack_astr calls pack_wstr.
edsiper
deleted the
cosmo0920-plug-missing-null-checks-for-in_winevtlog
branch
2 participants
I checked possible SEGV occurrence paths on in_winetvlog plugin.
And I found some of places could occur SEGV because of NULLs.
So, we need to plug those paths.
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
Bug Fixes
Chores