]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
3 years agoDEBUG: pools: add new build option DEBUG_POOL_TRACING
Willy Tarreau [Mon, 24 Jan 2022 14:52:51 +0000 (15:52 +0100)] 
DEBUG: pools: add new build option DEBUG_POOL_TRACING

This new option, when set, will cause the callers of pool_alloc() and
pool_free() to be recorded into an extra area in the pool that is expected
to be helpful for later inspection (e.g. in core dumps). For example it
may help figure that an object was released to a pool with some sub-fields
not yet released or that a use-after-free happened after releasing it,
with an immediate indication about the exact line of code that released
it (possibly an error path).

This only works with the per-thread cache, and even objects refilled from
the shared pool directly into the thread-local cache will have a NULL
there. That's not an issue since these objects have not yet been freed.
It's worth noting that pool_alloc_nocache() continues not to set any
caller pointer (e.g. when the cache is empty) because that would require
a possibly undesirable API change.

The extra cost is minimal (one pointer per object) and this completes
well with DEBUG_POOL_INTEGRITY.

3 years agoMINOR: pools: extend pool_cache API to pass a pointer to a caller
Willy Tarreau [Mon, 24 Jan 2022 14:51:50 +0000 (15:51 +0100)] 
MINOR: pools: extend pool_cache API to pass a pointer to a caller

This adds a caller to pool_put_to_cache() and pool_get_from_cache()
which will optionally be used to pass a pointer to their callers. For
now it's not used, only the API is extended to support this pointer.

3 years agoMINOR: pools: prepare POOL_EXTRA to be split into multiple extra fields
Willy Tarreau [Mon, 24 Jan 2022 14:44:26 +0000 (15:44 +0100)] 
MINOR: pools: prepare POOL_EXTRA to be split into multiple extra fields

Here the idea is to calculate the POOL_EXTRA size that is appended at
the end of a pool object based on the sum of enabled optional fields
so that we can more easily compute offsets and sizes depending on build
options.

For this, POOL_EXTRA is replaced with POOL_EXTRA_MARK which itself is
set either to sizeof(void*) or zero depending on whether we enable
marking the origin pool or not upon allocation.

3 years agoMINOR: pools: partially uninline pool_alloc()
Willy Tarreau [Mon, 24 Jan 2022 15:09:29 +0000 (16:09 +0100)] 
MINOR: pools: partially uninline pool_alloc()

The pool_alloc() function was already a wrapper to __pool_alloc() which
was also inlined but took a set of flags. This latter was uninlined and
moved to pool.c, and pool_alloc()/pool_zalloc() turned to macros so that
they can more easily evolve to support debugging options.

The number of call places made this code grow over time and doing only
this change saved ~1% of the whole executable's size.

3 years agoMINOR: pools: partially uninline pool_free()
Willy Tarreau [Mon, 24 Jan 2022 10:51:43 +0000 (11:51 +0100)] 
MINOR: pools: partially uninline pool_free()

The pool_free() function has become a bit big over time due to the
extra consistency checks. It used to remain inline only to deal
cleanly with the NULL pointer free that's quite present on some
structures (e.g. in stream_free()).

Here we're splitting the function in two:
  - __pool_free() does the inner block without the pointer test and
    becomes a function ;

  - pool_free() is now a macro that only checks the pointer and calls
    __pool_free() if needed.

The use of a macro versus an inline function is only motivated by an
easier intrumentation of the code later.

With this change, the code size reduces by ~1%, which means that at
this point all pool_free() call places used to represent more than
1% of the total code size.

3 years agoMINOR: ssl: fix build in release mode
Amaury Denoyelle [Mon, 24 Jan 2022 10:04:05 +0000 (11:04 +0100)] 
MINOR: ssl: fix build in release mode

Fix potential null pointer dereference. In fact, this case is not
possible, only a mistake in SSL ex-data initialization may cause it :
either connection is set or quic_conn, which allows to retrieve
the bind_conf.

A BUG_ON was already present but this does not cover release build.

3 years agoMINOR: quic: initialize ssl_sock_ctx alongside the quic_conn
Amaury Denoyelle [Tue, 18 Jan 2022 15:44:34 +0000 (16:44 +0100)] 
MINOR: quic: initialize ssl_sock_ctx alongside the quic_conn

Extract the allocation of ssl_sock_ctx from qc_conn_init to a dedicated
function qc_conn_alloc_ssl_ctx. This function is called just after
allocating a new quic_conn, without waiting for the initialization of
the connection. It allocates the ssl_sock_ctx and the quic_conn tasklet.

This change is now possible because the SSL callbacks are dealing with a
quic_conn instance.

This change is required to be able to delay the connection allocation
and handle handshake packets without it.

3 years agoMEDIUM: quic/ssl: add new ex data for quic_conn
Amaury Denoyelle [Wed, 19 Jan 2022 09:03:30 +0000 (10:03 +0100)] 
MEDIUM: quic/ssl: add new ex data for quic_conn

Allow to register quic_conn as ex-data in SSL callbacks. A new index is
used to identify it as ssl_qc_app_data_index.

Replace connection by quic_conn as SSL ex-data when initializing the QUIC
SSL session. When using SSL callbacks in QUIC context, the connection is
now NULL. Used quic_conn instead to retrieve the required parameters.
Also clean up

The same changes are conducted inside the QUIC SSL methods of xprt-quic
: connection instance usage is replaced by quic_conn.

3 years agoMINOR: quic: set listener accept cb on parsing
Amaury Denoyelle [Tue, 18 Jan 2022 14:39:02 +0000 (15:39 +0100)] 
MINOR: quic: set listener accept cb on parsing

Define a special accept cb for QUIC listeners to quic_session_accept().
This operation is conducted during the proto.add callback when creating
listeners.

A special care is now taken care when setting the standard callback
session_accept_fd() to not overwrite if already defined by the proto
layer.

3 years agoMINOR: quic: remove dereferencement of connection when possible
Amaury Denoyelle [Tue, 18 Jan 2022 15:50:58 +0000 (16:50 +0100)] 
MINOR: quic: remove dereferencement of connection when possible

Some functions of xprt-quic were still using connection instead of
quic_conn. This must be removed as the two are decorrelated : a
quic_conn can exist without a connection.

3 years agoMINOR: quic: fix indentation in qc_send_ppkts
Amaury Denoyelle [Tue, 18 Jan 2022 15:48:17 +0000 (16:48 +0100)] 
MINOR: quic: fix indentation in qc_send_ppkts

Adjust wrong mixing of tabs/spaces.

3 years agoMINOR: quic: add missing include in quic_sock
Amaury Denoyelle [Wed, 19 Jan 2022 15:18:44 +0000 (16:18 +0100)] 
MINOR: quic: add missing include in quic_sock

Add quic_sock.h include in corresponding source file quic_sock.c.

3 years agoDEBUG: pools: add new build option DEBUG_POOL_INTEGRITY
Willy Tarreau [Fri, 21 Jan 2022 18:00:25 +0000 (19:00 +0100)] 
DEBUG: pools: add new build option DEBUG_POOL_INTEGRITY

