Hi Willy, Following up on my previous email regarding the v2 patch series ([PATCH v2] port_only: invalid port parsing fix and strict/relaxed modes), I wanted to check if you have had a chance to look over it or if it is still under review.
Please let me know if any updates or further changes are needed. Thanks, Haerang 2026년 10월 17일 (토) 오전 4:50, Haerang Kim <[email protected]>님이 작성: > 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 <(214)%20748-3648>..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 >> >
