Hello Willy, Thanks a lot for the review, and sorry for the confusing v1: both mails carried "[PATCH 1/2]" in their subject; the second one should have been 2/2. This time all five patches are attached to this single mail instead of being pasted, so the tabs are preserved and no 1/5..5/5 mails will follow.
This v2 follows the split you proposed: parsing fix first, range check
as a separate non-backported change, then "strict"/"relaxed" and the
default flip for 3.5. Patches 1-3 do not depend on 4-5, so they can be
merged first if the last two need more discussion.
1/5 MINOR: tools: move http_get_host_port() to tools.c as
get_port_from_host()
Pure move and rename, 5 callers in h1.c and http_htx.c, no
functional change. get_host_port(), the name you suggested, is
already taken by the static inline in tools.h which returns the
port of a sockaddr in host byte order. To be backported with 2/5
(2.8 and above).
2/5 BUG/MINOR: sample: fix port_only parsing of invalid port values
get_port_from_host() + strl2irc(), an invalid port now gives 0
like an absent one; adds reg-tests/converter/port_only.vtc.
To be backported to 2.8 and above together with 1/5. Depends on
3e666065e, first released in 2.8.27, 3.0.26, 3.2.22, 3.3.13 and
3.4.3. 3.1 is left out (see below).
3/5 MEDIUM: sample: make port_only reject ports above 65535
65536+ now gives 0. MEDIUM as it changes the result of existing
expressions (the breaking change you mentioned). Not to be
backported, release notes.
4/5 MINOR: sample: add a "strict" / "relaxed" argument to port_only
Default stays "relaxed" in this patch. Not to be backported.
5/5 MEDIUM: sample: make "strict" the default mode of port_only
Only invalid inputs change; the "no port" case stays 0. MEDIUM
for the same reason as 3/5. Not to be backported, release notes
text in the commit message.
Choices made in this series:
- 1/5 is backported together with 2/5 so that master and stable share
the same code. 3.0, 3.2, 3.3 and 3.4 take both patches cleanly; 2.8
only has two context conflicts in 1/5 (http.h and http.c, because
http_status_add_range() does not exist there). If you would rather
keep the move out of stable, I can send a 2/5 variant calling
http_get_host_port() directly instead.
- an absent or empty port ("example.com", "example.com:") still gives
0 in both modes, as today, so the existing strings.vtc is untouched,
and ":0" is a valid 0. In strict mode only an invalid port, or a
colon or bracket structure in the host which does not follow the
RFC 3986 authority grammar, makes the converter fail; the host
characters themselves are not checked. relaxed keeps the last-colon
rule, as str2sa_range() does. The documented table (identical in
both modes unless noted; ":80" is the input with an empty host):
example.com, example.com:, example.com:0, [::1] -> 0
example.com:80, example.com:080, [::1]:80, :80 -> 80
example.com:http, example.com:-80, example.com:70000,
example.com:4294967376
-> relaxed 0, strict no sample
::1, a:b:80, [::1, []:80
-> relaxed 1, 80, 1, 80; strict no sample
- the "strict"/"relaxed" argument is not backported: I read "keeping
relaxed for earlier versions" as leaving the stable branches without
the argument, with only 1/5 and 2/5. Since they lack 3/5, "relaxed"
there would not mean the same as in 3.5. The doc shows the idiom
which loads on every version ("set-var(txn.port) ...,port_only" then
"var(txn.port,0)").
- 2/5 keeps strl2irc() as in v1, so a purely numeric port in
2147483648..4294967295, which the current code returns as-is, gives
0 from 2/5 on, in the stable branches too. If you see this as a
range reduction that should wait for 3/5, 2/5 can use strl2llrc()
instead, so that only non-digits and overflows change there.
- 3.1 is left out of the backport list: haproxy.org lists it as
unmaintained since 2026-Q1 and it lacks 3e666065e.
- the doc example "version_atleast(3.5-dev10)" names the first tag
which can contain this series; it can be adjusted at merge time.
Changes since v1:
- the parser moved to tools.c as get_port_from_host() (1/5) instead of
calling the HTTP helper from sample.c;
- strl2irc() is kept; the 0..65535 range check is split into its own
patch (3/5), which is not to be backported;
- the "strict" / "relaxed" argument (4/5) and the 3.5 default flip
(5/5) are new;
- the reg-test is now a separate file, port_only.vtc, so that it can
be backported (strings.vtc only exists in master), with the
expectations updated in each patch;
- patch numbering fixed; the patches are attached instead of pasted.
Tested on top of 5c1a2975d (3.5-dev9-1) with USE_OPENSSL=1 USE_LUA=1
ERR=1. Every commit builds without warning; port_only.vtc and
strings.vtc pass from 2/5 on, the whole reg-tests/converter directory
passes at 2/5 and on the final tree, and port_only.vtc fails as
expected on an unpatched build. 1/5 + 2/5 cherry-picked on 2.8, 3.0,
3.2, 3.3 and 3.4 build and pass port_only.vtc. Every row of the table
above was checked with curl over H1 (Host and X-Forwarded-Host) and
H2 on the 4/5 and 5/5 builds in all four spellings (over H2 the
scheme-based normalization strips a default port first, e.g.
"example.com:80" over h2c gives 0, and the results match the table
applied to that normalized value), together with the ACL,
set-var/var(,0) and log-format effects, config loading of the valid
and invalid spellings, and Lua (nil / "" on a strict failure, no
VmRSS growth over 200k calls). Not tested: HTTP/3 and 3.1.
Thanks,
Haerang
2026년 10월 12일 (월) 오후 12:12, Willy Tarreau <[email protected]>님이 작성:
> Hello,
>
> I'm realizing that we never responded to this patch, very likely because
> there are two patches titled "1/2" with the same subject and we probably
> waited for 2/2 to arrive...
>
> On Thu, Sep 10, 2026 at 08:56:46PM +0900, Haerang Kim wrote:
> > The port_only converter looks for the last ':' (or ']') in its input and
> > hands everything that follows to strl2ui(), which is documented as doing
> > "no check at all". Every byte is accumulated as if it were a digit using
> > "i = i * 10 - '0' + c" on an unsigned int, so a malformed port is not
> > rejected but silently turned into an arbitrary number:
> >
> > str(example.com:abc),port_only returns 5451
> > str(example.com:80abc),port_only returns 85451
> > str(example.com:-80),port_only returns 4294967076
> > str(example.com:99999999999999999999),port_only returns 1661992959
> > str(example.com:4294967739),port_only returns 443
> >
> > The input of this converter is normally the Host header, which is under
> > the client's control and which the HTTP parsers accept with such a port,
> > over HTTP/1 as well as HTTP/2. As a result the client picks the value the
> > converter returns: "Host: example.com:ZG" makes it return 443, and so
> > does "Host: example.com:4294967739" by wrapping around on 32 bits, which
> > does not even require a non-digit character. A configuration matching on
> > the port (for example "req.hdr(host),port_only -m int eq 443") can be
> > fooled this way, and a port forwarded to the server (for example in an
> > X-Forwarded-Port header) may carry a value the client never sent. Values
> > above 65535 could be returned as well.
>
> I gnerally agree. I'm pretty sure that that some users employ it to
> isolate a number that follows a colon and that reducing the value to
> 16 bits may cause problems. But the converter's name is pretty explicit,
> it's called "port_only" and if one relies on it to extract something not
> a port number, they're taking their own risks.
>
> > Fix this by using http_get_host_port(), which already performs the lookup
> > correctly by only accepting a ':' immediately followed by digits up to
> > the end of the input,
>
> I'm not fond of that layer inversion. If we'd change the HTTP parser layer
> we'd incidentally break the port_only parser. Maybe better move that
> parser to tools.c and call it get_host_port() so that it's generic, then
> you can use it from both places.
>
> > and by converting the result with strl2irc(), which
> > rejects non-digits and overflows on its own, the way other ports are
> > already parsed in src/http_fetch.c.
>
> Great.
>
> > As before, an input without a usable
> > port makes the converter return 0, so an invalid port is not
> > distinguishable from an absent one. The documentation is updated to say
> > so.
>
> Indeed, I didn't remember we also used zero to mark the port's absence
> instead of not returning anything. That probably remains OK, though it
> means we can't differentiate "example.org:0" from "example.org" despite
> clearly being different :-/
>
> > The host_only converter is left untouched on purpose: it keeps stripping
> > everything after the last colon, so for an input such as
> > "example.com:http" it still returns "example.com" while port_only now
> > returns 0. Making the pair agree would change the result for valid inputs
> > and is not needed to fix this bug.
>
> Yeah, let's not touch that one.
>
> > The converter was introduced in 2.7 by commit dd754cba1 ("MINOR: sample:
> > add the host_only and port_only converters"), so this patch should be
> > backported to all stable branches having it (2.8 and above).
> >
> > This depends on commit 3e666065e ("BUG/MINOR: http: fix an out-of-bounds
> > read in http_get_host_port() on empty host"), without which an empty Host
> > header makes the helper read one byte past the value. That commit is
> > already present in 2.8.28, 3.0.27 and 3.2.23, but not in 3.1.17, where it
> > must be backported first.
> > ---
> > Notes for reviewers, not part of the commit message:
> >
> > Testing:
> > - built and checked on master and on the 3.2.21 release tarball, which
> > this patch applies to with line offsets only;
> > - the reg-test added by the next patch passes on both patched builds and
> > fails on an unpatched 3.2.21, where "example.com:http" gives 63544;
> > - reg-tests/converter passes as a whole (19 passed, 6 skipped for the
> > SSL/Lua features my build lacks, 0 failed);
> > - valid ports are unchanged over HTTP/1 and HTTP/2, including leading
> > zeroes and 65535.
> >
> > Two more things I deliberately did not change, in case you would rather
> > see them handled differently (the host_only case is in the commit message
> > above):
> >
> > - an invalid port and an absent port both give 0 and stay
> > indistinguishable. Making the converter fail instead would also change
> > the existing "no port" behaviour and break working configurations.
>
> Exactly. Maybe we'll add an option ("strict") to the converter so that it
> doesn't return anything on an invalid value.
>
> > - a bracket-less IPv6 literal such as "::1" still gives 1. It is not a
> > valid Host header value, and the lookup involved is shared with h1.c
> > and http_htx.c.
>
> Sure but we can't decide what users use it with. Also I'm not fond of
> backporting the <65536 limitation, that's a breaking change. I think
> that what could be done is:
> - a first patch fixing all the parsing issues (overflows and invalid
> chars), marked for backport
> - a second patch that also enforces the 0..65535 range, that will not
> be backported and that will be part of the release notes as a
> breaking change.
>
> We could imagine a third patch adding a "strict" argument that would
> make all invalid values not return anything instead of zero, including
> those outside the range 0..65535, and we could even imagine that this
> one would not return 1 for "::1". Maybe support another value "relaxed"
> thta maintains the current behavior, then we can have another patch
> changing the default from "relaxed" to "strict" for 3.5 and keeping
> relaxed for earlier versions.
>
> > diff --git a/src/sample.c b/src/sample.c
> > index 59c14b7..239e980 100644
> > --- a/src/sample.c
> > +++ b/src/sample.c
> > @@ -4059,21 +4059,22 @@ static int sample_conv_host_only(const struct arg
> > *arg_p, struct sample *smp, vo
> > static int sample_conv_port_only(const struct arg *arg_p, struct sample
> > *smp, void *private)
> > {
> > /* Working cases: hostname00, hostname00:80, 127.0.0.1, 127.0.0.1:80,
> > [::1], [::1]:80 */
> > - char *beg = smp->data.u.str.area;
> > - char *end = smp->data.u.str.area + smp->data.u.str.data - 1;
> > - char *p;
> > + struct ist port;
> > + int port_val;
> >
> > - for (p = end; p >= beg; p--) {
> > - if (*p == ':' || *p == ']')
> > - break;
> > - }
> > + port = http_get_host_port(ist2(smp->data.u.str.area,
> > smp->data.u.str.data));
>
> Your mailer mangled spaces/tabs, better send the patch attached next
> time.
>
> Thanks!
> Willy
>
v2-0001-MINOR-tools-move-http_get_host_port-to-tools.c-as.patch
Description: Binary data
v2-0005-MEDIUM-sample-make-strict-the-default-mode-of-por.patch
Description: Binary data
v2-0003-MEDIUM-sample-make-port_only-reject-ports-above-6.patch
Description: Binary data
v2-0002-BUG-MINOR-sample-fix-port_only-parsing-of-invalid.patch
Description: Binary data
v2-0004-MINOR-sample-add-a-strict-relaxed-argument-to-por.patch
Description: Binary data
