Skip to content

Conversation

@edsiper
Copy link
Member

@edsiper edsiper commented Dec 4, 2025

Fixes #11238

Changes in this PR:

  • Incomplete error check: only checked ret == -1, but ctr_decode_msgpack_create() can return other error codes. When ctr is NULL on error, this caused NULL pointer dereference.

  • Double-free: called ctr_decode_msgpack_destroy() after successful flb_input_trace_append(), but that function takes ownership and destroys the context internally.


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

Release Notes

  • Bug Fixes
    • Improved error detection and logging for trace processing operations, ensuring more accurate error reporting.

✏️ Tip: You can customize this high-level summary in your review settings.

- Incomplete error check: only checked ret == -1, but ctr_decode_msgpack_create()
  can return other error codes. When ctr is NULL on error, this caused NULL
  pointer dereference.

- Double-free: called ctr_decode_msgpack_destroy() after successful
  flb_input_trace_append(), but that function takes ownership and destroys
  the context internally.

Signed-off-by: Eduardo Silva <[email protected]>
@coderabbitai
Copy link

coderabbitai bot commented Dec 4, 2025

Walkthrough

Fixes a double-free bug in the in_forward plugin's msgpack trace processing by correcting error handling logic and removing redundant cleanup code. The ctrace object ownership is now properly transferred to flb_input_trace_append which assumes destruction responsibility.

Changes

Cohort / File(s) Summary
Trace handling error fix
plugins/in_forward/fw_prot.c
Updated error handling to check for CTR_DECODE_MSGPACK_SUCCESS status; on failure logs with plg_error and returns -1; on success removes explicit ctr_destroy() call, relying on flb_input_trace_append to handle cleanup via ownership transfer.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Key focus areas:
    • Verify that flb_input_trace_append indeed takes ownership and destroys the ctrace object on success
    • Confirm error path handling properly releases resources when decode fails
    • Check that the CTR_DECODE_MSGPACK_SUCCESS check is the correct condition to evaluate

Suggested labels

bug, backport to v4.0.x, backport to v4.1.x

Suggested reviewers

  • cosmo0920
  • fujimotos
  • koleini

Poem

🐰 A double-free no more shall be,
The traces now roam wild and free!
Ownership passed with care so keen,
The cleanest forward flow we've seen! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
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.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and specifically describes the main changes: fixing segfault and double-free issues in trace path handling, which directly aligns with the PR's core objectives.
Linked Issues check ✅ Passed The PR addresses all primary objectives from issue #11238: fixes incomplete error checking for ctr_decode_msgpack_create() failure cases, removes the double-free by eliminating manual destruction after flb_input_trace_append(), and corrects memory ownership semantics.
Out of Scope Changes check ✅ Passed All changes are directly in-scope: modifications to fw_prot.c error handling and ownership semantics address the reported segfault and double-free without introducing unrelated changes.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch in_forward_traces-ownership

📜 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 c88c545 and cd3b6f4.

📒 Files selected for processing (1)
  • plugins/in_forward/fw_prot.c (2 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). (31)
  • 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: pr-windows-build / call-build-windows-package (Windows 32bit, x86, x86-windows-static, 3.31.6)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SIMD=Off, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_COMPILER_STRICT_POINTER_TYPES=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_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 (-DFLB_SIMD=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_COVERAGE=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_MEMORY=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DFLB_SANITIZE_THREAD=On, 3.31.6, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=On, 3.31.6, gcc, g++)
  • 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, clang, clang++)
  • GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_ADDRESS=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 (-DFLB_SMALL=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_JEMALLOC=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_JEMALLOC=On, 3.31.6, gcc, g++)
  • 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-without-cxx (3.31.6)
  • 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, gcc, g++, ubuntu-22.04, clang-12)
  • 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-centos-7
🔇 Additional comments (1)
plugins/in_forward/fw_prot.c (1)

1147-1163: Trace decode error handling and ownership transfer look correct

The updated check against CTR_DECODE_MSGPACK_SUCCESS ensures all decode failures are caught before using ctr, avoiding NULL dereferences. Keeping ctr_decode_msgpack_destroy(ctr) only on flb_input_trace_append() failure and documenting that the success path transfers ownership to flb_input_trace_append() removes the prior double-free without introducing leaks.


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 edsiper merged commit 8cc6da6 into master Dec 5, 2025
106 of 107 checks passed
@edsiper edsiper deleted the in_forward_traces-ownership branch December 5, 2025 23:44
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.

Double free in in_forward when processing msgpack traces

3 participants