When enabled, objects picked from the cache are checked for corruption
by comparing their contents against a pattern that was placed when they
were inserted into the cache. Objects are also allocated in the reverse
order, from the oldest one to the most recent, so as to maximize the
ability to detect such a corruption. The goal is to detect writes after
free (or possibly hardware memory corruptions). Contrary to DEBUG_UAF
this cannot detect reads after free, but may possibly detect later
corruptions and will not consume extra memory. The CPU usage will
increase a bit due to the cost of filling/checking the area and for the
preference for cold cache instead of hot cache, though not as much as
with DEBUG_UAF. This option is meant to be usable in production.

3 years agoMINOR: quic: Wrong packet number space selection
Frédéric Lécaille [Fri, 21 Jan 2022 15:52:56 +0000 (16:52 +0100)] 
MINOR: quic: Wrong packet number space selection

It is possible that the listener is in INITIAL state, but have to probe
with Handshake packets. In this case, when entering qc_prep_pkts() there
is nothing to do. We must select the next packet number space (or encryption
level) to be able to probe with such packet type.

3 years agoMINOR: quic: Add QUIC_FT_RETIRE_CONNECTION_ID parsing case
Frédéric Lécaille [Fri, 21 Jan 2022 12:55:03 +0000 (13:55 +0100)] 
MINOR: quic: Add QUIC_FT_RETIRE_CONNECTION_ID parsing case

At this time, we do not do anything. This is only to prevent a packet
from being parsed and to pass some test irrespective of the CIDs management.

3 years agoMINOR: quic: free SSL context on quic_conn free
Amaury Denoyelle [Thu, 20 Jan 2022 16:43:20 +0000 (17:43 +0100)] 
MINOR: quic: free SSL context on quic_conn free

Free the SSL context attached to the quic_conn when freeing the
connection. This fixes a memory leak for every QUIC connection.

3 years agoMINOR: quic: fix race-condition on xprt tasklet free
Amaury Denoyelle [Thu, 20 Jan 2022 16:43:02 +0000 (17:43 +0100)] 
MINOR: quic: fix race-condition on xprt tasklet free

Remove the unsafe call to tasklet_free in quic_close. At this stage the
tasklet may already be scheduled by an other threads even after if the
quic_conn refcount is now null. It will probably cause a crash on the
next tasklet processing.

Use tasklet_kill instead to ensure that the tasklet is freed in a
thread-safe way. Note that quic_conn_io_cb is not protected by the
refcount so only the quic_conn pinned thread must kill the tasklet.

3 years agoMINOR: quic: adjust quic_conn refcount decrement
Amaury Denoyelle [Thu, 20 Jan 2022 15:40:36 +0000 (16:40 +0100)] 
MINOR: quic: adjust quic_conn refcount decrement

Adjust slightly refcount code decrement on quic_conn close. A new
function named quic_conn_release is implemented. This function is
responsible to remove the quic_conn from CIDs trees and decrement the
refcount to free the quic_conn once all threads have finished to work
with it.

For now, quic_close is responsible to call it so the quic_conn is
scheduled to be free by upper layers. In the future, it may be useful to
delay it to be able to send remaining data or waiting for missing ACKs
for example.

This simplify quic_conn_drop which do not require the lock anymore.
Also, this can help to free the connection more quickly in some cases.

3 years agoMINOR: quic: do not use quic_conn after dropping it
Amaury Denoyelle [Fri, 21 Jan 2022 13:54:58 +0000 (14:54 +0100)] 
MINOR: quic: do not use quic_conn after dropping it

quic_conn_drop decrement the refcount and may free the quic_conn if
reaching 0. The quic_conn should not be dereferenced again after it in
any case even for traces.

3 years agoBUG/MINOR: stream: make the call_rate only count the no-progress calls
Willy Tarreau [Thu, 20 Jan 2022 17:42:16 +0000 (18:42 +0100)] 
BUG/MINOR: stream: make the call_rate only count the no-progress calls

We have an anti-looping protection in process_stream() that detects bugs
that used to affect a few filters like compression in the past which
sometimes forgot to handle a read0 or a particular error, leaving a
thread looping at 100% CPU forever. When such a condition is detected,
an alert it emitted and the process is killed so that it can be replaced
by a sane one:

  [ALERT]    (19061) : A bogus STREAM [0x274abe0] is spinning at 2057156
             calls per second and refuses to die, aborting now! Please
             report this error to developers [strm=0x274abe0,3 src=unix
             fe=MASTER be=MASTER dst=<MCLI> txn=(nil),0 txn.req=-,0
             txn.rsp=-,0 rqf=c02000 rqa=10000 rpf=88000021 rpa=8000000
             sif=EST,40008 sib=DIS,84018 af=(nil),0 csf=0x274ab90,8600
             ab=0x272fd40,1 csb=(nil),0
             cof=0x25d5d80,1300:PASS(0x274aaf0)/RAW((nil))/unix_stream(9)
             cob=(nil),0:NONE((nil))/NONE((nil))/NONE(0) filters={}]
    call trace(11):
    |       0x4dbaab [c7 04 25 01 00 00 00 00]: stream_dump_and_crash+0x17b/0x1b4
    |       0x4df31f [e9 bd c8 ff ff 49 83 7c]: process_stream+0x382f/0x53a3
    (...)

One problem with this detection is that it used to only count the call
rate because we weren't sure how to make it more accurate, but the
threshold was high enough to prevent accidental false positives.

There is actually one case that manages to trigger it, which is when
sending huge amounts of requests pipelined on the master CLI. Some
short requests such as "show version" are sufficient to be handled
extremely fast and to cause a wake up of an analyser to parse the
next request, then an applet to handle it, back and forth. But this
condition is not an error, since some data are being forwarded by
the stream, and it's easy to detect it.

This patch modifies the detection so that update_freq_ctr() only
applies to calls made without CF_READ_PARTIAL nor CF_WRITE_PARTIAL
set on any of the channels, which really indicates that nothing is
happening at all.

This is greatly sufficient and extremely effective, as the call above
is still caught (shutr being ignored by an analyser) while a loop on
the master CLI now has no effect. The "call_rate" field in the detailed
"show sess" output will now be much lower, except for bogus streams,
which may help spot them. This field is only there for developers
anyway so it's pretty fine to slightly adjust its meaning.

This patch could be backported to stable versions in case of reports
of such an issue, but as that's unlikely, it's not really needed.

3 years agoBUG/MEDIUM: mcli: always realign wrapping buffers before parsing them
Willy Tarreau [Thu, 20 Jan 2022 07:47:35 +0000 (08:47 +0100)] 
BUG/MEDIUM: mcli: always realign wrapping buffers before parsing them

Pipelined commands easily result in request buffers to wrap, and the
master-cli parser only deals with linear buffers since it needs contiguous
keywords to look for in a list. As soon as a buffer wraps, some commands
are ignored and the parser is called in loops because the wrapped data
do not leave the buffer.

Let's take the easiest path that's already used at the HTTP layer, we
simply realign the buffer if its input wraps. This rarely happens anyway
(typically once per buffer), remains reasonably cheap and guarantees this
cannot happen anymore.

This needs to be backported as far as 2.0.

