Skip to content

Conversation

@edsiper
Copy link
Member

@edsiper edsiper commented Oct 27, 2025

Fixes #11068

Restart the journal data cursor before enumerating the fields. Recent libsystemd releases require callers to reset the cursor between entries; otherwise the internal decompression context can be reused in an invalid state, which ultimately triggers a crash in ZSTD_freeDDict().


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
    • Prevented rare crashes and incorrect log processing when reading consecutive system journal entries by resetting internal decompression state between entries, improving stability and accuracy of imports.
    • Reduced risk of runtime symbol conflicts and related crashes by making the bundled static compression library’s symbols internal, improving isolation and reliability.

@coderabbitai
Copy link

coderabbitai bot commented Oct 27, 2025

Walkthrough

Added two resets of the systemd journal data cursor/decompression state in the systemd input plugin and set static libzstd visibility to hidden in the bundled zstd CMake build.

Changes

Cohort / File(s) Change Summary
Systemd input: journal data cursor reset
plugins/in_systemd/systemd.c
Inserted two calls to sd_journal_restart_data(ctx->j) — one immediately after advancing to the next journal entry and one just before enumerating entry fields — to reset the journal data/decompression state before processing entry data.
Bundled zstd: static library symbol visibility
lib/zstd-1.5.7/build/cmake/lib/CMakeLists.txt
Added set_target_properties(libzstd_static PROPERTIES C_VISIBILITY_PRESET hidden VISIBILITY_INLINES_HIDDEN YES) right after add_library(libzstd_static STATIC ...) to hide exported symbols from the static libzstd build.

Sequence Diagram(s)

sequenceDiagram
    participant Reader as in_systemd_collect
    participant Journal as sd_journal

    Note over Reader,Journal #EFEFEF: Per-entry processing (new calls highlighted)
    Reader->>Journal: sd_journal_next() / advance to next entry
    Reader->>Journal: sd_journal_restart_data()    -- reset decompression/cursor
    Reader->>Journal: sd_journal_enumerate_data()  -- enumerate fields
    Reader->>Journal: sd_journal_restart_data()    -- ensure fresh enumeration state
    alt field data present
        Journal-->>Reader: field data items
    else no data / end
        Journal-->>Reader: no more fields
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

  • Heterogeneous changes: one small runtime change in plugin logic, one build-system change affecting symbol visibility and linking.
  • Pay attention to:
    • Correct placement and potential redundant calls of sd_journal_restart_data in plugins/in_systemd/systemd.c.
    • Build implications of hiding symbols in libzstd_static (linking, symbol collisions, consumers that expected visible inlines).
    • Reproduce/test on affected platforms (RHEL10/systemd versions) to validate crash regression.

Poem

🐰 I hop in code with careful paws,
I nudge the journal, mend its flaws.
A hidden coat for zstd's song,
Two tiny taps to keep logs strong.
Happy hops — the system hums along.

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings, 1 inconclusive)
Check name Status Explanation Resolution
Out of Scope Changes Check ⚠️ Warning The PR includes changes to two files: plugins/in_systemd/systemd.c (cursor reset calls) and lib/zstd-1.5.7/build/cmake/lib/CMakeLists.txt (symbol visibility configuration). While the systemd.c changes are clearly aligned with the PR title and objective, the zstd CMakeLists.txt change adding visibility presets is not mentioned in the PR description, which states only that "the patch restarts the systemd journal data cursor before enumerating fields." This zstd build configuration change appears disconnected from the stated objective of cursor reset and lacks clear justification for inclusion in this PR. Either remove the zstd CMakeLists.txt change if it is not necessary for fixing the cursor reset issue, or provide clear documentation explaining how the symbol visibility control is an integral part of this fix and why both changes must be merged together rather than separately.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Linked Issues Check ❓ Inconclusive The PR addresses one key aspect of issue #11068 by implementing defensive checks around sd_journal_enumerate_data handling through cursor reset calls. However, issue #11068 requires comprehensive fixes including chunk integrity validation on load, detection and handling of irrecoverable chunks, comprehensive zstd dict/dctx lifecycle management, and ensuring storage.delete_irrecoverable_chunks semantics are honored. The changes in this PR implement only the cursor reset defensive check and appear to be a partial fix targeting one specific cause of the crash rather than the full set of requirements outlined in the linked issue. Clarify whether this PR is intended as a complete fix for issue #11068 or as a partial fix addressing only the cursor reset aspect. If it is a partial fix, document which requirements are addressed and which are deferred to other PRs. If it is intended to be complete, provide evidence that chunk validation, irrecoverable chunk handling, and zstd lifecycle management are also addressed.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "in_systemd: always reset cursor before iterating fields" is concise, clear, and directly summarizes the main change described in the summary: adding two calls to sd_journal_restart_data(ctx->j) to reset the decompression state before processing entry data and before enumerating fields. The title accurately reflects the primary objective of the patch without being overly vague or misleading.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch in_systemd-reset-cursor

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2a64814 and 6157556.

