MINOR: proxy/pool: prevent unnecessary calls to pool_gc()
Under certain soft-stopping conditions (ie: sticktable attached to proxy
and in-progress connections to the proxy that prevent haproxy from
exiting), manage_proxy() (p->task) will wake up every second to perform
a cleanup attempt on the proxy sticktable (to purge unused entries).
However, as reported by TimWolla in GH #2091, it was found that a
systematic call to pool_gc() could cause some CPU waste, mainly
because malloc_trim() (which is rather expensive) is being called
for each pool_gc() invocation.
As a result, such soft-stopping process could be spending a significant
amount of time in the malloc_trim->madvise() syscall for nothing.
Example "strace -c -f -p `pidof haproxy`" output (taken from
Tim's report):
To prevent this, we now only call pool_gc() when some memory is
really expected to be reclaimed as a direct result of the previous
stick table cleanup.
This is pretty straightforward since stktable_trash_oldest() returns
the number of trashed sticky sessions.
BUG/MINOR: quic: Missing padding in very short probe packets
This bug arrived with this commit:
MINOR: quic: Send PING frames when probing Initial packet number space
This may happen when haproxy needs to probe the peer with very short packets
(only one PING frame). In this case, the packet must be padded. There was clearly
a case which was removed by the mentionned commit above. That said, there was
an extra byte which was added to the PADDING frame before the mentionned commit
above. This is no more the case with this patch.
Thank you to @tatsuhiro-t (ngtcp2 manager) for having reported this issue which
was revealed by the keyupdate test (on client side).
BUG/MEDIUM: mux-h2: Be able to detect connection error during handshake
When a backend H2 connection is waiting the connection is fully established,
nothing is sent. However, it remains useful to detect connection error at
this stage. It is especially important to release H2 connection on connect
error. Be able to set H2_CF_ERR_PENDiNG or H2_CF_ERROR flags when the
underlying connection is not fully established will exclude the H2C to be
inserted in a idle list in h2_detach().
Without this fix, an H2C in PREFACE state and relying on a connection in
error can be inserted in the safe list. Of course, it will be purged if not
reused. But in the mean time, it can be reused. When this happens, the
connection remains in error and nothing happens. At the end a connection
error is returned to the client. On low traffic, we can imagine a scenario
where this dead connection is the only idle connection. If it is always
reused before being purged, no connection to the server is possible.
In addition, h2c_is_dead() is updated to declare as dead any H2 connection
with a pending error if its state is PREFACE or SETTINGS1 (thus if no
SETTINGS frame was received yet).
This patch should fix the issue #2092. It must be backported as far as 2.6.
BUG/MINOR: stats: Don't replace sc_shutr() by SE_FL_EOS flag yet
In commit c2c043ed4 ("BUG/MEDIUM: stats: Consume the request except when
parsing the POST payload"), a change about applet was pushed too early. The
applet must still call cf_shutr() when the response is fully sent. It is
planned to rely on SE_FL_EOS flag, just like connections. But it is not
possible for now.
However, at first glance, this bug has no visible effect.
Willy Tarreau [Tue, 28 Mar 2023 11:58:56 +0000 (13:58 +0200)]
[RELEASE] Released version 2.8-dev6
Released version 2.8-dev6 with the following main changes :
- BUG/MEDIUM: mux-pt: Set EOS on error on sending path if read0 was received
- MINOR: ssl: Change the ocsp update log-format
- MINOR: ssl: Use ocsp update task for "update ssl ocsp-response" command
- BUG/MINOR: ssl: Fix double free in ocsp update deinit
- MINOR: ssl: Accept certpath as param in "show ssl ocsp-response" CLI command
- MINOR: ssl: Add certificate path to 'show ssl ocsp-response' output
- BUG/MEDIUM: proxy: properly stop backends on soft-stop
- BUG/MEDIUM: resolvers: Properly stop server resolutions on soft-stop
- DEBUG: cli/show_fd: Display connection error code
- DEBUG: ssl-sock/show_fd: Display SSL error code
- BUG/MEDIUM: mux-h1: Don't block SE_FL_ERROR if EOS is not reported on H1C
- BUG/MINOR: tcp_sample: fix a bug in fc_dst_port and fc_dst_is_local sample fetches
- BUG/MINOR: quic: Missing STREAM frame length updates
- BUG/MEDIUM: connection: Preserve flags when a conn is removed from an idle list
- BUG/MINOR: mux-h2: make sure the h2c task exists before refreshing it
- MINOR: buffer: add br_count() to return the number of allocated bufs
- MINOR: buffer: add br_single() to check if a buffer ring has more than one buf
- BUG/MEDIUM: mux-h2: only restart sending when mux buffer is decongested
- BUG/MINOR: mux-h2: set CO_SFL_STREAMER when sending lots of data
- BUG/MINOR: quic: Missing STREAM frame data pointer updates
- MINOR: stick-table: add sc-add-gpc() to http-after-response
- MINOR: doc: missing entries for sc-add-gpc()
- BUG/MAJOR: qpack: fix possible read out of bounds in static table
- OPTIM: mux-h1: limit first read size to avoid wrapping
- MINOR: mux-h2: set CO_SFL_MSG_MORE when sending multiple buffers
- MINOR: ssl-sock: pass the CO_SFL_MSG_MORE info down the stack
- MINOR: quic: Stop stressing the acknowledgments process (RX ACK frames)
- BUG/MINOR: quic: Dysfunctional 01RTT packet number space probing
- BUG/MEDIUM: stream: do not try to free a failed stream-conn
- BUG/MEDIUM: mux-h2: do not try to free an unallocated h2s->sd
- BUG/MEDIUM: mux-h2: erase h2c->wait_event.tasklet on error path
- BUG/MEDIUM: stconn: don't set the type before allocation succeeds
- BUG/MINOR: stconn: fix sedesc memory leak on stream allocation failure
- MINOR: dynbuf: set POOL_F_NO_FAIL on buffer allocation
- MINOR: pools: preset the allocation failure rate to 1% with -dMfail
- BUG/MEDIUM: mux-h1: properly destroy a partially allocated h1s
- BUG/MEDIUM: applet: only set appctx->sedesc on successful allocation
- BUG/MINOR: quic: wake up MUX on probing only for 01RTT
- BUG/MINOR: quic: ignore congestion window on probing for MUX wakeup
- BUILD: thread: implement thread_harmless_end_sig() for threadless builds
- BUILD: thread: silence a build warning when threads are disabled
- MINOR: debug: support dumping the libs addresses when running in verbose mode
- BUG/MINOR: illegal use of the malloc_trim() function if jemalloc is used
- BUG/MINOR: trace: fix hardcoded level for TRACE_PRINTF
- BUG/MEDIUM: mux-quic: release data from conn flow-control on qcs reset
- MINOR: mux-quic: complete traces for qcs emission
- MINOR: mux-quic: adjust trace level for MAX_DATA/MAX_STREAM_DATA recv
- MINOR: mux-quic: add flow-control info to minimal trace level
- MINOR: pools: make sure 'no-memory-trimming' is always used
- MINOR: pools: intercept malloc_trim() instead of trying to plug holes
- MEDIUM: pools: move the compat code from trim_all_pools() to malloc_trim()
- MINOR: pools: export trim_all_pools()
- MINOR: pattern: use trim_all_pools() instead of a conditional malloc_trim()
- MINOR: tools: relax dlopen() on malloc/free checks
- MEDIUM: tools: further relax dlopen() checks too consider grouped symbols
- BUG/MINOR: pools: restore detection of built-in allocator
- MINOR: pools: report a replaced memory allocator instead of just malloc_trim()
- BUG/MINOR: h3: properly handle incomplete remote uni stream type
- BUG/MINOR: mux-quic: prevent CC status to be erased by shutdown
- MINOR: mux-quic: interrupt qcc_recv*() operations if CC scheduled
- MINOR: mux-quic: ensure CONNECTION_CLOSE is scheduled once per conn
- MINOR: mux-quic: close on qcs allocation failure
- MINOR: mux-quic: close on frame alloc failure
- BUG/MINOR: syslog: Request for more data if message was not fully received
- BUG/MEDIUM: stats: Consume the request except when parsing the POST payload
- DOC: config: set-var() dconv rendering issues
- BUG/MEDIUM: mux-h1: Wakeup H1C on shutw if there is no I/O subscription
- BUG/MINOR: applet/new: fix sedesc freeing logic
- BUG/MINOR: quic: Missing STREAM frame type updated
- BUILD: da: extends CFLAGS to support API v3 from 3.1.7 and onwards.
- BUG/MINOR: ssl: Stop leaking `err` in ssl_sock_load_ocsp()
David Carlier [Tue, 28 Mar 2023 05:24:49 +0000 (06:24 +0100)]
BUILD: da: extends CFLAGS to support API v3 from 3.1.7 and onwards.
Minor build update to still both support the v2 and v3 api from
the 3.1.7 release which supports a cache but would need a shift
in the HAProxy build not necessary at the moment.
In the second half of the year and for the next major HAProxy release
branch, v2 could be dropped altogether thus the next HAProxy 2.9
major release will contain more changes towards the v3 support
and reminder for the v2 EOL.
BUG/MINOR: quic: Missing STREAM frame type updated
This patch follows this commit which was not sufficient:
BUG/MINOR: quic: Missing STREAM frame data pointer updates
Indeed, after updating the ->offset field, the bit which informs the
frame builder of its presence must be systematically set.
This bug was revealed by the following BUG_ON() from
quic_build_stream_frame() :
bug condition "!!(frm->type & 0x04) != !!stream->offset.key" matched at src/quic_frame.c:515
This should fix the last crash occured on github issue #2074.
Since 465a6c8 ("BUG/MEDIUM: applet: only set appctx->sedesc on
successful allocation"), sedesc is attached to the appctx after the
task is successfully allocated.
If the task fails to allocate: current sedesc cleanup is performed
on appctx->sedesc which still points to NULL so sedesc won't be
freed.
This is fine when sedesc is provided as argument (!=NULL), but leads
to memory leaks if sedesc is allocated locally.
It was shown in GH #2086 that if sedesc != NULL when passed as
argument, it shouldn't be freed on error paths. This is what 465a6c8
was trying to address.
In an attempt to fix both issues at once, we do as Christopher
suggested: that is moving sedesc allocation attempt at the
end of the function, so that we don't have to free it in case
of error, thus removing the ambiguity.
(We won't risk freeing a sedesc that does not belong to us)
If we fail to allocate sedesc, then the task that was previously
created locally is simply destroyed.
This needs to be backported to 2.6 with 465a6c8 ("BUG/MEDIUM: applet:
only set appctx->sedesc on successful allocation")
[Copy pasting the original backport note from Willy:
In 2.6 the function is slightly
different and called appctx_new(), though the issue is exactly the
same.]
BUG/MEDIUM: mux-h1: Wakeup H1C on shutw if there is no I/O subscription
This old bug was revealed because of the commit 407210a34 ("BUG/MEDIUM:
stconn: Don't rearm the read expiration date if EOI was reached"). But it is
still possible to hit it if there is no server timeout. At first glance, the
2.8 is not affected. But the fix remains valid.
When a shutdown for writes if performed the H1 connection must be notified
to be released. If it is subscribed for any I/O events, it is not an
issue. But, if there is no subscription, no I/O event is reported to the H1
connection and it remains alive. If the message was already fully received,
nothing more happens.
On my side, I was able to trigger the bug by freezing the session. Some
users reported a spinning loop on process_stream(). Not sure how to trigger
the loop. To freeze the session, the client timeout must be reached while
the server response was already fully received. On old version (< 2.6), it
only happens if there is no server timeout.
To fix the issue, we must wake up the H1 connection on shutdown for writes
if there is no I/O subscription.
This patch must be backported as far as 2.0. It should fix the issue #2090
and #2088.
Since <cond> optional argument support was added to set-var() and friends
in 2.6 with 164726c ("DOC: vars: Add documentation about the set-var
conditions"), dconv is having a hard time rendering related keywords.
Everywhere `[,<cond> ...]` was inserted, html formatting is now broken.
Removing the space between <cond> and '...' allows dconv to properly parse
the token thus restores proper formatting without changing the meaning.
This was discovered when discussing about var() sample fetch doc issues
in GH #2087
BUG/MEDIUM: stats: Consume the request except when parsing the POST payload
The stats applet is designed to consume the request at the end, when it
finishes to send the response. And during the response forwarding, because
the request is not consumed, the applet states it will not consume
data. This avoid to wake the applet up in loop. When it finishes to send the
response, the request is consumed.
For POST requests, there is no issue because the response is small
enough. It is sent in one time and must be processed by HTTP analyzers. Thus
the forwarding is not performed by the applet itself. The applet is always
able to consume the request, regardless the payload length.
But for other requests, it may be an issue. If the response is too big to be
sent in one time and if the requests is not fully received when the response
headers are sent, the applet may be blocked infinitely, not consuming the
request. Indeed, in the case the applet will be switched in infinite forward
mode, the request will not be consumed immediately. At the end, the request
buffer is flushed. But if some data must still be received, the applet is not
woken up because it is still in a "not-consuming" mode.
So, to fix the issue, we must take care to re-enable data consuming when the
end of the response is reached.
BUG/MINOR: syslog: Request for more data if message was not fully received
In the syslog applet, when a message was not fully received, we must request
for more data by calling appctx_need_more_data() and not by setting
CF_READ_DONTWAIT flag on the request channel. Indeed, this flag is only used
to only try a read at once.
This patch could be backported as far as 2.4. On 2.5 and 2.4,
applet_need_more_data() must be replaced by si_cant_get().
Replace all BUG_ON() on frame allocation failure by a CONNECTION_CLOSE
sending with INTERNAL_ERROR code. This can happen for the following
cases :
* sending of MAX_STREAM_DATA
* sending of MAX_DATA
* sending of MAX_STREAMS_BIDI
In other cases (STREAM, STOP_SENDING, RESET_STREAM), an allocation
failure will only result in the current operation to be interrupted and
retried later. However, it may be desirable in the future to replace
this with a simpler CONNECTION_CLOSE emission to recover better under a
memory pressure issue.
Emit a CONNECTION_CLOSE with INTERNAL_ERROR code each time qcs
allocation fails. This can happen in two cases :
* when creating a local stream through application layer
* when instantiating a remote stream through qcc_get_qcs()
In both cases, error paths are already in place to interrupt the current
operation and a CONNECTION_CLOSE will be emitted soon after.
MINOR: mux-quic: ensure CONNECTION_CLOSE is scheduled once per conn
Add BUG_ON() statements to ensure qcc_emit_cc()/qcc_emit_cc_app() is not
called more than one time for each connection. This should improve code
resilience of MUX-QUIC and H3 and it will ensure that a scheduled
CONNECTION_CLOSE is not overwritten by another one with a different
error code.
This commit relies on the previous one to ensure all QUIC operations are
not conducted as soon as a CONNECTION_CLOSE has been prepared :
commit d7fbf458f8a4c5b09cbf0da0208fbad70caaca33
MINOR: mux-quic: interrupt most operations if CONNECTION_CLOSE scheduled
MINOR: mux-quic: interrupt qcc_recv*() operations if CC scheduled
Ensure that external MUX operations are interrupted if a
CONNECTION_CLOSE is scheduled. This was already the cases for some
functions. This is extended to the qcc_recv*() family for
MAX_STREAM_DATA, RESET_STREAM and STOP_SENDING.
Also, qcc_release_remote_stream() is skipped in qcs_destroy() if a
CONNECTION_CLOSE is already scheduled.
All of this will ensure we only proceed to minimal treatment as soon as
a CONNECTION_CLOSE is prepared. Indeed, all sending and receiving is
stopped as soon as a CONNECTION_CLOSE is emitted so only internal
cleanup code should be necessary at this stage.
This should prevent a registered CONNECTION_CLOSE error status to be
overwritten by an error in a follow-up treatment.
Amaury Denoyelle [Mon, 20 Mar 2023 16:34:22 +0000 (17:34 +0100)]
BUG/MINOR: mux-quic: prevent CC status to be erased by shutdown
HTTP/3 graceful shutdown operation is used to emit a GOAWAY followed by
a CONNECTION_CLOSE with H3_NO_ERROR status. It is used for every
connection on release which means that if a CONNECTION_CLOSE was already
registered for a previous error, its status code is overwritten.
To fix this, skip shutdown operation if a CONNECTION_CLOSE is already
registered at the MUX level. This ensures that the correct error status
is reported to the peer.
This should be backported up to 2.6. Note that qc_shutdown() does not
exists on 2.6 so modification will have to be made directly in
qc_release() as followed :
BUG/MINOR: h3: properly handle incomplete remote uni stream type
A H3 unidirectional stream is always opened with its stream type first
encoded as a QUIC variable integer. If the STREAM frame contains some
data but not enough to decode this varint, haproxy would crash due to an
ABORT_NOW() statement.
To fix this, ensure we support an incomplete stream type. In this case,
h3_init_uni_stream() returns 0 and the buffer content is not cleared.
Stream decoding will resume when new data are received for this stream
which should be enough to decode the stream type varint.
This bug has never occured on production because standard H3 stream types
are small enough to be encoded on a single byte.
Willy Tarreau [Wed, 22 Mar 2023 17:01:41 +0000 (18:01 +0100)]
MINOR: pools: report a replaced memory allocator instead of just malloc_trim()
Instead of reporting the inaccurate "malloc_trim() support" on -vv, let's
report the case where the memory allocator was actively replaced from the
one used at build time, as this is the corner case we want to be cautious
about. We also put a tainted bit when this happens so that it's possible
to detect it at run time (e.g. the user might have inherited it from an
environment variable during a reload operation).
The now unused is_trim_enabled() function was finally dropped.
Willy Tarreau [Wed, 22 Mar 2023 16:52:05 +0000 (17:52 +0100)]
BUG/MINOR: pools: restore detection of built-in allocator
The runtime detection of the default memory allocator was broken by
Commit d8a97d8f6 ("BUG/MINOR: illegal use of the malloc_trim() function
if jemalloc is used") due to a misunderstanding of its role. The purpose
is not to detect whether we're on non-jemalloc but whether or not the
allocator was changed from the one we booted with, in which case we must
be extra cautious and absolutely refrain from calling malloc_trim() and
its friends.
This was done only to drop the message saying that malloc_trim() is
supported, which will be totally removed in another commit, and could
possibly be removed even in older versions if this patch would get
backported since in the end it provides limited value.
Willy Tarreau [Wed, 22 Mar 2023 16:01:32 +0000 (17:01 +0100)]
MEDIUM: tools: further relax dlopen() checks too consider grouped symbols
There's a recurring issue regarding shared library loading from Lua. If
the imported library is linked with a different version of openssl but
doesn't use it, the check will trigger and emit a warning. In practise
it's not necessarily a problem as long as the API is the same, because
all symbols are replaced and the library will use the included ssl lib.
It's only a problem if the library comes with a different API because
the dynamic linker will only replace known symbols with ours, and not
all. Thus the loaded lib may call (via a static inline or a macro) a
few different symbols that will allocate or preinitialize structures,
and which will then pass them to the common symbols coming from a
different and incompatible lib, exactly what happens to users of Lua's
luaossl when building haproxy with quictls and without rebuilding
luaossl.
In order to better address this situation, we now define groups of
symbols that must always appear/disappear in a consistent way. It's OK
if they're all absent from either haproxy or the lib, it means that one
of them doesn't use them so there's no problem. But if any of them is
defined on any side, all of them must be in the exact same state on the
two sides. The symbols are represented using a bit in a mask, and the
mask of the group of other symbols they're related to. This allows to
check 64 symbols, this should be OK for a while. The first ones that
are tested for now are symbols whose combination differs between
openssl versions 1.0, 1.1, and 3.0 as well as quictls. Thus a difference
there will indicate upcoming trouble, but no error will mean that we're
running on a seemingly compatible API and that all symbols should be
replaced at once.
The same mechanism could possibly be used for pcre/pcre2, zlib and the
few other optional libs that may occasionally cause runtime issues when
used by dependencies, provided that discriminatory symbols are found to
distinguish them. But in practice such issues are pretty rare, mainly
because loading standard libs via Lua on a custom build of haproxy is
not pretty common.
In the event that further symbol compatibility issues would be reported
in the future, backporting this patch as well as the following series
might be an acceptable solution given that the scope of changes is very
narrow (the malloc stuff is needed so that the malloc/free tests can be
dropped):
BUG/MINOR: illegal use of the malloc_trim() function if jemalloc is used
MINOR: pools: make sure 'no-memory-trimming' is always used
MINOR: pools: intercept malloc_trim() instead of trying to plug holes
MEDIUM: pools: move the compat code from trim_all_pools() to malloc_trim()
MINOR: pools: export trim_all_pools()
MINOR: pattern: use trim_all_pools() instead of a conditional malloc_trim()
MINOR: tools: relax dlopen() on malloc/free checks
Willy Tarreau [Wed, 22 Mar 2023 14:47:51 +0000 (15:47 +0100)]
MINOR: tools: relax dlopen() on malloc/free checks
Now that we can provide a safe malloc_trim() we don't need to detect
anymore that some dependencies use a different set of malloc/free
functions than ours because they will use the same as those we're
seeing, and we control their use of malloc_trim(). The comment about
the incompatibility with DEBUG_MEM_STATS is not true anymore either
since the feature relies on macros so we're now OK.
This will stop catching libraries linked against glibc's allocator
when haproxy is natively built with jemalloc. This was especially
annoying since dlopen() on a lib depending on jemalloc() tends to
fail on TLS issues.
Willy Tarreau [Wed, 22 Mar 2023 14:32:58 +0000 (15:32 +0100)]
MEDIUM: pools: move the compat code from trim_all_pools() to malloc_trim()
We already have some generic code in trim_all_pools() to implement the
equivalent of malloc_trim() on jemalloc and macos. Instead of keeping the
logic there, let's just move it to our own malloc_trim() implementation
so that we can unify the mechanism and the logic. Now any low-level code
calling malloc_trim() will either be disabled by haproxy's config if the
user decides to, or will be mapped to the equivalent mechanism if malloc()
was intercepted by a preloaded jemalloc.
Trim_all_pools() preserves the benefit of serializing threads (which we
must not impose to other libs which could come with their own threads).
It means that our own code should mostly use trim_all_pools() instead of
calling malloc_trim() directly.
Willy Tarreau [Wed, 22 Mar 2023 13:55:25 +0000 (14:55 +0100)]
MINOR: pools: intercept malloc_trim() instead of trying to plug holes
As reported by Miroslav in commit d8a97d8f6 ("BUG/MINOR: illegal use of
the malloc_trim() function if jemalloc is used") there are still occasional
cases where it's discovered that malloc_trim() is being used without its
suitability being checked first. This is a problem when using another
incompatible allocator. But there's a class of use cases we'll never be
able to cover, it's dynamic libraries loaded from Lua. In order to address
this more reliably, we now define our own malloc_trim() that calls the
previous one after checking that the feature is supported and that the
allocator is the expected one. This way child libraries that would call
it will also be safe.
The function is intentionally left defined all the time so that it will
be possible to clean up some code that uses it by removing ifdefs.
Willy Tarreau [Wed, 22 Mar 2023 13:52:30 +0000 (14:52 +0100)]
MINOR: pools: make sure 'no-memory-trimming' is always used
The global option 'no-memory-trimming' was added in 2.6 with commit c4e56dc58 ("MINOR: pools: add a new global option "no-memory-trimming"")
but there were some cases left where it was not considered. Let's make
is_trim_enabled() also consider it.
Amaury Denoyelle [Wed, 22 Mar 2023 10:58:32 +0000 (11:58 +0100)]
MINOR: mux-quic: complete traces for qcs emission
Add traces for _qc_send_qcs() function. Most notably, traces have been
added each time a qc_stream_desc buffer allocation fails and when stream
or connection flow-level is reached. This should improve debugging for
emission issues.
Amaury Denoyelle [Wed, 22 Mar 2023 10:17:59 +0000 (11:17 +0100)]
BUG/MEDIUM: mux-quic: release data from conn flow-control on qcs reset
Connection flow-control level calculation is a bit complicated. To
ensure it is never exceeded, each time a transfer occurs from a
qcs.tx.buf to its qc_stream_desc buffer it is accounted in
qcc.tx.offsets at the connection level. This value is not decremented
even if the corresponding STREAM frame is rejected by the quic-conn
layer as its emission will be retried later.
In normal cases this works as expected. However there is an issue if a
qcs instance is removed with prepared data left. In this case, its data
is still accounted in qcc.tx.offsets despite being removed which may
block other streams. This happens every time a qcs is reset with
remaining data which will be discarded in favor of a RESET_STREAM frame.
To fix this, if a stream has prepared data in qcc_reset_stream(), it is
decremented from qcc.tx.offsets. A BUG_ON() has been added to ensure
qcs_destroy() is never called for a stream with prepared data left.
This bug can cause two issues :
* transfer freeze as data unsent from closed streams still count on the
connection flow-control limit and will block other streams. Note that
this issue was not reproduced so it's unsure if this really happens
without the following issue first.
* a crash on a BUG_ON() statement in qc_send() loop over
qc_send_frames(). Streams may remained in the send list with nothing
to send due to connection flow-control limit. However, limit is never
reached through qcc_streams_sent_done() so QC_CF_BLK_MFCTL flag is not
set which will allow the loop to continue.
The last case was reproduced after several minutes of testing using the
following command :
Miroslav Zagorac [Wed, 22 Mar 2023 11:52:19 +0000 (12:52 +0100)]
BUG/MINOR: illegal use of the malloc_trim() function if jemalloc is used
In the event that HAProxy is linked with the jemalloc library, it is still
shown that malloc_trim() is enabled when executing "haproxy -vv":
..
Support for malloc_trim() is enabled.
..
It's not so much a problem as it is that malloc_trim() is called in the
pat_ref_purge_range() function without any checking.
This was solved by setting the using_default_allocator variable to the
correct value in the detect_allocator() function and before calling
malloc_trim() it is checked whether the function should be called.
Willy Tarreau [Wed, 22 Mar 2023 10:37:54 +0000 (11:37 +0100)]
MINOR: debug: support dumping the libs addresses when running in verbose mode
Starting haproxy with -dL helps enumerate the list of libraries in use.
But sometimes in order to go further we'd like to see their address
ranges. This is already supported on the CLI's "show libs" but not on
the command line where it can sometimes help troubleshoot startup issues.
Let's dump them when in verbose mode. This way it doesn't change the
existing behavior for those trying to enumerate libs to produce an archive.
Willy Tarreau [Wed, 22 Mar 2023 09:28:50 +0000 (10:28 +0100)]
BUILD: thread: silence a build warning when threads are disabled
When threads are disabled, the compiler complains that we might be
accessing tg->abs[] out of bounds since the array is of size 1. It
cannot know that the condition to do this is never met, and given
that it's not in a fast path, we can make it more obvious.
Willy Tarreau [Wed, 22 Mar 2023 09:26:58 +0000 (10:26 +0100)]
BUILD: thread: implement thread_harmless_end_sig() for threadless builds
Building without thread support was broken in 2.8-dev2 with commit 7e70bfc8c ("MINOR: threads: add a thread_harmless_end() version that
doesn't wait") that forgot to define the function for the threadless
cases. No backport is needed.
Amaury Denoyelle [Tue, 21 Mar 2023 10:39:24 +0000 (11:39 +0100)]
BUG/MINOR: quic: ignore congestion window on probing for MUX wakeup
qc_notify_send() is used to wake up the MUX layer for sending. This
function first ensures that all sending condition are met to avoid to
wake up the MUX for unnecessarily.
One of this condition is to check if there is room in the congestion
window. However, when probe packets must be sent due to a PTO
expiration, RFC 9002 explicitely mentions that the congestion window
must be ignored which was not the case prior to this patch.
This commit fixes this by first setting <pto_probe> of 01RTT packet
space before invoking qc_notify_send(). This ensures that congestion
window won't be checked anymore to wake up the MUX layer until probing
packets are sent.
This commit replaces the following one which was not sufficient :
commit e25fce03ebe3307bc104d1f81356108e271d2bc3
BUG/MINOR: quic: Dysfunctional 01RTT packet number space probing
Amaury Denoyelle [Mon, 20 Mar 2023 17:29:36 +0000 (18:29 +0100)]
BUG/MINOR: quic: wake up MUX on probing only for 01RTT
On PTO probe timeout expiration, a probe packet must be emitted.
quic_pto_pktns() is used to determine for which packet space the timer
has expired. However, if MUX is already subscribed for sending, it is
woken up without checking first if this happened for the 01RTT packet
space.
It is unsure that this is really a bug as in most cases, MUX is
established only after Initial and Handshake packet spaces are removed.
However, the situation is not se clear when 0-RTT is used. For this
reason, adjust the code to explicitely check for the 01RTT packet space
before waking up the MUX layer.
This should be backported up to 2.6. Note that qc_notify_send() does not
exists in 2.6 so it should be replaced by the explicit block checking
(qc->subs && qc->subs->events & SUB_RETRY_SEND).
Willy Tarreau [Tue, 21 Mar 2023 09:50:51 +0000 (10:50 +0100)]
BUG/MEDIUM: applet: only set appctx->sedesc on successful allocation
If appctx_new_on() fails to allocate a task, it will not remove the
freshly allocated sedesc from the appctx despite freeing it, causing
a UAF. Let's only assign appctx->sedesc upon success.
This needs to be backported to 2.6. In 2.6 the function is slightly
different and called appctx_new(), though the issue is exactly the
same.
Willy Tarreau [Tue, 21 Mar 2023 09:44:44 +0000 (10:44 +0100)]
BUG/MEDIUM: mux-h1: properly destroy a partially allocated h1s
In h1c_frt_stream_new() and h1c_bck_stream_new(), if we fail to completely
initialize the freshly allocated h1s, typically because sc_attach_mux()
fails, we must use h1s_destroy() to de-initialize it. Otherwise it stays
attached to the h1c when released, causing use-after-free upon the next
wakeup. This can be triggered upon memory shortage.
Willy Tarreau [Tue, 21 Mar 2023 08:24:53 +0000 (09:24 +0100)]
MINOR: pools: preset the allocation failure rate to 1% with -dMfail
Using -dMfail alone does nothing unless tune.fail-alloc is set, which
renders it pretty useless as-is, and is not intuitive. Let's change
this so that the filure rate is preset to 1% when the option is set on
the command line. This allows to inject failures without having to edit
the configuration.
Willy Tarreau [Tue, 21 Mar 2023 08:15:13 +0000 (09:15 +0100)]
MINOR: dynbuf: set POOL_F_NO_FAIL on buffer allocation
b_alloc() is used to allocate a buffer. We can provoke fault injection
based on forced memory allocation failures using -dMfail on the command
line, but we know that the buffer_wait list is a bit weak and doesn't
always recover well. As such, submitting buffer allocation to such a
treatment seriously limits the usefulness of -dMfail which cannot really
be used for other purposes. Let's just disable it for buffers for now.
Willy Tarreau [Mon, 20 Mar 2023 18:53:14 +0000 (19:53 +0100)]
BUG/MINOR: stconn: fix sedesc memory leak on stream allocation failure
If we fail to allocate a new stream in sc_new_from_endp(), and the call
to sc_new() allocated the sedesc itself (which normally doesn't happen),
then it doesn't get released on the failure path. Let's explicitly
handle this case so that it's not overlooked and avoids some head
scratching sessions.
Willy Tarreau [Mon, 20 Mar 2023 18:45:41 +0000 (19:45 +0100)]
BUG/MEDIUM: stconn: don't set the type before allocation succeeds
There's an occasional crash that can be triggered in sc_detach_endp()
when calling conn->mux->detach() upon memory allocation error. The
problem in fact comes from sc_attach_mux(), which doesn't reset the
sc type flags upon tasklet allocation failure, leading to an attempt
at detaching an incompletely initialized stconn. Let's just attach
the sc after the tasklet allocation succeeds, not before.
Willy Tarreau [Mon, 20 Mar 2023 18:16:04 +0000 (19:16 +0100)]
BUG/MEDIUM: mux-h2: erase h2c->wait_event.tasklet on error path
On the allocation error path in h2_init() we may check if
h2c->wait_event.tasklet needs to be released but it has not yet been
zeroed. Let's do this before jumping to the freeing location.
This needs to be backported to all maintained versions.
Willy Tarreau [Mon, 20 Mar 2023 18:14:47 +0000 (19:14 +0100)]
BUG/MEDIUM: mux-h2: do not try to free an unallocated h2s->sd
In h2s_close() we may dereference h2s->sd to get the sc, but this
function may be called on allocation error paths, so we must check
for this specific condition. Let's also update the comment to make
it explicitly permitted.
Willy Tarreau [Mon, 20 Mar 2023 18:11:08 +0000 (19:11 +0100)]
BUG/MEDIUM: stream: do not try to free a failed stream-conn
In stream_free() if we fail to allocate s->scb() we go to the path where
we try to free it, and it doesn't like being called with a null at all.
It's easily reproducible with -dMfail,no-cache and "tune.fail-alloc 10"
in the global section.
BUG/MINOR: quic: Dysfunctional 01RTT packet number space probing
This bug arrived with this commit:
"MINOR: quic: implement qc_notify_send()".
The ->tx.pto_probe variable was no more set when qc_processt_timer() the timer
task for the connection responsible of detecting packet loss and probing upon
PTO expiration leading to interrupted stream transfers. This was revealed by
blackhole interop failed tests where one could see that qc_process_timer()
was wakeup without traces as follows in the log file:
"needs to probe 01RTT packet number space"
Must be backported to 2.7 and to 2.6 if the commit mentionned above
is backported to 2.6 in the meantime.
MINOR: quic: Stop stressing the acknowledgments process (RX ACK frames)
The ACK frame range of packets were handled from the largest to the smallest
packet number, leading to big number of ebtree insertions when the packet are
handled in the inverse way they are sent. This was detected a long time ago
but left in the code to stress our implementation. It is time to be more
efficient and process the packet so that to avoid useless ebtree insertions.
Modify qc_ackrng_pkts() responsible of handling the acknowledged packets from an
ACK frame range of acknowledged packets.
Willy Tarreau [Fri, 17 Mar 2023 15:13:05 +0000 (16:13 +0100)]
MINOR: ssl-sock: pass the CO_SFL_MSG_MORE info down the stack
Despite having replaced the SSL BIOs to use our own raw_sock layer, we
still didn't exploit the CO_SFL_MSG_MORE flag which is pretty useful to
avoid sending incomplete packets. It's particularly important for SSL
since the extra overhead almost guarantees that each send() will be
followed by an incomplete (and often odd-sided) segment.
We already have an xprt_st set of flags to pass info to the various
layers, so let's just add a new one, SSL_SOCK_SEND_MORE, that is set
or cleared during ssl_sock_from_buf() to transfer the knowledge of
CO_SFL_MSG_MORE. This way we can recover this information and pass
it to raw_sock.
This alone is sufficient to increase by ~5-10% the H2 bandwidth over
SSL when multiple streams are used in parallel.
Willy Tarreau [Fri, 17 Mar 2023 15:09:14 +0000 (16:09 +0100)]
MINOR: mux-h2: set CO_SFL_MSG_MORE when sending multiple buffers
Traces show that sendto() rarely has MSG_MORE on H2 despite sending
multiple buffers. The reason is that the loop iterating over the buffer
ring doesn't have this info and doesn't pass it down.
But now we know how many buffers are left to be sent, so we know whether
or not the current buffer is the last one. As such we can set this flag
for all buffers but the last one.
Willy Tarreau [Fri, 17 Mar 2023 11:30:38 +0000 (12:30 +0100)]
OPTIM: mux-h1: limit first read size to avoid wrapping
Before muxes were used, we used to refrain from reading past the
buffer's reserve. But with muxes which have their own buffer, this
rule was a bit forgotten, resulting in an extraneous read to be
performed just because the rx buffer cannot be entirely transferred
to the stream layer:
Here the server sends 16kB of payload after a headers block, the mux reads
16320 into the ibuf, and the stream layer consumes 15360 from the first
h1_rcv_buf(), which leaves 960 into the buffer and releases a few indexes.
The buffer cannot be realigned due to these remaining data, and a subsequent
read is made on 16 bytes, then again on 182 bytes.
By avoiding to read too much on the first call, we can avoid needlessly
filling this buffer:
This is much more efficient and uses less RAM since the first buffer that
was emptied can now be released.
Note that a further improvement (tested) consists in reading even less
(typically 1kB) so that most of the data are transferred in zero-copy, and
are not read until process_stream() is scheduled. This patch doesn't do that
for now so that it can be backported without any obscure impact.
Willy Tarreau [Fri, 17 Mar 2023 15:40:09 +0000 (16:40 +0100)]
BUG/MAJOR: qpack: fix possible read out of bounds in static table
CertiK Skyfall Team reported that passing an index greater than
QPACK_SHT_SIZE in a qpack instruction referencing a literal field
name with name reference or and indexed field line will cause a
read out of bounds that may crash the process, and confirmed that
this fix addresses the issue.
When sc-add-gpc() action was implemented in 5a72d03 ("MINOR:
stick-table: implement the sc-add-gpc() action"), its usage was
only documented for "http-request", but according to the code it
now applies everywhere sc-inc-gpc() is mentioned.
Adding the missing entries in the doc everywhere the action may
be used.
The issue was detected by the haproxy-controller bot and was reported
by Pratik Mohanty and Marko Juraga.
No backport needed, unless 5a72d03 ("MINOR: stick-table: implement the
sc-add-gpc() action") is being backported.
MINOR: stick-table: add sc-add-gpc() to http-after-response
sc-add-gpc() was implemented in 5a72d03 ("MINOR: stick-table:
implement the sc-add-gpc() action")
This new action was exposed everywhere sc-inc-gpc() is available,
except for http-after-response.
But there doesn't seem to be a technical constraint that prevents us from
exposing it in http-after-response.
It was probably overlooked, let's add it.
No backport needed, unless 5a72d03 ("MINOR: stick-table: implement the
sc-add-gpc() action") is being backported.
BUG/MINOR: quic: Missing STREAM frame data pointer updates
This patch follows this one which was not sufficient:
"BUG/MINOR: quic: Missing STREAM frame length updates"
Indeed, it is not sufficient to update the ->len and ->offset member
of a STREAM frame to move it forward. The data pointer must also be updated.
This is not done by the STREAM frame builder.
Willy Tarreau [Thu, 16 Mar 2023 16:30:30 +0000 (17:30 +0100)]
BUG/MINOR: mux-h2: set CO_SFL_STREAMER when sending lots of data
Emeric noticed that h2 bit-rate performance was always slightly lower
than h1 when the CPU is saturated. Strace showed that we were always
data in 2kB chunks, corresponding to the max_record size. What's
happening is that when this mechanism of dynamic record size was
introduced, the STREAMER flag at the stream level was relied upon.
Since all this was moved to the muxes, the flag has to be passed as
an argument to the snd_buf() function, but the mux h2 did not use it
despite a comment mentioning it, probably because before the multi-buf
it was not easy to figure the status of the buffer.
The solution here consists in checking if the mbuf is congested or not,
by checking if it has more than one buffer allocated. If so we set the
CO_SFL_STREAMER flag, otherwise we don't. This way moderate size
exchanges continue to be made over small chunks, but downloads will
be able to use the large ones.
While it could be backported to all supported versions, it would be
better to limit it to the last LTS, so let's do it for 2.7 and 2.6 only.
This patch requires previous commit "MINOR: buffer: add br_single() to
check if a buffer ring has more than one buf".
Willy Tarreau [Thu, 16 Mar 2023 15:47:44 +0000 (16:47 +0100)]
BUG/MEDIUM: mux-h2: only restart sending when mux buffer is decongested
During performance tests, Emeric faced a case where the wakeups of
sc_conn_io_cb() caused by h2_resume_each_sending_h2s() was multiplied
by 5-50 and a lot of CPU was being spent doing this for apparently no
reason.
The culprit is h2_send() not behaving well with congested buffers and
small SSL records. What happens when the output is congested is that
all buffers are full, and data are emitted in 2kB chunks, which are
sufficient to wake all streams up again to ask them to send data again,
something that will obviously only work for one of them at best, and
waste a lot of CPU in wakeups and memcpy() due to the small buffers.
When this happens, the performance can be divided by 2-2.5 on large
objects.
Here the chosen solution against this is to keep in mind that as long
as there are still at least two buffers in the ring after calling
xprt->snd_buf(), it means that the output is congested and there's
no point trying again, because these data will just be placed into
such buffers and will wait there. Instead we only mark the buffer
decongested once we're back to a single allocated buffer in the ring.
By doing so we preserve the ability to deal with large concurrent
bursts while not causing a thundering herd by waking all streams for
almost nothing.
This needs to be backported to 2.7 and 2.6. Other versions could
benefit from it as well but it's not strictly necessary, and we can
reconsider this option if some excess calls to sc_conn_io_cb() are
faced.
Note that this fix depends on this recent commit:
MINOR: buffer: add br_single() to check if a buffer ring has more than one buf
Willy Tarreau [Thu, 16 Mar 2023 16:30:04 +0000 (17:30 +0100)]
MINOR: buffer: add br_single() to check if a buffer ring has more than one buf
It's cheaper and cleaner than using br_count()==1 given that it just compares
two indexes, and that a ring having a single buffer is in a special case where
it is between empty and used up-to-1. In other words it's not congested.
Willy Tarreau [Thu, 16 Mar 2023 17:06:19 +0000 (18:06 +0100)]
BUG/MINOR: mux-h2: make sure the h2c task exists before refreshing it
When detaching a stream, if it's the last one and the mbuf is blocked,
we leave without freeing the stream yet. We also refresh the h2c task's
timeout, except that it's possible that there's no such task in case
there is no client timeout, causing a crash. The fix just consists in
doing this when the task exists.
This bug has always been there and is extremely hard to meet even
without a client timeout. This fix has to be backported to all
branches, but it's unlikely anyone has ever met it anyay.
BUG/MEDIUM: connection: Preserve flags when a conn is removed from an idle list
The commit 5e1b0e7bf ("BUG/MEDIUM: connection: Clear flags when a conn is
removed from an idle list") introduced a regression. CO_FL_SAFE_LIST and
CO_FL_IDLE_LIST flags are used when the connection is released to properly
decrement used/idle connection counters. if a connection is idle, these
flags must be preserved till the connection is really released. It may be
removed from the list but not immediately released. If these flags are lost
when it is finally released, the current number of used connections is
erroneously decremented. If means this counter may become negative and the
counters tracking the number of idle connecitons is not decremented,
suggesting a leak.
So, the above commit is reverted and instead we improve a bit the way to
detect an idle connection. The function conn_get_idle_flag() must now be
used to know if a connection is in an idle list. It returns the connection
flag corresponding to the idle list if the connection is idle
(CO_FL_SAFE_LIST or CO_FL_IDLE_LIST) or 0 otherwise. But if the connection
is scheduled to be removed, 0 is also returned, regardless the connection
flags.
This new function is used when the connection is temporarily removed from
the list to be used, mainly in muxes.
This patch should fix #2078 and #2057. It must be backported as far as 2.2.
Some STREAM frame lengths were not updated before being duplicated, built
of requeued contrary to their ack offsets. This leads haproxy to crash when
receiving acknowledgements for such frames with this thread #1 backtrace:
Thread 1 (Thread 0x7211b6ffd640 (LWP 986141)):
#0 ha_crash_now () at include/haproxy/bug.h:52
No locals.
#1 b_del (b=<optimized out>, del=<optimized out>) at include/haproxy/buf.h:436
No locals.
#2 qc_stream_desc_ack (stream=stream@entry=0x7211b6fd9bc8, offset=offset@entry=53176, len=len@entry=1122) at src/quic_stream.c:111
Thank you to @Tristan971 for having provided such traces which reveal this issue:
Note that haproxy has much more chance to crash if this frame is the last one
(fin bit set). But another condition must be fullfilled to update the ack offset.
A previous STREAM frame from the same stream with the same offset but with less
data must be acknowledged by the peer. This is the condition to update the ack offset.
For others frames without fin bit in the same conditions, I guess the stream may be
truncated because too much data are removed from the stream when they are
acknowledged.
BUG/MINOR: tcp_sample: fix a bug in fc_dst_port and fc_dst_is_local sample fetches
There is a bug in the smp_fetch_dport() function which affects the 'f' case,
also known as 'fc_dst_port' sample fetch.
conn_get_src() is used to retrieve the address prior to calling conn_dst().
But this is wrong: conn_get_dst() should be used instead.
Because of that, conn_dst() may return unexpected results since the dst
address is not guaranteed to be set depending on the conn state at the time
the sample fetch is used.
This was reported by Corin Langosch on the ML:
during his tests he noticed that using fc_dst_port in a log-format string
resulted in the correct value being printed in the logs but when he used it
in an ACL, the ACL did not evaluate properly.
This can be easily reproduced with the following test conf:
|frontend test-http
| bind 127.0.0.1:8080
| mode http
|
| acl test fc_dst_port eq 8080
| http-request return status 200 if test
| http-request return status 500 if !test
A request on 127.0.0.1:8080 should normally return 200 OK, but here it
will return a 500.
The same bug was also found in smp_fetch_dst_is_local() (fc_dst_is_local
sample fetch) by reading the code: the fix was applied twice.
This needs to be backported up to 2.5
[both sample fetches were introduced in 2.5 with 888cd70 ("MINOR:
tcp-sample: Add samples to get original info about client connection")]
BUG/MEDIUM: mux-h1: Don't block SE_FL_ERROR if EOS is not reported on H1C
When a connection error is encountered during a receive, the error is not
immediatly reported to the SE descriptor. We take care to process all
pending input data first. However, when the error is finally reported, a
fatal error is reported only if a read0 was also received. Otherwise, only a
pending error is reported.
With a raw socket, it is not an issue because shutdowns for read and write
are systematically reported too when a connection error is detected. So in
this case, the fatal error is always reported to the SE descriptor. But with
a SSL socket, in case of pure SSL error, only the connection error is
reported. This prevent the fatal error to go up. And because the connection
is in error, no more receive or send are preformed on the socket. The mux is
blocked till a timeout is triggered at the stream level, looping infinitly
to progress.
To fix the bug, during the demux stage, when there is no longer pending
data, the read error is reported to the SE descriptor, regardless the
shutdown for reads was received or not. No more data are expected, so it is
safe.
This patch should fix the issue #2046. It must be backported to 2.7.
BUG/MEDIUM: resolvers: Properly stop server resolutions on soft-stop
When HAproxy is stopping, the DNS resolutions must be stopped, except those
triggered from a "do-resolve" action. To do so, the resolutions themselves
cannot be destroyed, the current design is too complex. However, it is
possible to mute the resolvers tasks. The same is already performed with the
health-checks. On soft-stop, the tasks are still running periodically but
nothing if performed.
For the resolvers, when the process is stopping, before running a
resolution, we check all the requesters attached to this resolution. If t
least a request is a stream or if there is a requester attached to a running
proxy, a new resolution is triggered. Otherwise, we ignored the
resolution. It will be evaluated again on the next wakeup. This way,
"do-resolv" action are still working during soft-stop but other resoluation
are stopped.
Of course, it may be see as a feature and not a bug because it was never
performed. But it is in fact not expected at all to still performing
resolutions when HAProxy is stopping. In addution, a proxy option will be
added to change this behavior.
This patch partially fixes the issue #1874. It could be backported to 2.7
and maybe to 2.6. But no further.
BUG/MEDIUM: proxy: properly stop backends on soft-stop
On soft-stop, we must properlu stop backends and not only proxies with at
least a listener. This is mandatory in order to stop the health checks. A
previous fix was provided to do so (ba29687bc1 "BUG/MEDIUM: proxy: properly
stop backends"). However, only stop_proxy() function was fixed. When HAproxy
is stopped, this function is no longer used. So the same kind of fix must be
done on do_soft_stop_now().
This patch partially fixes the issue #1874. It must be backported as far as
2.4.
MINOR: ssl: Add certificate path to 'show ssl ocsp-response' output
The ocsp-related CLI commands tend to work with OCSP_CERTIDs as well as
certificate paths so the path should also be added to the output of the
"show ssl ocsp-response" command when no certid or path is provided.
MINOR: ssl: Accept certpath as param in "show ssl ocsp-response" CLI command
In order to increase usability, the "show ssl ocsp-response" also takes
a frontend certificate path as parameter. In such a case, it behaves the
same way as "show ssl cert foo.pem.ocsp".
BUG/MINOR: ssl: Fix double free in ocsp update deinit
If the last update before a deinit happens was successful, the pointer
to the httpclient in the ocsp update context was not reset while the
httpclient instance was already destroyed.
MINOR: ssl: Use ocsp update task for "update ssl ocsp-response" command
Instead of having a dedicated httpclient instance and its own code
decorrelated from the actual auto update one, the "update ssl
ocsp-response" will now use the update task in order to perform updates.
Since the cli command allows to update responses that were never
included in the auto update tree, a new flag was added to the
certificate_ocsp structure so that the said entry can be inserted into
the tree "by hand" and it won't be reinserted back into the tree after
the update process is performed. The 'update_once' flag "stole" a bit
from the 'fail_count' counter since it is the one less likely to reach
UINT_MAX among the ocsp counters of the certificate_ocsp structure.
This new logic required that every certificate_ocsp entry contained all
the ocsp-related information at all time since entries that are not
supposed to be configured automatically can still be updated through the
cli. The logic of the ssl_sock_load_ocsp was changed accordingly.
The dedicated proxy used for OCSP auto update is renamed OCSP-UPDATE
which should be more explicit than the previous HC_OCSP name. The
reference to the underlying httpclient is simply kept in the
documentation.
The certid is removed from the log line since it is not really
comprehensible and is replaced by the path to the corresponding frontend
certificate.
BUG/MEDIUM: mux-pt: Set EOS on error on sending path if read0 was received
It is more a less a revert of the commit b65af26e1 ("MEDIUM: mux-pt: Don't
always set a final error on SE on the sending path"). The PT multiplexer is
so simple that an error on the sending path is terminal. Unlike other muxes,
there is no connection level here. However, instead of reporting an final
error by setting SE_FL_ERROR, we set SE_FL_EOS flag instead if a read0 was
received on the underlying connection. Concretely, it is always true with
the current design of the raw socket layer. But it is cleaner this way.
Without this patch, it is possible to block a TCP socket if a connection
error is triggered when data are sent (for instance a broken pipe) while the
upper stream does not expect to receive more data.
Note the patch above introduced a regression because errors handling at the
connection level is quite simple. All errors are final. But we must keep in
mind it may change. And if so, this will require to move back on a 2-step
errors handling in the mux-pt.
Willy Tarreau [Fri, 10 Mar 2023 15:28:37 +0000 (16:28 +0100)]
[RELEASE] Released version 2.8-dev5
Released version 2.8-dev5 with the following main changes :
- MINOR: ssl: rename confusing ssl_bind_kws
- BUG/MINOR: config: crt-list keywords mistaken for bind ssl keywords
- BUG/MEDIUM: http-ana: Detect closed SC on opposite side during body forwarding
- BUG/MEDIUM: stconn: Don't rearm the read expiration date if EOI was reached
- MINOR: global: Add an option to disable the data fast-forward
- MINOR: haproxy: Add an command option to disable data fast-forward
- REGTESTS: Remove unsupported feature command in http_splicing.vtc
- BUG/MEDIUM: wdt: fix wrong thread being checked for sleeping
- BUG/MINOR: sched: properly report long_rq when tasks remain in the queue
- BUG/MEDIUM: sched: allow a bit more TASK_HEAVY to be processed when needed
- MINOR: threads: add flags to know if a thread is started and/or running
- MINOR: h3/hq-interop: handle no data in decode_qcs() with FIN set
- BUG/MINOR: mux-quic: transfer FIN on empty STREAM frame
- BUG/MINOR: mworker: prevent incorrect values in uptime
- MINOR: h3: add traces on decode_qcs callback
- BUG/MINOR: quic: Possible unexpected counter incrementation on send*() errors
- MINOR: quic: Add new traces about by connection RX buffer handling
- MINOR: quic: Move code to wakeup the timer task to avoid anti-amplication deadlock
- BUG/MINOR: quic: Really cancel the connection timer from qc_set_timer()
- MINOR: quic: Simplication for qc_set_timer()
- MINOR: quic: Kill the connections on ICMP (port unreachable) packet receipt
- MINOR: quic: Add traces to qc_kill_conn()
- MINOR: quic: Make qc_dgrams_retransmit() return a status.
- BUG/MINOR: quic: Missing call to task_queue() in qc_idle_timer_do_rearm()
- MINOR: quic: Add a trace to identify connections which sent Initial packet.
- MINOR: quic: Add <pto_count> to the traces
- BUG/MINOR: quic: Do not probe with too little Initial packets
- BUG/MINOR: quic: Wrong initialization for io_cb_wakeup boolean
- BUG/MINOR: quic: Do not drop too small datagrams with Initial packets
- BUG/MINOR: quic: Missing padding for short packets
- MINOR: quic: adjust request reject when MUX is already freed
- BUG/MINOR: quic: also send RESET_STREAM if MUX released
- BUG/MINOR: quic: acknowledge STREAM frame even if MUX is released
- BUG/MINOR: h3: prevent hypothetical demux failure on int overflow
- MEDIUM: h3: enforce GOAWAY by resetting higher unhandled stream
- MINOR: mux-quic: define qc_shutdown()
- MINOR: mux-quic: define qc_process()
- MINOR: mux-quic: implement client-fin timeout
- MEDIUM: mux-quic: properly implement soft-stop
- MINOR: quic: mark quic-conn as jobs on socket allocation
- MEDIUM: quic: trigger fast connection closing on process stopping
- MINOR: mux-h2/traces: do not log h2s pointer for dummy streams
- MINOR: mux-h2/traces: add a missing TRACE_LEAVE() in h2s_frt_handle_headers()
- BUG/MEDIUM: quic: Missing TX buffer draining from qc_send_ppkts()
- DEBUG: stream: Add a BUG_ON to never exit process_stream with an expired task
- DOC: config: Fix description of options about HTTP connection modes
- MINOR: proxy: Only consider backend httpclose option for server connections
- BUG/MINOR: haproxy: Fix option to disable the fast-forward
- DOC: config: Add the missing tune.fail-alloc option from global listing
- MINOR: cfgcond: Implement strstr condition expression
- MINOR: cfgcond: Implement enabled condition expression
- REGTESTS: Skip http_splicing.vtc script if fast-forward is disabled
- REGTESTS: Fix ssl_errors.vtc script to wait for connections close
- BUG/MINOR: mworker: stop doing strtok directly from the env
- BUG/MEDIUM: mworker: prevent inconsistent reload when upgrading from old versions
- BUG/MEDIUM: mworker: don't register mworker_accept_wrapper() when master FD is wrong
- MINOR: startup: HAPROXY_STARTUP_VERSION contains the version used to start
- BUG/MINOR: cache: Cache response even if request has "no-cache" directive
- BUG/MINOR: cache: Check cache entry is complete in case of Vary
- MINOR: compiler: add a TOSTR() macro to turn a value into a string
- BUG/MINOR: lua/httpclient: missing free in hlua_httpclient_send()
- BUG/MEDIUM: httpclient/lua: fix a race between lua GC and hlua_ctx_destroy
- MEDIUM: channel: Remove CF_READ_NOEXP flag
- MAJOR: channel: Remove flags to report READ or WRITE errors
- DEBUG: stream/trace: Add sedesc flags in trace messages
- MINOR: channel/stconn: Move rto/wto from the channel to the stconn
- MEDIUM: channel/stconn: Move rex/wex timer from the channel to the sedesc
- MEDIUM: stconn: Don't requeue the stream's task after I/O
- MEDIUM: stconn: Replace read and write timeouts by a unique I/O timeout
- MEDIUM: stconn: Add two date to track successful reads and blocked sends
- MINOR: applet/stconn: Add a SE flag to specify an endpoint does not expect data
- MAJOR: stream: Use SE descriptor date to detect read/write timeouts
- MINOR: stream: Dump the task expiration date in trace messages
- MINOR: stream: Report rex/wex value using the sedesc date in trace messages
- MINOR: stream: Use relative expiration date in trace messages
- MINOR: stconn: Always report READ/WRITE event on shutr/shutw
- CLEANUP: stconn: Remove old read and write expiration dates
- MINOR: stconn: Set half-close timeout using proxy settings
- MINOR: stconn: Remove half-closed timeout
- REGTESTS: cache: Use rxresphdrs to only get headers for 304 responses
- MINOR: stconn: Add functions to set/clear SE_FL_EXP_NO_DATA flag from endpoint
- BUG/MINOR: proto_ux: report correct error when bind_listener fails
- BUG/MINOR: protocol: fix minor memory leak in protocol_bind_all()
- MINOR: proto_uxst: add resume method
- MINOR: listener/api: add lli hint to listener functions
- MINOR: listener: add relax_listener() function
- MINOR: listener: workaround for closing a tiny race between resume_listener() and stopping
- MINOR: listener: make sure we don't pause/resume bypassed listeners
- BUG/MEDIUM: listener: fix pause_listener() suspend return value handling
- BUG/MINOR: listener: fix resume_listener() resume return value handling
- BUG/MEDIUM: resume from LI_ASSIGNED in default_resume_listener()
- MINOR: listener: pause_listener() becomes suspend_listener()
- BUG/MEDIUM: listener/proxy: fix listeners notify for proxy resume
- BUG/MINOR: sock_unix: match finalname with tempname in sock_unix_addrcmp()
- MEDIUM: proto_ux: properly suspend named UNIX listeners
- MINOR: proto_ux: ability to dump ABNS names in error messages
- MINOR: haproxy: always protocol unbind on startup error path
- BUILD: quic: 32-bits compilation issue with %zu in quic_rx_pkts_del()
- BUG/MINOR: ring: do not realign ring contents on resize
- MEDIUM: ring: make the offset relative to the head/tail instead of absolute
- CLEANUP: ring: remove the now unused ring's offset
- MINOR: config: add HAPROXY_BRANCH environment variable
- BUILD: thead: Fix several 32 bits compilation issues with uint64_t variables
- BUG/MEDIUM: fd: avoid infinite loops in fd_add_to_fd_list and fd_rm_from_fd_list
- BUG/MEDIUM: h1-htx: Never copy more than the max data allowed during parsing
- BUG/MINOR: stream: Remove BUG_ON about the task expiration in process_stream()
- MINOR: stream: Handle stream's timeouts in a dedicated function
- MEDIUM: stream: Eventually handle stream timeouts when exiting process_stream()
- MINOR: stconn: Report a send activity when endpoint is willing to consume data
- BUG/MEDIUM: stconn: Report a blocked send if some output data are not consumed
- MEDIUM: mux-h1: Don't expect data from server as long as request is unfinished
- MEDIUM: mux-h2: Don't expect data from server as long as request is unfinished
- MEDIUM: mux-quic: Don't expect data from server as long as request is unfinished
- DOC: config: Clarify the meaning of 'hold' in the 'resolvers' section
- DOC: config: Replace TABs by spaces
- BUG/MINOR: fd: used the update list from the fd's group instead of tgid
- BUG/MEDIUM: fd: make fd_delete() support being called from a different group
- CLEANUP: listener: only store conn counts for local threads
- MINOR: tinfo: make thread_set functions return nth group/mask instead of first
- MEDIUM: quic: improve fatal error handling on send
- MINOR: quic: consider EBADF as critical on send()
- BUG/MEDIUM: connection: Clear flags when a conn is removed from an idle list
- BUG/MINOR: mux-h1: Don't report an error on an early response close
- BUG/MINOR: http-check: Don't set HTX_SL_F_BODYLESS flag with a log-format body
- BUG/MINOR: http-check: Skip C-L header for empty body when it's not mandatory
- BUG/MINOR: http-fetch: recognize IPv6 addresses in square brackets in req.hdr_ip()
- REGTEST: added tests covering smp_fetch_hdr_ip()
- MINOR: quic: simplify return path in send functions
- MINOR: quic: implement qc_notify_send()
- MINOR: quic: purge txbuf before preparing new packets
- MEDIUM: quic: implement poller subscribe on sendto error
- MINOR: quic: notify on send ready
- BUG/MINOR: http-ana: Don't increment conn_retries counter before the L7 retry
- BUG/MINOR: http-ana: Do a L7 retry on read error if there is no response
- BUG/MEDIUM: http-ana: Don't close request side when waiting for response
- BUG/MINOR: mxu-h1: Report a parsing error on abort with pending data
- MINOR: ssl: Destroy ocsp update http_client during cleanup
- MINOR: ssl: Reinsert ocsp update entries later in case of unknown error
- MINOR: ssl: Add ocsp update success/failure counters
- MINOR: ssl: Store specific ocsp update errors in response and update ctx
- MINOR: ssl: Add certificate's path to certificate_ocsp structure
- MINOR: ssl: Add 'show ssl ocsp-updates' CLI command
- MINOR: ssl: Add sample fetches related to OCSP update
- MINOR: ssl: Use dedicated proxy and log-format for OCSP update
- MINOR: ssl: Reorder struct certificate_ocsp members
- MINOR: ssl: Increment OCSP update replay delay in case of failure
- MINOR: ssl: Add way to dump ocsp response in base64
- MINOR: ssl: Add global options to modify ocsp update min/max delay
- REGTESTS: ssl: Fix ocsp update crt-lists
- REGTESTS: ssl: Add test for new ocsp update cli commands
- MINOR: ssl: Add ocsp-update information to "show ssl crt-list"
- BUG/MINOR: ssl: Fix ocsp-update when using "add ssl crt-list"
- MINOR: ssl: Replace now.tv_sec with date.tv_sec in ocsp update task
- BUG/MINOR: ssl: Use 'date' instead of 'now' in ocsp stapling callback
- BUG/MEDIUM: quic: properly handle duplicated STREAM frames
- BUG/MINOR: cli: fix CLI handler "set anon global-key" call
- MINOR: http_ext: adding some documentation, forgot to inline function
- BUG/MINOR: quic: Do not send too small datagrams (with Initial packets)
- MINOR: quic: Add a BUG_ON_HOT() call for too small datagrams
- BUG/MINOR: quic: Ensure to be able to build datagrams to be retransmitted
- BUG/MINOR: quic: v2 Initial packets decryption failed
- MINOR: quic: Add traces about QUIC TLS key update
- BUG/MINOR: quic: Remove force_ack for Initial,Handshake packets
- BUG/MINOR: quic: Ensure not to retransmit packets with no ack-eliciting frames
- BUG/MINOR: quic: Do not resend already acked frames
- BUG/MINOR: quic: Missing detections of amplification limit reached
- MINOR: quic: Send PING frames when probing Initial packet number space
- BUG/MEDIUM: quic: do not crash when handling STREAM on released MUX
- BUG/MAJOR: fd/thread: fix race between updates and closing FD
- BUG/MEDIUM: dns: ensure ring offset is properly reajusted to head
- BUG/MINOR: mux-quic: properly init STREAM frame as not duplicated
- MINOR: quic: Do not accept wrong active_connection_id_limit values
- MINOR: quic: Store the next connection IDs sequence number in the connection
- MINOR: quic: Typo fix for ACK_ECN frame
- MINOR: quic: RETIRE_CONNECTION_ID frame handling (RX)
- MINOR: quic: Useless TLS context allocations in qc_do_rm_hp()
- MINOR: quic: Add spin bit support
- MINOR: quic: Add transport parameters to "show quic"
- BUG/MEDIUM: sink/forwarder: ensure ring offset is properly readjusted to head
- BUG/MINOR: dns: fix ring offset calculation on first read
- BUG/MINOR: dns: fix ring offset calculation in dns_resolve_send()
- MINOR: jwt: Add support for RSA-PSS signatures (PS256 algorithm)
- MINOR: h3: add traces on h3_init_uni_stream() error paths
- MINOR: quic: create a global list dedicated for closing QUIC conns
- MINOR: quic: handle new closing list in show quic
- MEDIUM: quic: release closing connections on stopping
- BUG/MINOR: quic: Wrong RETIRE_CONNECTION_ID sequence number check
- MINOR: fd/cli: report the polling mask in "show fd"
- CLEANUP: sock: always perform last connection updates before wakeup
- MINOR: quic: Do not stress the peer during retransmissions of lost packets
- BUG/MINOR: init: properly detect NUMA bindings on large systems
- BUG/MINOR: thread: report thread and group counts in the correct order
- BUG/MAJOR: fd/threads: close a race on closing connections after takeover
- MINOR: debug: add random delay injection with "debug dev delay-inj"
- BUG/MINOR: mworker: use MASTER_MAXCONN as default maxconn value
- BUG/MINOR: quic: Missing listener accept queue tasklet wakeups
- MINOR: quic_sock: un-statify quic_conn_sock_fd_iocb()
- DOC: config: fix typo "dependeing" in bind thread description
- DOC/CLEANUP: fix typos
This one is printed as the iocb in the "show fd" output, and arguably
this wasn't very convenient as-is:
293 : st=0x000123(cl heopI W:sRa R:sRA) ref=0 gid=1 tmask=0x8 umask=0x0 prmsk=0x8 pwmsk=0x0 owner=0x7f488487afe0 iocb=0x50a2c0(main+0x60f90)
Let's unstatify it and export it so that the symbol can now be resolved
from the various points that need it.
This open (-c) 64 QUIC connections (-n) 16384 h3 requets from (-t) 4 threads, i.e.
256 requests by connection. Such tests could not always pass and often ended with
such results displays by h2load:
finished in 53.74s, 38.11 req/s, 493.78KB/s
requests: 16384 total, 2944 started, 2048 done, 2048 succeeded, 14336
failed, 14336 errored, 0 timeout
status codes: 2048 2xx, 0 3xx, 0 4xx, 0 5xx
traffic: 25.92MB (27174537) total, 102.00KB (104448) headers (space
savings 1.92%), 25.80MB (27053569) data
UDP datagram: 3883 sent, 24330 received
min max mean sd ± sd
time for request: 48.75ms 502.86ms 134.12ms 75.80ms 92.68%
time for connect: 20.94ms 331.24ms 189.59ms 84.81ms 59.38%
time to 1st byte: 394.36ms 417.01ms 406.72ms 9.14ms 75.00%
req/s : 0.00 115.45 14.30 38.13 87.50%
The number of successful requests was always a multiple of 256.
Activating the traces also shew that some connections were blocked after having
successfully completed their handshakes due to the fact that the mux. The mux
is started upon the acceptation of the connection.
Under heavy load, some connections were never accepted. From the moment where
more than 4 (MAXACCEPT) connections were enqueued before a listener could be
woken up to accept at most 4 connections, the remaining connections were not
accepted ore lately at the second listener tasklet wakeup.
Add a call to tasklet_wakeup() to the accept list tasklet of the listeners to
wake up it if there are remaining connections to accept after having called
listener_accept(). In this case the listener must not be removed of this
accept list, if not at the next call it will not accept anything more.
BUG/MINOR: mworker: use MASTER_MAXCONN as default maxconn value
In environments where SYSTEM_MAXCONN is defined when compiling, the
master will use this value instead of the original minimal value which
was set to 100. When this happens, the master process could allocate
RAM excessively since it does not need to have an high maxconn. (For
example if SYSTEM_MAXCONN was set to 100000 or more)
This patch fixes the issue by using the new define MASTER_MAXCONN which
define a default maxconn of 100 for the master process.
Willy Tarreau [Thu, 9 Mar 2023 07:25:01 +0000 (08:25 +0100)]
MINOR: debug: add random delay injection with "debug dev delay-inj"
The goal is to send signals to random threads at random instants so that
they spin for a random delay in a relax() loop, trying to give back the
CPU to another competing hardware thread, in hope that from time to time
this can trigger in critical areas and increase the chances to provoke a
latent concurrency bug. For now none were observed.
For example, this command starts 64 such tasks waking after random delays
of 0-1ms and delivering signals to trigger such loops on 3 random threads:
for i in {1..64}; do
socat - /tmp/sock1 <<< "expert-mode on;debug dev delay-inj 2 3"
done
This command is only enabled when DEBUG_DEV is set at build time.
Willy Tarreau [Tue, 7 Mar 2023 18:11:02 +0000 (10:11 -0800)]
BUG/MAJOR: fd/threads: close a race on closing connections after takeover
As mentioned in commit 237e6a0d6 ("BUG/MAJOR: fd/thread: fix race between
updates and closing FD"), a race was found during stress tests involving
heavy backend connection reuse with many competing closes.
Here the problem is complex. The analysis in commit f69fea64e ("MAJOR:
fd: get rid of the DWCAS when setting the running_mask") that removed
the DWCAS in 2.5 overlooked a few races.
First, a takeover from thread1 could happen just after fd_update_events()
in thread2 validates it holds the tmask bit in the CAS loop. Since thread1
releases running_mask after the operation, thread2 will succeed the CAS
and both will believe the FD is theirs. This does explain the occasional
crashes seen with h1_io_cb() being called on a bad context, or
sock_conn_iocb() seeing conn->subs vanish after checking it. This issue
can be addressed using a DWCAS in both fd_takeover() and fd_update_events()
as it was before the patch above but this is not portable to all archs and
is not easy to adapt for those lacking it, due to some operations still
happening only on individual masks after the thread groups were added.
Second, the checks after fd_clr_running() for the current thread being
the last one is not sufficient: at the exact moment the operation
completes, another thread may also set and drop the running bit and see
itself as alone, and both can call _fd_close_orphan() in parallel. In
order to prevent this from happening, we cannot rely on the absence of
others, we need an explicit flag indicating that the FD must be closed.
One approach that was attempted consisted in playing with the thread_mask
but that was not reliable since it could still match between the late
deletion and the early insertion that follows. Instead, a new FD flag
was added, FD_MUST_CLOSE, that exactly indicates that the call to
_fd_delete_orphan() must be done. It is set by fd_delete(), and
atomically cleared by the first one which checks it, and which is the
only one to call _fd_delete_orphan().
With both points addressed, there's no more visible race left:
- takeover() only happens under the connection list's lock and cannot
compete with fd_delete() since fd_delete() must first remove the
connection from the list before deleting the FD. That's also why it
doesn't need to call _fd_delete_orphan() when dropping its running
bit.
- takeover() sets its running bit then atomically replaces the thread
mask, so that until that's done, it doesn't validate the condition
to end the synchonization loop in fd_update_events(). Once it's OK,
the previous thread's bit is lost, and this is checked for in
fd_update_events()
- fd_update_events() can compete with fd_delete() at various places
which are explained above. Since fd_delete() clears the thread mask
as after setting its running bit and after setting the FD_MUST_CLOSE
bit, the synchronization loop guarantees that the thread mask is seen
before going further, and that once it's seen, the FD_MUST_CLOSE flag
is already present.
- fd_delete() may start while fd_update_events() has already started,
but fd_delete() must hold a bit in thread_mask before starting, and
that is checked by the first test in fd_update_events() before setting
the running_mask.
- the poller's _update_fd() will not compete against _fd_delete_orphan()
nor fd_insert() thanks to the fd_grab_tgid() that's always done before
updating the polled_mask, and guarantees that we never pretend that a
polled_mask has a bit before the FD is added.
The issue is very hard to reproduce and is extremely time-sensitive.
Some tests were required with a 1-ms timeout with request rates
closely matching 1 kHz per server, though certain tests sometimes
benefitted from saturation. It was found that adding the following
slowdown at a few key places helped a lot and managed to trigger the
bug in 0.5 to 5 seconds instead of tens of minutes on a 20-thread
setup:
{ volatile int i = 10000; while (i--); }
Particularly, placing it at key places where only one of running_mask
or thread_mask is set and not the other one yet (e.g. after the
synchronization loop in fd_update_events or after dropping the
running bit) did yield great results.
Many thanks to Olivier Houchard for this expert help analysing these
races and reviewing candidate fixes.
The patch must be backported to 2.5. Note that 2.6 does not have tgid
in FDs, and that it requires a change of output on fd_clr_running() as
we need the previous bit. This is provided by carefully backporting
commit d6e1987612 ("MINOR: fd: make fd_clr_running() return the previous
value instead"). Tests have shown that the lack of tgid is a showstopper
for 2.6 and that unless a better workaround is found, it could still be
preferable to backport the minimum pieces required for fd_grab_tgid()
to 2.6 so that it stays stable long.
Willy Tarreau [Thu, 9 Mar 2023 10:39:51 +0000 (11:39 +0100)]
BUG/MINOR: thread: report thread and group counts in the correct order
In case too many thread groups are needed for the threads, we emit
an error indicating the problem. Unfortunately the threads and groups
counts were reversed.
Willy Tarreau [Thu, 9 Mar 2023 09:12:06 +0000 (10:12 +0100)]
BUG/MINOR: init: properly detect NUMA bindings on large systems
The NUMA detection code tries not to interfer with any taskset the user
could have specified in init scripts. For this it compares the number of
CPUs available with the number the process is bound to. However, the CPU
count is retrieved after being applied an upper bound of MAX_THREADS, so
if the machine has more than 64 CPUs, the comparison always fails and
makes haproxy think the user has already enforced a binding, and it does
not pin it anymore to a single NUMA node.
MINOR: quic: Do not stress the peer during retransmissions of lost packets
This issue was revealed by "Multiple streams" QUIC tracker test which very often
fails (locally) with a file of about 1Mbytes (x4 streams). The log of QUIC tracker
revealed that from its point of view, the 4 files were never all received entirely:
But this in contradiction with others QUIC tracker logs which confirms that haproxy
has really (re)sent the stream at the suspected offset(stream_12_rec_offset):
When detected as losts, the packets are enlisted, then their frames are
requeued in their packet number space by qc_requeue_nacked_pkt_tx_frms().
This was done using a local list which was spliced to the packet number
frame list. This had as bad effect to retransmit the frames in the inverse
order they have been sent. This is something the QUIC tracker go client
does not like at all!
Removing the frame splicing fixes this issue and allows haproxy to pass the
"Multiple streams" test.
Willy Tarreau [Thu, 2 Mar 2023 14:07:51 +0000 (15:07 +0100)]
CLEANUP: sock: always perform last connection updates before wakeup
Normally the task_wakeup() in sock_conn_io_cb() is expected to
happen on the same thread the FD is attached to. But due to the
way the code was arranged in the past (with synchronous callbacks)
we continue to update connections after the wakeup, which always
makes the reader have to think deeply whether it's possible or not
to call another thread there. Let's just move the tasklet_wakeup()
at the end to make sure there's no problem with that.
BUG/MINOR: quic: Wrong RETIRE_CONNECTION_ID sequence number check
This bug arrived with this commit: b5a8020e9 MINOR: quic: RETIRE_CONNECTION_ID frame handling (RX)
and was revealed by h3 interop tests with clients like s2n-quic and quic-go
as noticed by Amaury.
Indeed, one must check that the CID matching the sequence number provided by a received
RETIRE_CONNECTION_ID frame does not match the DCID of the packet.
Remove useless ->curr_cid_seq_num member from quic_conn struct.
The sequence number lookup must be done in qc_handle_retire_connection_id_frm()
to check the validity of the RETIRE_CONNECTION_ID frame, it returns the CID to be
retired into <cid_to_retire> variable passed as parameter to this function if
the frame is valid and if the CID was not already retired
quic-conn instances are marked as jobs. This prevent haproxy process to
stop while there is transfer in progress. To not delay process
termination, idle connections are woken up through their MUX instances
to be able to release them immediately.
However, there is no mechanism to wake up quic connections left on
closing or draining state. This means that haproxy process termination
is delayed until every closing quic connections timer has expired.
To improve this, a new function quic_handle_stopping() is called when
haproxy process is stopping. It simply wakes up the idle timer task of
all connections in the global closing list. These connections will thus
be released immediately to not interrupt haproxy process stopping.
A new global quic-conn list has been added by the previous patch. It will
contain every quic-conn in closing or draining state.
Thus, it is now easier to include or skip them on a "show quic" output :
when the default list on the current thread has been browsed entirely,
either we skip to the next thread or we look at the closing list on the
current thread.
MINOR: quic: create a global list dedicated for closing QUIC conns
When a CONNECTION_CLOSE is emitted or received, a QUIC connection enters
respectively in draining or closing state. These states are a loose
equivalent of TCP TIME_WAIT. No data can be exchanged anymore but the
connection is maintained during a certain timer to handle packet
reordering or loss.
A new global list has been defined for QUIC connections in
closing/draining state inside thread_ctx structure. Each time a
connection enters in one of this state, it will be moved from the
default global list to the new closing list.
The objective of this patch is to quickly filter connections on
closing/draining. Most notably, this will be used to wake up these
connections and avoid that haproxy process stopping is delayed by them.
A dedicated function qc_detach_th_ctx_list() has been implemented to
transfer a quic-conn from one list instance to the other. This takes
care of back-references attach to a quic-conn instance in case of a
running "show quic".
MINOR: jwt: Add support for RSA-PSS signatures (PS256 algorithm)
This patch adds the support for the PS algorithms when verifying JWT
signatures (rsa-pss). It was not managed during the first implementation
and previously raised an "Unmanaged algorithm" error.
The tests use the same rsa signature as the plain rsa tests (RS256 ...)
and the implementation simply adds a call to
EVP_PKEY_CTX_set_rsa_padding in the function that manages rsa and ecdsa
signatures.
The signatures in the reg-test were built thanks to the PyJWT python
library once again.
BUG/MINOR: dns: fix ring offset calculation in dns_resolve_send()
With 737d10f ("BUG/MEDIUM: dns: ensure ring offset is properly reajusted
to head") relative offset calculation was fixed in dns_session_io_handler()
and dns_process_req() functions.
But if we compare with the changes performed in the patch that introduced
the bug: d9c7188 ("MEDIUM: ring: make the offset relative to the head/tail
instead of absolute"), we can see that dns_resolve_send() is missing from
the patch.
Applying both 737d10f + ("BUG/MINOR: dns: fix ring offset calculation on
first read") to dns_resolve_send() function.
With this last commit, we should be back at pre d9c7188 behavior.
BUG/MINOR: dns: fix ring offset calculation on first read
With 737d10f ("BUG/MEDIUM: dns: ensure ring offset is properly reajusted
to head") ring offset is now properly re-adjusted in dns_session_io_handler()
and dns_process_req().
But the previous patch does not cope well if the first read is performed
on a non-empty ring since relative ofs will be computed from ds->ofs=0 or
dss->ofs_req=0.
In this case: relative offset could become invalid since we mix up relative
offsets with absolute offsets.
To fix this, we apply the same logic performed in d9c7188 ("MEDIUM: ring:
make the offset relative to the head/tail instead of absolute") for the
cli_io_handler_show_ring() function: that is using b_peek_ofs(buf, 0) to
set the contextual offset instead of hard-coding it to 0.
This should be considered as a minor bugfix since this bug was discovered by
reading the code: 737d10f already survived a good amount of stress-tests as
shown in GH #2068.
No backport needed as 737d10f is not marked for backports.
BUG/MEDIUM: sink/forwarder: ensure ring offset is properly readjusted to head
Since d9c7188 ("MEDIUM: ring: make the offset relative to the head/tail instead
of absolute"), ring offset calculation has changed: we don't rely on ring->ofs
absolute offset anymore.
But with the above patch, relative offset is not properly calculated in
sink_forward_oc_io_handler() and sink_forward_io_handler().
The issue here is the same as 737d10f ("BUG/MEDIUM: dns: ensure ring offset is
properly reajusted to head") since dns and sink_forward share the same
ring logic:
When the ring is becoming full, ring_write() will try to regain some space to
insert new data by calling b_del() on older messages. Here b_del() moves
buffer's head under the hood, and since ring->ofs cannot be used to "correct"
the relative offset, both sink_forward_oc_io_handler() and
sink_forward_io_handler() start to get invalid offset.
At this point, we will suffer from ring data corruption resulting in unexpected
behavior or process crashes.
This can be easily demonstrated with the following test:
Haproxy will forward incoming logs on udp@127.0.0.1:5114 to
tcp@127.0.0.1:5114
Then use the following tcp server:
nc -l -p 5114
With the following udp log sender:
|while [ 1 ]
|do
| logger --udp --server 127.0.0.1 -P 5114 -p user.warn "Test 7"
|done
Once the ring buffer is full (it takes less that a second to fill the 16k
buffer) haproxy starts to misbehave and the log forwarding stops.
We apply the same fix as in 737d10f ("BUG/MEDIUM: dns: ensure ring offset is
properly reajusted to head").
Please note the ~0 case that is handled slightly differently in this patch:
this is required to properly start reading from a non-empty ring. This case
will be fixed in dns related code in the following patch.
This does not need to be backported as d9c7188 was not marked for backports.
MINOR: quic: Add transport parameters to "show quic"
Modify quic_transport_params_dump() and others function relative to the
transport parameters value dump from TRACE() to make their output more
compact.
Add call to quic_transport_params_dump() to dump the transport parameters
from "show quic" CLI command.
Add QUIC_FL_RX_PACKET_SPIN_BIT new RX packet flag to mark an RX packet as having
the spin bit set. Idem for the connection with QUIC_FL_CONN_SPIN_BIT flag.
Implement qc_handle_spin_bit() to set/unset QUIC_FL_CONN_SPIN_BIT for the connection
as soon as a packet number could be deciphered.
Modify quic_build_packet_short_header() to set the spin bit when building
a short packet header.
projects / thirdparty / haproxy.git / log
