fix: properly handle IPv6 addresses in HTTP Host headers #11181
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds bracket-aware IPv6 validation and parsing to URL split routines, updates Host header composition to use runtime TLS status and bracket unbracketed IPv6 addresses (with zone-ID stripping), adds Windows IPv6 includes, and expands tests for IPv6/IPv4 Host header and URL parsing variations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test/Caller
participant URL as flb_utils_url_split*
participant Val as validate_ipv6_brackets()
participant Conn as flb_http_client / Connection
participant Host as Host Header Composer
T->>URL: parse URL (may contain IPv6)
URL->>Val: validate bracket usage if IPv6-like
alt invalid brackets
Val-->>URL: error
URL-->>T: return error (cleanup)
else valid or not IPv6
Val-->>URL: bracket info
URL-->>T: proto, host, port, uri (bracket-aware)
T->>Conn: open connection (check runtime TLS flag)
Conn->>Host: compose Host header (calls flb_stream_get_flag_status)
alt unbracketed IPv6 detected
Host-->>Conn: strip zone ID if present, bracket address, include port per rules
else IPv4/domain/already-bracketed
Host-->>Conn: format host[:port] (omit default TLS port when applicable)
end
Conn-->>T: connection + Host header ready
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
🧹 Nitpick comments (1)
tests/internal/http_client.c (1)
469-493: Good: Centralized Host header validation with cleanup.The
check_host_headerhelper centralizes Host header verification logic and handles cleanup properly when checks fail. Theextra_cleanupparameter allows proper resource deallocation for TLS test scenarios.Minor suggestion: Consider using a more specific type for
extra_cleanupinstead ofvoid *to improve type safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/flb_http_client.c(2 hunks)src/flb_utils.c(8 hunks)tests/internal/http_client.c(2 hunks)tests/internal/utils.c(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:24:55.855Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:52-56
Timestamp: 2025-08-29T06:24:55.855Z
Learning: ZSTD compression is always available in Fluent Bit and does not require conditional compilation guards. Unlike Arrow/Parquet which use #ifdef FLB_HAVE_ARROW guards, ZSTD is built unconditionally with flb_zstd.c included directly in src/CMakeLists.txt and a bundled ZSTD library at lib/zstd-1.5.7/.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:24:26.170Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:39-42
Timestamp: 2025-08-29T06:24:26.170Z
Learning: In Fluent Bit, ZSTD compression support is enabled by default and does not require conditional compilation guards (like #ifdef FLB_HAVE_ZSTD) around ZSTD-related code declarations and implementations.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components, ZSTD support is always available and doesn't need build-time conditionals.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:24:44.797Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:26-26
Timestamp: 2025-08-29T06:24:44.797Z
Learning: In Fluent Bit, ZSTD support is always available and enabled by default. The build system automatically detects and uses either the system libzstd library or builds the bundled ZSTD version. Unlike other optional dependencies like Arrow which use conditional compilation guards (e.g., FLB_HAVE_ARROW), ZSTD does not require conditional includes or build flags.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-09-08T11:21:33.975Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 10851
File: include/fluent-bit/flb_simd.h:60-66
Timestamp: 2025-09-08T11:21:33.975Z
Learning: Fluent Bit currently only supports MSVC compiler on Windows, so additional compiler compatibility guards may be unnecessary for Windows-specific code paths.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Applied to files:
src/flb_http_client.c
🧬 Code graph analysis (3)
src/flb_http_client.c (1)
src/flb_sds.c (2)
flb_sds_copy(260-281)flb_sds_printf(336-387)
src/flb_utils.c (2)
src/flb_sds.c (2)
flb_sds_create_len(58-76)flb_sds_create(78-90)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
tests/internal/http_client.c (4)
src/flb_http_client.c (3)
flb_http_get_header(1015-1032)flb_http_client_destroy(1701-1708)flb_http_client(827-872)include/fluent-bit/flb_mem.h (2)
flb_free(126-128)flb_calloc(84-96)src/flb_upstream.c (2)
flb_upstream_destroy(656-698)flb_upstream_create(290-363)src/flb_sds.c (1)
flb_sds_destroy(389-399)
🔇 Additional comments (7)
src/flb_utils.c (4)
1441-1462: LGTM!The IPv6 bracket validation function correctly handles the primary error cases: missing closing bracket, empty brackets, and closing bracket without opening bracket. The logic is straightforward and focused.
1494-1504: Good: Bracket-aware port parsing prevents false positives.The logic correctly uses the bracket position to determine where to search for the port separator. This prevents incorrectly treating colons within an IPv6 address as port separators.
1536-1547: Essential: NULL checks prevent use-after-free.The added NULL checks after
flb_copy_hostcalls are critical for handling malformed input gracefully. Without these checks, a NULL host pointer would propagate to output parameters, potentially causing crashes.
1568-1580: Improved: Comprehensive error cleanup prevents memory leaks.The enhanced error handling properly frees all allocated resources (host, port, uri) when URL parsing fails. This ensures no memory leaks occur on error paths.
tests/internal/utils.c (1)
39-56: Excellent: Comprehensive IPv6 test coverage.The added test cases thoroughly cover IPv6 URL parsing scenarios including:
- Basic bracketed IPv6 addresses with and without ports
- Full and compressed IPv6 formats
- IPv4-mapped IPv6 addresses
- Edge cases with malformed brackets (missing opening, missing closing, empty brackets)
- Query strings containing brackets
This test suite will help catch regressions in IPv6 URL parsing logic.
src/flb_http_client.c (1)
621-638: Good: Improved Host header construction with IPv6 support.The changes correctly handle IPv6 addresses in Host headers:
- Uses runtime TLS status check (
flb_stream_get_flag_status) for more reliable detection- Properly detects unbracketed IPv6 addresses using
inet_pton- Wraps IPv6 addresses with brackets when combined with port numbers
- Preserves existing behavior for IPv4, domain names, and already-bracketed addresses
Note:
inet_ptonbehavior with zone IDs (e.g., "fe80::1%eth0") varies by platform. The test suite should verify this edge case works correctly.tests/internal/http_client.c (1)
557-663: Excellent: Comprehensive Host header test coverage.The test suite thoroughly covers Host header formatting across multiple scenarios:
- IPv6 addresses (unbracketed, bracketed, various formats, zone IDs)
- IPv4 addresses and domain names
- TLS vs non-TLS with default and non-standard ports
- Edge cases like port 443 without TLS
This extensive coverage ensures the Host header logic works correctly across different configurations and will catch regressions.
ShelbyZ
force-pushed
the
ipv6-changes
branch
from
a979de3 to
bcdd0dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/flb_http_client.c (1)
585-645: IPv6-aware Host header construction and TLS check look correct; consider bracketing IPv6 also for TLS+443The updated
add_host_and_content_length()logic looks solid overall:
- Uses
flb_stream_get_flag_status(&u->base, FLB_IO_TLS)to decide TLS, which is more accurate than relying onc->flags.- For non‑TLS or non‑443, it correctly detects unbracketed IPv6 via
inet_pton(AF_INET6, ...)and emitsHost: [addr]:port, while keeping existing%s:%ibehavior for IPv4, domain names, or already-bracketed IPv6.- Error handling and SDS usage (
flb_sds_create_size,flb_sds_copy,flb_sds_printf) are safe, with proper cleanup on failure.One refinement you might consider: in the TLS+443 branch you currently copy
out_hostas‑is, so an unbracketed IPv6 literal would still produceHost: ::1(no brackets). If you want Host to be fully RFC‑compliant for IPv6 even when the port is omitted, you could reuse the sameinet_pton(AF_INET6, ...)check there and emit[addr]instead ofaddr. For example:- /* Check if connection uses TLS and port is 443 (HTTPS default) */ - if (flb_stream_get_flag_status(&u->base, FLB_IO_TLS) && out_port == 443) { - /* HTTPS on default port 443 - omit port from Host header */ - tmp = flb_sds_copy(host, out_host, strlen(out_host)); - } - else { + /* Check if connection uses TLS and port is 443 (HTTPS default) */ + if (flb_stream_get_flag_status(&u->base, FLB_IO_TLS) && out_port == 443) { + struct in6_addr addr; + + /* For IPv6, still emit brackets even when omitting the port */ + if (out_host && out_host[0] != '[' && + inet_pton(AF_INET6, out_host, &addr) == 1) { + tmp = flb_sds_printf(&host, "[%s]", out_host); + } + else { + tmp = flb_sds_copy(host, out_host, strlen(out_host)); + } + } + else { struct in6_addr addr; /* Check if out_host is an unbracketed IPv6 address */ if (out_host && out_host[0] != '[' && inet_pton(AF_INET6, out_host, &addr) == 1) { /* IPv6 address needs brackets when combined with port */ tmp = flb_sds_printf(&host, "[%s]:%i", out_host, out_port); } else { /* IPv4 address, domain name, or already bracketed IPv6 */ tmp = flb_sds_printf(&host, "%s:%i", out_host, out_port); } }This keeps your new behavior but tightens IPv6 Host header formatting in the HTTPS default‑port case as well.
src/flb_utils.c (1)
1464-1583: Bracket-aware URL splitting and cleanup improvements are solid; minor behavior note on ‘]’ in pathsThe updates to
flb_utils_url_split()andflb_utils_url_split_sds()look good:
- They now:
- Validate IPv6 brackets up front via
validate_ipv6_brackets().- Use the returned
bracketposition to search for the port:only after the closing], otherwise from the start of the host, and ensure any candidate:comes before the first/.- Delegate host extraction to
flb_copy_host()/flb_utils_copy_host_sds(), so IPv6 literals are stored without brackets while other hosts are unchanged.- Allocate
protocol,host,port, anduricarefully and free all of them on every error path (both string and SDS variants), eliminating previous leak risks.The only behavioral nuance to be aware of is that
validate_ipv6_brackets()operates on the entire remainder of the URL starting atp, so a URL that contains a]character in the path or query (with no opening[) will now be rejected, whereas it may have been accepted before. If you later decide URLs likehttp://example.com/a]bshould be allowed, you could tighten the validator to only search for brackets up to the first/(or passsepinto it); otherwise this stricter validation is reasonable for most configurations.Also applies to: 1585-1729
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/flb_http_client.c(2 hunks)src/flb_network.c(1 hunks)src/flb_utils.c(8 hunks)tests/internal/http_client.c(2 hunks)tests/internal/utils.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/internal/utils.c
- tests/internal/http_client.c
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-09-08T11:21:33.975Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 10851
File: include/fluent-bit/flb_simd.h:60-66
Timestamp: 2025-09-08T11:21:33.975Z
Learning: Fluent Bit currently only supports MSVC compiler on Windows, so additional compiler compatibility guards may be unnecessary for Windows-specific code paths.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:24:26.170Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:39-42
Timestamp: 2025-08-29T06:24:26.170Z
Learning: In Fluent Bit, ZSTD compression support is enabled by default and does not require conditional compilation guards (like #ifdef FLB_HAVE_ZSTD) around ZSTD-related code declarations and implementations.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:24:55.855Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:52-56
Timestamp: 2025-08-29T06:24:55.855Z
Learning: ZSTD compression is always available in Fluent Bit and does not require conditional compilation guards. Unlike Arrow/Parquet which use #ifdef FLB_HAVE_ARROW guards, ZSTD is built unconditionally with flb_zstd.c included directly in src/CMakeLists.txt and a bundled ZSTD library at lib/zstd-1.5.7/.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:24:44.797Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:26-26
Timestamp: 2025-08-29T06:24:44.797Z
Learning: In Fluent Bit, ZSTD support is always available and enabled by default. The build system automatically detects and uses either the system libzstd library or builds the bundled ZSTD version. Unlike other optional dependencies like Arrow which use conditional compilation guards (e.g., FLB_HAVE_ARROW), ZSTD does not require conditional includes or build flags.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components, ZSTD support is always available and doesn't need build-time conditionals.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Applied to files:
src/flb_http_client.c
🧬 Code graph analysis (2)
src/flb_utils.c (2)
src/flb_sds.c (2)
flb_sds_create_len(58-76)flb_sds_create(78-90)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
src/flb_http_client.c (1)
src/flb_sds.c (2)
flb_sds_copy(260-281)flb_sds_printf(336-387)
🔇 Additional comments (3)
src/flb_network.c (1)
30-36: Windows IPv6 header include looks correctAdding
<ws2tcpip.h>alongside<winsock2.h>underFLB_SYSTEM_WINDOWSmatches the existing use ofinet_pton/struct in6_addrin this file and aligns with the HTTP client changes; no further changes needed.src/flb_http_client.c (1)
36-43: Windows networking + compatibility includes are appropriateGuarding
<winsock2.h>and<ws2tcpip.h>withFLB_SYSTEM_WINDOWShere, and addingflb_compat.h, is consistent withflb_network.cand ensuresinet_pton/struct in6_addrare available on Windows without extra compiler-condition hacks (which matches the MSVC-only support model). Based on learnings.src/flb_utils.c (1)
1428-1462: IPv6 SDS host copy fix and bracket validator look correctThe
flb_utils_copy_host_sds()length change topos_end - pos_init - 2(IPv6) andpos_end - pos_init(non-IPv6) fixes the earlier off‑by‑one for non‑zeropos_initwhile preserving the existing behavior for the current callers (pos_init == 0). The newvalidate_ipv6_brackets()helper cleanly rejects malformed IPv6 host syntax (missing/empty brackets or stray]without[) and exposes the closing bracket position for later parsing, without introducing leaks or side effects.
ShelbyZ
force-pushed
the
ipv6-changes
branch
from
bcdd0dd to
347df83
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/flb_http_client.c (1)
626-643: Host header composition correctly handles TLS defaults and IPv6 bracketingThe updated
add_host_and_content_length():
- Uses
flb_stream_get_flag_status(&u->base, FLB_IO_TLS)plusout_port == 443to decide when to omit the port (HTTPS default), which is more robust than relying onc->flags.- For non‑default ports, detects unbracketed IPv6 literals via
AF_INET6and formatsHostas[%s]:%i, while leaving IPv4, domain names, and already‑bracketed IPv6 as%s:%i.This preserves existing behavior for IPv4 and domains, fixes the broken IPv6+port Host header, and keeps memory handling around the temporary SDS buffer correct. As a possible future enhancement (not required for this fix), you could consider also bracketing IPv6 literals when the port is omitted (e.g.,
Host: [::1]) for stricter RFC alignment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/flb_http_client.c(2 hunks)src/flb_network.c(1 hunks)src/flb_utils.c(8 hunks)tests/internal/http_client.c(2 hunks)tests/internal/utils.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/flb_network.c
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-09-08T11:21:33.975Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 10851
File: include/fluent-bit/flb_simd.h:60-66
Timestamp: 2025-09-08T11:21:33.975Z
Learning: Fluent Bit currently only supports MSVC compiler on Windows, so additional compiler compatibility guards may be unnecessary for Windows-specific code paths.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:24:26.170Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:39-42
Timestamp: 2025-08-29T06:24:26.170Z
Learning: In Fluent Bit, ZSTD compression support is enabled by default and does not require conditional compilation guards (like #ifdef FLB_HAVE_ZSTD) around ZSTD-related code declarations and implementations.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:24:55.855Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:52-56
Timestamp: 2025-08-29T06:24:55.855Z
Learning: ZSTD compression is always available in Fluent Bit and does not require conditional compilation guards. Unlike Arrow/Parquet which use #ifdef FLB_HAVE_ARROW guards, ZSTD is built unconditionally with flb_zstd.c included directly in src/CMakeLists.txt and a bundled ZSTD library at lib/zstd-1.5.7/.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:24:44.797Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:26-26
Timestamp: 2025-08-29T06:24:44.797Z
Learning: In Fluent Bit, ZSTD support is always available and enabled by default. The build system automatically detects and uses either the system libzstd library or builds the bundled ZSTD version. Unlike other optional dependencies like Arrow which use conditional compilation guards (e.g., FLB_HAVE_ARROW), ZSTD does not require conditional includes or build flags.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components, ZSTD support is always available and doesn't need build-time conditionals.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Applied to files:
src/flb_http_client.c
🧬 Code graph analysis (3)
tests/internal/http_client.c (4)
src/flb_http_client.c (3)
flb_http_get_header(1020-1037)flb_http_client_destroy(1706-1713)flb_http_client(832-877)include/fluent-bit/flb_mem.h (2)
flb_free(126-128)flb_calloc(84-96)src/flb_upstream.c (2)
flb_upstream_destroy(656-698)flb_upstream_create(290-363)src/flb_sds.c (1)
flb_sds_destroy(389-399)
src/flb_utils.c (2)
src/flb_sds.c (2)
flb_sds_create_len(58-76)flb_sds_create(78-90)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
src/flb_http_client.c (1)
src/flb_sds.c (2)
flb_sds_copy(260-281)flb_sds_printf(336-387)
🔇 Additional comments (5)
tests/internal/utils.c (1)
39-55: IPv6 URL split test coverage looks correct and thoroughThe new
url_checksentries nicely cover bracketed IPv6 (with/without ports, paths, IPv4‑embedded, and query strings) plus malformed bracket edge cases, and the expected host/port/uri values match the newflb_utils_url_split[_sds]semantics (host without brackets, defaults for 80/443, and-1on bad bracket syntax). No issues from a parsing or expectations standpoint.src/flb_http_client.c (1)
36-43: Windows networking includes and flb_compat usage are appropriateThe new
FLB_SYSTEM_WINDOWSblock for<winsock2.h>/<ws2tcpip.h>together with including<fluent-bit/flb_compat.h>at the top ensures the IPv6‑related types/functions used later in this file are available on Windows without affecting non‑Windows builds. This matches the project’s existing pattern for platform‑specific networking code.tests/internal/http_client.c (1)
495-555: Host header tests comprehensively validate IPv4/IPv6 and TLS behaviorThe new helpers (
test_host_header_format,test_tls_host_header_format) and associated tests exercise:
- IPv6 literals in both bracketed and unbracketed form (including compressed, full, IPv4‑mapped, all‑zeros, loopback).
- IPv4 addresses and domain names.
- TLS vs non‑TLS behavior for port 443 and non‑standard TLS ports.
- Zone‑ID variants (
fe80::1%eth0,[fe80::1%eth0]).- Explicit ports 80 and 443 for non‑TLS.
These cases mirror the logic in
add_host_and_content_length()and should prevent regressions in Host header formatting across the main combinations.Also applies to: 557-663, 673-683
src/flb_utils.c (2)
1428-1438: Corrected substring lengths inflb_utils_copy_host_sdsThe updated length calculations:
pos_end - pos_init - 2for bracketed IPv6 ([... ]) andpos_end - pos_initfor non‑bracketed hostsare now consistent with the starting pointer offset and correctly handle cases where
pos_init != 0. This fixes the previously reported off‑by‑one risk without changing behavior for the existing call sites that passpos_init == 0.
1464-1583: Bracket-aware URL splitting and cleanup logic look soundIn both
flb_utils_url_split()andflb_utils_url_split_sds():
- IPv6 hosts are first validated via
validate_ipv6_brackets, and when brackets are present you now search for the port separator':'starting at the closing']', avoiding confusion with IPv6 colons.flb_copy_host/flb_utils_copy_host_sdsare used to strip IPv6 brackets and return host literals like"::1"and"2001:db8::1", matching the new unit tests intests/internal/utils.c.- When the host substring is bracketed but malformed (missing
], empty[], or stray']'with no'['), both functions correctly fail and run the new error‑path cleanup.- Error paths now free any partially allocated
protocol,host,port, anduri(usingflb_free/flb_sds_destroyas appropriate), preventing leaks on invalid URLs or allocation failures.- The SDS variant continues to validate that the port consists only of digits before succeeding, which is consistent with its previous behavior.
Overall, the new parsing and cleanup behavior aligns with the added IPv6 URL tests and should be robust for both IPv4/hostname and bracketed IPv6 URLs.
Also applies to: 1585-1729
ShelbyZ
force-pushed
the
ipv6-changes
branch
from
347df83 to
b0a448a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/internal/http_client.c (2)
495-555: Clarifyflb_http_clientflags usage in TLS helperIn
test_tls_host_header_format, the last argument toflb_http_clientisFLB_IO_TLS, which is an I/O/upstream flag, while this parameter is conceptually for HTTP client flags (e.g., protocol/version options). TLS detection is already driven offu_conn_tls->upstreamviaflb_stream_get_flag_status, so passingFLB_IO_TLShere is unnecessary and could become confusing or brittle if bit spaces ever overlap.Consider passing
0for the flags argument and relying solely on the upstream flags to indicate TLS:- struct flb_http_client *c = flb_http_client(u_conn_tls, FLB_HTTP_GET, "/", - NULL, 0, host, port, NULL, FLB_IO_TLS); + struct flb_http_client *c = flb_http_client(u_conn_tls, FLB_HTTP_GET, "/", + NULL, 0, host, port, NULL, 0);The rest of the helper’s allocation and cleanup paths look sound.
557-668: Host header test coverage is strong; consider one more TLS+IPv6 non‑default port caseThe new tests exercise:
- IPv6 unbracketed vs bracketed hosts (
::1,[::1]) with ports,- IPv4 and domain hosts,
- TLS with default and non‑default ports,
- Multiple IPv6 formats and zone IDs (bracketed and unbracketed),
- IPv6+TLS default port (
[::1]without port in Host).This is a good spread and aligns with the new Host header rules. As a small extension, you might add a TLS + IPv6 + non‑standard port test (e.g.,
::1/ 8443) to mirrortest_https_non_standard_port_host_headerand lock in the expected[::1]:8443behavior under TLS with a non‑443 port. Optional, but would round out the matrix.Also applies to: 678-689
src/flb_utils.c (1)
1441-1474: IPv6 bracket validation now correctly scoped to the host portion
validate_ipv6_bracketslimits its search for']'to the substring before the first'/', and distinguishes between bracketed and non‑bracketed hosts. This prevents false negatives for URLs that legitimately contain']'in the path or query while still rejecting malformed host parts such as[]or hosts with a stray']'but no leading'['. The optionalout_bracketpointer is also a clean way to hand the closing bracket location back to callers.One minor stylistic point: since
pisconst char *, makingout_bracketaconst char **(andbracket/closingconst char *) would improve const‑correctness, but that’s purely optional and internal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/flb_http_client.c(2 hunks)src/flb_network.c(1 hunks)src/flb_utils.c(8 hunks)tests/internal/http_client.c(2 hunks)tests/internal/utils.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/internal/utils.c
- src/flb_http_client.c
- src/flb_network.c
🧰 Additional context used
🧬 Code graph analysis (2)
tests/internal/http_client.c (4)
src/flb_http_client.c (3)
flb_http_get_header(1029-1046)flb_http_client_destroy(1715-1722)flb_http_client(841-886)src/flb_upstream.c (2)
flb_upstream_destroy(656-698)flb_upstream_create(290-363)include/fluent-bit/flb_mem.h (2)
flb_free(126-128)flb_calloc(84-96)src/flb_sds.c (1)
flb_sds_destroy(389-399)
src/flb_utils.c (2)
src/flb_sds.c (2)
flb_sds_create_len(58-76)flb_sds_create(78-90)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
🔇 Additional comments (3)
tests/internal/http_client.c (1)
469-493: check_host_header cleanup now avoids use-after-free and looks correctThe revised error path destroys the HTTP client, then (optionally) destroys the upstream via
extra_cleanup->upstreambefore freeing the connection, and finally tears down the test context and exits. This ordering removes the previous use-after-free risk while keeping behavior confined to the failure path. No issues from a correctness or lifetime perspective.src/flb_utils.c (2)
1428-1438: Corrected substring length inflb_utils_copy_host_sdsThe new length computation for IPv6 (
pos_end - pos_init - 2) and the non‑IPv6 branch (pos_end - pos_init) now both respectpos_initas an offset, fixing the earlier off‑by‑one risk whenpos_init != 0while preserving current call sites that usepos_init == 0. This makes the helper safe for future callers that might pass non‑zero offsets.
1486-1517: Bracket‑aware URL splitting and cleanup paths look correctThe changes to both
flb_utils_url_splitandflb_utils_url_split_sdsintegratevalidate_ipv6_bracketsand use itsbracketout‑parameter to:
- Decide whether to search for the port separator
':'starting at the closing']'(IPv6 literal) or from the start of the host (IPv4/domain).- Ensure that any
':'chosen as a port separator occurs before the first'/', so colons in the path don’t affect parsing.- Route bracket mismatches or invalid host extractions (
flb_copy_host/flb_utils_copy_host_sdsreturningNULL) into a common error path.The new null‑checks on
host(and the existing checks forport/uriin the SDS variant) combined with the expandederror:blocks—now freeing protocol, host, port, and uri (viaflb_freeorflb_sds_destroyas appropriate)—ensure that malformed URLs or allocation failures surface as-1without leaking memory.Behavior for well‑formed IPv4, domain, and bracketed IPv6 URLs is preserved while malformed or mismatched bracket cases are now rejected cleanly.
Also applies to: 1548-1560, 1580-1592, 1608-1638, 1669-1681, 1726-1740
ShelbyZ
force-pushed
the
ipv6-changes
branch
from
b0a448a to
03ce384
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ShelbyZ
force-pushed
the
ipv6-changes
branch
from
03ce384 to
75572ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/internal/http_client.c (3)
495-555: Test helpers reuse config safely but double‑destroy TLS upstreamThe non‑TLS helper (
test_host_header_format) has a clean lifecycle: context, client, destroy client, destroy context.In
test_tls_host_header_format, however,u_tlsis destroyed twice: once insidetest_tls_host_header_formatand once indirectly whentest_ctx_destroy(ctx)is called (sincectx->uwas earlier created withflb_upstream_create).u_tlsis separate fromctx->u, so this is fine; but note that passingctxintocheck_host_headerand then always callingtest_ctx_destroy(ctx)after also destroyingu_tlsis slightly asymmetric with the non‑TLS helper. Consider a small comment or renamingextra_cleanuparg to clarify this dual ownership to future readers.
583-639: ipv6_formats_host_header loop is correct; minor duplication onlyThe loop over IPv6 test cases correctly creates a client per case, checks the Host header, and frees both the SDS and client before moving on. Error handling exits early on failure with proper cleanup.
There is some duplication vs
test_host_header_format; you could factor those tests through the helper for consistency, but it’s not required for correctness.
641-674: Zone‑ID and HTTPS IPv6 expectations may diverge from current header logicSeveral expectations deserve a quick alignment check with
add_host_and_content_length:
test_ipv6_zone_id_host_headerexpects[fe80::1%eth0]:8080.test_ipv6_bracketed_zone_id_host_headerexpects[fe80::1%eth0]:8080.test_https_ipv6_default_port_host_headerexpects[::1].test_https_ipv6_non_standard_port_host_headerexpects[::1]:8443.Given the current client code, unbracketed
fe80::1%eth0will be detected as IPv6 (after stripping the zone) but the formatted Host still uses the original string, including the zone ID. If you decide to drop the zone ID from Host (to avoid%issues), these tests will also need updating. The IPv6 HTTPS cases match the current bracket‑with/without‑port rules and look good.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/flb_http_client.c(2 hunks)src/flb_network.c(1 hunks)src/flb_utils.c(8 hunks)tests/internal/http_client.c(2 hunks)tests/internal/utils.c(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/internal/utils.c
- src/flb_network.c
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-09-08T11:21:33.975Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 10851
File: include/fluent-bit/flb_simd.h:60-66
Timestamp: 2025-09-08T11:21:33.975Z
Learning: Fluent Bit currently only supports MSVC compiler on Windows, so additional compiler compatibility guards may be unnecessary for Windows-specific code paths.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:24:26.170Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:39-42
Timestamp: 2025-08-29T06:24:26.170Z
Learning: In Fluent Bit, ZSTD compression support is enabled by default and does not require conditional compilation guards (like #ifdef FLB_HAVE_ZSTD) around ZSTD-related code declarations and implementations.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:24:55.855Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:52-56
Timestamp: 2025-08-29T06:24:55.855Z
Learning: ZSTD compression is always available in Fluent Bit and does not require conditional compilation guards. Unlike Arrow/Parquet which use #ifdef FLB_HAVE_ARROW guards, ZSTD is built unconditionally with flb_zstd.c included directly in src/CMakeLists.txt and a bundled ZSTD library at lib/zstd-1.5.7/.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:24:44.797Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:26-26
Timestamp: 2025-08-29T06:24:44.797Z
Learning: In Fluent Bit, ZSTD support is always available and enabled by default. The build system automatically detects and uses either the system libzstd library or builds the bundled ZSTD version. Unlike other optional dependencies like Arrow which use conditional compilation guards (e.g., FLB_HAVE_ARROW), ZSTD does not require conditional includes or build flags.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components, ZSTD support is always available and doesn't need build-time conditionals.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Applied to files:
src/flb_http_client.c
🧬 Code graph analysis (3)
src/flb_utils.c (2)
src/flb_sds.c (2)
flb_sds_create_len(58-76)flb_sds_create(78-90)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
tests/internal/http_client.c (4)
src/flb_http_client.c (3)
flb_http_get_header(1048-1065)flb_http_client_destroy(1734-1741)flb_http_client(860-905)src/flb_upstream.c (2)
flb_upstream_destroy(656-698)flb_upstream_create(290-363)include/fluent-bit/flb_mem.h (2)
flb_free(126-128)flb_calloc(84-96)src/flb_sds.c (1)
flb_sds_destroy(389-399)
src/flb_http_client.c (1)
src/flb_sds.c (2)
flb_sds_printf(336-387)flb_sds_copy(260-281)
🔇 Additional comments (11)
src/flb_http_client.c (2)
36-43: Windows networking includes look correct and scopedIncluding
<winsock2.h>and<ws2tcpip.h>underFLB_SYSTEM_WINDOWSalongsideflb_compat.his consistent with the rest of the codebase and provides the needed IPv6 /inet_ptonsymbols on Windows. No further guards seem necessary.
649-671: TLS default‑port Host formatting looks correct; consider TLS detection semanticsUsing
flb_stream_get_flag_status(&u->base, FLB_IO_TLS)without_port == 443to decide when to omit the port and optionally bracket IPv6 appears correct and aligns Host with standard HTTPS conventions. The IPv6 vs non‑IPv6 Host formats ([addr]vshost) also match RFC expectations.One behavioral nuance: this check uses the upstream’s TLS status, not per‑connection state. If upstream flags and connection TLS mode can diverge, that might misclassify some connections. If that never happens by design, this is fine; otherwise, you may want to rely on connection‑level TLS state.
tests/internal/http_client.c (4)
469-493: check_host_header cleanup ordering looks safe nowThe revised
check_host_headercorrectly destroys the upstream before freeing theextra_cleanupconnection, and only does that on the error path. On the success path you free the connection and upstream in the caller, so there’s no use‑after‑free or double‑free here.
557-575: Basic Host header tests align with new formatting rulesThe simple cases (
::1,[::1], IPv4, and domain with non‑default port) all match the expected formats fromadd_host_and_content_length(bracketed IPv6 with port, bare IPv4/domain with port). These tests should catch regressions in Host formatting logic.
577-581: HTTPS default‑port Host expectation matches implementationExpecting
Host: example.comfor TLS with port 443 matches the new logic that omits the port for HTTPS default. This is a good regression test for the TLS default‑port special case.
684-696: TEST_LIST registration is consistent and comprehensiveAll new host‑header test functions are properly registered in
TEST_LIST, covering IPv4/IPv6, bracketed/non‑bracketed, TLS vs non‑TLS, default and non‑default ports, and zone IDs. This should give good coverage of the new behavior.src/flb_utils.c (5)
1428-1438: flb_utils_copy_host_sds length calculations now correctUsing
pos_end - pos_init - 2for bracketed IPv6 ([host]) andpos_end - pos_initfor non‑bracketed hosts fixes the previous off‑by‑one whenpos_init != 0. With thepos_end-1']'check, both paths are safe and consistent withflb_copy_host.
1441-1482: IPv6 bracket validation correctly scoped to host (incl. query/fragment)
validate_ipv6_bracketsnow limits']'checks to the substring before the first/,?, or#, usingmemchrwith an explicit length. This avoids rejecting URLs where]appears in the path/query while still catching malformed host brackets and empty[]. Returning the closing bracket viaout_bracketalso cleanly supports later host/port parsing.
1484-1529: URI helpers handle bare query/fragment correctly
create_uri_with_slashand its SDS variant ensure:
- Empty or NULL URI ⇒
"/".- URIs starting with
?or#get a leading/(so?q=1becomes/??q=1?).One subtlety: you’re using
create_uri_with_slash(tmp)wheretmpalready points at the first of/,?, or#. For the?/#case that produces"/" + uri_part, which is exactly what you want; no double question marks occur._sdscorrectly wraps the strdup’ed result and frees the intermediate buffer.
1541-1656: url_split: improved IPv6, URI, and cleanup logic; check host copy semanticsThe revised
flb_utils_url_split:
- Validates IPv6 brackets in the host section before searching for
':', avoiding misinterpreting]in path/query.- Uses
flb_copy_hostfor host extraction and allows a missing explicit port (later defaulting based on scheme).- Uses
strpbrk(p, "/?#")to locate the start of the URI (including bare query/fragment) andcreate_uri_with_slash()to normalize it.- Cleans up all allocated pieces on failure paths.
Two small points to confirm:
flb_copy_hoststill takespos_init/pos_endrelative to the local pointer, and you consistently call it withpos_init=0,pos_end=tmp-porstrlen(p), so offsets remain correct.For malformed bracketed hosts (e.g.,
[::1without]),flb_copy_hostreturnsNULLand you surface-1after logging; this is the intended behavior for invalid URLs.Overall, the behavior looks correct and more robust than before.
1661-1814: url_split_sds: SDS counterpart mirrors logic and error handlingThe SDS version now mirrors the plain
char *implementation:
- Uses
validate_ipv6_brackets,flb_utils_copy_host_sds, andstrpbrkto extract host, port, and URI correctly for both IPv4 and bracketed IPv6.- Normalizes URIs via
create_uri_with_slash_sds, including bare query/fragment and empty cases.- Defaults ports based on protocol and validates that
portis numeric.- Cleans up all allocated SDS values on error.
The control flow and ownership semantics match the non‑SDS version, so behavior should be consistent across both APIs.
This commit fixes IPv6 address handling in HTTP client Host headers by adding bracket notation when required and improving URL parsing validation. Changes: - Add automatic bracket wrapping for unbracketed IPv6 addresses in Host headers for both standard and non-standard ports - Add IPv6 bracketing for HTTPS default port (443) to ensure RFC compliance even when port is omitted (e.g., Host: [::1]) - Fix off-by-one error in IPv6 bracket stripping (was removing one extra character) - Fix incorrect length calculation in flb_utils_copy_host_sds for bracketed IPv6 extraction (changed from absolute position to relative length to properly account for pos_init offset) - Strip IPv6 zone IDs (e.g., %eth0) from Host headers per RFC 3986 which prohibits zone IDs in URIs (e.g., fe80::1%eth0 becomes [fe80::1]:8080 in Host header) - Perform zone ID stripping before inet_pton() validation to ensure proper IPv6 address detection for link-local addresses - Add URI path prepending for URLs with query/fragment but no path (e.g., http://example.com?query=1 becomes /?query=1) per RFC 7230 - Constrain IPv6 bracket validation to host portion only, preventing false negatives when brackets appear in URL paths or query strings - Update validate_ipv6_brackets() to recognize '?' and '#' as host delimiters in addition to '/' - Refactor URL parsing logic to eliminate duplication - Use memchr with length limit for consistent and safe bracket detection in both IPv6 and non-IPv6 cases - Improve error handling in URL parsing with proper cleanup on failure - Update TLS flag checking to use flb_stream_get_flag_status() for more reliable detection Tests: - Add test for IPv6 with HTTPS on default port 443 - Add test cases for IPv6 addresses with zone IDs (verifying zone ID stripping behavior) - Add test cases for brackets in URL paths and query strings - Add test cases for malformed bracket scenarios Signed-off-by: Shelby Hagman <[email protected]>
ShelbyZ
force-pushed
the
ipv6-changes
branch
from
75572ce to
0425514
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/flb_utils.c (1)
1661-1814: Mirror host/port colon scoping in the SDS URL splitter
flb_utils_url_split_sds()mirrors the non‑SDS splitter: it now usesvalidate_ipv6_brackets(),flb_utils_copy_host_sds(), andcreate_uri_with_slash_sds()and enforces numeric ports with a digit loop before succeeding. Error paths correctly destroy any allocated SDS components.For consistency and to avoid subtle divergence, the same optional refinement suggested for
flb_utils_url_split()—restricting the':'search to the host substring (before'/','?', or'#') instead of scanning the entire remainder—should also be applied here so SDS and non‑SDS behavior stay aligned.
🧹 Nitpick comments (2)
tests/internal/utils.c (1)
39-75: IPv6 and bracket-in-URI test coverage is solid; consider one more edge caseThe new
url_checksentries nicely exercise:
- Bracketed IPv6 with/without explicit ports.
- IPv6 with mixed path/query/fragment, including
]characters outside the host.- Malformed bracket host scenarios that should now fail URL parsing.
Given the tighter host/URI split logic, you may also want a follow‑up case where
:appears only in the query or fragment (e.g.https://example.com?q=a:b) to guard against accidentally treating that colon as a port separator in future refactors. Current behavior here predates this PR, so this can be deferred.src/flb_utils.c (1)
1531-1659: Host/port/URI split is robust, but consider constraining port detection to the host segmentThe new
flb_utils_url_split()logic cleanly integrates:
- Early
validate_ipv6_brackets()over just the host substring.- IPv6‑aware host extraction via
flb_copy_host(), including bracket stripping.- URI delimiting via
strpbrk(p, "/?#")and normalization withcreate_uri_with_slash().One remaining corner case is that port detection still uses:
if (bracket) { tmp = strchr(bracket, ':'); } else { tmp = strchr(p, ':'); }and only validates
tmpagainst the first'/'. This means a colon in the query or fragment (e.g.https://example.com?q=a:b) with no path slash could still be misinterpreted as a port separator, a behavior that predates this PR.Given you already compute host boundaries for bracket validation, you could optionally tighten the search for
':'to the host range (before'/','?', or'#'), e.g.:- sep = strchr(p, '/'); - if (validate_ipv6_brackets(p, &bracket) < 0) { + sep = strchr(p, '/'); + if (validate_ipv6_brackets(p, &bracket) < 0) { flb_errno(); goto error; } - if (bracket) { - tmp = strchr(bracket, ':'); - } else { - tmp = strchr(p, ':'); - } + /* Compute end of host segment (before '/', '?', or '#') */ + const char *host_end = sep; + const char *qf = strpbrk(p, "?#"); + + if (!host_end || (qf && qf < host_end)) { + host_end = qf; + } + if (!host_end) { + host_end = p + strlen(p); + } + + if (bracket) { + /* For bracketed IPv6, only ports after ']' are valid */ + tmp = strchr(bracket, ':'); + } + else { + /* Non‑IPv6: limit ':' search to the host portion */ + tmp = memchr(p, ':', host_end - p); + }This keeps the new IPv6 behavior intact while avoiding accidental port parsing from query/fragment colons.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/flb_http_client.c(2 hunks)src/flb_network.c(1 hunks)src/flb_utils.c(8 hunks)tests/internal/http_client.c(2 hunks)tests/internal/utils.c(1 hunks)
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-09-08T11:21:33.975Z
Learnt from: cosmo0920
Repo: fluent/fluent-bit PR: 10851
File: include/fluent-bit/flb_simd.h:60-66
Timestamp: 2025-09-08T11:21:33.975Z
Learning: Fluent Bit currently only supports MSVC compiler on Windows, so additional compiler compatibility guards may be unnecessary for Windows-specific code paths.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit CMakeLists.txt, the system library preference flags are defined as FLB_PREFER_SYSTEM_LIB_ZSTD and FLB_PREFER_SYSTEM_LIB_KAFKA with the FLB_ prefix.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:25:02.561Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:7-7
Timestamp: 2025-08-29T06:25:02.561Z
Learning: In Fluent Bit, ZSTD (zstandard) compression library is bundled directly in the source tree at `lib/zstd-1.5.7` and is built unconditionally as a static library. Unlike optional external dependencies, ZSTD does not use conditional compilation guards like `FLB_HAVE_ZSTD` and is always available. Headers like `<fluent-bit/flb_zstd.h>` can be included directly without guards.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components such as ARROW/PARQUET (which use `#ifdef FLB_HAVE_ARROW` guards), ZSTD support is always available and doesn't need build-time conditionals. ZSTD headers are included directly without guards across multiple plugins and core components.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:24:26.170Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:39-42
Timestamp: 2025-08-29T06:24:26.170Z
Learning: In Fluent Bit, ZSTD compression support is enabled by default and does not require conditional compilation guards (like #ifdef FLB_HAVE_ZSTD) around ZSTD-related code declarations and implementations.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:24:55.855Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:52-56
Timestamp: 2025-08-29T06:24:55.855Z
Learning: ZSTD compression is always available in Fluent Bit and does not require conditional compilation guards. Unlike Arrow/Parquet which use #ifdef FLB_HAVE_ARROW guards, ZSTD is built unconditionally with flb_zstd.c included directly in src/CMakeLists.txt and a bundled ZSTD library at lib/zstd-1.5.7/.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:24:44.797Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: src/aws/flb_aws_compress.c:26-26
Timestamp: 2025-08-29T06:24:44.797Z
Learning: In Fluent Bit, ZSTD support is always available and enabled by default. The build system automatically detects and uses either the system libzstd library or builds the bundled ZSTD version. Unlike other optional dependencies like Arrow which use conditional compilation guards (e.g., FLB_HAVE_ARROW), ZSTD does not require conditional includes or build flags.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-29T06:25:27.250Z
Learnt from: shadowshot-x
Repo: fluent/fluent-bit PR: 10794
File: tests/internal/aws_compress.c:93-107
Timestamp: 2025-08-29T06:25:27.250Z
Learning: In Fluent Bit, ZSTD compression is enabled by default and is treated as a core dependency, not requiring conditional compilation guards like `#ifdef FLB_HAVE_ZSTD`. Unlike some other optional components, ZSTD support is always available and doesn't need build-time conditionals.
Applied to files:
src/flb_http_client.c
📚 Learning: 2025-08-31T12:46:11.940Z
Learnt from: ThomasDevoogdt
Repo: fluent/fluent-bit PR: 9277
File: .github/workflows/pr-compile-check.yaml:147-151
Timestamp: 2025-08-31T12:46:11.940Z
Learning: In fluent-bit, the correct CMake flag for using system librdkafka is `FLB_PREFER_SYSTEM_LIB_KAFKA=ON`.
Applied to files:
src/flb_http_client.c
🧬 Code graph analysis (3)
src/flb_http_client.c (1)
src/flb_sds.c (2)
flb_sds_printf(336-387)flb_sds_copy(260-281)
tests/internal/http_client.c (3)
src/flb_http_client.c (3)
flb_http_get_header(1052-1069)flb_http_client(864-909)flb_http_client_destroy(1738-1745)src/flb_sds.c (1)
flb_sds_destroy(389-399)src/flb_upstream.c (2)
flb_upstream_create(290-363)flb_upstream_destroy(656-698)
src/flb_utils.c (2)
src/flb_sds.c (2)
flb_sds_create_len(58-76)flb_sds_create(78-90)include/fluent-bit/flb_mem.h (1)
flb_free(126-128)
🔇 Additional comments (8)
src/flb_network.c (1)
30-36: Windows IPv6 header inclusion looks correctIncluding
<ws2tcpip.h>alongside<winsock2.h>underFLB_SYSTEM_WINDOWSmatches the usage ofinet_pton/struct in6_addrlater in this file and is appropriately guarded.src/flb_utils.c (2)
1428-1439: Host SDS extraction now correctly respects start/end offsetsThe updated
flb_utils_copy_host_sds()length calculations (pos_end - pos_init - 2for bracketed IPv6,pos_end - pos_initotherwise) are consistent with the exclusivepos_endconvention and fix the prior off‑by‑one whenpos_init != 0. Calling sites that pass0still behave as before, and future non‑zero offsets will now be safe.
1441-1529: IPv6 bracket validation and URI normalization helpers look correct
validate_ipv6_brackets()now:
- Limits its scan to the host portion (up to the first of
'/','?','#'or end of string).- Enforces
'[host]'shape for IPv6, rejecting empty[]and any stray']'in non‑bracketed hosts.- Returns the closing bracket position when present, which callers correctly reuse to search for the port separator only after
].
create_uri_with_slash()/_sds():
- Treat empty/missing URI as
"/".- Prepend
'/'when the URI starts with'?'or'#', aligning with the new tests that expect"/?..."and"/#...".- Avoid duplicate leading
'/'when the URI already begins with a path.All call sites in
flb_utils_url_split*handle allocation failures through the sharederror:paths, so there are no obvious leaks.src/flb_http_client.c (2)
36-43: Windows socket includes and compat header placement are appropriate
- Guarding
<winsock2.h>and<ws2tcpip.h>withFLB_SYSTEM_WINDOWSavoids impacting non‑Windows builds.- Including
<fluent-bit/flb_compat.h>early ensures any platform shims (e.g., forinet_pton) are available before use in this file.This matches existing cross‑platform patterns in the codebase.
585-701: Host header construction correctly handles IPv6, zone IDs, and HTTPS defaultsThe reworked
add_host_and_content_length():
- Determines
out_host/out_portas before, honoringc->hostoverrides andproxied_host/proxied_port.- Detects unbracketed IPv6 hosts, optionally stripping a
%zonesuffix intoaddr_bufand validating withinet_pton(AF_INET6, ...).- Uses the stripped IPv6 (
host_for_header) only when a valid IPv6+zone combination was detected; otherwise the original host string is used.- For TLS endpoints on port 443:
- IPv6: formats
Hostas[%s](no port), ensuring brackets for RFC‑3986 compliance.- Non‑IPv6: uses just the host name (no
:443), as expected for default HTTPS.- For all other cases:
- IPv6:
[%s]:port.- Non‑IPv6 (IPv4, DNS names, already‑bracketed IPv6):
host:port.This matches the new tests in
tests/internal/http_client.cfor IPv4, domain hosts, IPv6, bracketed IPv6, and zone‑ID cases across TLS/non‑TLS and default/non‑default ports, and preserves the previous proxy/keepalive behavior.tests/internal/http_client.c (3)
469-545: Host header helper utilities are well‑factored and manage resources safely
check_host_header()encapsulates the common Host lookup + comparison and always destroys the returned SDS.test_host_header_format()andtest_tls_host_header_format():
- Create isolated
test_ctx/client (and standalone TLS upstream/connection for TLS) per invocation.- Correctly clean up on both success and failure paths (
flb_http_client_destroy,flb_upstream_destroy,flb_free,test_ctx_destroy).This keeps the individual test cases small and makes it easy to add further host‑header scenarios.
547-639: Comprehensive Host header matrix tests validate IPv6/TLS behaviorThe new tests systematically cover:
- IPv6:
- Unbracketed (
"::1","2001:db8::1","::ffff:192.0.2.1","fe80::1","::") and bracketed ("[::1]","[fe80::1%eth0]") forms.- Zone IDs in unbracketed form (
"fe80::1%eth0") and their stripping in the Host header.- IPv4 and domain names, both HTTP and HTTPS.
- Port handling:
- Non‑default ports for HTTP and HTTPS (
8080,8443).- Default HTTPS 443 with and without IPv6, checking that the Host header omits
:443but brackets IPv6.- TLS vs non‑TLS behavior via
test_host_header_formatvstest_tls_host_header_format.The expectations (e.g.,
[fe80::1]:8443,[::1]for HTTPS+443, pass‑through for already‑bracketed zone IDs) line up with the newadd_host_and_content_length()logic and codify the intended semantics well.
641-663: New tests are correctly integrated into TEST_LISTAll newly added host header tests are registered in
TEST_LISTwith clear, descriptive names and no reordering of existing tests. This ensures they run under the existing test harness without impacting earlier coverage.
2 participants
This commit fixes IPv6 address handling in HTTP client Host headers
by adding bracket notation when required and improving URL parsing
validation.
Changes:
Host headers for both standard and non-standard ports
compliance even when port is omitted (e.g., Host: [::1])
extra character)
bracketed IPv6 extraction (changed from absolute position to
relative length to properly account for pos_init offset)
false negatives when brackets appear in URL paths or query strings
detection in both IPv6 and non-IPv6 cases
more reliable detection
Tests:
Addresses #10699
ECR images with changes:
Testing
Before we can approve your change; please submit the following in a comment:
Conf/Logs available at - https://gist.github.com/ShelbyZ/63f6edd05fec039feae6c8c615db52e2 (includes full fluent-bit and valgrind)
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
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
New Features
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.