3 years agoBUG/MEDIUM: mcli: do not try to parse empty buffers
Willy Tarreau [Thu, 20 Jan 2022 07:31:50 +0000 (08:31 +0100)] 
BUG/MEDIUM: mcli: do not try to parse empty buffers

When pcli_parse_request() is called with an empty buffer, it still tries
to parse it and can go on believing it finds an empty request if the last
char before the beginning of the buffer is a '\n'. In this case it overwrites
it with a zero and processes it as an empty command, doing nothing but not
making the buffer progress. This results in an infinite loop that is stopped
by the watchdog. For a reason related to another issue (yet to be fixed),
this can easily be reproduced by pipelining lots of commands such as
"show version".

Let's add a length check after the search for a '\n'.

This needs to be backported as far as 2.0.

3 years agoBUG/MEDIUM: cli: Never wait for more data on client shutdown
Christopher Faulet [Tue, 18 Jan 2022 07:44:23 +0000 (08:44 +0100)] 
BUG/MEDIUM: cli: Never wait for more data on client shutdown

When a shutdown is detected on the cli, we try to execute all pending
commands first before closing the connection. It is required because
commands execution is serialized. However, when the last part is a partial
command, the cli connection is not closed, waiting for more data. Because
there is no timeout for now on the cli socket, the connection remains
infinitely in this state. And because the maxconn is set to 10, if it
happens several times, the cli socket quickly becomes unresponsive because
all its slots are waiting for more data on a closed connections.

This patch should fix the issue #1512. It must be backported as far as 2.0.

3 years agoMINOR: quic: Probe even if coalescing
Frédéric Lécaille [Wed, 19 Jan 2022 17:54:18 +0000 (18:54 +0100)] 
MINOR: quic: Probe even if coalescing

Again, we fix a reminiscence of the way we probed before probing by packet.
When we were probing by datagram we inspected <prv_pkt> to know if we were
coalescing several packets. There is no need to do that at all when probing by packet.
Furthermore this could lead to blocking situations where we want to probe but
are limited by the congestion control (<cwnd> path variable). This must not be
the case. When probing we must do it regardless of the congestion control.

3 years agoMINOR: quic: Release asap TX frames to be transmitted
Frédéric Lécaille [Wed, 19 Jan 2022 16:48:40 +0000 (17:48 +0100)] 
MINOR: quic: Release asap TX frames to be transmitted

This is done only for ack-eliciting frames to be sent from Initial and Handshake
packet number space when discarding them.

3 years agoMINOR: quic: Release RX Initial packets asap
Frédéric Lécaille [Wed, 19 Jan 2022 16:29:48 +0000 (17:29 +0100)] 
MINOR: quic: Release RX Initial packets asap

This is to free up some space in the RX buffer as soon as possible.

3 years agoMINOR: quic: Speeding up handshake completion
Frédéric Lécaille [Mon, 17 Jan 2022 17:16:27 +0000 (18:16 +0100)] 
MINOR: quic: Speeding up handshake completion

If a client resend Initial CRYPTO data, this is because it did not receive all
the server Initial CRYPTO data. With this patch we prepare a fast retransmission
without waiting for the PTO timer expiration sending old Initial CRYPTO data,
coalescing them with Handshake CRYPTO if present in the same datagram. Furthermore
we send also a datagram made of previously sent Hanshashke CRYPTO data if any.

3 years agoMINOR: quic: Probe regardless of the congestion control
Frédéric Lécaille [Mon, 17 Jan 2022 16:56:20 +0000 (17:56 +0100)] 
MINOR: quic: Probe regardless of the congestion control

When probing, we must not take into an account the congestion control window.
This was not completely correctly implemented: qc_build_frms() could fail
because of this limit when comparing the head of the packet againts the
congestion control window. With this patch we make it fail only when
we are not probing.

3 years agoMINOR: quic: Send two ack-eliciting packets when probing packet number spaces
Frédéric Lécaille [Mon, 17 Jan 2022 13:26:12 +0000 (14:26 +0100)] 
MINOR: quic: Send two ack-eliciting packets when probing packet number spaces

This is to avoid too much PTO timer expirations for 01RTT and Handshake packet
number spaces. Furthermore we are not limited by the anti-amplication for 01RTT
packet number space. According to the RFC we can send up to two packets.

3 years agoCLEANUP: quic: Replace <nb_pto_dgrams> by <probe>
Frédéric Lécaille [Mon, 17 Jan 2022 10:06:10 +0000 (11:06 +0100)] 
CLEANUP: quic: Replace <nb_pto_dgrams> by <probe>

This modification should have come with this commit:
    "MINOR: quic: Remove nb_pto_dgrams quic_conn struct member"
where the nb_pto_dgrams quic_conn struct member was removed.

3 years agoMINOR: quic: Add the number of TX bytes to traces
Frédéric Lécaille [Mon, 17 Jan 2022 09:51:43 +0000 (10:51 +0100)] 
MINOR: quic: Add the number of TX bytes to traces

This should be helpful to diagnose some issues regarding packet loss
and recovery issues.

3 years agoMINOR: quic: Splice the frames which could not be added to packets
Frédéric Lécaille [Fri, 14 Jan 2022 19:39:18 +0000 (20:39 +0100)] 
MINOR: quic: Splice the frames which could not be added to packets

When building packets to send, we build frames computing their sizes
to have more chance to be added to new packets. There are rare cases
where this packet coult not be built because of the congestion control
which may for instance prevent us from building a packet with padding
(retransmitted Initial packets). In such a case, the pre-built frames
were lost because added to the packet frame list but not move packet
to the packet number space they come from.

With this patch we add the frames to the packet only if it could be built
and move them back to the packet number space if not.

3 years agoMINOR: quic: Remove the packet number space TX MT_LIST
Frédéric Lécaille [Fri, 14 Jan 2022 19:23:22 +0000 (20:23 +0100)] 
MINOR: quic: Remove the packet number space TX MT_LIST

There is no need to use an MT_LIST to store frames to send from a packet
number space. This is a reminiscence for multi-threading support for the TX part.

3 years agoMINOR: quic: Retransmit the TX frames in the same order
Frédéric Lécaille [Fri, 14 Jan 2022 14:51:52 +0000 (15:51 +0100)] 
MINOR: quic: Retransmit the TX frames in the same order

This is only to please the peer. We resend the TX frames in the
same order they have been sent.

3 years agoMEDIUM: h2/hpack: emit a Dynamic Table Size Update after settings change
Willy Tarreau [Thu, 13 Jan 2022 15:00:12 +0000 (16:00 +0100)] 
MEDIUM: h2/hpack: emit a Dynamic Table Size Update after settings change

As reported by @jinsubsim in github issue #1498, there is an
interoperability issue between nghttp2 as a client and a few servers
among which haproxy (in fact likely all those which do not make use
of the dynamic headers table in responses or which do not intend to
use a larger table), when reducing the header table size below 4096.
These are easily testable this way:

  nghttp -v -H":method: HEAD" --header-table-size=0 https://$SITE

It will result in a compression error for those which do not start
with an HPACK dynamic table size update opcode.

