gzip: skip FCOMMENT terminator in streaming decompressor#11969
gzip: skip FCOMMENT terminator in streaming decompressor#11969saddamr3e wants to merge 1 commit into
Conversation
Signed-off-by: saddamr3e <saddamr3e@gmail.com>
📝 WalkthroughWalkthroughIn ChangesFCOMMENT skip 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)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
| xlen++; | ||
|
|
There was a problem hiding this comment.
Prevent xlen wraparound in optional-header skipping
Line 810 increments xlen, but xlen is a uint16_t while strnlen() returns size_t. For long FCOMMENT/FNAME fields, truncation can under-skip bytes and misalign parsing. Use size_t for xlen.
Suggested fix
- uint16_t xlen;
+ size_t xlen;🤖 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 `@src/flb_gzip.c` around lines 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.
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
Tests