http_client: anchor response header lookup to line start - #12269
http_client: anchor response header lookup to line start#12269univbyn-blip wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe HTTP client now matches response headers only at the start of a header line. The lookup continues scanning after embedded matches until it finds a valid line-start match. ChangesHTTP header parsing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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)
src/flb_http_client.c (1)
144-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd regression coverage for the line-start matching contract.
The visible test in
tests/internal/http_client.c(Lines 652-689) checks only a validX-Traceheader. Add a response containingx-goog-stored-content-length: 26\r\nContent-Length: 0\r\nand assert that the parsed value is0. Also test an embedded-only occurrence and assertFLB_HTTP_NOT_FOUND.As per coding guidelines: “Validate both success and failure paths, including invalid payloads, boundary sizes, and null or missing fields.”
🤖 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_http_client.c` around lines 144 - 157, Extend the HTTP client tests around the existing X-Trace coverage to validate line-start header matching: parse a response containing x-goog-stored-content-length followed by Content-Length: 0 and assert the value is 0, then test a response where the requested header appears only embedded in another header and assert FLB_HTTP_NOT_FOUND.Source: Coding guidelines
🤖 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_http_client.c`:
- Around line 151-157: Update the header search loop around strcasestr so
rejected matches advance directly to the next newline rather than incrementing p
by one byte. Preserve matching only at the start of the response or immediately
after '\n', and stop safely at the end of c->resp.data to avoid rescanning the
response body.
---
Nitpick comments:
In `@src/flb_http_client.c`:
- Around line 144-157: Extend the HTTP client tests around the existing X-Trace
coverage to validate line-start header matching: parse a response containing
x-goog-stored-content-length followed by Content-Length: 0 and assert the value
is 0, then test a response where the requested header appears only embedded in
another header and assert FLB_HTTP_NOT_FOUND.
🪄 Autofix
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 Plus
Run ID: fba8c055-3be6-4c23-a296-d3dc163f5dfc
📒 Files selected for processing (1)
src/flb_http_client.c
header_lookup() used an unanchored strcasestr() to find response headers, so a header name that embeds the name of another header is matched by mistake: GCS S3-compatible PutObject 200 responses include 'x-goog-stored-content-length: N' before 'Content-Length: 0', causing content_length to be parsed as N and the client to wait for a body that never arrives - every successful upload was judged as failed and retried, creating duplicate objects. Only accept a header match placed at the beginning of a line. Fixes fluent#12267 Signed-off-by: yena <univbyn@gmail.com>
ec28e4a to
1271795
Compare
1 participant
PR Title
http_client: anchor response header lookup to line start
PR Body
Fixes #12267 (also explains #8525)
header_lookup()located response headers with an unanchoredstrcasestr()over the response buffer. A header whose name embeds thename of another header is matched by mistake: GCS S3-compatible
PutObject 200 responses include
x-goog-stored-content-length: <N>(the size of the stored object, always > 0) before the real
Content-Length: 0. The lookup forContent-Length:matched the tailof
x-goog-stored-content-length:, socontent_lengthwas parsed asNand the client kept waiting for a response body that never arrives.With the default
net.io_timeout 0the wait lasted until the serverreset the idle connection (~4 minutes on GCS), and every successful
upload was reported as a failure (
http_do=-1withHTTP Status: 200)and retried — producing one duplicate object per attempt, indefinitely.
This change only accepts a match placed at the beginning of a line
(start of buffer or right after
\n), so embedded header names areskipped. Behavior for well-formed matches is unchanged.
Verified against a captured GCS PutObject response header block:
before:
content_length = 26(fromx-goog-stored-content-length),after:
content_length = 0(real header). AWS S3-style responses areunaffected (no header embeds another header name).
Testing
Before we can approve your change; please submit the following in a comment:
(예: 여기에 이슈 #12267 의 재현 config / debug 로그를 코멘트로 첨부)
Documentation
Backporting
Summary by CodeRabbit