validate msgpack record array length before indexing#11973
Conversation
Signed-off-by: saddamr3e <saddamr3e@gmail.com>
Signed-off-by: saddamr3e <saddamr3e@gmail.com>
Signed-off-by: saddamr3e <saddamr3e@gmail.com>
📝 WalkthroughWalkthroughAdds MessagePack array size checks in ChangesMsgPack Array Bounds Validation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/internal/flb_time.c (1)
489-517: ⚡ Quick winAdd pack-path regression coverage alongside the time-path test.
This test validates the
flb_time_pop_from_msgpackfix well, but the PR also hardensflb_metadata_pop_from_msgpack/pack_print_fluent_recordand those paths are not regression-tested here. Adding short-array malformed-shape tests for the pack path would prevent partial regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/internal/flb_time.c` around lines 489 - 517, The test function test_pop_from_msgpack_short_array currently only validates the time-path through pop_from_array calls but does not cover the pack-path functions flb_metadata_pop_from_msgpack and pack_print_fluent_record that were also hardened in the PR. Add additional test cases within this function using the same short-array and malformed-shape msgpack structures (empty array, single element, valid record) but call flb_metadata_pop_from_msgpack and pack_print_fluent_record instead of pop_from_array to provide regression coverage for the pack-path as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/flb_pack.c`:
- Around line 866-872: The shape validation checks for msgpack objects are in
place, but they do not guarantee successful decoding of the timestamp data,
particularly for malformed EXT payloads. The `tms` variable can remain
uninitialized if a pop helper function fails during timestamp decoding. Add
return value checks for the pop helper functions that decode the timestamp from
the array elements validated by the shape checks shown in the diff, and return
early with an error code if any pop operation fails before attempting to use the
decoded `tms` value in subsequent processing.
---
Nitpick comments:
In `@tests/internal/flb_time.c`:
- Around line 489-517: The test function test_pop_from_msgpack_short_array
currently only validates the time-path through pop_from_array calls but does not
cover the pack-path functions flb_metadata_pop_from_msgpack and
pack_print_fluent_record that were also hardened in the PR. Add additional test
cases within this function using the same short-array and malformed-shape
msgpack structures (empty array, single element, valid record) but call
flb_metadata_pop_from_msgpack and pack_print_fluent_record instead of
pop_from_array to provide regression coverage for the pack-path as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 540c26a9-ca81-40b6-8e79-1b457c2b0d3f
📒 Files selected for processing (3)
src/flb_pack.csrc/flb_time.ctests/internal/flb_time.c
pack_print_fluent_record() ignored the return values of flb_time_pop_from_msgpack() and flb_metadata_pop_from_msgpack(), so a short or malformed record array would reject in the helpers yet still print with an undecoded timestamp. Check both returns and stop on failure. Add a regression test for flb_metadata_pop_from_msgpack() covering empty, single-element and short inner arrays, and declare the function in flb_pack.h so the test can call it. Signed-off-by: Saddam <saddamr3e@gmail.com>
|
Also added pack-path regression coverage in tests/internal/pack.c: test_metadata_pop_from_msgpack_short_array exercises flb_metadata_pop_from_msgpack with empty, single-element and short-inner-array records (it was non-static but had no prototype, so I added one in flb_pack.h). And pack_print_fluent_record now checks both pop return values so a rejected record no longer prints an undecoded timestamp. flb-it-pack and flb-it-flb_time both pass under -DSANITIZE_ADDRESS=On. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
include/fluent-bit/flb_pack.h (1)
107-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid making this helper part of the installed API just for test coverage.
The PR notes this declaration was added so
tests/internal/pack.ccan call the function, but putting it ininclude/fluent-bit/flb_pack.hmakes the msgpack-specific signature part of the public ABI. Prefer a private/internal header (or a test-local forward declaration) so this helper can still evolve without creating an external contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/fluent-bit/flb_pack.h` around lines 107 - 108, The flb_metadata_pop_from_msgpack helper should not be exposed through the installed public header, since its msgpack-specific signature would become part of the external API. Move the declaration out of flb_pack.h into a private/internal header or use a test-local forward declaration in tests/internal/pack.c, and keep the helper reachable there without changing the public ABI.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@include/fluent-bit/flb_pack.h`:
- Around line 107-108: The flb_metadata_pop_from_msgpack helper should not be
exposed through the installed public header, since its msgpack-specific
signature would become part of the external API. Move the declaration out of
flb_pack.h into a private/internal header or use a test-local forward
declaration in tests/internal/pack.c, and keep the helper reachable there
without changing the public ABI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb537bdd-6a83-431a-a30a-58907df38f92
📒 Files selected for processing (3)
include/fluent-bit/flb_pack.hsrc/flb_pack.ctests/internal/pack.c
🚧 Files skipped from review as they are similar to previous changes (1)
- src/flb_pack.c
1 participant
flb_time_pop_from_msgpack() validates that the inner timestamp array has
two elements but never checks the outer record array before dereferencing
it:
A record array with fewer than two elements ([] or a lone timestamp)
reads ptr[0]/ptr[1] past the array. flb_metadata_pop_from_msgpack() and
the pack_print_fluent_record() helper have the same gap. The count is
validated in these callees because they sit behind the storage_backlog
reload path and the out_lib/library entrypoints.
Added a regression test under tests/internal/flb_time.c; before the fix
it pops a 1-element array and reads a garbage timestamp, after the fix
the short arrays are rejected. The internal suite passes under
-DSANITIZE_ADDRESS=On.
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:
[N/A] Example configuration file for the change
[N/A] Debug log output from testing the change
[N/A] Attached Valgrind output that shows no leaks or memory corruption was found
[N/A] Run local packaging test showing all targets (including any new ones) build.
[N/A] Set
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
Tests