wasm: Plug wasm heap leakages #11076
Conversation
cosmo0920
requested review from
edsiper,
fujimotos and
koleini
as code owners
WalkthroughPre-lookup of the target WASM function, unified argument construction, and consolidated cleanup paths were added. Tag and record buffers are explicitly freed on all success and failure paths for both JSON and MSGPACK variants; returned strings are validated, copied to host memory, and buffers cleaned before returning. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Host as Fluent‑Bit (host)
participant Wasm as WASM runtime/module
rect rgba(56,165,255,0.06)
Host->>Wasm: lookup function (by name)
alt function not found
Wasm-->>Host: NULL
Host-->>Host: goto cleanup_fail (early return)
else function found
Wasm-->>Host: func_ptr
end
end
rect rgba(34,197,94,0.06)
Host->>Host: build func_args[6], compute args_size
Host->>Wasm: duplicate tag_buffer, record_buffer
Host->>Wasm: call function with args
alt call failure
Wasm-->>Host: error
Host-->>Host: goto cleanup_fail
else call success
Wasm-->>Host: return string address (addr) or NULL
Host->>Host: validate addr, convert to native pointer
Host->>Host: host_copy result (duplicate into host memory)
Host-->>Wasm: free tag_buffer, free record_buffer
Host-->>Host: return host_copy
end
end
rect rgba(239,68,68,0.06)
note right of Host: cleanup_fail frees any allocated buffers\nand ensures no leak before returning NULL
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ 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 |
![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
🧹 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.
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
cosmo0920
force-pushed
the
cosmo0920-plug-wasm-heap-leaks
branch
from
372fba6 to
6156d1a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/wasm/flb_wasm.c (2)
300-320: Consider consolidating cleanup logic and checking strdup failure.The cleanup code (freeing tag_buffer and record_buffer) is duplicated between the success path (lines 302-309) and the cleanup_fail path (lines 312-319). Additionally,
flb_strdupat line 300 can fail and return NULL, but this isn't distinguished from other error cases.Apply this refactor to reduce duplication:
func_result = wasm_runtime_addr_app_to_native(fw->module_inst, func_args[0]); if (func_result == NULL) { goto cleanup_fail; } host_copy = (char *) flb_strdup(func_result); - - if (fw->tag_buffer) { - wasm_runtime_module_free(fw->module_inst, fw->tag_buffer); - fw->tag_buffer = 0; - } - if (fw->record_buffer) { - wasm_runtime_module_free(fw->module_inst, fw->record_buffer); - fw->record_buffer = 0; - } - return host_copy; + /* Fall through to cleanup */ + cleanup_fail: if (fw->tag_buffer) { wasm_runtime_module_free(fw->module_inst, fw->tag_buffer); fw->tag_buffer = 0; } if (fw->record_buffer) { wasm_runtime_module_free(fw->module_inst, fw->record_buffer); fw->record_buffer = 0; } - return NULL; + return host_copy; }This ensures cleanup always happens and simplifies the logic.
409-428: Same cleanup duplication as JSON variant.The cleanup logic is duplicated between success (lines 410-417) and failure (lines 420-427) paths. Consider applying the same consolidation pattern suggested for the JSON variant to reduce maintenance burden and ensure consistency.
Apply the same refactoring pattern as suggested for
flb_wasm_call_function_format_json:func_result = wasm_runtime_addr_app_to_native(fw->module_inst, func_args[0]); if (func_result == NULL) { goto cleanup_fail; } host_copy = (char *) flb_strdup(func_result); - if (fw->tag_buffer) { - wasm_runtime_module_free(fw->module_inst, fw->tag_buffer); - fw->tag_buffer = 0; - } - if (fw->record_buffer) { - wasm_runtime_module_free(fw->module_inst, fw->record_buffer); - fw->record_buffer = 0; - } - return host_copy; + /* Fall through to cleanup */ + cleanup_fail: if (fw->tag_buffer) { wasm_runtime_module_free(fw->module_inst, fw->tag_buffer); fw->tag_buffer = 0; } if (fw->record_buffer) { wasm_runtime_module_free(fw->module_inst, fw->record_buffer); fw->record_buffer = 0; } - return NULL; + return host_copy; }
📜 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 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: 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_SANITIZE_MEMORY=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, gcc, g++)
- GitHub Check: run-ubuntu-unit-tests (-DSANITIZE_UNDEFINED=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 (-DFLB_SIMD=Off, 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_SIMD=Off, 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_SMALL=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 (-DSANITIZE_ADDRESS=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, 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, clang, clang++)
- 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, clang, clang++, ubuntu-22.04, clang-12)
- 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-compile-system-libs (-DFLB_PREFER_SYSTEM_LIBS=On, 3.31.6, gcc, g++, ubuntu-24.04, clang-14)
- GitHub Check: pr-compile-centos-7
- GitHub Check: pr-compile-without-cxx (3.31.6)
- GitHub Check: PR - fuzzing test
🔇 Additional comments (2)
src/wasm/flb_wasm.c (2)
257-264: Excellent optimization: pre-lookup prevents buffer leaks.Moving the function lookup before buffer allocation is a smart improvement. If the function doesn't exist, we now return early without allocating buffers, eliminating a leak scenario.
375-388: Helpful clarification on tag vs records treatment.The comment explaining that tag is a C string (requiring +1 for null terminator) while records is binary data (no +1) is valuable for maintainability, especially given the context-specific handling.
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