Commit Graph

18840 Commits

Author SHA1 Message Date
Christopher Faulet
05ed05b84a REGTESTS: http_request_buffer: Add a barrier to not mix up log messages
Depending on the timing, time to time, the log messages can be mixed. A
client can start and be fully handled by HAProxy (including its log message)
before the log message of the previous client was emitted or received.  To
fix the issue, a barrier was added to be sure to eval the "expect" rule on
logs before starting the next client.

This patch should fix the issue #1847. It may be backported to all branches
containing this reg-tests.
2022-09-01 19:46:28 +02:00
Christopher Faulet
f348ecd67a BUG/MINOR: regex: Properly handle PCRE2 lib compiled without JIT support
The PCRE2 JIT support is buggy. If HAProxy is compiled with USE_PCRE2_JIT
option while the PCRE2 library is compiled without the JIT support, any
matching will fail because pcre2_jit_compile() return value is not properly
handled. We must fall back on pcre2_match() if PCRE2_ERROR_JIT_BADOPTION
error is returned.

This patch should fix the issue #1848. It must be backported as far as 2.4.
2022-09-01 19:34:46 +02:00
Willy Tarreau
32872db605 MINOR: sink/ring: rotate non-empty file-backed contents only
If the service is rechecked before a reload, that may cause the config
to be parsed twice and file-backed rings to be lost.

Here we make sure that such a ring does contain information before
deciding to rotate it. This way the first process starting after some
writes will cause a rotate but not subsequent ones until new writes
are applied.

An attempt was also made to disable rotations on checks but this was a
bad idea, as the ring is still initialized and this causes the contents
to be lost. The choice of initializing the ring during parsing is
questionable but the config check ought to be as close as possible to a
real start, and we could imagine that the ring is used by some code
during startup (e.g. lua). So this approach was abandonned and config
checks also cause a rotation, as the purpose of this rotation is to
preserve latest information against accidental removal.
2022-09-01 08:25:34 +02:00
William Lallemand
e0fa91ffe1 BUG/MINOR: ssl: leak of ckch_inst_link in ckch_inst_free() v2
ckch_inst_free() unlink the ckch_inst_link structure but never free it.

It can't be fixed simply because cli_io_handler_commit_cafile_crlfile()
is using a cafile_entry list to iterate a list of ckch_inst entries
to free. So both cli_io_handler_commit_cafile_crlfile() and
ckch_inst_free() would modify the list at the same time.

In order to let the caller manipulate the ckch_inst_link,
ckch_inst_free() now checks if the element is still attached before
trying to detach and free it.

For this trick to work, the caller need to do a LIST_DEL_INIT() during
the iteration over the ckch_inst_link.

list_for_each_entry was also replace by a while (!LIST_ISEMPTY()) on the
head list in cli_io_handler_commit_cafile_crlfile() so the iteration
works correctly, because it could have been stuck on the first detached
element. list_for_each_entry_safe() is not enough to fix the issue since
multiple element could have been removed.

Must be backported as far as 2.5.
2022-08-31 15:24:01 +02:00
Frédéric Lécaille
bccbad2654 BUG/MINOR: quic: TX frames memleak
Missing call to pool_free() for quic_frame objects

