[PATCH 2/2] MINOR: connection: Transform safety check in PROXYv2 parsing into BUG_ON()
With BUG_ON() being enabled by default it is more useful to use a BUG_ON() instead of an effectively never-taken if, as any incorrect assumptions will become much more visible. see 488ee7fb6e4a388bb68153341826a6391da794e9 --- src/connection.c | 9 - 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/connection.c b/src/connection.c index f78028451..c156d9313 100644 --- a/src/connection.c +++ b/src/connection.c @@ -1098,12 +1098,11 @@ int conn_recv_proxy(struct connection *conn, int flag) } /* Verify that the PROXYv2 header ends at a TLV boundary. -* This is technically unreachable, because the TLV parsing already -* verifies that a TLV does not exceed the total length and also -* that there is space for a TLV header. +* This is can not be true, because the TLV parsing already +* verifies that a TLV does not exceed the total length and +* also that there is space for a TLV header. */ - if (tlv_offset != total_v2_len) - goto bad_header; + BUG_ON(tlv_offset != total_v2_len); /* unsupported protocol, keep local connection address */ break; -- 2.35.1
[PATCH 1/2] CLEANUP: connection: Indicate unreachability to the compiler in conn_recv_proxy
Transform the unreachability comment into a call to `my_unreachable()` to allow the compiler from benefitting from it. see d1b15b6e9b4d4d378a6169929a86f25b95eafc57 see 615f81eb5ad3e8c691901db8ce3e6a4a6b6efa49 --- src/connection.c | 10 -- 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/connection.c b/src/connection.c index 1d53eed79..f78028451 100644 --- a/src/connection.c +++ b/src/connection.c @@ -1073,9 +1073,8 @@ int conn_recv_proxy(struct connection *conn, int flag) if (!isttest(conn->proxy_authority)) goto fail; if (istcpy(&conn->proxy_authority, tlv, PP2_AUTHORITY_MAX) < 0) { - /* This is technically unreachable, because we verified above -* that the TLV value fits. -*/ + /* This is impossible, because we verified that the TLV value fits. */ + my_unreachable(); goto fail; } break; @@ -1087,9 +1086,8 @@ int conn_recv_proxy(struct connection *conn, int flag) if (!isttest(conn->proxy_unique_id)) goto fail; if (istcpy(&conn->proxy_unique_id, tlv, UNIQUEID_LEN) < 0) { - /* This is technically unreachable, because we verified above -* that the TLV value fits. -*/ + /* This is impossible, because we verified that the TLV value fits. */ + my_unreachable(); goto fail; } break; -- 2.35.1
Re: Option to reject invalid percent encoded URLs?
On Fri, Feb 25, 2022 at 11:57 AM Tim Düsterhus wrote: > As far as I can tell currently this is only possible to achieve as a > side-effect of the 'percent-*' normalizers of http-request normalize-uri: > > http-request normalize-uri percent-to-uppercase strict > > will reject requests with invalid percent-encoding (and at the same time > also uppercase the A-F characters in percent encoding). Thanks Tim, exactly what I was looking for!
Re: Option to reject invalid percent encoded URLs?
Jesse, On 2/25/22 18:25, Jesse Hathaway wrote: invalid percent encoded URLs with a 400 Bad Request. Is there an option to have HAProxy reject them as well, I couldn't find one in the docs, As far as I can tell currently this is only possible to achieve as a side-effect of the 'percent-*' normalizers of http-request normalize-uri: http-request normalize-uri percent-to-uppercase strict will reject requests with invalid percent-encoding (and at the same time also uppercase the A-F characters in percent encoding). Best regards Tim Düsterhus
Option to reject invalid percent encoded URLs?
HAProxy accepts invalid percent encoded URLs, such as http://example.com/%, whereas some web servers, such as nginx, reject invalid percent encoded URLs with a 400 Bad Request. Is there an option to have HAProxy reject them as well, I couldn't find one in the docs, though accept-invalid-http-request seems to imply HAProxy would reject invalid URLs by default? Yours kindly, Jesse Hathaway
[ANNOUNCE] haproxy-2.6-dev2
Hi, HAProxy 2.6-dev2 was released on 2022/02/25. It added 212 new commits after version 2.6-dev1. First, there was the usual bag of bug fixes for such a dev release (~35), as well as a few build fixes. As usual there are ~50 updates to QUIC; certain small but annoying internal architecture limitations are currently being addressed, so I wouldn't be surprised to see a steady flow of patches in that area in the next releases. The main focus for this version is: - end of the migration to OpenSSL 3.0's native API, except for the engines, which will continue to work with the old API for now, and will need to be architected differently for the new one (probably long and tedious work, not even sure if any engine adopted that yet). - first pass of conn_stream rework. The addition of the muxes in 1.8 forced us to insert new layers in order not to break existing stuff but these ones need to be progressively remerged with other adjacent layers to reduce the risks of bugs bugs, improve performance and maintainability. The goal is to pass through less layers between muxes and streams so that we can get back to a more linear architecture like we had in 1.6 with more direct communications between layers. Easier said than done, but at least that first series now looked stable enough to be merged instead of being continually rebased. More work is expected there in the forthcoming weeks, so that we have a solid base that will help backport fixes from 2.7-dev to 2.6 later without taking risks. - the master CLI now supports a debug mode via "mcli-debug-mode on" that allows all regular CLI commands to be visible and accessible for the master process, in order to debug it (e.g. see connections to workers etc). Expert-mode is also available and a set of flags indicating the current mode are now displayed in the prompt. - httpclient updates, to support forcing a destination and setting a transfer timeout. - improved debugging of the memory pools: now instead of fiddling with build-time options to enable/disable certain debugging features, it becomes possible to enable/disable them from the command line by passing some memory debugging keywords after "-dM". The build-time options still exist and only fix the default settings. - small improvements to the BUG_ON() macro and its cousins. More are expected to come soon so that we can start to stuff them at plenty of other places in the code and allow the level of validation to be adjusted at build time, and more importantly that the reasonable checks remain active by default. - basic pool debugging and first-level BUG_ON() are now enabled by default in the makefile. - the CI now runs with full debugging turned on, and Coverity now also covers QUIC - a few new regtests, mainly for SSL stuff. I sincerely hope that the long series of painful bugs that kept us away from coding is far behind and that we'll soon be able to add more entries above. Stable branches 2.3 and older should see a release next week I think. Please find the usual URLs below : Site index : http://www.haproxy.org/ Discourse: http://discourse.haproxy.org/ Slack channel: https://slack.haproxy.org/ Issue tracker: https://github.com/haproxy/haproxy/issues Wiki : https://github.com/haproxy/wiki/wiki Sources : http://www.haproxy.org/download/2.6/src/ Git repository : http://git.haproxy.org/git/haproxy.git/ Git Web browsing : http://git.haproxy.org/?p=haproxy.git Changelog: http://www.haproxy.org/download/2.6/src/CHANGELOG Cyril's HTML doc : http://cbonte.github.io/haproxy-dconv/ Willy --- Complete changelog : Amaury Denoyelle (30): MINOR: h3: hardcode the stream id of control stream MINOR: mux-quic: remove quic_transport_params_update MINOR: quic: rename local tid variable MINOR: quic: remove unused xprt rcv_buf operation MINOR: quic: take out xprt snd_buf operation MINOR: quic: use a global dghlrs for each thread BUG/MEDIUM: quic: fix crash on CC if mux not present MINOR: qpack: fix typo in trace BUG/MINOR: quic: fix FIN stream signaling BUG/MINOR: h3: fix the header length for QPACK decoding MINOR: h3: remove transfer-encoding header MINOR: h3: add documentation on h3_decode_qcs MINOR: h3: set properly HTX EOM/BODYLESS on HEADERS parsing MINOR: mux-quic: implement rcv_buf MINOR: mux-quic: set EOS on rcv_buf MINOR: h3: set CS_FL_NOT_FIRST MINOR: h3: report frames bigger than rx buffer MINOR: h3: extract HEADERS parsing in a dedicated function MINOR: h3: implement DATA parsing MINOR: h3: report error on HEADERS/DATA parsing MINOR: h3: remove unused return value on decode_qcs MINOR: mux-quic: fix a possible null dereference in qc_timeout_task MINOR: quic: do not
[ANNOUNCE] haproxy-2.4.14
Hi, HAProxy 2.4.14 was released on 2022/02/25. It added 26 new commits after version 2.4.13. The main issues fixed in this version are: - A major issue in the H2 multiplexer. An error during the response processing, after the HEADERS frame parsing, led to a wakeup loop consuming all the CPU because the error was not properly reported to the upper layer. For instance, this happened if an invalid header value, an invalid status code or a forbidden header was found in the response. Note that only HAProxy >= 2.4 are affected by this issue. - A FD leak on reload failures. When the master process is reloaded on a new config, it will try to connect to the previous process' socket to retrieve all known listening FDs to be reused by the new listeners. If listeners were removed, their unused FDs are simply closed. However there's a catch. In case a socket fails to bind, the master will cancel its startup and switch to wait mode for a new operation to happen. In this case it didn't close the possibly remaining FDs that were left unused. - A FD leak of a sockpair upon a failed reload. When starting HAProxy in master-worker, the master pre-allocate a struct mworker_proc and do a socketpair() before the configuration parsing. If the configuration loading failed, the FD was never closed because they aren't part of listener, they are not even in the fdtab. - Some issues about errors on buffers allocation. First, in the H1 multiplexer. If we failed to send data because we failed to allocate the H1 output buffer, the H1 stream was erroneously woken up. This led to a wakeup loop to send more data while it is not possible because there is no output buffer. Then, in process_stream(), if we failed to allocate the channel response buffer while a connect or an analysis timeout occurred, the stream was woken up in loop because its task was requeued with an expired date. Now an error is reported when this happens and the stream processing is interrupted. Note there is a mechanism to deal with errors on buffers allocation. Unfortunately, since the 1.7, this mechanism is broken. And it is even worse now with the multiplexers. All this part must be refactored. But for now, HAProxy may be partially frozen if too many entities are waiting for a buffer. - Some alignment problems that were found when using gcc-11 + RHEL8, resulting in instant crashes on startup. - An issue with multi-line ESMTP response in the mailer code. - An issue in the resolvers code with domain names with a trailing dot. The trailing dot was not ignored as expected and a junk character was added at the end of the encoded part of the domain name. The remaining is the usual bunch of fixes and improvements. As usual, people using the 2.4 branch are encouraged to migrate to this version. Thanks everyone for your help and your contributions! Please find the usual URLs below : Site index : http://www.haproxy.org/ Discourse: http://discourse.haproxy.org/ Slack channel: https://slack.haproxy.org/ Issue tracker: https://github.com/haproxy/haproxy/issues Wiki : https://github.com/haproxy/wiki/wiki Sources : http://www.haproxy.org/download/2.4/src/ Git repository : http://git.haproxy.org/git/haproxy-2.4.git/ Git Web browsing : http://git.haproxy.org/?p=haproxy-2.4.git Changelog: http://www.haproxy.org/download/2.4/src/CHANGELOG Cyril's HTML doc : http://cbonte.github.io/haproxy-dconv/ --- Complete changelog : Christopher Faulet (6): BUG/MINOR: sink: Use the right field in appctx context in release callback BUG/MEDIUM: resolvers: Really ignore trailing dot in domain names BUG/MEDIUM: htx: Be sure to have a buffer to perform a raw copy of a message BUG/MEDIUM: mux-h1: Don't wake h1s if mux is blocked on lack of output buffer BUG/MAJOR: mux-h2: Be sure to always report HTX parsing error to the app layer BUG/MEDIUM: stream: Abort processing if response buffer allocation fails Ilya Shipitsin (4): BUILD: adopt script/build-ssl.sh for OpenSSL-3.0.0beta2 CI: github actions: add OpenSSL-3.0.0 builds CI: github actions: relax OpenSSL-3.0.0 version comparision CI: github actions: update OpenSSL to 3.0.1 Lukas Tribus (1): BUG/MINOR: mailers: negotiate SMTP, not ESMTP William Lallemand (5): BUG/MINOR: mworker: fix a FD leak of a sockpair upon a failed reload BUILD: fix compilation for OpenSSL-3.0.0-alpha17 CI: github actions: -Wno-deprecated-declarations with OpenSSL 3.0.0 CI: github: switch to OpenSSL 3.0.0 BUG/MINOR: tools: url2sa reads ipv4 too far Willy Tarreau (10): MINOR: sock: move the unused socket cleaning code into its own function BUG/MEDIUM: mworker: close unused transferred FDs on load failure BUG/MEDIUM: fd: always alig
[ANNOUNCE] haproxy-2.5.4
Hi, HAProxy 2.5.4 was released on 2022/02/25. It added 8 new commits after version 2.5.3. A major issue in the H2 multiplexer was fixed in this release. An error during the response processing, after the HEADERS frame parsing, led to a wakeup loop consuming all the CPU because the error was not properly reported to the upper layer. For instance, this happened if an invalid header value, an invalid status code or a forbidden header was found in the response. Note that only HAProxy >= 2.4 are affected by this issue. In addition, some issues about errors on buffers allocation were fixed. First, in the H1 multiplexer. If we failed to send data because we failed to allocate the H1 output buffer, the H1 stream was erroneously woken up. This led to a wakeup loop to send more data while it is not possible because there is no output buffer. Then, in process_stream(), if we failed to allocate the channel response buffer while a connect or an analysis timeout occurred, the stream was woken up in loop because its task was requeued with an expired date. Now an error is reported when this happens and the stream processing is interrupted. Note there is a mechanism to deal with errors on buffers allocation. Unfortunately, since the 1.7, this mechanism is broken. And it is even worse now with the multiplexers. All this part must be refactored. But for now, HAProxy may be partially Frozen if too many entities are waiting for a buffer. Other commits are doc improvements and small fixes here and there. As usual, people using the 2.5 branch are encouraged to migrate to this version. Thanks everyone for your help and your contributions! Please find the usual URLs below : Site index : http://www.haproxy.org/ Discourse: http://discourse.haproxy.org/ Slack channel: https://slack.haproxy.org/ Issue tracker: https://github.com/haproxy/haproxy/issues Wiki : https://github.com/haproxy/wiki/wiki Sources : http://www.haproxy.org/download/2.5/src/ Git repository : http://git.haproxy.org/git/haproxy-2.5.git/ Git Web browsing : http://git.haproxy.org/?p=haproxy-2.5.git Changelog: http://www.haproxy.org/download/2.5/src/CHANGELOG Cyril's HTML doc : http://cbonte.github.io/haproxy-dconv/ --- Complete changelog : Christian Ruppert (1): DOC: Fix usage/examples of deprecated ACLs Christopher Faulet (4): BUG/MEDIUM: htx: Be sure to have a buffer to perform a raw copy of a message BUG/MEDIUM: mux-h1: Don't wake h1s if mux is blocked on lack of output buffer BUG/MAJOR: mux-h2: Be sure to always report HTX parsing error to the app layer BUG/MEDIUM: stream: Abort processing if response buffer allocation fails Willy Tarreau (3): BUG/MINOR: proxy: preset the error message pointer to NULL in parse_new_proxy() REGTESTS: fix the race conditions in 40be_2srv_odd_health_checks CI: github: enable pool debugging by default -- Christopher Faulet
Re: [PATCH] MINOR: sample: Add srv_rtt server round trip time sample
On Fri, Feb 25, 2022 at 03:39:02PM +0100, Aleksandar Lazic wrote: > > > diff --git a/reg-tests/sample_fetches/srv_rtt.vtc > > > b/reg-tests/sample_fetches/srv_rtt.vtc > > > new file mode 100644 > > > index 0..c0ad0cbae > > > --- /dev/null > > > +++ b/reg-tests/sample_fetches/srv_rtt.vtc > > > @@ -0,0 +1,34 @@ > > > +varnishtest "srv_rtt sample fetch Test" > > > + > > > +#REQUIRE_VERSION=2.6 > > > > Note, we *might* need to add a new macro to detect support for TCP_INFO. > > We still don't have the config predicates to detect support for certain > > keywords or sample fetch functions so that's not easy, but it's possible > > that this test will break on some OS like cygwin. If so we could work > > around this temporarily using "EXCLUDE_TARGETS" and in the worst case we > > could mark it broken for the time it takes to completely solve this. > > Agree here the suggestion is to add something like USE_GETSOCKOPT to be able > to make '#REQUIRE_OPTIONS=GETSOCKOPT', something like similar to > USE_GETADDRINFO, right? No, it's not even this, it would in fact need to detect TCP_INFO which comes from the system's includes. That's why we won't do that and instead we'll later finish to add more predicates to detect supported config keywords. This way if we mention a dependency on "bc_rtt", we know we can skip this test on systems which don't include it based on haproxy's internal features detection at build time. Do not worry for this one, leave it as-is, we'll see what breaks on the CI (likely only cygwin) and we'll exclude it/them. Cheers, Willy
Re: [PATCH] MINOR: sample: Add srv_rtt server round trip time sample
Hi Willy. On 25.02.22 14:54, Willy Tarreau wrote: Hi Alex, On Thu, Feb 24, 2022 at 03:03:59AM +0100, Aleksandar Lazic wrote: Hi. Here the first patch for feature request "New Balancing algorithm (Peak) EWMA #1570" Note, I don't think it is needed for this algo as long as we instead use measured response time and/or health check time. But regardless it's something useful to have. A few comments below: Thanks you for your much valuable feedback. I think also that the rtt information as fetch sample could be useful. From e95bf6a4bf107fdc59696c4b4a4ef7b03133b813 Mon Sep 17 00:00:00 2001 From: Aleksandar Lazic Date: Thu, 24 Feb 2022 02:56:21 +0100 Subject: [PATCH] MINOR: sample: Add srv_rtt server round trip time sample This sample fetch get the server round trip time You should mention "TCP round trip time" since it's measured at the TCP level. +srv_rtt : integer + Returns the Round Trip Time (RTT) measured by the kernel for the server + connection. is facultative, by default the unit is milliseconds. + can be set to "ms" for milliseconds or "us" for microseconds. If the server + connection is not established, if the connection is not TCP or if the + operating system does not support TCP_INFO, for example Linux kernels before + 2.4, the sample fetch fails. I would rather call it "bc_rtt" since it's not the server but the backend connection. Technically speaking it indeed requires a connection to be established and will only report the value for *this* connection and not anything stateless related to the server. Thta's more in line with what we have for the frontend connection already with fc_rtt. You mentioned "unit" but it does not appear in the keyword syntax. Also I think it would be useful to have all other fc_* from the same section turned to bc_* (fc_rttvar, fc_retrans, etc), as it can sometimes explain long response times in logs. diff --git a/reg-tests/sample_fetches/srv_rtt.vtc b/reg-tests/sample_fetches/srv_rtt.vtc new file mode 100644 index 0..c0ad0cbae --- /dev/null +++ b/reg-tests/sample_fetches/srv_rtt.vtc @@ -0,0 +1,34 @@ +varnishtest "srv_rtt sample fetch Test" + +#REQUIRE_VERSION=2.6 Note, we *might* need to add a new macro to detect support for TCP_INFO. We still don't have the config predicates to detect support for certain keywords or sample fetch functions so that's not easy, but it's possible that this test will break on some OS like cygwin. If so we could work around this temporarily using "EXCLUDE_TARGETS" and in the worst case we could mark it broken for the time it takes to completely solve this. Agree here the suggestion is to add something like USE_GETSOCKOPT to be able to make '#REQUIRE_OPTIONS=GETSOCKOPT', something like similar to USE_GETADDRINFO, right? (...) diff --git a/src/tcp_sample.c b/src/tcp_sample.c index 19edcd243..7b8b616cb 100644 --- a/src/tcp_sample.c +++ b/src/tcp_sample.c @@ -446,6 +446,20 @@ smp_fetch_fc_reordering(const struct arg *args, struct sample *smp, const char * return 0; return 1; } + +/* get the mean rtt of a client connection */ +static int +smp_fetch_srv_rtt(const struct arg *args, struct sample *smp, const char *kw, void *private) +{ + if (!get_tcp_info(args, smp, 1, 0)) + return 0; + + /* By default or if explicitly specified, convert rtt to ms */ + if (!args || args[0].type == ARGT_STOP || args[0].data.sint == TIME_UNIT_MS) + smp->data.u.sint = (smp->data.u.sint + 500) / 1000; + + return 1; +} That's another reason for extending the existing keywords, avoiding code duplication. You can have all your new keywords map to the fc_* equivalent and just change this: - if (!get_tcp_info(args, smp, 0, 0)) + if (!get_tcp_info(args, smp, *kw == 'b', 0)) Please update the comments on top of the functions to mention that they're called for both "fc_*" and "bc_*" depending on the side, and that's OK. Let me go back to the "drawing board" and will send a update as soon as there is a update :-) thanks, Willy Regards Alex
Re: [PATCH] MINOR: sample: Add srv_rtt server round trip time sample
Hi Alex, On Thu, Feb 24, 2022 at 03:03:59AM +0100, Aleksandar Lazic wrote: > Hi. > > Here the first patch for feature request "New Balancing algorithm (Peak) EWMA > #1570" Note, I don't think it is needed for this algo as long as we instead use measured response time and/or health check time. But regardless it's something useful to have. A few comments below: > From e95bf6a4bf107fdc59696c4b4a4ef7b03133b813 Mon Sep 17 00:00:00 2001 > From: Aleksandar Lazic > Date: Thu, 24 Feb 2022 02:56:21 +0100 > Subject: [PATCH] MINOR: sample: Add srv_rtt server round trip time sample > > This sample fetch get the server round trip time You should mention "TCP round trip time" since it's measured at the TCP level. > +srv_rtt : integer > + Returns the Round Trip Time (RTT) measured by the kernel for the server > + connection. is facultative, by default the unit is milliseconds. > > + can be set to "ms" for milliseconds or "us" for microseconds. If the server > + connection is not established, if the connection is not TCP or if the > + operating system does not support TCP_INFO, for example Linux kernels > before > + 2.4, the sample fetch fails. I would rather call it "bc_rtt" since it's not the server but the backend connection. Technically speaking it indeed requires a connection to be established and will only report the value for *this* connection and not anything stateless related to the server. Thta's more in line with what we have for the frontend connection already with fc_rtt. You mentioned "unit" but it does not appear in the keyword syntax. Also I think it would be useful to have all other fc_* from the same section turned to bc_* (fc_rttvar, fc_retrans, etc), as it can sometimes explain long response times in logs. > diff --git a/reg-tests/sample_fetches/srv_rtt.vtc > b/reg-tests/sample_fetches/srv_rtt.vtc > new file mode 100644 > index 0..c0ad0cbae > --- /dev/null > +++ b/reg-tests/sample_fetches/srv_rtt.vtc > @@ -0,0 +1,34 @@ > +varnishtest "srv_rtt sample fetch Test" > + > +#REQUIRE_VERSION=2.6 Note, we *might* need to add a new macro to detect support for TCP_INFO. We still don't have the config predicates to detect support for certain keywords or sample fetch functions so that's not easy, but it's possible that this test will break on some OS like cygwin. If so we could work around this temporarily using "EXCLUDE_TARGETS" and in the worst case we could mark it broken for the time it takes to completely solve this. (...) > diff --git a/src/tcp_sample.c b/src/tcp_sample.c > index 19edcd243..7b8b616cb 100644 > --- a/src/tcp_sample.c > +++ b/src/tcp_sample.c > @@ -446,6 +446,20 @@ smp_fetch_fc_reordering(const struct arg *args, struct > sample *smp, const char * > return 0; > return 1; > } > + > +/* get the mean rtt of a client connection */ > +static int > +smp_fetch_srv_rtt(const struct arg *args, struct sample *smp, const char > *kw, void *private) > +{ > + if (!get_tcp_info(args, smp, 1, 0)) > + return 0; > + > + /* By default or if explicitly specified, convert rtt to ms */ > + if (!args || args[0].type == ARGT_STOP || args[0].data.sint == > TIME_UNIT_MS) > + smp->data.u.sint = (smp->data.u.sint + 500) / 1000; > + > + return 1; > +} That's another reason for extending the existing keywords, avoiding code duplication. You can have all your new keywords map to the fc_* equivalent and just change this: - if (!get_tcp_info(args, smp, 0, 0)) + if (!get_tcp_info(args, smp, *kw == 'b', 0)) Please update the comments on top of the functions to mention that they're called for both "fc_*" and "bc_*" depending on the side, and that's OK. thanks, Willy