There is a possible interpretation of the H2 and HPACK specs that
says that an HPACK encoder must send an HPACK headers table update
confirming the new size it will be using after having acknowledged
it, because since it's possible for a decoder to advertise a late
SETTINGS and change it after transfers have begun, the initially
advertised value might very well be seen as a first change from the
initial setting, and the HPACK spec doesn't specify the side which
causes the change that triggers a DTSU update, which was essentially
summed up in this question from nghttp2's author when this issue
was already raised 6 years ago, but which didn't really find a solid
response by then:

  https://lists.w3.org/Archives/Public/ietf-http-wg/2015OctDec/0107.html

The ongoing consensus based on what some servers are doing and that aims
at limiting interoperability issues seems to be that a DTSU is expected
for each reduction from the current size, which should be reflected in
the next revision of the H2 spec:

  https://github.com/httpwg/http2-spec/pull/1005

Given that we do not make use of this table we can emit a DTSU of zero
before encoding any HPACK frame. However, some clients do not support
receiving DTSU with such values (e.g. VTest) so we cannot do it
inconditionnally!

The current patch aims at sticking as close to the spec as possible by
proceeding this way:
  - when a SETTINGS_HEADER_TABLE_SIZE is received, a flag is set
    indicating that the value changed
  - before sending any HPACK frame, this flag is checked to see if
    an update is wanted and if none was sent
  - in this case a DTSU of size zero is emitted and a flag is set
    to mention it was emitted so that it never has to be sent again

This addresses the problem with nghttp2 without affecting VTest.

More context is available here:
  https://github.com/nghttp2/nghttp2/issues/1660
  https://lists.w3.org/Archives/Public/ietf-http-wg/2021OctDec/0235.html

Many thanks to @jinsubsim for this report and participating to the issue
that led to an improvement of the H2 spec.

This should be backported to stable releases in a timely manner, ideally
as far as 2.4 once the h2spec update is merged, then to other versions
after a few months of observation or in case an issue around this is
reported.

3 years agoBUG/MINOR: cli: avoid O(bufsize) parsing cost on pipelined commands
Willy Tarreau [Wed, 19 Jan 2022 16:23:52 +0000 (17:23 +0100)] 
BUG/MINOR: cli: avoid O(bufsize) parsing cost on pipelined commands

Sending pipelined commands on the CLI using a semi-colon as a delimiter
has a cost that grows linearly with the buffer size, because co_getline()
is called for each word and looks up a '\n' in the whole buffer while
copying its contents into a temporary buffer.

This causes huge parsing delays, for example 3s for 100k "show version"
versus 110ms if parsed only once for a default 16k buffer.

This patch makes use of the new co_getdelim() function to support both
an LF and a semi-colon as delimiters so that it's no more needed to parse
the whole buffer, and that commands are instantly retrieved. We still
need to rely on co_getline() in payload mode as escapes and semi-colons
are not used there.

It should likely be backported where CLI processing speed matters, but
will require to also backport previous patch "MINOR: channel: add new
function co_getdelim() to support multiple delimiters". It's worth noting
that backporting it without "MEDIUM: cli: yield between each pipelined
command" would significantly increase the ratio of disconnections caused
by empty request buffers, for the sole reason that the currently slow
parsing grants more time to request data to come in. As such it would
be better to backport the patch above before taking this one.

3 years agoMINOR: channel: add new function co_getdelim() to support multiple delimiters
Willy Tarreau [Wed, 19 Jan 2022 16:19:52 +0000 (17:19 +0100)] 
MINOR: channel: add new function co_getdelim() to support multiple delimiters

For now we have co_getline() which reads a buffer and stops on LF, and
co_getword() which reads a buffer and stops on one arbitrary delimiter.
But sometimes we'd need to stop on a set of delimiters (CR and LF, etc).

This patch adds a new function co_getdelim() which takes a set of delimiters
as a string, and constructs a small map (32 bytes) that's looked up during
parsing to stop after the first delimiter found within the set. It also
supports an optional escape character that skips a delimiter (typically a
backslash). For the rest it works exactly like the two other variants.

3 years agoMEDIUM: cli: yield between each pipelined command
Willy Tarreau [Wed, 19 Jan 2022 16:11:36 +0000 (17:11 +0100)] 
MEDIUM: cli: yield between each pipelined command

Pipelining commands on the CLI is sometimes needed for batched operations
such as map deletion etc, but it causes two problems:
  - some possibly long-running commands will be run in series without
    yielding, possibly causing extremely long latencies that will affect
    quality of service and even trigger the watchdog, as seen in github
    issue #1515.

  - short commands that end on a buffer size boundary, when not run in
    interactive mode, will often cause the socket to be closed when
    the last command is parsed, because the buffer is empty.

This patch proposes a small change to this: by yielding in the CLI applet
after processing a command when there are data left, we significantly
reduce the latency, since only one command is executed per call, and
we leave an opportunity for the I/O layers to refill the request buffer
with more commands, hence to execute all of them much more often.

With this change there's no more watchdog triggered on long series of
"del map" on large map files, and the operations are much less disturbed.
It would be desirable to backport this patch to stable versions after some
period of observation in recent versions.

3 years agoDOC: management: mark "set server ssl" as deprecated
William Lallemand [Wed, 19 Jan 2022 14:17:08 +0000 (15:17 +0100)] 
DOC: management: mark "set server ssl" as deprecated

This command was integrated in 2.4 when it was not possible to handle
SSL with dynamic servers, this is now possible so we should prefer this
way.

Must be backported in 2.5.

3 years agoCI: refactor OpenTracing build script
Ilya Shipitsin [Sat, 15 Jan 2022 09:23:37 +0000 (14:23 +0500)] 
CI: refactor OpenTracing build script

re-use scripts/build-ot.sh in CI again. Bump opentracing-cpp to 1.6.0

3 years agoBUG/MEDIUM: server: avoid changing healthcheck ctx with set server ssl
William Dauchy [Thu, 6 Jan 2022 15:57:15 +0000 (16:57 +0100)] 
BUG/MEDIUM: server: avoid changing healthcheck ctx with set server ssl

While giving a fresh try to `set server ssl` (which I wrote), I realised
the behavior is a bit inconsistent. Indeed when using this command over
a server with ssl enabled for the data path but also for the health
check path we have:

- data and health check done using tls
- emit `set server be_foo/srv0 ssl off`
- data path and health check path becomes plain text
- emit `set server be_foo/srv0 ssl on`
- data path becomes tls and health check path remains plain text

while I thought the end result would be:
- data path and health check path comes back in tls

In the current code we indeed erase all connections while deactivating,
but restore only the data path while activating.  I made this mistake in
the past because I was testing with a case where the health check plain
text by default.

There are several ways to solve this issue. The cleanest one would
probably be to avoid changing the health check connection when we use
`set server ssl` command, and create a new command `set server
ssl-check` to change this. For now I assumed this would be ok to simply
avoid changing the health check path and be more consistent.

This patch tries to address that and also update the documentation. It
should not break the existing usage with health check on plain text, as
in this case they should have `no-check-ssl` in defaults.  Without this
patch, it makes the command unusable in an env where you have a list of
server to add along the way with initial `server-template`, and all
using tls for data and healthcheck path.

