-
Couldn't load subscription status.
- Fork 1.8k
wasm: Plug wasm heap leakages #11076
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
WalkthroughThe changes fix a memory leak in the WASM filter by introducing explicit cleanup paths for tag and record buffers allocated during function calls. The fix implements unified error handling with a shared cleanup flow and ensures proper buffer deallocation in both JSON and MSGPACK format variants. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/wasm/flb_wasm.c (3)
278-278: Use array element size for portability.The calculation
sizeof(func_args) / sizeof(uint32_t)assumes the array elements are alwaysuint32_t. For consistency and safety, usesizeof(func_args[0])instead.Apply this diff:
- args_size = sizeof(func_args) / sizeof(uint32_t); + args_size = sizeof(func_args) / sizeof(func_args[0]);
300-310: Consider NULL check after flb_strdup.If
flb_strdupfails due to memory exhaustion, the function returns NULL without logging the specific failure. Consider checkinghost_copyand logging a warning before cleanup.Apply this diff to add explicit error handling:
host_copy = (char *) flb_strdup(func_result); +if (host_copy == NULL) { + flb_error("Failed to duplicate WASM function result to host memory"); + goto cleanup_fail; +} if (fw->tag_buffer) {
382-388: Inconsistent casting with JSON version.This function explicitly casts values to
uint32_twhen populatingfunc_args, while the JSON version at lines 272-277 omits the casts. For consistency, either both should cast or neither should.Also note that line 388 correctly uses
sizeof(func_args[0])which is more portable than the JSON version'ssizeof(uint32_t).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/wasm/flb_wasm.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). (30)
- 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_SIMD=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=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_COVERAGE=On, 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_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 (-DFLB_SANITIZE_MEMORY=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, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=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_THREAD=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=Off, 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_SMALL=On, 3.31.6, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DFLB_JEMALLOC=Off, 3.31.6, gcc, g++)
- GitHub Check: pr-compile-without-cxx (3.31.6)
- 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, 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, gcc, g++, ubuntu-22.04, clang-12)
- GitHub Check: pr-compile-centos-7
- GitHub Check: PR - fuzzing test
🔇 Additional comments (6)
src/wasm/flb_wasm.c (6)
257-264: Good defensive check added.The early function lookup prevents allocating WASM buffers when the target function doesn't exist, avoiding unnecessary cleanup overhead.
280-298: Error handling correctly routes to cleanup.All failure paths after buffer allocation properly jump to the cleanup label, ensuring no leaks occur on any error condition.
311-320: Cleanup path correctly implemented.The failure cleanup properly frees allocated WASM buffers. The duplication with the success path is acceptable given the short length and clarity of control flow.
375-380: Good clarification of data handling.The comment clearly explains why the tag includes the null terminator (+1) while the binary records data does not, reducing confusion for future maintainers.
390-407: Error handling correctly implemented.Consistent with the JSON version, all failure paths properly jump to cleanup, ensuring buffer deallocation.
409-428: Cleanup implementation matches JSON version.The buffer management is correct. The same optional improvement regarding NULL check after
flb_strdup(line 409) applies here as noted for the JSON version.
All of the possibilities to leak Wasm heaps are plugged in this PR.
Closes #11072
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