Must be backported to 2.6.
2022-08-31 15:20:29 +02:00
Frédéric Lécaille
3a9b944955 MINOR: quic: Move traces about RX/TX bytes from QUIC_EV_CONN_PRSAFRM event
Move these traces to QUIC_EV_CONN_SPPKTS trace event. They were displayed
at a useless location. Make them displayed just after having sent a packet
and when checking the anti-amplication limit.
Useful to diagnose issues in relation with the recovery.
2022-08-31 15:20:24 +02:00
Willy Tarreau
d8009a1ca6 BUILD: debug: make sure debug macros are never empty
As outlined in commit f7ebe584d7 ("BUILD: debug: Add braces to if
statement calling only CHECK_IF()"), the BUG_ON() family of macros
is incorrectly defined to be empty when debugging is disabled, and
that can lead to trouble. Make sure they always fall back to the
usual "do { } while (0)". This may be backported to 2.6 if needed,
though no such issue was met there to date.
2022-08-31 10:53:53 +02:00
Willy Tarreau
3ff9610356 BUG/MINOR: dev/udp: properly preset the rx address size
addrlen was not preset to sizeof(addr) on rx, resulting in the address
often not being filled and response packets not always flowing back.

Let's also consistently use "addr" in the bind call (it points to
frt_addr there but it's a bit confusing).
2022-08-31 10:39:09 +02:00
William Lallemand
0bfa3e7ff2 BUG/MINOR: ssl: revert two wrong fixes with ckhi_link
This reverts commit 056ad01d55.
This reverts commit ddd480cbdc.

The architecture is ambiguous here: ckch_inst_free() is detaching and
freeing the "ckch_inst_link" linked list which must be free'd only from
the cafile_entry side.

The problem was also hidden by the fix ddd480c ("BUG/MEDIUM: ssl: Fix a
UAF when old ckch instances are released") which change the ckchi_link
inner loop by a safe one. However this can't fix entirely the problem
since both __ckch_inst_free_locked() could remove several nodes in the
ckchi_link linked list.

This revert is voluntary reintroducing a memory leak before really fixing
the problem.

Must be backported in 2.5 + 2.6.
2022-08-30 18:12:28 +02:00
Christopher Faulet
ddd480cbdc BUG/MEDIUM: ssl: Fix a UAF when old ckch instances are released
When old chck instances is released at the end of "commit ssl ca-file" or
"commit ssl crl-file" commands, the link is released. But we walk through
the list using the unsafe macro. list_for_each_entry_safe() must be used.

This bug was introduced by commit 056ad01d5 ("BUG/MINOR: ssl: leak of
ckch_inst_link in ckch_inst_free()"). Thus this patch must be backported as
far as 2.5.
2022-08-30 16:27:51 +02:00
Christopher Faulet
f611248d8c BUG/MINOR: tcpcheck: Disable QUICKACK for default tcp-check (with no rule)
The commit 871dd8211 ("BUG/MINOR: tcpcheck: Disable QUICKACK only if data
should be sent after connect") introduced a regression. It removes the test
on the next rule to be able to disable TCP_QUICKACK when only a connect is
performed (so no next rule).

This patch must be backported as far as 2.2.
2022-08-30 10:31:16 +02:00
William Lallemand
056ad01d55 BUG/MINOR: ssl: leak of ckch_inst_link in ckch_inst_free()
ckch_inst_free() unlink the ckch_inst_link structure but never free it.
It can cause a memory leak upon a ckch_inst_free() done with CLI
operation.

Bug introduced by commit 4458b97 ("MEDIUM: ssl: Chain ckch instances in
ca-file entries").

Must be backported as far as 2.5.
2022-08-29 18:53:34 +02:00
William Lallemand
946580e17a BUG/MINOR: ssl: fix deinit of the ca-file tree
Commit b0c4827 ("BUG/MINOR: ssl: free the cafile entries on deinit")
introduced a double free.

The node was never removed from the tree before its free.

Fix issue #1836.

Must be backported where b0c4827 was backported. (2.6 for now).
2022-08-29 18:51:39 +02:00
Frédéric Lécaille
3a56137048 MINOR: quic: Add a trace to distinguish the datagram from the packets inside
Without such a trace, we do not know when a datagram is sent. Only trace for
the packets inside the datagrams were displayed.

Must be backported to 2.6.
2022-08-29 18:46:40 +02:00
Frédéric Lécaille
c242832af3 BUG/MINOR: quic: Missing header protection AES cipher context initialisations (draft-v2)
This bug arrived with this commit:
   "MINOR: quic: Add reusable cipher contexts for header protection"

haproxy could crash because of missing cipher contexts initializations for
the header protection and draft-v2 Initial secrets. This was due to the fact
that these initialization both for RX and TX secrets were done outside of
qc_new_isecs(). The role of this function is definitively to initialize these
cipher contexts in addition to the derived secrets. Indeed this function is called
by qc_new_conn() which initializes the connection but also by qc_conn_finalize()
which also calls qc_new_isecs() in case of a different QUIC version was negotiated
by the peers from the one used by the client for its first Initial packet.

This was reported by "v2" QUIC interop test with at least picoquic as client.

Must be backported to 2.6.
2022-08-29 18:46:40 +02:00
Willy Tarreau
c6fc77404e MINOR: raw-sock: don't try to send if an error was already reported
There's no point trying to send() on a socket on which an error was already
reported. This wastes syscalls. Till now it was possible to occasionally
see an attempt to sendto() after epoll_wait() had reported EPOLLERR.
2022-08-29 18:45:27 +02:00
Willy Tarreau
2c30de3b90 BUG/MINOR: epoll: do not actively poll for Rx after an error
In 2.2, commit 5d7dcc2a8 ("OPTIM: epoll: always poll for recv if neither
active nor ready") was added to compensate for the fact that our iocbs
are almost always asynchronous now and do not have the opportunity to
update the FD correctly. As such, they just perform a wakeup, the FD is
turned to inactive, the tasklet wakes up, performs the I/O, updates the
FD, most of the time this is done withing the same polling loop, and the
update cancels itself in the poller without having to switch the FD off
then on.

The issue was that when deciding to claim an FD was active for reads
if it was active for writes, we forgot one situation that unfortunately
causes excessive wakeups: dealing with errors. Indeed, errors are
reported and keep ringing as long as the FD is active for sending even
if the consumer disabled the FD for receiving. Usually this only causes
one extra wakeup for the time it takes to consider a potential write
subscriber and to call it, though with many tasks in a run queue, it
can last a bit longer and be reported more often.

The fix consists in checking that we really want to get more receive
events on this FD, that is:
  - that no prevous EPOLLERR was reported
  - that the FD doesn't carry a sticky error
  - that the FD is not shut for reads

With this, after the last epoll_wait() reports EPOLLERR, one last recv()
is performed to flush pending data and the FD is immediately unregistered.

It's probably not needed to backport this as its effects are not much
visible, though it should not harm.

Before, EPOLLERR was seen twice:

  accept4(4, {sa_family=AF_INET, sin_port=htons(22314), sin_addr=inet_addr("127.0.0.1")}, [128 => 16], SOCK_NONBLOCK) = 8
  accept4(4, 0x261b160, [128], SOCK_NONBLOCK) = -1 EAGAIN (Resource temporarily unavailable)
  recvfrom(8, "POST / HTTP/1.1\r\nConnection: close\r\nTransfer-encoding: chunk"..., 16320, 0, NULL, NULL) = 66
  socket(AF_INET, SOCK_STREAM, IPPROTO_IP) = 9
  connect(9, {sa_family=AF_INET, sin_port=htons(8002), sin_addr=inet_addr("127.0.0.1")}, 16) = -1 EINPROGRESS (Operation now in progress)
  epoll_ctl(3, EPOLL_CTL_ADD, 8, {events=EPOLLIN|EPOLLRDHUP, data={u32=8, u64=8}}) = 0
  epoll_ctl(3, EPOLL_CTL_ADD, 9, {events=EPOLLIN|EPOLLOUT|EPOLLRDHUP, data={u32=9, u64=9}}) = 0
  epoll_wait(3, [{events=EPOLLOUT, data={u32=9, u64=9}}], 200, 355) = 1
  recvfrom(9, 0x25cfb30, 16320, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
  sendto(9, "POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\n", 47, MSG_DONTWAIT|MSG_NOSIGNAL, NULL, 0) = 47
  epoll_ctl(3, EPOLL_CTL_MOD, 9, {events=EPOLLIN|EPOLLRDHUP, data={u32=9, u64=9}}) = 0
  epoll_wait(3, [{events=EPOLLIN|EPOLLERR|EPOLLHUP|EPOLLRDHUP, data={u32=9, u64=9}}], 200, 354) = 1
  recvfrom(9, "HTTP/1.1 200 OK\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", 16320, 0, NULL, NULL) = 57
  sendto(8, "HTTP/1.1 200 OK\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", 57, MSG_DONTWAIT|MSG_NOSIGNAL, NULL, 0) = 57
->epoll_wait(3, [{events=EPOLLIN|EPOLLERR|EPOLLHUP|EPOLLRDHUP, data={u32=9, u64=9}}], 200, 354) = 1
  epoll_ctl(3, EPOLL_CTL_DEL, 9, 0x7ffe0b65fb24) = 0
  epoll_wait(3, [{events=EPOLLIN, data={u32=8, u64=8}}], 200, 354) = 1
  recvfrom(8, "A\n0123456789\r\n0\r\n\r\n", 16320, 0, NULL, NULL) = 19
  close(9)                          = 0
  close(8)                          = 0

After, EPOLLERR is only seen only once, with one less call to epoll_wait():

  accept4(4, {sa_family=AF_INET, sin_port=htons(22362), sin_addr=inet_addr("127.0.0.1")}, [128 => 16], SOCK_NONBLOCK) = 8
  accept4(4, 0x20d0160, [128], SOCK_NONBLOCK) = -1 EAGAIN (Resource temporarily unavailable)
  recvfrom(8, "POST / HTTP/1.1\r\nConnection: close\r\nTransfer-encoding: chunk"..., 16320, 0, NULL, NULL) = 66
  socket(AF_INET, SOCK_STREAM, IPPROTO_IP) = 9
  connect(9, {sa_family=AF_INET, sin_port=htons(8002), sin_addr=inet_addr("127.0.0.1")}, 16) = -1 EINPROGRESS (Operation now in progress)
  epoll_ctl(3, EPOLL_CTL_ADD, 8, {events=EPOLLIN|EPOLLRDHUP, data={u32=8, u64=8}}) = 0
  epoll_ctl(3, EPOLL_CTL_ADD, 9, {events=EPOLLIN|EPOLLOUT|EPOLLRDHUP, data={u32=9, u64=9}}) = 0
  epoll_wait(3, [{events=EPOLLOUT, data={u32=9, u64=9}}], 200, 411) = 1
  recvfrom(9, 0x2084b30, 16320, 0, NULL, NULL) = -1 EAGAIN (Resource temporarily unavailable)
  sendto(9, "POST / HTTP/1.1\r\ntransfer-encoding: chunked\r\n\r\n", 47, MSG_DONTWAIT|MSG_NOSIGNAL, NULL, 0) = 47
  epoll_ctl(3, EPOLL_CTL_MOD, 9, {events=EPOLLIN|EPOLLRDHUP, data={u32=9, u64=9}}) = 0
  epoll_wait(3, [{events=EPOLLIN|EPOLLERR|EPOLLHUP|EPOLLRDHUP, data={u32=9, u64=9}}], 200, 411) = 1
  recvfrom(9, "HTTP/1.1 200 OK\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", 16320, 0, NULL, NULL) = 57
  sendto(8, "HTTP/1.1 200 OK\r\ncontent-length: 0\r\nconnection: close\r\n\r\n", 57, MSG_DONTWAIT|MSG_NOSIGNAL, NULL, 0) = 57
  epoll_ctl(3, EPOLL_CTL_DEL, 9, 0x7ffc95d46f04) = 0
  epoll_wait(3, [{events=EPOLLIN, data={u32=8, u64=8}}], 200, 411) = 1
  recvfrom(8, "A\n0123456789\r\n0\r\n\r\n", 16320, 0, NULL, NULL) = 19
  close(9)                          = 0
  close(8)                          = 0
2022-08-29 18:45:27 +02:00
Willy Tarreau
cad42a78b8 BUG/MEDIUM: mux-h1: do not refrain from signaling errors after end of input
In 2.6-dev4, a fix for truncated response was brought with commit 99bbdbcc2
("BUG/MEDIUM: mux-h1: only turn CO_FL_ERROR to CS_FL_ERROR with empty ibuf"),
trying to address the situation where an error is present at the connection
level but some data are still pending to be read by the stream. However,
this patch did not consider the case where the stream was no longer willing
to read the pending data, resulting in a situation where some aborted
transfers could lead to excessive CPU usage by causing constant stream
wakeups for which no error was reported. This perfectly matches what was
observed and reported in github issue #1842. It's not trivial to reproduce,
but aborting HTTP/1 pipelining in the middle of transfers seems to give
good results (using h2load and Ctrl-C in the middle).

The fix was incorrct as the error should be held only if there were data
that the stream was able to read. This is the approach taken by this patch,
which also checks via SE_FL_EOI | SE_FL_EOS that the stream will be able
to consume the pending data.

Note that the loop was provoked by the attempt by sc_conn_io_cb() itself
to call sc_conn_send() which resulted in a write subscription in
h1_subscribe() which immediately calls a tasklet_wakeup() since the
event is ready, and that it is now stopped by the presence of SE_FL_ERROR
that is checked in sc_conn_io_cb(). It seems that an extra check down the
send() path to refrain from subscribing when the connection is in error
could speed up error detection or at least avoid a risk of loops in this
case, but this is tricky. In addition, there's already SE_FL_ERR_PENDING
that seems more suitable for reporting when there are pending data, but
similarly, it probably isn't checked well enough to be suitable for
backports.

FWIW the issue may (unreliably) be reproduced by chaining haproxy to
httpterm and issuing:

  (printf "GET /?s=10g HTTP/1.1\r\n\r\n"; sleep 0.1; printf "\r\n") | \
    nc6 --half-close 0 8001 | head -c1000000000 >/dev/null

It's necessary to play with the size of the head command that's supposed
to trigger the error at some point. A variant involving h2load in h1 mode
and multiple pipelined streams, that is stopped with Ctrl-C also tends to
work.

As the fix above was backported as far as 2.0, it would be tempting to
backport this one as far. However tests have shown that the oldest
version that can trigger this issue is 2.5, maybe due to subtle
differences in older ones, so it's probably not worth going further
until an issue is reported. Note that in 2.5 and older, the SE_FL_*
flags are applied on the conn_stream instead, as CS_FL_*.

Special thanks go to Felipe W Damasio for providing lots of detailed data
allowing to quickly spot the root cause of the problem.
2022-08-29 18:45:27 +02:00
Christopher Faulet
4a20972a95 BUG/MINOR: hlua: Rely on CF_EOI to detect end of message in HTTP applets
applet:getline() and applet:receive() functions for HTTP applets must rely
on the channel flags to detect the end of the message and not on HTX
flags. It means CF_EOI must be used instead of HTX_FL_EOM.

It is important because the HTX flag is transient. Because there is no flag
on HTTP applets to save the info, it is not reliable. However CF_EOI once
set is never removed. So it is safer to rely on it. Otherwise, the call to
these functions hang.

This patch must be backported as far as 2.4.
2022-08-29 15:37:17 +02:00
Christopher Faulet
b372f16d35 BUG/MEDIUM: peers: Don't start resync on reload if local peer is not up-to-date
On a reload, if the previous resync was not finished, the freshly old worker
must not try to start a new resync. Otherwise, it will compete with the
older wokers, slowing down or blocking the resync. Only an up-to-date woker
must try to perform a local resync.

This patch must be backported as far as 2.0 (and maybe to 1.8 too).
2022-08-29 11:38:02 +02:00
Christopher Faulet
19a82b9495 BUG/MEDIUM: peers: Don't use resync timer when local resync is in progress
When a worker is stopped, the resync timer is used to limit in time the
connection stage to the new worker to perform the local resync. However,
this timer must be stopped when the resync is in progress and it must be
re-armed if the resync is interrupted (for instance because another
reload). Otherwise, if the resync is a bit long, an old worker may be killed
too early.

This bug was introduce by the commit 160fff665 ("BUG/MEDIUM: peers: limit
reconnect attempts of the old process on reload"). It must be backported as
far as 2.0.
2022-08-29 11:38:02 +02:00
Christopher Faulet
13db4bdbc6 BUG/MEDIUM: peers: Add connect and server timeut to peers proxy
Only the client timeout was set. Nothing prevent a peer applet to stall
during a connect or waiting a message from a remote peer. To avoid any
issue, it is important to also set connection and server timeouts. The
connect timeout is set to 1s and the server timeout is set to 5s.

This patch must be backported to all supported versions.
2022-08-29 11:38:02 +02:00
Christopher Faulet
42a0662910 BUG/MEDIUM: spoe: Properly update streams waiting for a ACK in async mode
A bug was introduced by the commit b042e4f6f ("BUG/MAJOR: spoe: properly
detach all agents when releasing the applet"). The fix is not correct. We
really want to known if the released appctx is the last one or not. It is
important when async mode is used. If there are still running applets, we
just need to remove the reference on the current applet from streams in the
async waiting queue.

With the commit above, in async mode, if there are still running applets, it
will work as expected. Otherwise a processing timeout will be reported for
all these streams. So it is not too bad. But for other modes (sync and
pipelining), the async waiting queue is always empty. If at least one stream
is waiting to send a message, a new applet is created. It is an issue if the
SPOA is unhealthy because the number of running applets may explode.

However, the commit above tried to fix an issue. The bug is in fact when an
new SPOE applet is created. On success, we must remove reference on the
current appctx from the streams in the async waiting queue.

This patch must be backported as far as 1.8.
2022-08-29 09:57:33 +02:00
Frédéric Lécaille
149c531fa1 BUG/MINOR: quic: Frames added to packets even if not built.
Several frames could remain as not build into <frm_list> built by qc_build_frms()
after having stopped at the first building error. So only one frame was reinserted in
the frame list passed as parameter to qc_do_build_pkt(). Then <frm_list> was
spliced to the packet frame list even its frames were not built, nor attached to
any packet. Such frames had their ->pkt member set to NULL, but considered as
built, then sent leading to a crash in qc_release_frm() where ->pkt is dereferenced.

This issue was again reported by useful traces provided by Tristan in GH #1808.

Must be backported to 2.6.
2022-08-27 18:33:19 +02:00
Frédéric Lécaille
e35463c767 BUG/MINOR: quic: Null packet dereferencing from qc_dup_pkt_frms() trace
This function must duplicate frames be resent from packets. Some of
them are still in flight, others have already been detected as lost.
In this case the original frame ->pkt member is NULL.
Add a trace to distinguish these cases.

Thank you to Tristan for having reported this issue in GH #1808.

Must be backported to 2.6.
2022-08-27 10:29:30 +02:00
William Lallemand
b5c2cd461d DOC: configuration.txt: do-resolve must use host_only to remove its port.
The do-resolve action does not support a port in its parameter string,
the host_only converter must be used.

Must be backported to 2.6.
2022-08-26 17:00:22 +02:00
William Lallemand
d78dfe7891 BUG/MINOR: httpclient: fix resolution with port
Fix the resolution in the httpclient when a port is associated to a
domain. The do-resolve action doesn't support a port in its input.

Must be backported to 2.6. Require the "host_only" converter to be
backported.
2022-08-26 17:00:22 +02:00
William Lallemand
dd754cba16 MINOR: sample: add the host_only and port_only converters
Add 2 converters that can manipulate the value of an Host header.
host_only will return the host without any port, and port_only will
return the port.
2022-08-26 17:00:22 +02:00
William Lallemand
1ef2460934 DOC: configuration: do-resolve doesn't work with a port in the string
Fix the documentation about do-resolve to handle the case where a port
is associated to the hostname in the Host header.

Must be backported as far as 2.0.
2022-08-26 16:51:05 +02:00
Frédéric Lécaille
eba9088a7c Revert "MINOR: quic: Remove useless traces about references to TX packets"
This reverts commit f61398a7ca.
After having checked a version with more traces and reproduced the issue
as reported by Tristan in GH #1808, there are remaining cases where
a duplicated but not already sent frame have to be marked as acked because
the frame it was copied from was acknowledeged before its copied was sent.

Must be backported to 2.6.
2022-08-25 16:06:48 +02:00
Frédéric Lécaille
f61398a7ca MINOR: quic: Remove useless traces about references to TX packets
Since this commit:
    "BUG/MINOR: quic: Wrong list_for_each_entry() use when building packets from
     qc_do_build_pkt()"
there is no more reason that frames can be released without having been
sent, i.e. frames with non null ->pkt member. This ->pkt is the packet
the frame is attached to.

Must be backported to 2.6.
2022-08-25 07:35:47 +02:00
Frédéric Lécaille
560ddfa003 CLEANUP: quic: Remove a useless check in qc_lstnr_pkt_rcv()
This function parses the QUIC packet from a UDP datagram. It was originally
supposed to be run by several thread. Here we remove a section of code
where the current thread checks there is not another thread which has already
inserted the new quic_conn it is trying to insert in the connections tree.

Must be backported to 2.6 to ease the future backports to come.
2022-08-24 18:59:23 +02:00
Frédéric Lécaille
f34c1c9568 CLEANUP: quic: No more use ->rx_list MT_LIST entry point (quic_rx_packet)
This quic_rx_packet is definitively no more used.

Should be backported to 2.6 to ease the future backports.
2022-08-24 18:17:13 +02:00
Frédéric Lécaille
15773f2101 BUG/MINOR: quic: Stalled connections (missing I/O handler wakeup)
This was due to a missing I/O handler tasklet wakeup in process_timer() when
detecting packet loss. As, qc_release_lost_pkts() could remove the lost packets
from the in flight packets count, qc_set_timer() could cancel the timer used
to wakeup the connection I/O handler. Then the connection could remain idle
until it ends.

Must be backported to 2.6.
2022-08-24 18:13:30 +02:00
Frédéric Lécaille
277c4629e7 BUG/MINOR: quic: Leak in qc_release_lost_pkts() for non in flight TX packets
Packets with null "in flight" lengths are kept as the others packets as sent
but not already acknowledeged in the by packet number space trees.
But qc_release_lost_pkts() relied on this in fligh length to release the
memory allocated for this packets. We must release the memory allocated for
all the lost packets regardless of their in fligh lengths.

Modify this function to do nothing if the list of lost packets passed
as argument is empty. Stop using <lost_bytes> variable to decide if some packets
memory must be released or not.
Modify the callers to stop checking if this list is empty.

Should helping in fixing memory leak as reported by Tristan in GH #1801.

Must be backported to 2.6.
2022-08-24 18:13:30 +02:00
Frédéric Lécaille
5f6c25e447 Revert "BUG/MINOR: quix: Memleak for non in flight TX packets"
This reverts commit da9c441886.

Indeed this commit prevented the ACK only packets to be used as other packets
when they are acknowledged. Even if not ack-eliciting packets they are
acknowledged alongside others packets. Such acknowledged ACK only packets
must be used for instance to compute the RTT.

Must be backported to 2.6 if da9c441 was backported to 2.6.
2022-08-24 18:12:59 +02:00
William Lallemand
b10b1196b8 MINOR: resolvers: shut the warning when "default" resolvers is implicit
Shut the connect() warning of resolvers_finalize_config() when the
configuration was not emitted manually.

This shuts the warning for the "default" resolvers which is created
automatically for the httpclient.

Must be backported in 2.6.
2022-08-24 14:56:42 +02:00
Christopher Faulet
529b6a3a2c REGTESTS: Fix prometheus script to perform HTTP health-checks
TCP Health-checks are enabled on server "s2". However it expects to receive
an HTTP requests. So HAProxy configuration must be changed to perform HTTP
health-checks instead. Otherwise, depending on the timing, an error can be
triggered if a check is performed before the end of the script.

This scripts never failed because TCP_QUICKACK was disabled, adding some
latency on health-checks. But since the last fix, it is an issue.

This patch should be backported as far as 2.4.
2022-08-24 12:17:34 +02:00
Christopher Faulet
871dd82117 BUG/MINOR: tcpcheck: Disable QUICKACK only if data should be sent after connect
It is only a real problem for agent-checks when there is no agent string to
send. The condition to disable TCP_QUICKACK was only based on the action
type following the connect one. But it is not always accurate. indeed, for
agent-checks, there is always a SEND action. But if there is no "agent-send"
string defined, nothing is sent. In this case, this adds 200ms of latency
with no reason.

To fix the bug, a flag is now used on the CONNECT action to instruct there
are data that should be sent after the connect. For health-checks, this flag
is set if the action following the connect is a SEND action. For
agent-checks, it is set if an "agent-send" string is defined.

This patch should fix the issue #1836. It must be backported as far as 2.2.
2022-08-24 11:59:04 +02:00
William Lallemand
6020c4e44e BUG/MINOR: mworker: does not create the "default" resolvers in wait mode
When doing a re-exec, the master was creating a "default" resolvers,
which could result in a warning emitted because the "default" resolvers
of the configuration file is not available anymore.

Skip the creating of the "default" resolvers in wait mode, this is not
useful in the master.

Must be backported as far as 2.6.
2022-08-24 11:28:29 +02:00
William Lallemand
866b88bc95 BUG/MINOR: resolvers: return the correct value in resolvers_finalize_config()
Patch c31577f ("MEDIUM: resolvers: continue startup if network is
unavailable") was not working correctly. Indeed
resolvers_finalize_config() was returning a ERR type, but a postparser
is supposed to return 0 or 1.

The return value was never right, however it was only a problem since c31577f.

Must be backported in every stable branch.
2022-08-24 10:11:17 +02:00
Brad Smith
02fd3caa8f BUILD: tcp_sample: fix build of get_tcp_info() on OpenBSD
The build on OpenBSD is broken since commit 5c83e3a15 ("MINOR: tcp_sample:
clarifying samples support per os, for further expansion."), hence it
only affects 2.7 and 2.6.

It looks like this changed things in such a way that if TCP_INFO is added
but the OS is not added to the list of OS's it will not build.

Extend support for get_tcp_info to OpenBSD.

This must be backported to 2.6.
2022-08-24 05:23:13 +02:00
Willy Tarreau
8bd146d8af MEDIUM: peers: limit the number of updates sent at once
As seen in GH issue #1770, peers synchronization do not cope well with
very large buffers because by default the only two reasons for stopping
the processing of updates is either that the end was reached or that
the buffer is full. This can cause high latencies, and even rightfully
trigger the watchdog when the operations are numerous and slowed down
by competition on the stick-table lock.

This patch introduces a limit to the number of messages one may send
at once, which now defaults to 200, regardless of the buffer size. This
means taking and releasing the lock up to 400 times in a row, which is
costly enough to let some other parts work.

After some observation this could be backported to 2.6. If so, however,
previous commits "BUG/MEDIUM: applet: fix incorrect check for abnormal
return condition from handler" and "BUG/MINOR: applet: make the call_rate
only count the no-progress calls" must be backported otherwise the call
rate might trigger the looping protection.
2022-08-23 20:19:11 +02:00
Willy Tarreau
df3cab1ca1 BUG/MINOR: applet: make the call_rate only count the no-progress calls
This is very similar to what we did in commit 6c539c4b8 ("BUG/MINOR:
stream: make the call_rate only count the no-progress calls"), it's
better to only count the call rate with no progress than to count all
calls and try to figure if there's no progress, because a fast running
applet might once satisfy the whole condition and trigger the bug. This
typically happens when artificially limiting the number of messages sent
at once by an applet, but could happen with plenty of highly interactive
applets.

This patch could be backported to stable versions if there are any
indications that it might be useful there.
2022-08-23 20:19:11 +02:00
Willy Tarreau
8a3f58280f BUG/MEDIUM: applet: fix incorrect check for abnormal return condition from handler
We have quite numerous checks for abnormal applet handler behavior which
are supposed to trigger the loop protection. However, consecutive to
commit 15252cd9c ("MEDIUM: stconn: move the RXBLK flags to the stream
connector") that was merged into 2.6-dev12, one flag was incorrectly
renamed, and the check for an applet waiting for a buffer that is present
mistakenly turned to a check for missing room in the buffer. This erroneous
test could mistakenly trigger on applets that perform intensive I/Os doing
small exchanges each (e.g. cache, peers or HTTP client) if the load would
be sustained (>100k iops). For the cache this could represent higher than
13 Gbps on an object at least 1.6 GB large for example, which is quite
unlikely but theoretically possible.

This fix needs to be backported to 2.6.
2022-08-23 20:19:11 +02:00
Frédéric Lécaille
a2d8ad20a3 MINOR: quic: Replace MT_LISTs by LISTs for RX packets.
Replace ->rx.pqpkts quic_enc_level struct member MT_LIST by an LIST.
Same thing for ->list quic_rx_packet struct member MT_LIST.
Update the code consequently. This was a reminisence of the multithreading
support (several threads by connection).

Must be backported to 2.6
2022-08-23 17:55:02 +02:00
Frédéric Lécaille
b8047de11a BUG/MINOR: quic: Safer QUIC frame builders
Do not rely on the fact the callers of qc_build_frm() handle their
buffer passed to function the correct way (without leaving garbage).
Make qc_build_frm() update the buffer passed as argument only if
the frame it builds is well formed.

As far as I sse, there is no such callers which does not handle
carefully such buffers.

Must be backported to 2.6.
2022-08-23 17:40:09 +02:00
Frédéric Lécaille
a8a6043240 BUG/MINOR: quic: Wrong list_for_each_entry() use when building packets from qc_do_build_pkt()
This is list_for_each_entry_safe() which must be used if we want to delete elements
inside its code block. This could explain that some frames which were not built were added
to packets with a NULL ->pkt member.

Thank you to Tristan for having reported this issue through backtraces in GH #1808

Must be backported to 2.6.
2022-08-23 12:06:40 +02:00
Frédéric Lécaille
da9c441886 BUG/MINOR: quix: Memleak for non in flight TX packets
First, these packets must not be inserted in the tree of TX packets.
They are never explicitely acknowledged (for instance an ACK only
packet will never be acknowledged). Furthermore, if taken into an account
these packets may uselessly disturb the congestion control. We do not care
if they are lost or not. Furthermore as the ->in_fligh_len member value is null
they were not released by qc_release_lost_pkts() which rely on these values
to decide to release the allocated memory for such packets.

Must be backported to 2.6.
2022-08-22 19:06:08 +02:00
William Lallemand
16972e19d4 REGTESTS: launch http_reuse_always in mworker mode
We don't have enough tests with the mworker mode, and even less that
have no master CLI (-S) configured. Let's run this one with -W, it
shouldn't have any impact.

VTest won't be able to catch a lot of things for now, but that's a first
step.
2022-08-22 13:09:40 +02:00