📒 Files selected for processing (1)
  • lib/zstd-1.5.7/build/cmake/lib/CMakeLists.txt (1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
PR: fluent/fluent-bit#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:

  • lib/zstd-1.5.7/build/cmake/lib/CMakeLists.txt
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
PR: fluent/fluent-bit#10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.

Applied to files:

  • lib/zstd-1.5.7/build/cmake/lib/CMakeLists.txt
📚 Learning: 2025-08-29T06:24:44.797Z
Learnt from: shadowshot-x
PR: fluent/fluent-bit#10794
File: src/aws/flb_aws_compress.c:26-26
Timestamp: 2025-08-29T06:24:44.797Z
Learning: In Fluent Bit, ZSTD support is always available and enabled by default. The build system automatically detects and uses either the system libzstd library or builds the bundled ZSTD version. Unlike other optional dependencies like Arrow which use conditional compilation guards (e.g., FLB_HAVE_ARROW), ZSTD does not require conditional includes or build flags.

Applied to files:

  • lib/zstd-1.5.7/build/cmake/lib/CMakeLists.txt
📚 Learning: 2025-08-29T06:24:55.855Z
Learnt from: shadowshot-x
PR: fluent/fluent-bit#10794
File: src/aws/flb_aws_compress.c:52-56
Timestamp: 2025-08-29T06:24:55.855Z
Learning: ZSTD compression is always available in Fluent Bit and does not require conditional compilation guards. Unlike Arrow/Parquet which use #ifdef FLB_HAVE_ARROW guards, ZSTD is built unconditionally with flb_zstd.c included directly in src/CMakeLists.txt and a bundled ZSTD library at lib/zstd-1.5.7/.

Applied to files:

  • lib/zstd-1.5.7/build/cmake/lib/CMakeLists.txt
⏰ 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). (30)
  • GitHub Check: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-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 64bit, x64, x64-windows-static, 3.31.6)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SMALL=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_ARROW=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_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 (-DFLB_SANITIZE_THREAD=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_MEMORY=On, 3.31.6, gcc, g++)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=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_SIMD=Off, 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_SMALL=On, 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_COVERAGE=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 (-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_JEMALLOC=On, 3.31.6, gcc, g++)
  • GitHub Check: PR - fuzzing test
  • GitHub Check: pr-compile-without-cxx (3.31.6)
  • GitHub Check: pr-compile-centos-7
  • 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-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-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 (1)
lib/zstd-1.5.7/build/cmake/lib/CMakeLists.txt (1)

140-156: Well-motivated symbol visibility control for static bundled zstd.

This change correctly hides all symbols in the static libzstd archive to prevent symbol interposition when libsystemd (linked against system libzstd) loads at runtime. The rationale in the comment block is clear: ABI incompatibility between bundled and system zstd versions can cause crashes during library deinitialization.

The C_VISIBILITY_PRESET hidden and VISIBILITY_INLINES_HIDDEN YES properties are appropriately applied only to the static target; the shared library path remains unchanged and independently configurable. The placement immediately after library creation and before other target configuration is correct.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@edsiper
Copy link
Member Author

edsiper commented Oct 28, 2025

Added another update to avoid zstd exporting symbols...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fluent-bit crashes with a coredump when running on RHEL10

2 participants