gzip: skip FCOMMENT terminator in streaming decompressor#11969
gzip: skip FCOMMENT terminator in streaming decompressor#11969saddamr3e wants to merge 2 commits into
Conversation
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)
📝 WalkthroughWalkthrough
ChangesFCOMMENT field fix and regression test
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_gzip.c`:
- Around line 810-811: The variable `xlen` is declared as `uint16_t` but
receives values from `strnlen()` which returns `size_t`. This causes truncation
for large FCOMMENT/FNAME fields, leading to wraparound and misaligned parsing.
Change the declaration of `xlen` from `uint16_t` to `size_t` to properly handle
the full range of values returned by `strnlen()` and prevent wraparound during
the optional-header skipping logic.
🪄 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: 8b42eb2e-887e-401c-96ca-e3025311dffb
📒 Files selected for processing (2)
src/flb_gzip.ctests/internal/gzip.c
strnlen() returns size_t; widening xlen avoids truncation when skipping long FNAME/FCOMMENT fields. Signed-off-by: Saddam <saddamr3e@gmail.com>
The streaming gzip decompressor parses the optional header fields before handing the rest of the stream to inflate. The FNAME branch advances past the field's terminating NUL, but the FCOMMENT branch stops one byte short and leaves the comment's NUL in place. That stray byte then gets treated as the start of the deflate body (or as the FHCRC bytes), so any gzip payload that carries a comment field fails to decompress on this path. in_forward feeds network payloads through this decompressor, so a client that sets the COMMENT flag trips it. I ran into it while building a stream with a comment field and watching
flb_decompressreturn a failure instead of the original data. Incrementingxlenthe same way the FNAME branch does skips the terminator and keeps the read position aligned.Testing
Send a forward message with
compressedset togzipwhere the gzip stream carries an FCOMMENT field. Addedtests/internal/gzip.c:decompress_with_comment, which frames a gzip payload with a comment and runs it throughflb_decompress. It fails on the unpatched tree (the decompressor returns an error and reconstructs nothing) and passes after the change.ok-package-testlabel to test for all targets.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
FCOMMENT, ensuring the decompressor consumes the comment data properly and produces accurate output.Tests
FCOMMENT) into the stream and verifies decompressed bytes match the original payload.