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>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds minimum-size validation checks to MessagePack array parsing in ChangesMsgPack Array Bounds Validation
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant pack_print_fluent_record
participant flb_time_pop_from_msgpack
participant flb_metadata_pop_from_msgpack
Caller->>pack_print_fluent_record: parse root array
pack_print_fluent_record->>pack_print_fluent_record: validate root size >= 2
pack_print_fluent_record->>pack_print_fluent_record: validate o array size == 2
pack_print_fluent_record->>flb_time_pop_from_msgpack: pop time
flb_time_pop_from_msgpack-->>pack_print_fluent_record: status
alt time extraction failed
pack_print_fluent_record-->>Caller: return -1
else success
pack_print_fluent_record->>flb_metadata_pop_from_msgpack: pop metadata
flb_metadata_pop_from_msgpack-->>pack_print_fluent_record: status
alt metadata extraction failed
pack_print_fluent_record-->>Caller: return -1
else success
pack_print_fluent_record-->>Caller: continue processing
end
end
Suggested reviewers: 🚥 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
The prototype was added to flb_pack.h only so tests/internal/pack.c could call the function, which made the msgpack-specific signature part of the installed API. Declare it locally in the test instead. Signed-off-by: Saddam <saddamr3e@gmail.com>
|
Fair point on the header. Dropped the prototype from flb_pack.h and declared it locally in tests/internal/pack.c instead, so the helper stays out of the installed API. flb-it-pack and flb-it-flb_time still pass. |
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