For 2.6 we should probably reconsider and add `set server ssl-check`
command for better granularity of cases.

If this solution is accepted, this patch should be backported up to >=
2.4.

The alternative solution was to restore the previous state, but I
believe this will create even more confusion in the future.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
3 years agoBUILD/MINOR: fix solaris build with clang.
David Carlier [Thu, 13 Jan 2022 19:16:48 +0000 (19:16 +0000)] 
BUILD/MINOR: fix solaris build with clang.

clang 9 sets the level to POSIX 2004.

3 years agoBUG/MINOR: httpclient/lua: don't pop the lua stack when getting headers
William Lallemand [Fri, 14 Jan 2022 16:59:01 +0000 (17:59 +0100)] 
BUG/MINOR: httpclient/lua: don't pop the lua stack when getting headers

hlua_httpclient_table_to_hdrs() does a lua_pop(L, 1) at the end of the
function, this is supposed to be done in the caller and it is already be
done in hlua_httpclient_send().

This call has the consequence of poping the next parameter of the
httpclient, ignoring it.

This patch fixes the issue by removing the lua_pop(L, 1).

Must be backported in 2.5.

3 years agoBUG/MINOR: httpclient: set default Accept and User-Agent headers
William Lallemand [Fri, 14 Jan 2022 13:10:33 +0000 (14:10 +0100)] 
BUG/MINOR: httpclient: set default Accept and User-Agent headers

Some servers require at least an Accept and a User-Agent header in the
request. This patch sets some default value.

Must be backported in 2.5.

3 years agoBUG/MINOR: httpclient: don't send an empty body
William Lallemand [Fri, 14 Jan 2022 13:08:34 +0000 (14:08 +0100)] 
BUG/MINOR: httpclient: don't send an empty body

Forbid the httpclient to send an empty chunked client when there is no
data to send. It does happen when doing a simple GET too.

Must be backported in 2.5.

3 years agoCI: github actions: use cache for OpenTracing
Ilya Shipitsin [Thu, 13 Jan 2022 06:36:28 +0000 (11:36 +0500)] 
CI: github actions: use cache for OpenTracing

this caches OpenTracing libs between builds, should save couple of minutes
for each build.

3 years agoBUG/MEDIUM: htx: Adjust length to add DATA block in an empty HTX buffer
Christopher Faulet [Wed, 12 Jan 2022 13:03:42 +0000 (14:03 +0100)] 
BUG/MEDIUM: htx: Adjust length to add DATA block in an empty HTX buffer

htx_add_data() is able to partially consume data. However there is a bug
when the HTX buffer is empty.  The data length is not properly
adjusted. Thus, if it exceeds the HTX buffer size, no block is added. To fix
the issue, the length is now adjusted first.

This patch must be backported as far as 2.0.

3 years agoMINOR: quic: Do not wakeup the I/O handler before the mux is started
Frédéric Lécaille [Wed, 12 Jan 2022 16:46:56 +0000 (17:46 +0100)] 
MINOR: quic: Do not wakeup the I/O handler before the mux is started

If we wakeup the I/O handler before the mux is started, it is possible
it has enough time to parse the ClientHello TLS message and update the
mux transport parameters, leading to a crash.
So, we initialize ->qcc quic_conn struct member at the very last time,
when the mux if fully initialized. The condition to wakeup the I/O handler
from lstnr_rcv_pkt() is: xprt context and mux both initialized.
Note that if the xprt context is initialized, it implies its tasklet is
initialized. So, we do not check anymore this latter condition.

3 years agoMINOR: quic: As server, skip 0-RTT packet number space
Frédéric Lécaille [Wed, 12 Jan 2022 14:32:55 +0000 (15:32 +0100)] 
MINOR: quic: As server, skip 0-RTT packet number space

This is true only when we are building packets. A QUIC server never
sends 0-RTT packets. So let't skip the associated TLS encryption level.

3 years agoMINOR: pools: enable pools with DEBUG_FAIL_ALLOC as well
Willy Tarreau [Wed, 12 Jan 2022 15:28:06 +0000 (16:28 +0100)] 
MINOR: pools: enable pools with DEBUG_FAIL_ALLOC as well

During 2.4-dev, fault injection was enabled for cached pools with commit
207c09509 ("MINOR: pools: move the fault injector to __pool_alloc()"),
except that the condition for CONFIG_HAP_POOLS still depended on
DEBUG_FAIL_ALLOC not being set, which limits the usability to cases
where the define is set by hand. Let's remove it from the equation as
this is not a constraint anymore. While a bit old, there's no need to
backport this as it's only used during development.

3 years agoBUG/MEDIUM: connection: properly leave stopping list on error
Willy Tarreau [Wed, 12 Jan 2022 16:24:26 +0000 (17:24 +0100)] 
BUG/MEDIUM: connection: properly leave stopping list on error

