]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
2 years agoBUG/MEDIUM: stconn: Don't rearm the read expiration date if EOI was reached
Christopher Faulet [Tue, 14 Feb 2023 10:01:51 +0000 (11:01 +0100)] 
BUG/MEDIUM: stconn: Don't rearm the read expiration date if EOI was reached

At the stream level, the read expiration date is unset if a shutr was
received but not if the end of input was reached. If we know no more data
are excpected, there is no reason to let the read expiration date armed,
except to respect clientfin/serverfin timeout on some circumstances.

This patch could slowly be backported as far as 2.2.

2 years agoBUG/MEDIUM: http-ana: Detect closed SC on opposite side during body forwarding
Christopher Faulet [Tue, 14 Feb 2023 09:48:02 +0000 (10:48 +0100)] 
BUG/MEDIUM: http-ana: Detect closed SC on opposite side during body forwarding

During the payload forwarding, since the commit f2b02cfd9 ("MAJOR: http-ana:
Review error handling during HTTP payload forwarding"), when an error
occurred on one side, we don't rely anymore on a specific HTTP message state
to detect it on the other side. However, nothing was added to detect the
error. Thus, when this happens, a spinning loop may be experienced and an
abort because of the watchdog.

To fix the bug, we must detect the opposite side is closed by checking the
opposite SC state. Concretly, in http_end_request() and http_end_response(),
we wait for the other side iff the HTTP message state is lower to
HTTP_MSG_DONE (the message is not finished) and the SC state is not
SC_ST_CLO (the opposite side is not closed). In these function, we don't
care if there was an error on the opposite side. We only take care to detect
when we must stop waiting the other side.

This patch should fix the issue #2042. No backport needed.

2 years agoBUG/MINOR: config: crt-list keywords mistaken for bind ssl keywords
William Lallemand [Mon, 13 Feb 2023 14:24:01 +0000 (15:24 +0100)] 
BUG/MINOR: config: crt-list keywords mistaken for bind ssl keywords

This patch fixes an issue in the "-dK" keywords dumper, which was
mistakenly displaying the "crt-list" keywords for "bind ssl" keywords.

The patch fixes the issue by dumping the "crt-list" keywords in its own
section, and dumping the "bind" keywords which are in the "SSL" scope
with a "bind ssl" prefix.

This commit depends on the previous "MINOR: ssl: rename confusing
ssl_bind_kws" commit.

Must be backported in 2.6.

Diff of the `./haproxy -dKall -q -c -f /dev/null` output before and
after the patch in 2.8-dev4:

     | @@ -190,30 +190,9 @@ listen
     |   use-fcgi-app
     |   bind <addr> accept-netscaler-cip +1
     |   bind <addr> accept-proxy
     | - bind <addr> allow-0rtt
     | - bind <addr> alpn +1
     |   bind <addr> backlog +1
     | - bind <addr> ca-file +1
     | - bind <addr> ca-ignore-err +1
     | - bind <addr> ca-sign-file +1
     | - bind <addr> ca-sign-pass +1
     | - bind <addr> ca-verify-file +1
     | - bind <addr> ciphers +1
     | - bind <addr> ciphersuites +1
     | - bind <addr> crl-file +1
     | - bind <addr> crt +1
     | - bind <addr> crt-ignore-err +1
     | - bind <addr> crt-list +1
     | - bind <addr> curves +1
     |   bind <addr> defer-accept
     | - bind <addr> ecdhe +1
     |   bind <addr> expose-fd +1
     | - bind <addr> force-sslv3
     | - bind <addr> force-tlsv10
     | - bind <addr> force-tlsv11
     | - bind <addr> force-tlsv12
     | - bind <addr> force-tlsv13
     | - bind <addr> generate-certificates
     |   bind <addr> gid +1
     |   bind <addr> group +1
     |   bind <addr> id +1
     | @@ -225,48 +204,52 @@ listen
     |   bind <addr> name +1
     |   bind <addr> namespace +1
     |   bind <addr> nice +1
     | - bind <addr> no-ca-names
     | - bind <addr> no-sslv3
     | - bind <addr> no-tls-tickets
     | - bind <addr> no-tlsv10
     | - bind <addr> no-tlsv11
     | - bind <addr> no-tlsv12
     | - bind <addr> no-tlsv13
     | - bind <addr> npn +1
     | - bind <addr> prefer-client-ciphers
     |   bind <addr> process +1
     |   bind <addr> proto +1
     |   bind <addr> severity-output +1
     |   bind <addr> shards +1
     | - bind <addr> ssl
     | - bind <addr> ssl-max-ver +1
     | - bind <addr> ssl-min-ver +1
     | - bind <addr> strict-sni
     |   bind <addr> tcp-ut +1
     |   bind <addr> tfo
     |   bind <addr> thread +1
     | - bind <addr> tls-ticket-keys +1
     |   bind <addr> transparent
     |   bind <addr> uid +1
     |   bind <addr> user +1
     |   bind <addr> v4v6
     |   bind <addr> v6only
     | - bind <addr> verify +1
     |   bind <addr> ssl allow-0rtt
     |   bind <addr> ssl alpn +1
     |   bind <addr> ssl ca-file +1
     | + bind <addr> ssl ca-ignore-err +1
     | + bind <addr> ssl ca-sign-file +1
     | + bind <addr> ssl ca-sign-pass +1
     |   bind <addr> ssl ca-verify-file +1
     |   bind <addr> ssl ciphers +1
     |   bind <addr> ssl ciphersuites +1
     |   bind <addr> ssl crl-file +1
     | + bind <addr> ssl crt +1
     | + bind <addr> ssl crt-ignore-err +1
     | + bind <addr> ssl crt-list +1
     |   bind <addr> ssl curves +1
     |   bind <addr> ssl ecdhe +1
     | + bind <addr> ssl force-sslv3
     | + bind <addr> ssl force-tlsv10
     | + bind <addr> ssl force-tlsv11
     | + bind <addr> ssl force-tlsv12
     | + bind <addr> ssl force-tlsv13
     | + bind <addr> ssl generate-certificates
     |   bind <addr> ssl no-ca-names
     | + bind <addr> ssl no-sslv3
     | + bind <addr> ssl no-tls-tickets
     | + bind <addr> ssl no-tlsv10
     | + bind <addr> ssl no-tlsv11
     | + bind <addr> ssl no-tlsv12
     | + bind <addr> ssl no-tlsv13
     |   bind <addr> ssl npn +1
     | - bind <addr> ssl ocsp-update +1
     | + bind <addr> ssl prefer-client-ciphers
     |   bind <addr> ssl ssl-max-ver +1
     |   bind <addr> ssl ssl-min-ver +1
     | + bind <addr> ssl strict-sni
     | + bind <addr> ssl tls-ticket-keys +1
     |   bind <addr> ssl verify +1
     |   server <name> <addr> addr +1
     |   server <name> <addr> agent-addr +1
     | @@ -591,6 +574,23 @@ listen
     |   http-after-response unset-var*
     |  userlist
     |  peers
     | +crt-list
     | + allow-0rtt
     | + alpn +1
     | + ca-file +1
     | + ca-verify-file +1
     | + ciphers +1
     | + ciphersuites +1
     | + crl-file +1
     | + curves +1
     | + ecdhe +1
     | + no-ca-names
     | + npn +1
     | + ocsp-update +1
     | + ssl-max-ver +1
     | + ssl-min-ver +1
     | + verify +1
     |  # List of registered CLI keywords:
     |  @!<pid> [MASTER]
     |  @<relative pid> [MASTER]

2 years agoMINOR: ssl: rename confusing ssl_bind_kws
William Lallemand [Mon, 13 Feb 2023 09:58:13 +0000 (10:58 +0100)] 
MINOR: ssl: rename confusing ssl_bind_kws

The ssl_bind_kw structure is exclusively used for crt-list keyword, it
must be named otherwise to remove the confusion.

The structure was renamed ssl_crtlist_kws.

2 years ago[RELEASE] Released version 2.8-dev4 v2.8-dev4
Willy Tarreau [Tue, 14 Feb 2023 15:55:17 +0000 (16:55 +0100)] 
[RELEASE] Released version 2.8-dev4

Released version 2.8-dev4 with the following main changes :
    - BUG/MINOR: stats: fix source buffer size for http dump
    - BUG/MEDIUM: stats: fix resolvers dump
    - BUG/MINOR: stats: fix ctx->field update in stats_dump_proxy_to_buffer()
    - BUG/MINOR: stats: fix show stats field ctx for servers
    - BUG/MINOR: stats: fix STAT_STARTED behavior with full htx
    - MINOR: quic: Update version_information transport parameter to draft-14
    - BUG/MINOR: stats: Prevent HTTP "other sessions" counter underflows
    - BUG/MEDIUM: thread: fix extraneous shift in the thread_set parser
    - BUG/MEDIUM: listener/thread: bypass shards setting on failed thread resolution
    - BUG/MINOR: ssl/crt-list: warn when a line is malformated
    - BUG/MEDIUM: stick-table: do not leave entries in end of window during purge
    - BUG/MINOR: clock: do not mix wall-clock and monotonic time in uptime calculation
    - BUG/MEDIUM: cache: use the correct time reference when comparing dates
    - MEDIUM: clock: force internal time to wrap early after boot
    - BUILD: ssl/ocsp: ssl_ocsp-t.h depends on ssl_sock-t.h
    - MINOR: ssl/ocsp: add a function to check the OCSP update configuration
    - MINOR: cfgparse/server: move (min/max)conn postparsing logic into dedicated function
    - BUG/MINOR: server/add: ensure minconn/maxconn consistency when adding server
    - BUG/MEDIUM: stconn: Schedule a shutw on shutr if data must be sent first
    - BUG/MEDIUM: quic: fix crash when "option nolinger" is set in the frontend
    - MINOR: quic: implement a basic "show quic" CLI handler
    - MINOR: quic: display CIDs and state in "show quic"
    - MINOR: quic: display socket info on "show quic"
    - MINOR: quic: display infos about various encryption level on "show quic"
    - MINOR: quic: display Tx stream info on "show quic"
    - MINOR: quic: filter closing conn on "show quic"
    - BUG/MINOR: quic: fix filtering of closing connections on "show quic"
    - BUG/MEDIUM: stconn: Don't needlessly wake the stream on send during fast-forward
    - BUG/MINOR: quic: fix type bug on "show quic" for 32-bits arch
    - BUG/MINOR: mworker: fix uptime for master process
    - BUG/MINOR: clock/stats: also use start_time not start_date in HTML info
    - BUG/MEDIUM: stconn: stop to enable/disable reads from streams via si_update_rx
    - BUG/MEDIUM: quic: Buffer overflow when looking through QUIC CLI keyword list
    - DOC: proxy-protocol: fix wrong byte in provided example
    - MINOR: ssl-ckch: Stop to test CF_WRITE_ERROR to commit CA/CRL file
    - MINOR: bwlim: Remove useless test on CF_READ_ERROR to detect the last packet
    - BUG/MINOR: http-ana: Fix condition to set LAST termination flag
    - BUG/MINOR: mux-h1: Don't report an H1C error on client timeout
    - BUG/MEDIUM: spoe: Don't set the default traget for the SPOE agent frontend
    - BUG/MINOR: quic: Wrong datagram dispatch because of qc_check_dcid()
    - BUG/CRITICAL: http: properly reject empty http header field names

2 years agoBUG/CRITICAL: http: properly reject empty http header field names
Willy Tarreau [Thu, 9 Feb 2023 20:36:54 +0000 (21:36 +0100)] 
BUG/CRITICAL: http: properly reject empty http header field names

The HTTP header parsers surprizingly accepts empty header field names,
and this is a leftover from the original code that was agnostic to this.

When muxes were introduced, for H2 first, the HPACK decompressor needed
to feed headers lists, and since empty header names were strictly
forbidden by the protocol, the lists of headers were purposely designed
to be terminated by an empty header field name (a principle that is
similar to H1's empty line termination). This principle was preserved
and generalized to other protocols migrated to muxes (H1/FCGI/H3 etc)
without anyone ever noticing that the H1 parser was still able to deliver
empty header field names to this list. In addition to this it turns out
that the HPACK decompressor, despite a comment in the code, may
successfully decompress an empty header field name, and this mistake
was propagated to the QPACK decompressor as well.

The impact is that an empty header field name may be used to truncate
the list of headers and thus make some headers disappear. While for
H2/H3 the impact is limited as haproxy sees a request with missing
headers, and headers are not used to delimit messages, in the case of
HTTP/1, the impact is significant because the presence (and sometimes
contents) of certain sensitive headers is detected during the parsing.
Thus, some of these headers may be seen, marked as present, their value
extracted, but never delivered to upper layers and obviously not
forwarded to the other side either. This can have for consequence that
certain important header fields such as Connection, Upgrade, Host,
Content-length, Transfer-Encoding etc are possibly seen as different
between what haproxy uses to parse/forward/route and what is observed
in http-request rules and of course, forwarded. One direct consequence
is that it is possible to exploit this property in HTTP/1 to make
affected versions of haproxy forward more data than is advertised on
the other side, and bypass some access controls or routing rules by
crafting extraneous requests.  Note, however, that responses to such
requests will normally not be passed back to the client, but this can
still cause some harm.

This specific risk can be mostly worked around in configuration using
the following rule that will rely on the bug's impact to precisely
detect the inconsistency between the known body size and the one
expected to be advertised to the server (the rule works from 2.0 to
2.8-dev):

  http-request deny if { fc_http_major 1 } !{ req.body_size 0 } !{ req.hdr(content-length) -m found } !{ req.hdr(transfer-encoding) -m found } !{ method CONNECT }

This will exclusively block such carefully crafted requests delivered
over HTTP/1. HTTP/2 and HTTP/3 do not need content-length, and a body
that arrives without being announced with a content-length will be
forwarded using transfer-encoding, hence will not cause discrepancies.
In HAProxy 2.0 in legacy mode ("no option http-use-htx"), this rule will
simply have no effect but will not cause trouble either.

A clean solution would consist in modifying the loops iterating over
these headers lists to check the header name's pointer instead of its
length (since both are zero at the end of the list), but this requires
to touch tens of places and it's very easy to miss one. Functions such
as htx_add_header(), htx_add_trailer(), htx_add_all_headers() would be
good starting points for such a possible future change.

Instead the current fix focuses on blocking empty headers where they
are first inserted, hence in the H1/HPACK/QPACK decoders. One benefit
of the current solution (for H1) is that it allows "show errors" to
report a precise diagnostic when facing such invalid HTTP/1 requests,
with the exact location of the problem and the originating address:

  $ printf "GET / HTTP/1.1\r\nHost: localhost\r\n:empty header\r\n\r\n" | nc 0 8001
  HTTP/1.1 400 Bad request
  Content-length: 90
  Cache-Control: no-cache
  Connection: close
  Content-Type: text/html

  <html><body><h1>400 Bad request</h1>
  Your browser sent an invalid request.
  </body></html>

  $ socat /var/run/haproxy.stat <<< "show errors"
  Total events captured on [10/Feb/2023:16:29:37.530] : 1

  [10/Feb/2023:16:29:34.155] frontend decrypt (#2): invalid request
    backend <NONE> (#-1), server <NONE> (#-1), event #0, src 127.0.0.1:31092
    buffer starts at 0 (including 0 out), 16334 free,
    len 50, wraps at 16336, error at position 33
    H1 connection flags 0x00000000, H1 stream flags 0x00000810
    H1 msg state MSG_HDR_NAME(17), H1 msg flags 0x00001410
    H1 chunk len 0 bytes, H1 body len 0 bytes :

    00000  GET / HTTP/1.1\r\n
    00016  Host: localhost\r\n
    00033  :empty header\r\n
    00048  \r\n

I want to address sincere and warm thanks for their great work to the
team composed of the following security researchers who found the issue
together and reported it: Bahruz Jabiyev, Anthony Gavazzi, and Engin
Kirda from Northeastern University, Kaan Onarlioglu from Akamai
Technologies, Adi Peleg and Harvey Tuch from Google. And kudos to Amaury
Denoyelle from HAProxy Technologies for spotting that the HPACK and
QPACK decoders would let this pass despite the comment explicitly
saying otherwise.

This fix must be backported as far as 2.0. The QPACK changes can be
dropped before 2.6. In 2.0 there is also the equivalent code for legacy
mode, which doesn't suffer from the list truncation, but it would better
be fixed regardless.

CVE-2023-25725 was assigned to this issue.

2 years agoBUG/MINOR: quic: Wrong datagram dispatch because of qc_check_dcid()
Frédéric Lécaille [Mon, 13 Feb 2023 15:14:24 +0000 (16:14 +0100)] 
BUG/MINOR: quic: Wrong datagram dispatch because of qc_check_dcid()

There was a parenthesis placed in the wrong place for a memcmp().
As a consequence, clients could not reuse a UDP address for a new connection.

Must be backported to 2.7.

2 years agoBUG/MEDIUM: spoe: Don't set the default traget for the SPOE agent frontend
Christopher Faulet [Mon, 13 Feb 2023 10:37:26 +0000 (11:37 +0100)] 
BUG/MEDIUM: spoe: Don't set the default traget for the SPOE agent frontend

The commit d5983cef8 ("MINOR: listener: remove the useless ->default_target
field") revealed a bug in the SPOE. No default-target must be defined for
the SPOE agent frontend. SPOE applets are used on the frontend side and a
TCP connection is established on the backend side.

Because of this bug, since the commit above, the stream target is set to the
SPOE applet instead of the backend connection, leading to a spinning loop on
the applet when it is released because are unable to close the backend side.

This patch should fix the issue #2040. It only affects the 2.8-DEV but to
avoid any future bug, it should be backported to all stable versions.

2 years agoBUG/MINOR: mux-h1: Don't report an H1C error on client timeout
Christopher Faulet [Mon, 6 Feb 2023 17:14:47 +0000 (18:14 +0100)] 
BUG/MINOR: mux-h1: Don't report an H1C error on client timeout

When a client timeout is reported by the H1 mux, it is not an error but an
abort. Thus, H1C_F_ERROR flag must not be set. It is espacially important to
not inhibit the send. Because of this bug, a 408-Request-time-out is
reported in logs but the error message is not sent to the client.

This patch must be backported to 2.7.

2 years agoBUG/MINOR: http-ana: Fix condition to set LAST termination flag
Christopher Faulet [Thu, 26 Jan 2023 18:02:07 +0000 (19:02 +0100)] 
BUG/MINOR: http-ana: Fix condition to set LAST termination flag

We should not report LAST data in log if the response is in TUNNEL mode on
client close/timeout because there is no way to be sure it is the last
data. It means, it can only be reported in DONE, CLOSING or CLOSE states.

No backport needed.

2 years agoMINOR: bwlim: Remove useless test on CF_READ_ERROR to detect the last packet
Christopher Faulet [Thu, 26 Jan 2023 15:11:02 +0000 (16:11 +0100)] 
MINOR: bwlim: Remove useless test on CF_READ_ERROR to detect the last packet

There is already a test on CF_EOI and CF_SHUTR. The last one is always set
when a read error is reported. Thus there is no reason to check
CF_READ_ERROR.

2 years agoMINOR: ssl-ckch: Stop to test CF_WRITE_ERROR to commit CA/CRL file
Christopher Faulet [Thu, 26 Jan 2023 07:03:39 +0000 (08:03 +0100)] 
MINOR: ssl-ckch: Stop to test CF_WRITE_ERROR to commit CA/CRL file

This change was performed on all applet I/O handlers but one was missed. In
the CLI I/O handler used to commit a CA/CRL file, we can remove the test on
CF_WRITE_ERROR because there is already a test on CF_SHUTW.

2 years agoDOC: proxy-protocol: fix wrong byte in provided example
Willy Tarreau [Sun, 12 Feb 2023 08:26:48 +0000 (09:26 +0100)] 
DOC: proxy-protocol: fix wrong byte in provided example

There was a mistake in the example of proxy-proto frame
provided, it cannot end with 0x02 but only 0x20 or 0x21
since the version is in the upper 4 bits and the lower ones
are 0 for LOCAL or 1 for PROXY, hence the example should be:

  \x0D\x0A\x0D\x0A\x00\x0D\x0A\x51\x55\x49\x54\x0A\x20

Thanks to Bram Grit for reporting this mistake.

2 years agoBUG/MEDIUM: quic: Buffer overflow when looking through QUIC CLI keyword list
Frédéric Lécaille [Sat, 11 Feb 2023 19:24:42 +0000 (20:24 +0100)] 
BUG/MEDIUM: quic: Buffer overflow when looking through QUIC CLI keyword list

This has been detected by libasan as follows:

=================================================================
==3170559==ERROR: AddressSanitizer: global-buffer-overflow on address 0x55cf77faad08 at pc 0x55cf77a87370 bp 0x7ffc01bdba70 sp 0x7ffc01bdba68
READ of size 8 at 0x55cf77faad08 thread T0
    #0 0x55cf77a8736f in cli_find_kw src/cli.c:335
    #1 0x55cf77a8a9bb in cli_parse_request src/cli.c:792
    #2 0x55cf77a8c385 in cli_io_handler src/cli.c:1024
    #3 0x55cf77d19ca1 in task_run_applet src/applet.c:245
    #4 0x55cf77c0b6ba in run_tasks_from_lists src/task.c:634
    #5 0x55cf77c0cf16 in process_runnable_tasks src/task.c:861
    #6 0x55cf77b48425 in run_poll_loop src/haproxy.c:2934
    #7 0x55cf77b491cf in run_thread_poll_loop src/haproxy.c:3127
    #8 0x55cf77b4bef2 in main src/haproxy.c:3783
    #9 0x7fb8b0693d09 in __libc_start_main ../csu/libc-start.c:308
    #10 0x55cf7764f4c9 in _start (/home/flecaille/src/haproxy-untouched/haproxy+0x1914c9)

0x55cf77faad08 is located 0 bytes to the right of global variable 'cli_kws' defined in 'src/quic_conn.c:7834:27' (0x55cf77faaca0) of size 104
SUMMARY: AddressSanitizer: global-buffer-overflow src/cli.c:335 in cli_find_kw
Shadow bytes around the buggy address:

According to cli_find_kw() code and cli_kw_list struct definition, the second
member of this structure ->kw[] must be a null-terminated array.
Add a last element with default initializers to <cli_kws> global variable which
is impacted by this bug.

This bug arrived with this commit:
   15c74702d MINOR: quic: implement a basic "show quic" CLI handler

Must be backported to 2.7 where this previous commit has been already
backported.

2 years agoBUG/MEDIUM: stconn: stop to enable/disable reads from streams via si_update_rx
Christopher Faulet [Fri, 10 Feb 2023 16:37:11 +0000 (17:37 +0100)] 
BUG/MEDIUM: stconn: stop to enable/disable reads from streams via si_update_rx

It is not really a bug because it does not fix any known issue. And it is
flagged as MEDIUM because it is sensitive. But if there are some extra calls
to process_stream(), it can be an issue because, in si_update_rx(), we may
disable reading for the SC when outgoing data are blocked in the input
channel. But it is not really the process_stream() job to take care of
that. This may block data receipt.

It is an old code, mainly here to avoid wakeup in loop on the stats
applet. Today, it seems useless and can lead to bugs. An endpoint is
responsible to block the SC if it waits for some room and the opposite
endpoint is responsible to unblock it when some data are sent. The stream
should not interfere on this part.

This patch could be backported to 2.7 after a period of observation. And it
should only be backported to lower versions if an issue is reported.

2 years agoBUG/MINOR: clock/stats: also use start_time not start_date in HTML info
Willy Tarreau [Fri, 10 Feb 2023 15:53:35 +0000 (16:53 +0100)] 
BUG/MINOR: clock/stats: also use start_time not start_date in HTML info

For an unknown reason in the change of uptime calculation for the HTML
page didn't make it to commit 6093ba47c ("BUG/MINOR: clock: do not mix
wall-clock and monotonic time in uptime calculation"). Let's address it
as well otherwise the stats page will display an incorrect uptime.

No backport needed unless the patch above is backported.

2 years agoBUG/MINOR: mworker: fix uptime for master process
Amaury Denoyelle [Fri, 10 Feb 2023 14:25:45 +0000 (15:25 +0100)] 
BUG/MINOR: mworker: fix uptime for master process

Uptime calculation for master process was incorrect as it used
<start_date> as its timestamp base time. Fix this by using the scheduler
time <start_time> for this.

The impact of this bug is minor as timestamp base time is only used for
"show proc" CLI output. it was highlighted by the following commit.
which caused a negative value to be displayed for the master process
uptime on "show proc" output.

  28360dc53f715c497fff822523f92769d8b1627d
  MEDIUM: clock: force internal time to wrap early after boot

This should be backported up to 2.0.

2 years agoBUG/MINOR: quic: fix type bug on "show quic" for 32-bits arch
Amaury Denoyelle [Fri, 10 Feb 2023 08:25:22 +0000 (09:25 +0100)] 
BUG/MINOR: quic: fix type bug on "show quic" for 32-bits arch

Incorrect printf format specifier "%lu" was used on "show quic" handler
for uint64_t. This breaks build on 32-bits architecture. To fix this
portability issue, force an explicit cast to unsigned long long with
"%llu" specifier.

This must be backported up to 2.7.

2 years agoBUG/MEDIUM: stconn: Don't needlessly wake the stream on send during fast-forward
Christopher Faulet [Thu, 9 Feb 2023 13:14:38 +0000 (14:14 +0100)] 
BUG/MEDIUM: stconn: Don't needlessly wake the stream on send during fast-forward

With a connection, when data are received, if these data are sent to the
opposite side because the fast-forwarding is possible, the stream may be
woken up on some conditions (at the end of sc_app_chk_snd_conn()):

  * The channel is shut for write
  * The SC is not in the "established" state
  * The stream must explicitly be woken up on write and all data was sent
  * The connection was just established.

A bug on the last condition was introduced with the commit d89884153
("MEDIUM: channel: Use CF_WRITE_EVENT instead of CF_WRITE_PARTIAL"). The
stream is now woken up on any write events.

This patch fixes this issue and restores the original behavior. No backport
is needed.

2 years agoBUG/MINOR: quic: fix filtering of closing connections on "show quic"
Amaury Denoyelle [Thu, 9 Feb 2023 17:18:45 +0000 (18:18 +0100)] 
BUG/MINOR: quic: fix filtering of closing connections on "show quic"

Filtering of closing/draining connections on "show quic" was not
properly implemented. This causes the extra argument "all" to display
all connections to be without effect. This patch fixes this and restores
the output of all connections.

This must be backported up to 2.7.

2 years agoMINOR: quic: filter closing conn on "show quic"
Amaury Denoyelle [Wed, 1 Feb 2023 16:31:02 +0000 (17:31 +0100)] 
MINOR: quic: filter closing conn on "show quic"

Reduce default "show quic" output by masking connection on
closing/draing state due to a CONNECTION_CLOSE emission/reception. These
connections can still be displayed using the special argument "all".

This should be backported up to 2.7.

2 years agoMINOR: quic: display Tx stream info on "show quic"
Amaury Denoyelle [Wed, 1 Feb 2023 16:05:36 +0000 (17:05 +0100)] 
MINOR: quic: display Tx stream info on "show quic"

Complete "show quic" handler by displaying information about
quic_stream_desc entries. These structures are used to emit stream data
and store them until acknowledgment is received.

This should be backported up to 2.7.

2 years agoMINOR: quic: display infos about various encryption level on "show quic"
Amaury Denoyelle [Wed, 1 Feb 2023 16:05:10 +0000 (17:05 +0100)] 
MINOR: quic: display infos about various encryption level on "show quic"

Complete "show quic" handler by displaying various information related
to each encryption level and packet number space. Most notably, ack
ranges and bytes in flight are present to help debug retransmission
issues.

This should be backported up to 2.7.

2 years agoMINOR: quic: display socket info on "show quic"
Amaury Denoyelle [Wed, 1 Feb 2023 16:04:26 +0000 (17:04 +0100)] 
MINOR: quic: display socket info on "show quic"

Complete "show quic" handler by displaying information related to the
quic_conn owned socket. First, the FD is printed, followed by the
address of the local and remote endpoint.

This should be backported up to 2.7.

2 years agoMINOR: quic: display CIDs and state in "show quic"
Amaury Denoyelle [Wed, 1 Feb 2023 10:54:43 +0000 (11:54 +0100)] 
MINOR: quic: display CIDs and state in "show quic"

Complete "show quic" handler. Source and destination CIDs are printed
for every connection. This is complete by a state info to reflect if
handshake is completed and if a CONNECTION_CLOSE has been emitted or
received and the allocation status of the attached MUX. Finally the idle
timer expiration is also printed.

This should be backported up to 2.7.

2 years agoMINOR: quic: implement a basic "show quic" CLI handler
Amaury Denoyelle [Wed, 1 Feb 2023 09:18:26 +0000 (10:18 +0100)] 
MINOR: quic: implement a basic "show quic" CLI handler

Implement a basic "show quic" CLI handler. This command will be useful
to display various information on all the active QUIC frontend
connections.

This work is heavily inspired by "show sess". Most notably, a global
list of quic_conn has been introduced to be able to loop over them. This
list is stored per thread in ha_thread_ctx.

Also add three CLI handlers for "show quic" in order to allocate and
free the command context. The dump handler runs on thread isolation.
Each quic_conn is referenced using a back-ref to handle deletion during
handler yielding.

For the moment, only a list of raw quic_conn pointers is displayed. The
handler will be completed over time with more information as needed.

This should be backported up to 2.7.

2 years agoBUG/MEDIUM: quic: fix crash when "option nolinger" is set in the frontend
Willy Tarreau [Thu, 9 Feb 2023 16:53:41 +0000 (17:53 +0100)] 
BUG/MEDIUM: quic: fix crash when "option nolinger" is set in the frontend

Commit 0aba11e9e ("MINOR: quic: remove unnecessary quic_session_accept()")
overlooked one problem, in session_accept_fd() at the end, there's a bunch
of FD-specific stuff that either sets up or resets the socket at the TCP
level. The tests are mostly performed for AF_INET/AF_INET6 families but
they're only for one part (i.e. to avoid setting up TCP options on UNIX
sockets). Other pieces continue to configure the socket regardless of its
family. All of this directly acts on the FD, which is not correct since
the FD is not valid here, it corresponds to the QUIC handle. The issue
is much more visible when "option nolinger" is enabled in the frontend,
because the access to fdatb[cfd].state immediately crashes on the first
connection, as can be seen in github issue #2030.

This patch bypasses this setup for FD-less connections, such as QUIC.
However some of them could definitely be relevant to the QUIC stack, or
even to UNIX sockets sometimes. A better long-term solution would consist
in implementing a setsockopt() equivalent at the protocol layer that would
be used to configure the socket, either the FD or the QUIC conn depending
on the case. Some of them would not always be implemented but that would
allow to unify all this code.

This fix must be backported everywhere the commit above is backported,
namely 2.6 and 2.7.

Thanks to github user @twomoses for the nicely detailed report.

2 years agoBUG/MEDIUM: stconn: Schedule a shutw on shutr if data must be sent first
Christopher Faulet [Wed, 8 Feb 2023 15:18:48 +0000 (16:18 +0100)] 
BUG/MEDIUM: stconn: Schedule a shutw on shutr if data must be sent first

The commit 7f59d68fe ("BUG/MEDIIM: stconn: Flush output data before
forwarding close to write side") introduced a regression. When the read side
is closed, the close is not forwarded to the write side if there are some
pending outgoind data. The idea is to foward data first and the close the
write side. However, when fast-forwarding is enabled and last data block is
received with the read0, the close is never forwarded.

We cannot revert the commit above because it really fix an issue. However,
we can schedule the shutdown for write by setting CF_SHUTW_NOW flag on the
write side. Indeed, it is the purpose of this flag.

To not replicate ugly and hardly maintainable code block at different places
in stconn.c, an helper function is used. Thus, sc_cond_forward_shutw() must
be called to know if the close can be fowarded or not. It returns 1 if it is
possible. In this case, the caller is responsible to forward the close to
the write side. Otherwise, if the close cannot be forwarded, 0 is
returned. It happens when it should not be performed at all. Or when it
should only be delayed, waiting for the input channel to be flushed. In this
last case, the CF_SHUTW_NOW flag is set in the output channel.

This patch should fix the issue #2033. It must be backported with the commit
above, thus at least as far as 2.2.

2 years agoBUG/MINOR: server/add: ensure minconn/maxconn consistency when adding server
Aurelien DARRAGON [Wed, 8 Feb 2023 10:55:08 +0000 (11:55 +0100)] 
BUG/MINOR: server/add: ensure minconn/maxconn consistency when adding server

When a new server was added through the cli using "server add" command,
the maxconn/minconn consistency check historically implemented in
check_config_validity() for static servers was missing.

As a result, when adding a server with the maxconn parameter without the
minconn set, the server was unable to handle any connection because
srv_dynamic_maxconn() would always return 0.

Consider the following reproducer:

    |  global
    |    stats socket /tmp/ha.sock mode 660 level admin expose-fd listeners
    |
    |  defaults
    |  timeout client 5s
    |  timeout server 5s
    |  timeout connect 5s
    |
    |  frontend test
    |    mode http
    |    bind *:8081
    |    use_backend farm
    |
    |  listen dummyok
    |    bind localhost:18999
    |    mode http
    |    http-request return status 200 hdr test "ok"
    |
    |  backend farm
    |    mode http

Start haproxy and perform the following :

  echo "add server farm/t1 127.0.0.1:18999 maxconn 100" | nc -U /tmp/ha.sock
  echo "enable server farm/t1" | nc -U /tmp/ha.sock

  curl localhost:8081 # -> 503 after 5s connect timeout

Thanks to ("MINOR: cfgparse/server: move (min/max)conn postparsing logic into
dedicated function"), we are now able to perform the consistency check after
the new dynamic server has been parsed.
This is enough to fix the issue documented here that was reported by
Thomas Pedoussaut on the ML.

This commit depends on:
 - ("MINOR: cfgparse/server: move (min/max)conn postparsing logic into
     dedicated function")

It must be backported to 2.6 and 2.7

2 years agoMINOR: cfgparse/server: move (min/max)conn postparsing logic into dedicated function
Aurelien DARRAGON [Wed, 8 Feb 2023 10:49:02 +0000 (11:49 +0100)] 
MINOR: cfgparse/server: move (min/max)conn postparsing logic into dedicated function

In check_config_validity() function, we performed some consistency checks to
adjust minconn/maxconn attributes for each declared server.

We move this logic into a dedicated function named srv_minmax_conn_apply()
to be able to perform those checks later in the process life when needed
(ie: dynamic servers)

2 years agoMINOR: ssl/ocsp: add a function to check the OCSP update configuration
William Lallemand [Tue, 7 Feb 2023 17:38:05 +0000 (18:38 +0100)] 
MINOR: ssl/ocsp: add a function to check the OCSP update configuration

Deduplicate the code which checks the OCSP update in the ckch_store and
in the crtlist_entry.

Also, jump immediatly to error handling when the ERR_FATAL is catched.

2 years agoBUILD: ssl/ocsp: ssl_ocsp-t.h depends on ssl_sock-t.h
William Lallemand [Tue, 7 Feb 2023 17:28:30 +0000 (18:28 +0100)] 
BUILD: ssl/ocsp: ssl_ocsp-t.h depends on ssl_sock-t.h

ssl_ocsp-t.h uses SSL_SOCK_NUM_KEYTYPES which is defined in
ssl_sock-t.h.

No backport needed.

2 years agoMEDIUM: clock: force internal time to wrap early after boot
Willy Tarreau [Tue, 7 Feb 2023 13:44:44 +0000 (14:44 +0100)] 
MEDIUM: clock: force internal time to wrap early after boot

GH issue #2034 clearly indicates yet another case of time roll-over
that went badly. Issues that happen only once every 50 days are hard
to detect and debug, and are usually reported more or less synchronized
from multiple sources. This patch finally does what had long been planned
but never done yet, which is to force the time to wrap early after boot
so that any such remaining issue can be spotted quicker. The margin delay
here is 20s (it may be changed by setting BOOT_TIME_WRAP_SEC to another
value). This value seems sufficient to permit failed health checks to
succeed and traffic to come in and possibly start to update some time
stamps (accept dates in logs, freq counters, stick-tables expiration
dates etc).

It could theoretically be helpful to have this in 2.7, but as can be
seen with the two patches below, we've already had incorrect use cases
of the internal monotonic time when the wall-clock one was needed, so
we could expect to detect other ones in the future. Note that this will
*not* induce bugs, it will only make them happen much faster (i.e. no
need to wait for 50 days before seeing them). If it were to eventually
be backported, these two previous patches must also be backported:

    BUG/MINOR: clock: use distinct wall-clock and monotonic start dates
    BUG/MEDIUM: cache: use the correct time reference when comparing dates

2 years agoBUG/MEDIUM: cache: use the correct time reference when comparing dates
Willy Tarreau [Tue, 7 Feb 2023 14:22:41 +0000 (15:22 +0100)] 
BUG/MEDIUM: cache: use the correct time reference when comparing dates

The cache makes use of dates advertised by external components, such
as "last-modified" or "date". As such these are wall-clock dates, and
not internal dates. However, all comparisons are mistakenly made based
on the internal monotonic date which is designed to drift from the wall
clock one in order to catch up with stolen time (which can sometimes be
intense in VMs). As such after some run time some objects may fail to
validate or fail to expire depending on the direction of the drift. This
is particularly visible when applying an offset to the internal time to
force it to wrap soon after startup, as it will be shifted up to 49.7
days in the future depending on the current date; what happens in this
case is that the reg-test "cache_expires.vtc" fails on the 3rd test by
returning stale contents from the cache at the date of this commit.

It is really important that all external dates are compared against
"date" and not "now" for this reason.

This fix needs to be backported to all versions.

2 years agoBUG/MINOR: clock: do not mix wall-clock and monotonic time in uptime calculation
Willy Tarreau [Tue, 7 Feb 2023 14:52:14 +0000 (15:52 +0100)] 
BUG/MINOR: clock: do not mix wall-clock and monotonic time in uptime calculation

We've had a start date even before the internal monotonic clock existed,
but once the monotonic clock was added, the start date was not updated
to distinguish the wall clock time units and the internal monotonic time
units. The distinction is important because both clocks do not necessarily
progress at the same speed. The very rare occurrences of the wall-clock
date are essentially for human consumption and communication with third
parties (e.g. report the start date in "show info" for monitoring
purposes). However currently this one is also used to measure the distance
to "now" as being the process' uptime. This is actually not correct. It
only works because for now the two dates are initialized at the exact
same instant at boot but could still be wrong if the system's date shows
a big jump backwards during startup for example. In addition the current
situation prevents us from enforcing an abritrary offset at boot to reveal
some heisenbugs.

This patch adds a new "start_time" at boot that is set from "now" and is
used in uptime calculations. "start_date" instead is now set from "date"
and will always reflect the system date for human consumption (e.g. in
"show info"). This way we're now sure that any drift of the internal
clock relative to the system date will not impact the reported uptime.

This could possibly be backported though it's unlikely that anyone has
ever noticed the problem.

2 years agoBUG/MEDIUM: stick-table: do not leave entries in end of window during purge
Aleksey Ponomaryov [Tue, 7 Feb 2023 18:27:06 +0000 (19:27 +0100)] 
BUG/MEDIUM: stick-table: do not leave entries in end of window during purge

At some moments expired stick table records stop being removed. This
happens when the internal time wraps around the 32-bit limit, or every
49.7 days. What precisely happens is that some elements that are collected
close to the end of the time window (2^32 - table's "expire" setting)
might have been updated and will be requeued further, at the beginning
of the next window. Here, three bad situations happen:

  - the incorrect integer-based comparison that is not aware of wrapping
    will result in the scan to restart from the freshly requeued element,
    skipping all those at the end of the window. The net effect of this
    is that at each wakeup of the expiration task, only one element from
    the end of the window will be expired, and other ones will remain
    there for a very long time, especially if they have to wait for all
    the predecessors to be picked one at a time after slow wakeups due
    to a long expiration ; this is what was observed in issue #2034
    making the table fill up and appear as not expiring at all, and it
    seems that issue #2024 reports the same problem at the same moment
    (since such issues happen for everyone roughly at the same time
    when the clock doesn't drift too much).

  - the elements that were placed at the beginning of the next window
    are skipped as well for as long as there are refreshed entries at
    the end of the previous window, so these ones participate to filling
    the table as well. This is cause by the restart from the current,
    updated node that is generally placed after most other less recently
    updated elements.

  - once the last element at the end of the window is picked, suddenly
    there is a large amount of expired entries at the beginning of the
    next window that all have to be requeued. If the expiration delay
    is large, the number can be big and it can take a long time, which
    can very likely explain the periodic crashes reported in issue #2025.
    Limiting the batch size as done in commit dfe79251d ("BUG/MEDIUM:
    stick-table: limit the time spent purging old entries") would make
    sense for process_table_expire() as well.

This patch addresses the incorrect tree scan algorithm to make sure that:
  - there's always a next element to compare against, even when dealing
    with the last one in the tree, the first one must be used ;

  - time comparisons used to decide whether to restart from the current
    element use tick_is_lt() as it is the only case where we know the
    current element will be placed before any other one (since the tree
    respects insertion ordering for duplicates)

In order to reproduce the issue, it was found that injecting traffic on
a random key that spans over half of the size of a table whose expiration
is set to 15s while the date is going to wrap in 20s does exhibit an
increase of the table's size 5s after startup, when entries start to be
pushed to the next window. It's more effective when a second load
generator constantly hammers a same key to be certain that none of them
is ready to expire. This doesn't happen anymore after this patch.

This fix needs to be backported to all stable versions. The bug has been
there for as long as the stick tables were introduced in 1.4-dev7 with
commit 3bd697e07 ("[MEDIUM] Add stick table (persistence) management
functions and types"). A cleanup could consists in deduplicating that
code by having process_table_expire() call __stktable_trash_oldest(),
with that one improved to support an optional time check.

2 years agoBUG/MINOR: ssl/crt-list: warn when a line is malformated
William Lallemand [Tue, 7 Feb 2023 16:06:35 +0000 (17:06 +0100)] 
BUG/MINOR: ssl/crt-list: warn when a line is malformated

Display a warning when some text exists between the filename and the
options. This part is completely ignored so if there are filters here,
they were never parsed.

This could be backported in every versions. In the older versions, the
parsing was done in ssl_sock_load_cert_list_file() in ssl_sock.c.

2 years agoBUG/MEDIUM: listener/thread: bypass shards setting on failed thread resolution
Willy Tarreau [Mon, 6 Feb 2023 17:06:14 +0000 (18:06 +0100)] 
BUG/MEDIUM: listener/thread: bypass shards setting on failed thread resolution

Aurélien reported that the BUG_ON(!new_ts.nbgrp) added in 2.8-dev3 by
commit 50440457e ("MEDIUM: config: restrict shards, not bind_conf to one
group each") can trigger on some invalid configs where the thread_set on
the "bind" line couldn't be resolved. The reason is that we still enter
the parsing loop (as it was done previously) and we possibly have no
group to work on (which was the purpose of this assertion). There we
need to bypass all this block on such a condition.

No backport is needed.

2 years agoBUG/MEDIUM: thread: fix extraneous shift in the thread_set parser
Willy Tarreau [Mon, 6 Feb 2023 17:01:50 +0000 (18:01 +0100)] 
BUG/MEDIUM: thread: fix extraneous shift in the thread_set parser

Aurélien reported a bug making a statement such as "thread 2-2" fail for
a config made of exactly 2 threads. What happens is that the parser for
the "thread" keyword scans a range of thread numbers from either 1..64
or 0,-1,-2 for special values, and presets the bit masks accordingly in
the thread set, except that due to the 1..64 range, the shift length must
be reduced by one. Not doing this causes empty masks for single-bit values
that are exactly equal to the number of threads in the group and fails to
properly parse.

No backport is needed as this was introduced in 2.8-dev3 by commit
bef43dfa6 ("MINOR: thread: add a simple thread_set API").

2 years agoBUG/MINOR: stats: Prevent HTTP "other sessions" counter underflows
Frédéric Lécaille [Mon, 6 Feb 2023 08:23:56 +0000 (09:23 +0100)] 
BUG/MINOR: stats: Prevent HTTP "other sessions" counter underflows

Due to multithreading concurrency, it is difficult at this time to figure
out how this counter may become negative. This simple patch only checks this
will never be the case.

This issue arrives with this commit:
 "9969adbcdc MINOR: stats: add by HTTP version cumulated number of sessions and requests"
So, this patch should be backported when the latter has been backported.

2 years agoMINOR: quic: Update version_information transport parameter to draft-14
Frédéric Lécaille [Mon, 6 Feb 2023 10:54:07 +0000 (11:54 +0100)] 
MINOR: quic: Update version_information transport parameter to draft-14

This is necessary to make our stack negotiate the QUIC versions with clients.
(See https://author-tools.ietf.org/iddiff?url1=draft-ietf-quic-version-negotiation-13&url2=draft-ietf-quic-version-negotiation-14&difftype=--html)

Must be backported to 2.7.

2 years agoBUG/MINOR: stats: fix STAT_STARTED behavior with full htx
Aurelien DARRAGON [Thu, 2 Feb 2023 18:01:02 +0000 (19:01 +0100)] 
BUG/MINOR: stats: fix STAT_STARTED behavior with full htx

When stats_putchk() fails to peform the dump because available data space in
htx is less than the number of bytes pending in the dump buffer, we wait
for more room in the htx (ie: sc_need_room()) to retry the dump attempt
on the next applet invocation.

To provide consistent output, we have to make sure that the stat ctx is not
updated (or at least correctly reverted) in case stats_putchk() fails so
that the new dumping attempt behaves just like the previous (failed) one.

STAT_STARTED is not following this logic, the flag is set in
stats_dump_fields_json() as soon as some data is written to the output buffer.

It's done too early: we need to delay this step after the stats_putchk() has
successfully returned if we want to correctly handle the retries attempts.

Because of this, JSON output could suffer from extraneous ',' characters which
could make json parsers unhappy.

For example, this is the kind of errors you could get when using
`python -m json.tool` on such badly formatted outputs:

   "Expecting value: line 1 column 2 (char 1)"

Unfortunately, fixing this means that the flag needs to be enabled at
multiple places, which is what we're doing in this patch.
(in stats_dump_proxy_to_buffer() where stats_dump_one_line() is involved
by underlying stats_dump_{fe,li,sv,be} functions)

Thereby, this raises the need for a cleanup to reduce code duplication around
stats_dump_proxy_to_buffer() function and simplify things a bit.

It could be backported to 2.6 and 2.7

2 years agoBUG/MINOR: stats: fix show stats field ctx for servers
Aurelien DARRAGON [Thu, 2 Feb 2023 17:13:30 +0000 (18:13 +0100)] 
BUG/MINOR: stats: fix show stats field ctx for servers

In ("MINOR: stats: introduce stats field ctx"), we forgot
to apply the patch to servers.

This prevents "BUG/MINOR: stats: fix show stat json buffer limitation"
from working with servers dump.

We're adding the missing part related to servers dump.

This commit should be backported with the aforementioned commits.

2 years agoBUG/MINOR: stats: fix ctx->field update in stats_dump_proxy_to_buffer()
Aurelien DARRAGON [Fri, 3 Feb 2023 10:43:05 +0000 (11:43 +0100)] 
BUG/MINOR: stats: fix ctx->field update in stats_dump_proxy_to_buffer()

When ctx->field was introduced with ("MINOR: stats: introduce stats field ctx")
a mistake was made for the STAT_PX_ST_LI state in stats_dump_proxy_to_buffer():

current_field reset is placed after the for loop, ie: after multiple lines
are dumped. Instead it should be placed right after each li line is dumped.

This could cause some output inconsistencies (missing fields), especially when
http dump is used with JSON output and "socket-stats" option is enabled
on the proxy, because when htx is full we restore the ctx->field with
current_field (which contains outdated value in this case).

This should be backported with ("MINOR: stats: introduce stats field ctx")

2 years agoBUG/MEDIUM: stats: fix resolvers dump
Aurelien DARRAGON [Thu, 2 Feb 2023 16:27:27 +0000 (17:27 +0100)] 
BUG/MEDIUM: stats: fix resolvers dump

In ("BUG/MEDIUM: stats: Rely on a local trash buffer to dump the stats"),
we forgot to apply the patch in resolvers.c which provides the
stats_dump_resolvers() function that is involved when dumping with "resolvers"
domain.

As a consequence, resolvers dump was broken because stats_dump_one_line(),
which is used in stats_dump_resolv_to_buffer(), implicitely uses trash_chunk
from stats.c to prepare the dump, and stats_putchk() is then called with
global trash (currently empty) as output data.

Given that trash_dump variable is static and thus only available within stats.c
we change stats_putchk() function prototype so that the function does not take
the output buffer as an argument. Instead, stats_putchk() will implicitly use
the local trash_dump variable declared in stats.c.

It will also prevent further mixups between stats_dump_* functions and
stats_putchk().

This needs to be backported with ("BUG/MEDIUM: stats: Rely on a local trash
buffer to dump the stats")

2 years agoBUG/MINOR: stats: fix source buffer size for http dump
Aurelien DARRAGON [Fri, 3 Feb 2023 07:31:42 +0000 (08:31 +0100)] 
BUG/MINOR: stats: fix source buffer size for http dump

In ("BUG/MINOR: stats: use proper buffer size for http dump"),
we used trash.size as source buffer size before applying the htx
overhead computation.

It is safer to use res->buf.size instead since res_htx (which is <htx> argument
passed to stats_putchk() in http context) is made from res->buf:

in http_stats_io_handler:
    | res_htx = htx_from_buf(&res->buf);

This will prevent the hang bug from showing up again if res->buf.size were to be
less than trash.size (which is set according to tune.bufsize).

This should be backported with ("BUG/MINOR: stats: use proper buffer size for http dump")

2 years ago[RELEASE] Released version 2.8-dev3 v2.8-dev3
Willy Tarreau [Sat, 4 Feb 2023 09:51:05 +0000 (10:51 +0100)] 
[RELEASE] Released version 2.8-dev3

Released version 2.8-dev3 with the following main changes :
    - BUG/MINOR: sink: make sure to always properly unmap a file-backed ring
    - DEV: haring: add a new option "-r" to automatically repair broken files
    - BUG/MINOR: ssl: Fix leaks in 'update ssl ocsp-response' CLI command
    - MINOR: ssl: Remove debug fprintf in 'update ssl ocsp-response' cli command
    - MINOR: connection: add a BUG_ON() to detect destroying connection in idle list
    - MINOR: mux-quic/h3: send SETTINGS as soon as transport is ready
    - BUG/MINOR: h3: fix GOAWAY emission
    - BUG/MEDIUM: mux-quic: fix crash on H3 SETTINGS emission
    - BUG/MEDIUM: hpack: fix incorrect huffman decoding of some control chars
    - BUG/MINOR: log: release global log servers on exit
    - BUG/MINOR: ring: release the backing store name on exit
    - BUG/MINOR: sink: free the forwarding task on exit
    - CLEANUP: trace: remove the QUIC-specific ifdefs
    - MINOR: trace: add a TRACE_ENABLED() macro to determine if a trace is active
    - MINOR: trace: add a trace_no_cb() dummy callback for when to use no callback
    - MINOR: trace: add the long awaited TRACE_PRINTF()
    - MINOR: h2: add h2_phdr_to_ist() to make ISTs from pseudo headers
    - MEDIUM: mux-h2/trace: add tracing support for headers
    - CLEANUP: mux-h2/trace: shorten the name of the header enc/dec functions
    - DEV: hpack: fix `trash` build regression
    - MINOR: http_htx: add http_append_header() to append value to header
    - MINOR: http_htx: add http_prepend_header() to prepend value to header
    - MINOR: sample: add ARGC_OPT
    - MINOR: proxy: introduce http only options
    - MINOR: proxy/http_ext: introduce proxy forwarded option
    - REGTEST: add ifnone-forwardfor test
    - MINOR: proxy: move 'forwardfor' option to http_ext
    - MINOR: proxy: move 'originalto' option to http_ext
    - MINOR: http_ext: introduce http ext converters
    - MINOR: http_ext: add rfc7239_is_valid converter
    - MINOR: http_ext: add rfc7239_field converter
    - MINOR: http_ext: add rfc7239_n2nn converter
    - MINOR: http_ext: add rfc7239_n2np converter
    - REGTEST: add RFC7239 forwarded header tests
    - OPTIM: http_ext/7239: introduce c_mode to save some space
    - MINOR: http_ext/7239: warn the user when fetch is not available
    - MEDIUM: proxy/http_ext: implement dynamic http_ext
    - MINOR: cfgparse/http_ext: move post-parsing http_ext steps to http_ext
    - DOC: config: fix option spop-check proxy compatibility
    - BUG/MINOR: fcgi-app: prevent 'use-fcgi-app' in default section
    - DOC: config: 'http-send-name-header' option may be used in default section
    - BUG/MINOR: mux-h2: Fix possible null pointer deref on h2c in _h2_trace_header()
    - BUG/MINOR: http_ext/7239: ipv6 dumping relies on out of scope variables
    - BUG/MEDIUM: h3: do not crash if no buf space for trailers
    - OPTIM: h3: skip buf realign if no trailer to encode
    - MINOR: mux-quic/h3: define stream close callback
    - BUG/MEDIUM: h3: handle STOP_SENDING on control stream
    - BUG/MINOR: h3: reject RESET_STREAM received for control stream
    - MINOR: h3: add missing traces on closure
    - BUG/MEDIUM: ssl: wrong eviction from the session cache tree
    - BUG/MINOR: h3: fix crash due to h3 traces
    - BUG/MINOR: h3: fix crash due to h3 traces
    - BUG/MEDIUM: thread: consider secondary threads as idle+harmless during boot
    - BUG/MINOR: stats: use proper buffer size for http dump
    - BUILD: makefile: fix PCRE overriding specific lib path
    - MINOR: quic: remove fin from quic_stream frame type
    - MINOR: quic: ensure offset is properly set for STREAM frames
    - MINOR: quic: define new functions for frame alloc
    - MINOR: quic: refactor frame deallocation
    - MEDIUM: quic: implement a retransmit limit per frame
    - MINOR: quic: add config for retransmit limit
    - OPTIM: htx: inline the most common memcpy(8)
    - CLEANUP: quic: no need for atomics on packet refcnt
    - MINOR: stats: add by HTTP version cumulated number of sessions and requests
    - BUG/MINOR: quic: Possible stream truncations under heavy loss
    - BUG/MINOR: quic: Too big PTO during handshakes
    - MINOR: quic: Add a trace about variable states in qc_prep_fast_retrans()
    - BUG/MINOR: quic: Do not ignore coalesced packets in qc_prep_fast_retrans()
    - MINOR: quic: When probing Handshake packet number space, also probe the Initial one
    - BUG/MAJOR: quic: Possible crash when processing 1-RTT during 0-RTT session
    - MEDIUM: quic: Remove qc_conn_finalize() from the ClientHello TLS callbacks
    - BUG/MINOR: quic: Unchecked source connection ID
    - MEDIUM: listener: move the analysers mask to the bind_conf
    - MINOR: listener: move maxseg and tcp_ut to bind_conf
    - MINOR: listener: move maxaccept from listener to bind_conf
    - MINOR: listener: move the backlog setting from listener to bind_conf
    - MINOR: listener: move the maxconn parameter to the bind_conf
    - MINOR: listener: move the ->accept callback to the bind_conf
    - MINOR: listener: remove the useless ->default_target field
    - MINOR: listener: move the nice field to the bind_conf
    - MINOR: listener: move the NOLINGER option to the bind_conf
    - MINOR: listener: move the NOQUICKACK option to the bind_conf
    - MINOR: listener: move the DEF_ACCEPT option to the bind_conf
    - MINOR: listener: move TCP_FO to bind_conf
    - MINOR: listener: move the ACC_PROXY and ACC_CIP options to bind_conf
    - MINOR: listener: move LI_O_UNLIMITED and LI_O_NOSTOP to bind_conf
    - MINOR: listener: get rid of LI_O_TCP_L4_RULES and LI_O_TCP_L5_RULES
    - CLEANUP: listener: remove the now unused options field
    - MINOR: listener: remove the now useless LI_F_QUIC_LISTENER flag
    - CLEANUP: config: remove test for impossible case regarding bind thread mask
    - MINOR: thread: add a simple thread_set API
    - MEDIUM: listener/config: make the "thread" parser rely on thread_sets
    - CLEANUP: config: stop using bind_tgroup and bind_thread
    - CLEANUP: listener/thread: remove now unused bind_conf's bind_tgroup/bind_thread
    - CLEANUP: listener/config: remove the special case for shards==1
    - MEDIUM: config: restrict shards, not bind_conf to one group each
    - BUG/MEDIUM: quic: do not split STREAM frames if no space
    - BUILD: thread: fix build warnings with older gcc compilers

2 years agoBUILD: thread: fix build warnings with older gcc compilers
Willy Tarreau [Sat, 4 Feb 2023 09:49:01 +0000 (10:49 +0100)] 
BUILD: thread: fix build warnings with older gcc compilers

The "{ 0 }" form to initialize an empty structure triggers build warnings
on gcc 4.8, let's use the more common "{ }" instead.

2 years agoBUG/MEDIUM: quic: do not split STREAM frames if no space
Amaury Denoyelle [Fri, 3 Feb 2023 17:39:06 +0000 (18:39 +0100)] 
BUG/MEDIUM: quic: do not split STREAM frames if no space

When building STREAM frames in a packet buffer, if a frame is too large
it will be splitted in two. A shorten version will be used and the
original frame will be modified to represent the remaining space.

To ensure there is enough space to store the frame data length encoded
as a QUIC integer, we use the function max_available_room(). This
function can return 0 if there not only a small space left which is
insufficient for the frame header and the shorten data. Prior to this
patch, this wasn't check and an empty unneeded STREAM frame was built
and sent for nothing.

Change this by checking the value return by max_available_room(). If 0,
do not try to split this frame and continue to the next ones in the
packet.

On 2.6, this patch serves as an optimization which will prevent the building
of unneeded empty STREAM frames.

On 2.7, this behavior has the side-effect of triggering a BUG_ON()
statement on quic_build_stream_frame(). This BUG_ON() ensures that we do
not use quic_frame with OFF bit set if its offset is 0. This can happens
if the condition defined above is reproduced for a STREAM frame at
offset 0. An empty unneeded frame is built as descibed. The problem is
that the original frame is modified with its OFF bit set even if the
offset is still 0.

This must be backported up to 2.6.

2 years agoMEDIUM: config: restrict shards, not bind_conf to one group each
Willy Tarreau [Fri, 3 Feb 2023 14:52:07 +0000 (15:52 +0100)] 
MEDIUM: config: restrict shards, not bind_conf to one group each

Now that we're using thread_sets there's no need to restrict an entire
bind_conf to 1 group, the real concern being the FD, we can move that
restriction to the shard only. This means that as long as we have enough
shards and that they're properly aligned on group boundaries (i.e. shards
are an integer divider of the number of threads), we can support "bind"
lines spanning more than one group.

The check is still performed for shards to span more than one group,
and an error is emitted when this happens. But at least now it becomes
possible to have this:

    global
       nbthread 256

    frontend foo
       bind :1111 shards 4
       bind :2222 shards by-thread

2 years agoCLEANUP: listener/config: remove the special case for shards==1
Willy Tarreau [Fri, 3 Feb 2023 13:53:34 +0000 (14:53 +0100)] 
CLEANUP: listener/config: remove the special case for shards==1

In fact this case is already handled by the regular shards code, there
is no need to special-case it.

2 years agoCLEANUP: listener/thread: remove now unused bind_conf's bind_tgroup/bind_thread
Willy Tarreau [Thu, 2 Feb 2023 16:01:10 +0000 (17:01 +0100)] 
CLEANUP: listener/thread: remove now unused bind_conf's bind_tgroup/bind_thread

Not needed anymore since last commit, let's get rid of it.

2 years agoCLEANUP: config: stop using bind_tgroup and bind_thread
Willy Tarreau [Tue, 31 Jan 2023 18:37:54 +0000 (19:37 +0100)] 
CLEANUP: config: stop using bind_tgroup and bind_thread

Let's now retrieve the first thread group and its mask from the
thread_set so that we don't need these fields in the bind_conf anymore.
For now we're still limited to the first group (like before) but that
allows to get rid of these fields and to make sure that there's nothing
"special" being done there anymore.

2 years agoMEDIUM: listener/config: make the "thread" parser rely on thread_sets
Willy Tarreau [Tue, 31 Jan 2023 18:31:27 +0000 (19:31 +0100)] 
MEDIUM: listener/config: make the "thread" parser rely on thread_sets

Instead of reading and storing a single group and a single mask for a
"thread" directive on a bind line, we now store the complete range in
a thread set that's stored in the bind_conf. The bind_parse_thread()
function now just calls parse_thread_set() to complete the current set,
which starts empty, and thread_resolve_group_mask() was updated to
support retrieving thread group numbers or absolute thread numbers
directly from the pre-filled thread_set, and continue to feed bind_tgroup
and bind_thread. The CLI parsers which were pre-initialized to set the
bind_tgroup to 1 cannot do it anymore as it would prevent one from
restricting the thread set. Instead check_config_validity() now detects
the CLI frontend and passes the info down to thread_resolve_group_mask()
that will automatically use only the group 1's threads for these
listeners. The same is done for the peers listeners for now.

At this step it's already possible to start with all previous valid
configs as well as extended ones supporting comma-delimited thread
sets. In addition the parser already accepts large ranges spanning
multiple groups, but since the underlying listeners infrastructure
is not read, for now we're maintaining a specific check against this
at the higher level of the config validity check.

The patch is a bit large because thread resolution is performed in
multiple steps, so we need to adjust all of them at once to preserve
functional and technical consistency.

2 years agoMINOR: thread: add a simple thread_set API
Willy Tarreau [Tue, 31 Jan 2023 18:27:48 +0000 (19:27 +0100)] 
MINOR: thread: add a simple thread_set API

The purpose is to be able to store large thread sets, defined by ranges
that may cross group boundaries, as well as define lists of groups and
masks. The thread_set struct implements the storage, and the parser is
in parse_thread_set(), with a focus on "bind" lines, but not only.

2 years agoCLEANUP: config: remove test for impossible case regarding bind thread mask
Willy Tarreau [Tue, 31 Jan 2023 18:14:31 +0000 (19:14 +0100)] 
CLEANUP: config: remove test for impossible case regarding bind thread mask

During 2.5 development, a fallback was implemented for bind "thread"
directives that would not map to existing threads, with commit e3f4d7496
("MEDIUM: config: resolve relative threads on bind lines to absolute ones").
The approch consisted in remapping the threads to other ones. But now
that relative threads and not absolute threads are stored in this mask,
this case cannot happen anymore, and this confusing hack is not needed
anymore.

2 years agoMINOR: listener: remove the now useless LI_F_QUIC_LISTENER flag
Willy Tarreau [Thu, 12 Jan 2023 19:20:57 +0000 (20:20 +0100)] 
MINOR: listener: remove the now useless LI_F_QUIC_LISTENER flag

This flag is only used to tag a QUIC listener, which we now know by
its bind_conf's xprt as well. It's only used to decide whether or not
to perform an extra initialization step on the listener. Let's drop it
as well as the flags field.

With the various fields and options moved, the listener struct reduced
by 48 bytes total.

2 years agoCLEANUP: listener: remove the now unused options field
Willy Tarreau [Thu, 12 Jan 2023 19:10:11 +0000 (20:10 +0100)] 
CLEANUP: listener: remove the now unused options field

All options that made sense were moved to the bind_conf, and remaining
ones were removed. This field isn't used at all anymore. The thr_idx
field was moved there to plug the hole.

2 years agoMINOR: listener: get rid of LI_O_TCP_L4_RULES and LI_O_TCP_L5_RULES
Willy Tarreau [Thu, 12 Jan 2023 19:03:38 +0000 (20:03 +0100)] 
MINOR: listener: get rid of LI_O_TCP_L4_RULES and LI_O_TCP_L5_RULES

LI_O_TCP_L4_RULES and LI_O_TCP_L5_RULES are only set by from the proxy
based on the presence or absence of tcp_req l4/l5 rules. It's basically
as cheap to check the list as it is to check the flag, except that there
is no need to maintain a copy. Let's get rid of them, and this may ease
addition of more dynamic stuff later.

2 years agoMINOR: listener: move LI_O_UNLIMITED and LI_O_NOSTOP to bind_conf
Willy Tarreau [Thu, 12 Jan 2023 18:58:42 +0000 (19:58 +0100)] 
MINOR: listener: move LI_O_UNLIMITED and LI_O_NOSTOP to bind_conf

These two flags are entirely for internal use and are even per proxy
in practice since they're used for peers and CLI to indicate (for the
first one) that the listener(s) are not subject to connection limits,
and for the second that the listener(s) should not be stopped on
soft-stop. No need to keep them in the listeners, let's move them to
the bind_conf under names BC_O_UNLIMITED and BC_O_NOSTOP.

2 years agoMINOR: listener: move the ACC_PROXY and ACC_CIP options to bind_conf
Willy Tarreau [Thu, 12 Jan 2023 18:48:50 +0000 (19:48 +0100)] 
MINOR: listener: move the ACC_PROXY and ACC_CIP options to bind_conf

These are only set per bind line and used when creating a sessions,
we can move them to the bind_conf under the names BC_O_ACC_PROXY and
BC_O_ACC_CIP respectively.

2 years agoMINOR: listener: move TCP_FO to bind_conf
Willy Tarreau [Thu, 12 Jan 2023 18:45:58 +0000 (19:45 +0100)] 
MINOR: listener: move TCP_FO to bind_conf

It's set per bind line ("tfo") and only used in tcp_bind_listener() so
there's no point keeping the address family tests, let's just store the
flag in the bind_conf under the name BC_O_TCP_FO.

2 years agoMINOR: listener: move the DEF_ACCEPT option to the bind_conf
Willy Tarreau [Thu, 12 Jan 2023 18:42:48 +0000 (19:42 +0100)] 
MINOR: listener: move the DEF_ACCEPT option to the bind_conf

This option is set per bind line, and was only set stored when the
address family is AF_INET4 or AF_INET6. That's pointless since it's
used only in tcp_bind_listener() which is only used for such families
as well, so it can now be moved to the bind_conf under the name
BC_O_DEF_ACCEPT.

2 years agoMINOR: listener: move the NOQUICKACK option to the bind_conf
Willy Tarreau [Thu, 12 Jan 2023 18:40:42 +0000 (19:40 +0100)] 
MINOR: listener: move the NOQUICKACK option to the bind_conf

It solely depends on the bind line so let's move it there under the
name BC_O_NOQUICKACK.

2 years agoMINOR: listener: move the NOLINGER option to the bind_conf
Willy Tarreau [Thu, 12 Jan 2023 18:37:07 +0000 (19:37 +0100)] 
MINOR: listener: move the NOLINGER option to the bind_conf

It's currently declared per-frontend, though it would make sense to
support it per-line but in no case per-listener. Let's move the option
to a bind_conf option BC_O_NOLINGER.

2 years agoMINOR: listener: move the nice field to the bind_conf
Willy Tarreau [Thu, 12 Jan 2023 18:32:45 +0000 (19:32 +0100)] 
MINOR: listener: move the nice field to the bind_conf

This is another bind line setting which can move to the bind_conf.
Note that it leaves a 2-byte hole in the listener struct.

2 years agoMINOR: listener: remove the useless ->default_target field
Willy Tarreau [Thu, 12 Jan 2023 18:18:34 +0000 (19:18 +0100)] 
MINOR: listener: remove the useless ->default_target field

This field is used by stream_new() to optionally set the applet the
stream will connect to for simple proxies like the CLI for example.
But it has never been configurable to anything and is always strictly
equal to the frontend's ->default_target. Let's just drop it and make
stream_new() only use the frontend's. It makes more sense anyway as
we don't want the proxy to work differently based on the "bind" line.
This idea was brought in 1.6 hoping that the h2 implementation would
use applets for decoding (which was dropped after the very first
attempt in 1.8).

2 years agoMINOR: listener: move the ->accept callback to the bind_conf
Willy Tarreau [Thu, 12 Jan 2023 18:10:17 +0000 (19:10 +0100)] 
MINOR: listener: move the ->accept callback to the bind_conf

The accept callback directly derives from the upper layer, generally
it's session_accept_fd(). As such it's also defined per bind line
so it makes sense to move it there.

2 years agoMINOR: listener: move the maxconn parameter to the bind_conf
Willy Tarreau [Thu, 12 Jan 2023 17:59:37 +0000 (18:59 +0100)] 
MINOR: listener: move the maxconn parameter to the bind_conf

The maxconn is set per bind line so let's move it there. This might
possibly even slightly reduce inter-thread contention since this one
is read-mostly and it was stored next to nbconn which changes for
each connection setup or teardown.

2 years agoMINOR: listener: move the backlog setting from listener to bind_conf
Willy Tarreau [Thu, 12 Jan 2023 17:55:13 +0000 (18:55 +0100)] 
MINOR: listener: move the backlog setting from listener to bind_conf

The backlog setting is also defined by the bind_conf, so let's move
it there.

2 years agoMINOR: listener: move maxaccept from listener to bind_conf
Willy Tarreau [Thu, 12 Jan 2023 17:52:23 +0000 (18:52 +0100)] 
MINOR: listener: move maxaccept from listener to bind_conf

Like for previous values, maxaccept is really per-bind_conf, so let's
move it there. Some frontends (peers, log) set it to 1 so the assignment
was slightly moved.

2 years agoMINOR: listener: move maxseg and tcp_ut to bind_conf
Willy Tarreau [Thu, 12 Jan 2023 17:42:49 +0000 (18:42 +0100)] 
MINOR: listener: move maxseg and tcp_ut to bind_conf

These two arguments were only set and only used with tcpv4/tcpv6. Let's
just store them into the bind_conf instead of duplicating them for all
listeners since they're fixed per "bind" line.

2 years agoMEDIUM: listener: move the analysers mask to the bind_conf
Willy Tarreau [Thu, 12 Jan 2023 17:39:42 +0000 (18:39 +0100)] 
MEDIUM: listener: move the analysers mask to the bind_conf

When bind_conf were created, some elements such as the analysers mask
ought to have moved there but that wasn't the case. Now that it's
getting clearer that bind_conf provides all binding parameters and
the listener is essentially a listener on an address, it's starting
to get really confusing to keep such parameters in the listener, so
let's move the mask to the bind_conf. We also take this opportunity
for pre-setting the mask to the frontend's upon initalization. Now
several loops have one less argument to take care of.

2 years agoBUG/MINOR: quic: Unchecked source connection ID
Frédéric Lécaille [Fri, 3 Feb 2023 15:15:08 +0000 (16:15 +0100)] 
BUG/MINOR: quic: Unchecked source connection ID

The SCID (source connection ID) used by a peer (client or server) is sent into the
long header of a QUIC packet in clear. But it is also sent into the transport
parameters (initial_source_connection_id). As these latter are encrypted into the
packet, one must check that these two pieces of information do not differ
due to a packet header corruption. Furthermore as such a connection is unusuable
it must be killed and must stop as soon as possible processing RX/TX packets.

Implement qc_kill_con() to flag a connection as unusable and to kille it asap
waking up the idle timer task to release the connection.

Add a check to quic_transport_params_store() to detect that the SCIDs do not
match and make it call qc_kill_con().

Add several tests about connection to be killed at several critial locations,
especially in the TLS stack callback to receive CRYPTO data from or derive secrets,
and before preparing packet after having received others.

Must be backported to 2.6 and 2.7.

2 years agoMEDIUM: quic: Remove qc_conn_finalize() from the ClientHello TLS callbacks
Frédéric Lécaille [Wed, 1 Feb 2023 16:56:57 +0000 (17:56 +0100)] 
MEDIUM: quic: Remove qc_conn_finalize() from the ClientHello TLS callbacks

This is a bad idea to make the TLS ClientHello callback call qc_conn_finalize().
If this latter fails, this would generate a TLS alert and make the connection
send packet whereas it is not functional. But qc_conn_finalize() job was to
install the transport parameters sent by the QUIC listener. This installation
cannot be done at any time. This must be done after having possibly negotiated
the QUIC version and before sending the first Handshake packets. It seems
the better moment to do that in when the Handshake TX secrets are derived. This
has been found inspecting the ngtcp2 code. Calling SSL_set_quic_transport_params()
too late would make the ServerHello to be sent without the transport parameters.

The code for the connection update which was done from qc_conn_finalize() has
been moved to quic_transport_params_store(). So, this update is done as soon as
possible.

Add QUIC_FL_CONN_TX_TP_RECEIVED to flag the connection as having received the
peer transport parameters. Indeed this is required when the ClientHello message
is splitted between packets.

Add QUIC_FL_CONN_FINALIZED to protect the connection from calling qc_conn_finalize()
more than one time. This latter is called only when the connection has received
the transport parameters and after returning from SSL_do_hanshake() which is the
function which trigger the TLS ClientHello callback call.

Remove the calls to qc_conn_finalize() from from the TLS ClientHello callbacks.

Must be backported to 2.6. and 2.7.

2 years agoBUG/MAJOR: quic: Possible crash when processing 1-RTT during 0-RTT session
Frédéric Lécaille [Wed, 1 Feb 2023 09:31:35 +0000 (10:31 +0100)] 
BUG/MAJOR: quic: Possible crash when processing 1-RTT during 0-RTT session

This bug was revealed by some C1 interop tests (heavy hanshake packet
corruption) when receiving 1-RTT packets with a key phase update.
This lead the packet to be decrypted with the next key phase secrets.
But this latter is initialized only after the handshake is complete.

In fact, 1-RTT must never be processed before the handshake is complete.
Relying on the "qc->mux_state == QC_MUX_NULL" condition to check the
handshake is complete is wrong during 0-RTT sessions when the mux
is initialized before the handshake is complete.

Must be backported to 2.7 and 2.6.

2 years agoMINOR: quic: When probing Handshake packet number space, also probe the Initial one
Frédéric Lécaille [Tue, 31 Jan 2023 16:32:06 +0000 (17:32 +0100)] 
MINOR: quic: When probing Handshake packet number space, also probe the Initial one

This is not really a bug fix but an improvement. When the Handshake packet number
space has been detected as needed to be probed, we should also try to probe the
Initial packet number space if there are still packets in flight. Furthermore
we should also try to send up to two datagrams.

Must be backported to 2.6 and 2.7.

2 years agoBUG/MINOR: quic: Do not ignore coalesced packets in qc_prep_fast_retrans()
Frédéric Lécaille [Tue, 31 Jan 2023 09:10:06 +0000 (10:10 +0100)] 
BUG/MINOR: quic: Do not ignore coalesced packets in qc_prep_fast_retrans()

This function is called only when probing only one packet number space
(Handshake) or two times the same one (Application). So, there is no risk
to prepare two times the same frame when uneeded because we wanted to
probe two packet number spaces. The condition "ignore the packets which
has been coalesced to another one" is not necessary. More importantly
the bug is when we want to prepare a Application packet which has
been coalesced to an Handshake packet. This is always the case when
the first Application packet is sent. It is always coalesced to
an Handshake packet with an ACK frame. So, when lost, this first
application packet was never resent. It contains the HANDSHAKE_DONE
frame to confirm the completion of the handshake to the client.

Must be backported to 2.6 and 2.7.

2 years agoMINOR: quic: Add a trace about variable states in qc_prep_fast_retrans()
Frédéric Lécaille [Mon, 30 Jan 2023 16:27:32 +0000 (17:27 +0100)] 
MINOR: quic: Add a trace about variable states in qc_prep_fast_retrans()

This has already been very useful to diagnose retransmission issues.

Must be backported to 2.6 and 2.7.

2 years agoBUG/MINOR: quic: Too big PTO during handshakes
Frédéric Lécaille [Thu, 26 Jan 2023 14:18:17 +0000 (15:18 +0100)] 
BUG/MINOR: quic: Too big PTO during handshakes

During the handshake and when the handshake has not been confirmed
the acknowledgement delays reported by the peer may be larger
than max_ack_delay. max_ack_delay SHOULD be ignored before the
handshake is completed when computing the PTO. But the current code considered
the wrong condition "before the hanshake is completed".

Replace the enum value QUIC_HS_ST_COMPLETED by QUIC_HS_ST_CONFIRMED to
fix this issue. In quic_loss.c, the parameter passed to quic_pto_pktns()
is renamed to avoid any possible confusion.

Must be backported to 2.7 and 2.6.

2 years agoBUG/MINOR: quic: Possible stream truncations under heavy loss
Frédéric Lécaille [Thu, 26 Jan 2023 14:07:39 +0000 (15:07 +0100)] 
BUG/MINOR: quic: Possible stream truncations under heavy loss

This may happen during retransmission of frames which can be splitted
(CRYPTO, or STREAM frames). One may have to split a frame to be
retransmitted due to the QUIC protocol properties (packet size limitation
and packet field encoding sizes). The remaining part of a frame which
cannot be retransmitted must be detached from the original frame it is
copied from. If not, when the really sent part will be acknowledged
the remaining part will be acknowledged too but not sent!

Must be backported to 2.7 and 2.6.

2 years agoMINOR: stats: add by HTTP version cumulated number of sessions and requests
Frédéric Lécaille [Wed, 18 Jan 2023 10:52:21 +0000 (11:52 +0100)] 
MINOR: stats: add by HTTP version cumulated number of sessions and requests

Add cum_sess_ver[] new array of counters to count the number of cumulated
HTTP sessions by version (h1, h2 or h3).
Implement proxy_inc_fe_cum_sess_ver_ctr() to increment these counter.
This function is called each a HTTP mux is correctly initialized. The QUIC
must before verify the application operations for the mux is for h3 before
calling proxy_inc_fe_cum_sess_ver_ctr().
ST_F_SESS_OTHER stat field for the cumulated of sessions others than
HTTP sessions is deduced from ->cum_sess_ver counter (for all the session,
not only HTTP sessions) from which the HTTP sessions counters are substracted.

Add cum_req[] new array of counters to count the number of cumulated HTTP
requests by version and others than HTTP requests. This new member replace ->cum_req.
Modify proxy_inc_fe_req_ctr() which increments these counters to pass an HTTP
version, 0 special values meaning "other than an HTTP request". This is the case
for instance for syslog.c from which proxy_inc_fe_req_ctr() is called with 0
as version parameter.
ST_F_REQ_TOT stat field compputing for the cumulated number of requests is modified
to count the sum of all the cum_req[] counters.

As this patch is useful for QUIC, it must be backported to 2.7.

2 years agoCLEANUP: quic: no need for atomics on packet refcnt
Willy Tarreau [Thu, 2 Feb 2023 14:37:24 +0000 (15:37 +0100)] 
CLEANUP: quic: no need for atomics on packet refcnt

This is a leftover from the implementation's history, but the
quic_rx_packet and quic_tx_packet ref counts were still atomically
updated. It was found in perf top that the cost of the atomic inc
in quic_tx_packet_refinc() alone was responsible for 1% of the CPU
usage at 135 Gbps. Given that packets are only processed on their
assigned thread we don't need that anymore and this can be replaced
with regular non-atomic operations.

Doing this alone has reduced the CPU usage of qc_do_build_pkt()
from 3.6 to 2.5% and increased the overall bit rate by about 1%.

2 years agoOPTIM: htx: inline the most common memcpy(8)
Willy Tarreau [Thu, 2 Feb 2023 14:32:20 +0000 (15:32 +0100)] 
OPTIM: htx: inline the most common memcpy(8)

On high traffic benchmarks, it's visible the the CPU is dominated by
calls to memcpy(), and many of those come from htx functions. It was
measured that 63% of those coming from htx are made on 8-byte blocks
which really are not worth a call to the function since a single
read-write cycle does it fine.

This commit adds an inline htx_memcpy() function that explicitly
checks for this length and just copies the data without that call.
It's even likely that it could be detected on const sizes, though
that was not done. This is already effective in reducing the number
of calls to memcpy().

2 years agoMINOR: quic: add config for retransmit limit
Amaury Denoyelle [Tue, 31 Jan 2023 10:44:50 +0000 (11:44 +0100)] 
MINOR: quic: add config for retransmit limit

Define a new configuration option "tune.quic.max-frame-loss". This is
used to specify the limit for which a single frame instance can be
detected as lost. If exceeded, the connection is closed.

This should be backported up to 2.7.

2 years agoMEDIUM: quic: implement a retransmit limit per frame
Amaury Denoyelle [Fri, 27 Jan 2023 16:54:15 +0000 (17:54 +0100)] 
MEDIUM: quic: implement a retransmit limit per frame

Add a <loss_count> new field in quic_frame structure. This field is set
to 0 and incremented each time a sent packet is declared lost. If
<loss_count> reached a hard-coded limit, the connection is deemed as
failing and is closed immediately with a CONNECTION_CLOSE using
INTERNAL_ERROR.

By default, limit is set to 10. This should ensure that overall memory
usage is limited if a peer behaves incorrectly.

This should be backported up to 2.7.

2 years agoMINOR: quic: refactor frame deallocation
Amaury Denoyelle [Thu, 2 Feb 2023 15:12:09 +0000 (16:12 +0100)] 
MINOR: quic: refactor frame deallocation

Define a new function qc_frm_free() to handle frame deallocation. New
BUG_ON() statements ensure that the deallocated frame is not referenced
by other frame. To support this, all LIST_DELETE() have been replaced by
LIST_DEL_INIT(). This should enforce that frame deallocation is robust.

As a complement, qc_frm_unref() has been moved into quic_frame module.
It is justified as this is a utility function related to frame
deallocation. It allows to use it in quic_pktns_tx_pkts_release() before
calling qc_frm_free().

This should be backported up to 2.7.

2 years agoMINOR: quic: define new functions for frame alloc
Amaury Denoyelle [Fri, 27 Jan 2023 16:47:49 +0000 (17:47 +0100)] 
MINOR: quic: define new functions for frame alloc

Define two utility functions for quic_frame allocation :
* qc_frm_alloc() is used to allocate a new frame
* qc_frm_dup() is used to allocate a new frame by duplicating an
  existing one

Theses functions are useful to centralize quic_frame initialization.
Note that pool_zalloc() is replaced by a proper pool_alloc() + explicit
initialization code.

This commit will simplify implementation of the per frame retransmission
limitation. Indeed, a new counter will be added in quic_frame structure
which must be initialized to 0.

This should be backported up to 2.7.

2 years agoMINOR: quic: ensure offset is properly set for STREAM frames
Amaury Denoyelle [Thu, 2 Feb 2023 15:45:07 +0000 (16:45 +0100)] 
MINOR: quic: ensure offset is properly set for STREAM frames

Care must be taken when reading/writing offset for STREAM frames. A
special OFF bit is set in the frame type to indicate that the field is
present. If not set, it is assumed that offset is 0.

To represent this, offset field of quic_stream structure must always be
initialized with a valid value in regards with its frame type OFF bit.

The previous code has no bug in part because pool_zalloc() is used to
allocate quic_frame instances. To be able to use pool_alloc(), offset is
always explicitely set to 0. If a non-null value is used, OFF bit is set
at the same occasion. A new BUG_ON() statement is added on frame builder
to ensure that the caller has set OFF bit if offset is non null.

This should be backported up to 2.7.

2 years agoMINOR: quic: remove fin from quic_stream frame type
Amaury Denoyelle [Thu, 2 Feb 2023 13:59:36 +0000 (14:59 +0100)] 
MINOR: quic: remove fin from quic_stream frame type

A dedicated <fin> field was used in quic_stream structure. However, this
info is already encoded in the frame type field as specified by QUIC
protocol.

In fact, only code for packet reception used the <fin> field. On the
sending side, we only checked for the FIN bit. To align both sides,
remove the <fin> field and only used the FIN bit.

This should be backported up to 2.7.

2 years agoBUILD: makefile: fix PCRE overriding specific lib path
Amaury Denoyelle [Fri, 3 Feb 2023 08:23:56 +0000 (09:23 +0100)] 
BUILD: makefile: fix PCRE overriding specific lib path

PCRE relies on pcre-config binary tool to provide includes/libs paths.
This may generate standard entries such as '/usr/lib' which will
override more specific ones if present before them on the linking step.

This situation was encountered when building with both QuicTLS and PCRE.
This generates a linking error as the default SSL libraries were used
for linking even with correct SSL flags pointing to QuicTLS dirs.

To fix this issue, USE_PCRE and its affiliated options have been moved
at the end of 'use_opts' variable. Indeed, related CFLAGS/LDFLAGS are
concatenated in their order of appearance through the macro
collect_opts_flags (see include/make/options.mk). PCRE in the last
position ensures it won't override specific entries declared before.

2 years agoBUG/MINOR: stats: use proper buffer size for http dump
Aurelien DARRAGON [Thu, 2 Feb 2023 14:03:12 +0000 (15:03 +0100)] 
BUG/MINOR: stats: use proper buffer size for http dump

In an attempt to fix GH #1873, ("BUG/MEDIUM: stats: Rely on a local trash
buffer to dump the stats") explicitly reduced output buffer size to leave
enough space for htx overhead under http context.

Github user debtsandbooze, who first reported the issue, came back to us
and said he was still able to make the http dump "hang" with the new fix.

After some tests, it became clear that htx_add_data_atonce() could fail from
time to time in stats_putchk(), even if htx was completely empty:

In http context, buffer size is maxed out at channel_htx_recv_limit().
Unfortunately, channel_htx_recv_limit() is not what we're looking for here
because limit() doesn't compute the proper htx overhead.

Using buf_room_for_htx_data() instead of channel_htx_recv_limit() to compute
max "usable" data space seems to be the last piece of work required for
the previous fix to work properly.

This should be backported everywhere the aforementioned commit is.

2 years agoBUG/MEDIUM: thread: consider secondary threads as idle+harmless during boot
Aurelien DARRAGON [Fri, 27 Jan 2023 14:13:28 +0000 (15:13 +0100)] 
BUG/MEDIUM: thread: consider secondary threads as idle+harmless during boot

idle and harmless bits in the tgroup_ctx structure were not explicitly
set during boot.

    | struct tgroup_ctx ha_tgroup_ctx[MAX_TGROUPS] = { };

As the structure is first statically initialized,
.threads_harmless and .threads_idle are automatically zero-
initialized by the compiler.

Unfortulately, this means that such threads are not considered idle
nor harmless by thread_isolate(_full)() functions until they enter
the polling loop (thread_harmless_now() and thread_idle_now() are
respectively called before entering the polling loop)

Because of this, any attempt to call thread_isolate() or thread_isolate_full()
during a startup phase with nbthreads >= 2 will cause thread_isolate to
loop until every secondary threads make it through their first polling loop.

If the startup phase is aborted during boot (ie: "-c" option to check the
configuration), secondary threads may be initialized but will never be started
(ie: they won't enter the polling loop), thus thread_isolate()
could would loop forever in such cases.

We can easily reveal the bug with this patch reproducer:

    |  diff --git a/src/haproxy.c b/src/haproxy.c
    |  index e91691658..0b733f6ee 100644
    |  --- a/src/haproxy.c
    |  +++ b/src/haproxy.c
    |  @@ -2317,6 +2317,10 @@ static void init(int argc, char **argv)
    |    if (pr || px) {
    |    /* At least one peer or one listener has been found */
    |    qfprintf(stdout, "Configuration file is valid\n");
    |  + printf("haproxy will loop...\n");
    |  + thread_isolate();
    |  + printf("we will never reach this\n");
    |  + thread_release();
    |    deinit_and_exit(0);
    |    }
    |    qfprintf(stdout, "Configuration file has no error but will not start (no listener) => exit(2).\n");

Now we start haproxy with a valid config:
$> haproxy -c -f valid.conf
Configuration file is valid
haproxy will loop...

^C

------------------------------------------------------------------------------

This did not cause any issue so far because no early deinit paths require
full thread isolation. But this may change when new features or requirements
are introduced, so we should fix this before it becomes a real issue.

To fix this, we explicitly assign .threads_harmless and .threads_idle
to .threads_enabled value in thread_map_to_groups() function during boot.
This is the proper place to do this since as long as .threads_enabled is not
explicitly set, its default value is also 0 (zero-initialized by the compiler)

code snippet from thread_isolate() function:
       ulong te = _HA_ATOMIC_LOAD(&ha_tgroup_info[tgrp].threads_enabled);
       ulong th = _HA_ATOMIC_LOAD(&ha_tgroup_ctx[tgrp].threads_harmless);

       if ((th & te) == te)
           break;

Thus thread_isolate(_full()) won't be looping forever in thread_isolate()
even if it were to be used before thread_map_to_groups() is executed.

No backport needed unless this is a requirement.

2 years agoBUG/MINOR: h3: fix crash due to h3 traces
Amaury Denoyelle [Tue, 31 Jan 2023 14:50:16 +0000 (15:50 +0100)] 
BUG/MINOR: h3: fix crash due to h3 traces

This commit is identical to the preceeding patch. However, these traces
are from another patch with a different backport scope :
    56a86ddfb97d740f965503f6b5991fafa347308c
    MINOR: h3: add missing traces on closure

This must be backported up to 2.7 where above patch is scheduled.

2 years agoBUG/MINOR: h3: fix crash due to h3 traces
Amaury Denoyelle [Tue, 31 Jan 2023 15:01:22 +0000 (16:01 +0100)] 
BUG/MINOR: h3: fix crash due to h3 traces

First H3 traces argument must be a connection instance or a NULL. Some
new traces were added recently with a qcc instance which caused a crash
when traces are activated.

This trace was added by the following patch :
    87f8766d3fbd10f9e8bf4902d37712612db64df5
    BUG/MEDIUM: h3: handle STOP_SENDING on control stream

This must be backported up to 2.6 along with the above patch.

2 years agoBUG/MEDIUM: ssl: wrong eviction from the session cache tree
William Lallemand [Tue, 31 Jan 2023 13:12:28 +0000 (14:12 +0100)] 
BUG/MEDIUM: ssl: wrong eviction from the session cache tree

When using WolfSSL, there are some cases were the SSL_CTX_sess_new_cb is
called with an existing session ID. These cases are not met with
OpenSSL.

When the ID is found in the session tree during the insertion, the
shared_block len is not set to 0 and is not used. However if later the
block is reused, since the len is not set to 0, the release callback
will be called an ebmb_delete will be tried on the block, even if it's
not in the tree, provoking a crash.

The code was buggy from the beginning, but the case never happen with
openssl which changes the ID.

Must be backported in every maintained branches.

2 years agoMINOR: h3: add missing traces on closure
Amaury Denoyelle [Mon, 30 Jan 2023 14:36:51 +0000 (15:36 +0100)] 
MINOR: h3: add missing traces on closure

Add traces for function h3_shutdown() / h3_send_goaway(). This should
help to debug problems related to connection closure.

This should be backported up to 2.7.

2 years agoBUG/MINOR: h3: reject RESET_STREAM received for control stream
Amaury Denoyelle [Mon, 30 Jan 2023 11:13:22 +0000 (12:13 +0100)] 
BUG/MINOR: h3: reject RESET_STREAM received for control stream

This commit is similar to the previous one. It reports an error if a
RESET_STREAM is received for the remote control stream. This will
generate a CONNECTION_CLOSE with H3_CLOSED_CRITICAL_STREAM error.

Note that contrary to the previous bug related to STOP_SENDING, this bug
was not encountered in real environment. As such, it is labelled as
MINOR. However, it could triggered the same crash as the previous patch.

This should be backported up to 2.6.

2 years agoBUG/MEDIUM: h3: handle STOP_SENDING on control stream
Amaury Denoyelle [Mon, 30 Jan 2023 11:12:43 +0000 (12:12 +0100)] 
BUG/MEDIUM: h3: handle STOP_SENDING on control stream

Before this patch, STOP_SENDING reception was considered valid even on
H3 control stream. This causes the emission in return of RESET_STREAM
and eventually the closure and freeing of the QCS instance. This then
causes a crash during connection closure as a GOAWAY frame is emitted on
the control stream which is now released.

To fix this crash, STOP_SENDING on the control stream is now properly
rejected as specified by RFC 9114. The new app_ops close callback is
used which in turn will generate a CONNECTION_CLOSE with error
H3_CLOSED_CRITICAL_STREAM.

This bug was detected in github issue #2006. Note that however it is
triggered by an incorrect client behavior. It may be useful to determine
which client behaves like this. If this case is too frequent,
STOP_SENDING should probably be silently ignored.

To reproduce this issue, quiche was patched to emit a STOP_SENDING on
its send() function in quiche/src/lib.rs:
     pub fn send(&mut self, out: &mut [u8]) -> Result<(usize, SendInfo)> {
-        self.send_on_path(out, None, None)
+        let ret = self.send_on_path(out, None, None);
+        self.streams.mark_stopped(3, true, 0);
+        ret
     }

This must be backported up to 2.6 along with the preceeding commit :
  MINOR: mux-quic/h3: define close callback

2 years agoMINOR: mux-quic/h3: define stream close callback
Amaury Denoyelle [Mon, 30 Jan 2023 11:12:11 +0000 (12:12 +0100)] 
MINOR: mux-quic/h3: define stream close callback

Define a new qcc_app_ops callback named close(). This will be used to
notify app-layer about the closure of a stream by the remote peer. Its
main usage is to ensure that the closure is allowed by the application
protocol specification.

For the moment, close is not implemented by H3 layer. However, this
function will be mandatory to properly reject a STOP_SENDING on the
control stream and preventing a later crash. As such, this commit must
be backported with the next one on 2.6.

This is related to github issue #2006.