The stopping-list management introduced by commit d3a88c1c3 ("MEDIUM:
connection: close front idling connection on soft-stop") missed two
error paths in the H1 and H2 muxes. The effect is that if a stream
or HPACK table couldn't be allocated for these incoming connections,
we would leave with the connection freed still attached to the
stopping_list and it would never leave it, resulting in use-after-free
hence either a crash or a data corruption.

This is marked as medium as it only happens under extreme memory pressure
or when playing with tune.fail-alloc. Other stability issues remain in
such a case so that abnormal behaviors cannot be explained by this bug
alone.

This must be backported to 2.4.

3 years agoMINOR: quic: free xprt tasklet on its thread
Amaury Denoyelle [Wed, 12 Jan 2022 13:54:23 +0000 (14:54 +0100)] 
MINOR: quic: free xprt tasklet on its thread

Free the ssl_sock_ctx tasklet in quic_close() instead of
quic_conn_drop(). This ensures that the tasklet is destroyed safely by
the same thread.

This has no impact as the free operation was previously conducted with
care and should not be responsible of any crash.

3 years agoMEDIUM: quic: implement Retry emission
Amaury Denoyelle [Tue, 11 Jan 2022 13:16:37 +0000 (14:16 +0100)] 
MEDIUM: quic: implement Retry emission

Implement the emission of Retry packets. These packets are emitted in
response to Initial from clients without token. The token from the Retry
packet contains the ODCID from the Initial packet.

By default, Retry packet emission is disabled and the handshake can
continue without address validation. To enable Retry, a new bind option
has been defined named "quic-force-retry". If set, the handshake must be
conducted only after receiving a token in the Initial packet.

3 years agoMINOR: quic: define retry_source_connection_id TP
Amaury Denoyelle [Tue, 11 Jan 2022 11:03:09 +0000 (12:03 +0100)] 
MINOR: quic: define retry_source_connection_id TP

Define a new QUIC transport parameter retry_source_connection_id. This
parameter is set only by server, after issuing a Retry packet.

3 years agoMEDIUM: quic: implement Initial token parsing
Amaury Denoyelle [Tue, 11 Jan 2022 13:11:32 +0000 (14:11 +0100)] 
MEDIUM: quic: implement Initial token parsing

Implement the parsing of token from Initial packets. It is expected that
the token contains a CID which is the DCID from the Initial packet
received from the client without token which triggers a Retry packet.
This CID is then used for transport parameters.

Note that at the moment Retry packet emission is not implemented. This
will be achieved in a following commit.

3 years agoMINOR: quic: implement Retry TLS AEAD tag generation
Amaury Denoyelle [Tue, 11 Jan 2022 10:57:00 +0000 (11:57 +0100)] 
MINOR: quic: implement Retry TLS AEAD tag generation

Implement a new QUIC TLS related function
quic_tls_generate_retry_integrity_tag(). This function can be used to
calculate the AEAD tag of a Retry packet.

3 years agoMINOR: quic: add config parse source file
Amaury Denoyelle [Tue, 11 Jan 2022 10:54:59 +0000 (11:54 +0100)] 
MINOR: quic: add config parse source file

Create a new dedicated source file for QUIC related options parsing on
the bind line.

3 years agoMINOR: quic: fix return of quic_dgram_read
Amaury Denoyelle [Tue, 11 Jan 2022 13:20:46 +0000 (14:20 +0100)] 
MINOR: quic: fix return of quic_dgram_read

It is expected that quic_dgram_read() returns the total number of bytes
read. Fix the return value when the read has been successful. This bug
has no impact as in the end the return value is not checked by the
caller.

3 years agoMINOR: quic: Do not dereference ->conn quic_conn struct member
Frédéric Lécaille [Wed, 12 Jan 2022 08:46:02 +0000 (09:46 +0100)] 
MINOR: quic: Do not dereference ->conn quic_conn struct member

->conn quic_conn struct member is a connection struct object which may be
released from several places. With this patch we do our best to stop dereferencing
this member as much as we can.

3 years agoREGTESTS: ssl: Fix ssl_errors regtest with OpenSSL 1.0.2
Remi Tricot-Le Breton [Tue, 11 Jan 2022 16:29:24 +0000 (17:29 +0100)] 
REGTESTS: ssl: Fix ssl_errors regtest with OpenSSL 1.0.2

This test was broken with OpenSSL 1.0.2 after commit a996763619d
(BUG/MINOR: ssl: Store client SNI in SSL context in case of ClientHello
error) because it expected the default TLS version to be 1.3 in some
cases (when it can't be the case with OpenSSL 1.0.2).

3 years agoMINOR: quid: Add traces quic_close() and quic_conn_io_cb()
Frédéric Lécaille [Tue, 11 Jan 2022 13:43:50 +0000 (14:43 +0100)] 
MINOR: quid: Add traces quic_close() and quic_conn_io_cb()

This is to have an idea of possible remaining issues regarding the
connection terminations.

3 years agoMINOR: quic: Wrong CRYPTO frame concatenation
Frédéric Lécaille [Mon, 10 Jan 2022 17:31:07 +0000 (18:31 +0100)] 
MINOR: quic: Wrong CRYPTO frame concatenation

This commit was not correct:
 "MINOR: quic: Only one CRYPTO frame by encryption level"
Indeed, when receiving CRYPTO data from TLS stack for a packet number space,
there are rare cases where there is already other frames than CRYPTO data frames
in the packet number space, especially for 01RTT packet number space.  This is
very often with quant as client.

3 years agoMINOR: quic: Flag the connection as being attached to a listener
Frédéric Lécaille [Mon, 10 Jan 2022 11:10:10 +0000 (12:10 +0100)] 
MINOR: quic: Flag the connection as being attached to a listener

We do not rely on connection objects to know if we are a listener or not.

3 years agoMINOR: quic: Reset ->conn quic_conn struct member when calling qc_release()
Frédéric Lécaille [Mon, 10 Jan 2022 10:40:33 +0000 (11:40 +0100)] 
MINOR: quic: Reset ->conn quic_conn struct member when calling qc_release()

There may be remaining locations where ->conn quic_conn struct member
is used. So let's reset this.
Add a trace to have an idead when this connection is released.

3 years agoMINOR: quic: Remaining TRACEs with connection as firt arg
Frédéric Lécaille [Mon, 10 Jan 2022 10:00:16 +0000 (11:00 +0100)] 
MINOR: quic: Remaining TRACEs with connection as firt arg

This is a quic_conn struct which is expected by TRACE_()* macros

3 years agoBUILD: cpuset: fix build issue on macos introduced by previous change
David CARLIER [Sat, 8 Jan 2022 09:59:38 +0000 (09:59 +0000)] 
BUILD: cpuset: fix build issue on macos introduced by previous change

The build on macos was broken by recent commit df91cbd58 ("MINOR: cpuset:
switch to sched_setaffinity for FreeBSD 14 and above."), let's move the
variable declaration inside the ifdef.

3 years agoCI: github actions: clean default step conditions
Ilya Shipitsin [Fri, 7 Jan 2022 15:09:35 +0000 (20:09 +0500)] 
CI: github actions: clean default step conditions

step condition "if: ${{ !failure() }}" was added in 2ef4c7c84363f5a9b80a2093df1370514319db28
during my experiments. As Tim Düsterhus mentioned, that condition is default and may be omitted.

3 years agoDOC: internals: document the pools architecture and API
Willy Tarreau [Tue, 11 Jan 2022 13:48:20 +0000 (14:48 +0100)] 
DOC: internals: document the pools architecture and API

The purpose here is to explain how memory pools work, what their
architecture is depending on the build options (4 possible combinations),
and how the various build options affect their behavior.

Two pool-specific macros that were previously documented in initcalls
were moved to pools.txt.

3 years agoBUG/MAJOR: mux-h1: Don't decrement .curr_len for unsent data
Christopher Faulet [Mon, 10 Jan 2022 16:27:51 +0000 (17:27 +0100)] 
BUG/MAJOR: mux-h1: Don't decrement .curr_len for unsent data

A regression was introduced by commit 140f1a58 ("BUG/MEDIUM: mux-h1: Fix
splicing by properly detecting end of message"). To detect end of the
outgoing message, when the content-length is announced, we count amount of
data already sent. But only data really sent must be counted.

If the output buffer is full, we can fail to send data (fully or
partially). In this case, we must take care to only count sent
data. Otherwise we may think too much data were sent and an internal error
may be erroneously reported.

This patch should fix issues #1510 and #1511. It must be backported as far
as 2.4.

3 years agoBUG/MINOR: ssl: Store client SNI in SSL context in case of ClientHello error
Remi Tricot-Le Breton [Fri, 7 Jan 2022 16:12:01 +0000 (17:12 +0100)] 
BUG/MINOR: ssl: Store client SNI in SSL context in case of ClientHello error

If an error is raised during the ClientHello callback on the server side
(ssl_sock_switchctx_cbk), the servername callback won't be called and
the client's SNI will not be saved in the SSL context. But since we use
the SSL_get_servername function to return this SNI in the ssl_fc_sni
sample fetch, that means that in case of error, such as an SNI mismatch
with a frontend having the strict-sni option enabled, the sample fetch
would not work (making strict-sni related errors hard to debug).

This patch fixes that by storing the SNI as an ex_data in the SSL
context in case the ClientHello callback returns an error. This way the
sample fetch can fallback to getting the SNI this way. It will still
first call the SSL_get_servername function first since it is the proper
way of getting a client's SNI when the handshake succeeded.

In order to avoid memory allocations are runtime into this highly used
runtime function, a new memory pool was created to store those client
SNIs. Its entry size is set to 256 bytes since SNIs can't be longer than
255 characters.

This fixes GitHub #1484.

It can be backported in 2.5.

3 years agoBUG/MEDIUM: mworker: don't use _getsocks in wait mode
William Lallemand [Fri, 7 Jan 2022 17:19:42 +0000 (18:19 +0100)] 
BUG/MEDIUM: mworker: don't use _getsocks in wait mode

Since version 2.5 the master is automatically re-executed in wait-mode
when the config is successfully loaded, puting corner cases of the wait
mode in plain sight.

When using the -x argument and with the right timing, the master will
try to get the FDs again in wait mode even through it's not needed
anymore, which will harm the worker by removing its listeners.

However, if it fails, (and it's suppose to, sometimes), the
master will exit with EXIT_FAILURE because it does not have the
MODE_MWORKER flag, but only the MODE_MWORKER_WAIT flag. With the
consequence of killing the workers.

This patch fixes the issue by restricting the use of _getsocks to some
modes.

This patch must be backported in every version supported, even through
the impact should me more harmless in version prior to 2.5.

3 years agoMINOR: quic: Non-optimal use of a TX buffer
Frédéric Lécaille [Fri, 7 Jan 2022 13:32:31 +0000 (14:32 +0100)] 
MINOR: quic: Non-optimal use of a TX buffer

When full, after having reset the writer index, let's reuse the TX buffer in
any case.

3 years agoMINOR: quic: Missing retransmission from qc_prep_fast_retrans()
Frédéric Lécaille [Thu, 6 Jan 2022 16:28:05 +0000 (17:28 +0100)] 
MINOR: quic: Missing retransmission from qc_prep_fast_retrans()

In fact we must look for the first packet with some ack-elicting frame to
in the packet number space tree to retransmit from. Obviously there
may be already retransmit packets which are not deemed as lost and
still present in the packet number space tree for TX packets.

3 years agoMINOR: quic: Only one CRYPTO frame by encryption level
Frédéric Lécaille [Tue, 4 Jan 2022 22:15:40 +0000 (23:15 +0100)] 
MINOR: quic: Only one CRYPTO frame by encryption level

When receiving CRYPTO data from the TLS stack, concatenate the CRYPTO data
to the first allocated CRYPTO frame if present. This reduces by one the number
of handshake packets built for a connection with a standard size certificate.

3 years agoBUILD: makefile: add -Wno-atomic-alignment to work around clang abusive warning
Willy Tarreau [Fri, 7 Jan 2022 13:51:56 +0000 (14:51 +0100)] 
BUILD: makefile: add -Wno-atomic-alignment to work around clang abusive warning

As reported in github issue #1502, clang, when building for i386, will
try to use CMPXCHG8B-based loops for 64-bit atomic operations, and emits
warnings for all 64-bit operands that are not 64-bit aligned, an alignment
that is *not* required by the ABI, that the compiler itself does not
enforce, and that the intel SDM clearly says is not required on this
32-bit platform for this operation. But this is likely an excessive
outcome of the same code being used in 64-bit for CMPXCHG16B which does
require proper alignment. Firefox already gave up on this one 3 years
ago, let's not waste our time arguing and just shut up the warning
instead. It might hide some real bugs in the future but till now
experience showed that overall it's unlikely.

This should be backported to all maintained branches that use 64-bit
atomic ops (e.g. for counters).

Thanks to Brad Smith for reporting it and confirming that shutting the
warning addresses it.

3 years agoCLEANUP: assorted typo fixes in the code and comments
Ilya Shipitsin [Fri, 7 Jan 2022 09:46:15 +0000 (14:46 +0500)] 
CLEANUP: assorted typo fixes in the code and comments

This is 30th iteration of typo fixes

3 years agoCI: refactor spelling check
Ilya Shipitsin [Fri, 7 Jan 2022 09:42:54 +0000 (14:42 +0500)] 
CI: refactor spelling check

let us switch to codespell github actions instead of invocation from cmdline.
also, "ifset,thrid,strack,ba,chck,hel,unx,mor" added to whitelist, those are
variable names and special terms widely used in HAProxy

3 years agoMINOR: cpuset: switch to sched_setaffinity for FreeBSD 14 and above.
David CARLIER [Thu, 6 Jan 2022 18:53:50 +0000 (18:53 +0000)] 
MINOR: cpuset: switch to sched_setaffinity for FreeBSD 14 and above.

Following up previous update on cpuset-t.h. Ultimately, at some point
 the cpuset_setaffinity code path could be removed.

3 years agoMINOR: proxy: add option idle-close-on-response
William Dauchy [Wed, 5 Jan 2022 21:53:24 +0000 (22:53 +0100)] 
MINOR: proxy: add option idle-close-on-response

Avoid closing idle connections if a soft stop is in progress.

By default, idle connections will be closed during a soft stop. In some
environments, a client talking to the proxy may have prepared some idle
connections in order to send requests later. If there is no proper retry
on write errors, this can result in errors while haproxy is reloading.
Even though a proper implementation should retry on connection/write
errors, this option was introduced to support back compat with haproxy <
v2.4. Indeed before v2.4, we were waiting for a last request to be able
to add a "connection: close" header and advice the client to close the
connection.

In a real life example, this behavior was seen in AWS using the ALB in
front of a haproxy. The end result was ALB sending 502 during haproxy
reloads.
This patch was tested on haproxy v2.4, with a regular reload on the
process, and a constant trend of requests coming in. Before the patch,
we see regular 502 returned to the client; when activating the option,
the 502 disappear.

This patch should help fixing github issue #1506.
In order to unblock some v2.3 to v2.4 migraton, this patch should be
backported up to v2.4 branch.

Signed-off-by: William Dauchy <wdauchy@gmail.com>
[wt: minor edits to the doc to mention other options to care about]
Signed-off-by: Willy Tarreau <w@1wt.eu>
3 years agoMINOR: quic: Re-arm the PTO timer upon datagram receipt
Frédéric Lécaille [Tue, 4 Jan 2022 16:03:11 +0000 (17:03 +0100)] 
MINOR: quic: Re-arm the PTO timer upon datagram receipt

When block by the anti-amplification limit, this is the responsability of the
client to unblock it sending new datagrams. On the server side, even if not
well parsed, such datagrams must trigger the PTO timer arming.

3 years agoMINOR: quic: PTO timer too often reset
Frédéric Lécaille [Tue, 4 Jan 2022 15:59:42 +0000 (16:59 +0100)] 
MINOR: quic: PTO timer too often reset

It must be reset when the anti-amplication was reached but only if the
peer address was not validated.

3 years agoMINOR: quic: Flag asap the connection having reached the anti-amplification limit
Frédéric Lécaille [Tue, 4 Jan 2022 15:57:37 +0000 (16:57 +0100)] 
MINOR: quic: Flag asap the connection having reached the anti-amplification limit

The best location to flag the connection is just after having built the packet
which reached the anti-amplication limit.

3 years agoMINOR: quic: Prepare Handshake packets asap after completed handshake
Frédéric Lécaille [Mon, 3 Jan 2022 16:25:53 +0000 (17:25 +0100)] 
MINOR: quic: Prepare Handshake packets asap after completed handshake

Switch back to QUIC_HS_ST_SERVER_HANDSHAKE state after a completed handshake
if acks must be send.
Also ensure we build post handshake frames only one time without using prev_st
variable and ensure we discard the Handshake packet number space only one time.

3 years agoMINOR: quic: Do not drop secret key but drop the CRYPTO data
Frédéric Lécaille [Mon, 3 Jan 2022 16:00:35 +0000 (17:00 +0100)] 
MINOR: quic: Do not drop secret key but drop the CRYPTO data

We need to be able to decrypt late Handshake packets after the TLS secret
keys have been discarded. If not the peer send Handshake packet which have
not been acknowledged. But for such packets, we discard the CRYPTO data.

3 years agoMINOR: quic: Improve qc_prep_pkts() flexibility
Frédéric Lécaille [Mon, 3 Jan 2022 10:14:30 +0000 (11:14 +0100)] 
MINOR: quic: Improve qc_prep_pkts() flexibility

We want to be able to choose the encryption levels to be used
by qc_prep_pkts() outside of it.

3 years agoMINOR: quic: Comment fix.
Frédéric Lécaille [Fri, 31 Dec 2021 15:37:58 +0000 (16:37 +0100)] 
MINOR: quic: Comment fix.

When we drop a packet with unknown length, this is the entire datagram which
must be skipped.

3 years agoMINOR: quic: Probe several packet number space upon timer expiration
Frédéric Lécaille [Fri, 31 Dec 2021 15:35:28 +0000 (16:35 +0100)] 
MINOR: quic: Probe several packet number space upon timer expiration

When the loss detection timer expires, we SHOULD include new data in
our probing packets (RFC 9002 par 6.2.4. Sending Probe Packets).

3 years agoMINOR: quic: Probe Initial packet number space more often
Frédéric Lécaille [Thu, 30 Dec 2021 22:16:51 +0000 (23:16 +0100)] 
MINOR: quic: Probe Initial packet number space more often

Especially when the PTO expires for Handshake packet number space and when
Initial packets are still flying (for QUIC servers).

3 years agoMINOR: quic: Speeding up Handshake Completion
Frédéric Lécaille [Thu, 30 Dec 2021 15:14:20 +0000 (16:14 +0100)] 
MINOR: quic: Speeding up Handshake Completion

According to RFC 9002 par. 6.2.3. when receving duplicate Initial CRYPTO
data a server may a packet containing non unacknowledged before the PTO
expiry.

3 years agoMINOR: quic: qc_prep_pkts() code moving
Frédéric Lécaille [Wed, 29 Dec 2021 16:18:21 +0000 (17:18 +0100)] 
MINOR: quic: qc_prep_pkts() code moving

Move the switch default case code out of the switch to improve the readibily.

3 years agoMINOR: quic: Useless test in qc_prep_pkts()
Frédéric Lécaille [Wed, 29 Dec 2021 15:00:47 +0000 (16:00 +0100)] 
MINOR: quic: Useless test in qc_prep_pkts()

These tests were there to initiate PTO probing but they are not correct.
Furthermore they may break the PTO probing process and lead to useless packet
building.

3 years agoMINOR: quic: Wrong packet number space trace in qc_prep_pkts()
Frédéric Lécaille [Wed, 29 Dec 2021 14:36:25 +0000 (15:36 +0100)] 
MINOR: quic: Wrong packet number space trace in qc_prep_pkts()

It was always the first packet number space information which was dumped.

3 years agoMINOR: quic: Remove nb_pto_dgrams quic_conn struct member
Frédéric Lécaille [Wed, 29 Dec 2021 11:04:13 +0000 (12:04 +0100)] 
MINOR: quic: Remove nb_pto_dgrams quic_conn struct member

For now on we rely on tx->pto_probe pktns struct member to inform
the packet building function we want to probe.

3 years agoMINOR: quic: Wrong ack_delay compution before calling quic_loss_srtt_update()
Frédéric Lécaille [Tue, 28 Dec 2021 13:27:43 +0000 (14:27 +0100)] 
MINOR: quic: Wrong ack_delay compution before calling quic_loss_srtt_update()

RFC 9002 5.3. Estimating smoothed_rtt and rttvar:
MUST use the lesser of the acknowledgment delay and the peer's max_ack_delay
after the handshake is confirmed.

3 years agoMINOR: quic: Wrong loss time computation in qc_packet_loss_lookup()
Frédéric Lécaille [Mon, 27 Dec 2021 17:15:27 +0000 (18:15 +0100)] 
MINOR: quic: Wrong loss time computation in qc_packet_loss_lookup()

This part as been modified by the RFC since our first implementation.

3 years agoMINOR: quic: Wrong packet number space computation for PTO
Frédéric Lécaille [Mon, 27 Dec 2021 17:13:13 +0000 (18:13 +0100)] 
MINOR: quic: Wrong packet number space computation for PTO

This leaded to make quic_pto_pktns() return 01RTT packet number space
when initiating a probing even if the handshake was not completed!

3 years agoMINOR: quic: Wrong first packet number space computation
Frédéric Lécaille [Mon, 27 Dec 2021 17:09:22 +0000 (18:09 +0100)] 
MINOR: quic: Wrong first packet number space computation

I really do not know where does these inversion come from.

3 years agoMINOR: quic: Add trace about in flight bytes by packet number space
Frédéric Lécaille [Mon, 27 Dec 2021 16:42:51 +0000 (17:42 +0100)] 
MINOR: quic: Add trace about in flight bytes by packet number space

This parameter is useful to diagnose packet loss detection issues.

3 years agoMINOR: quic: Wrong traces after rework
Frédéric Lécaille [Mon, 27 Dec 2021 14:12:09 +0000 (15:12 +0100)] 
MINOR: quic: Wrong traces after rework

TRACE_*() macros must take a quic_conn struct as first argument.

3 years agoBUG/MEDIUM: http-ana: Preserve response's FLT_END analyser on L7 retry
Christopher Faulet [Tue, 4 Jan 2022 09:56:03 +0000 (10:56 +0100)] 
BUG/MEDIUM: http-ana: Preserve response's FLT_END analyser on L7 retry

When a filter is attached on a stream, the FLT_END analyser must not be
removed from the response channel on L7 retry. It is especially important
because CF_FLT_ANALYZE flag is still set. This means the synchronization
between the two sides when the filter ends can be blocked. Depending on the
timing, this can freeze the stream infinitely or lead to a spinning loop.

Note that the synchronization between the two sides at the end of the
analysis was introduced because the stream was reused in HTTP between two
transactions. But, since the HTX was introduced, a new stream is created for
each transaction. So it is probably possible to remove this step for 2.2 and
higher.

This patch must be backported as far as 2.0.