haproxy-3.0.git
7 months agoBUG/MEDIUM: thread: use pthread_self() not ha_pthread[tid] in set_affinity
Willy Tarreau [Wed, 12 Mar 2025 14:54:36 +0000 (15:54 +0100)]
BUG/MEDIUM: thread: use pthread_self() not ha_pthread[tid] in set_affinity

A bug was uncovered by the work on NUMA. It only triggers in the CI
with libmusl due to a race condition. What happens is that the call
to set_thread_cpu_affinity() is done very early in the polling loop,
and that it relies on ha_pthread[tid] instead of pthread_self(). The
problem is that ha_pthread[tid] is only set by the return from
pthread_create(), which might happen later depending on the number of
CPUs available to run the starting thread.

Let's just use pthread_self() here. ha_pthread[] is only used to send
signals between threads, there's no point in using it here.

This can be backported to 2.6.

(cherry picked from commit 12383fd9f5b3614dbffec6260b82659c3c5fd0df)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 77494a9104e6d3ad5d8ca24f2fb343ab2e4b1c13)
Signed-off-by: Willy Tarreau <w@1wt.eu>

7 months agoMINOR: startup: adjust alert messages, when capabilities are missed
Valentine Krasnobaeva [Fri, 7 Mar 2025 12:42:27 +0000 (13:42 +0100)]
MINOR: startup: adjust alert messages, when capabilities are missed

CAP_SYS_ADMIN support was added, in order to access sockets in namespaces. So
let's adjust the alert at startup, where we check preserved capabilities from
global.last_checks. Let's mention here cap_sys_admin as well.

(cherry picked from commit 7d427134fe01e8af56dfa48c6d9e6ecc5defe562)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 77b340e17d3da9fc78ed113ebe98b2101c10994c)
Signed-off-by: Willy Tarreau <w@1wt.eu>

7 months agoBUG/MINOR: cfgparse-tcp: relax namespace bind check
Damien Claisse [Fri, 20 Dec 2024 13:36:34 +0000 (13:36 +0000)]
BUG/MINOR: cfgparse-tcp: relax namespace bind check

Commit 5cbb278 introduced cap_sys_admin support, and enforced checks for
both binds and servers. However, when binding into a namespace, the bind
is done before dropping privileges. Hence, checking that we have
cap_sys_admin capability set in this case is not needed (and it would
decrease security to add it).
For users starting haproxy with other user than root and without
cap_sys_admin, bind should have already failed.
As a consequence, relax runtime check for binds into a namespace.

(cherry picked from commit f0a07f834c001c5b505e84b0f0b103e530e87d1b)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 42086a672888f569e009149130fad7b19b3fd13d)
Signed-off-by: Willy Tarreau <w@1wt.eu>

7 months agoBUG/MINOR: stream: fix age calculation in "show sess" output
Willy Tarreau [Thu, 6 Mar 2025 17:56:13 +0000 (18:56 +0100)]
BUG/MINOR: stream: fix age calculation in "show sess" output

The "show sess" output reports an age that's based on the last byte of
the HTTP request instead of the stream creation date, due to a confusion
between logs->request_ts and the request_date sample fetch function. Most
of the time these are equal except when the request is not yet full for
any reason (e.g. wait-body). This explains why a few "show sess" could
report a few new streams aged by 99 days for example.

Let's perform the correct request timestamp calculation like the sample
fetch function does, by adding t_idle and t_handshake to the accept_ts.
Now the stream's age is correct and can be correctly used with the
"show sess older <age>" variant.

This issue was introduced in 2.9 and the fix can be backported to 3.0.

(cherry picked from commit 1cdf2869f6757946546a2ef102ce822e95de78f8)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 17085371afad2728713a3cc409ce37f2137b27d8)
Signed-off-by: Willy Tarreau <w@1wt.eu>

7 months agoCI: github: fix h2spec.config proxy names
William Lallemand [Tue, 4 Mar 2025 10:44:03 +0000 (11:44 +0100)]
CI: github: fix h2spec.config proxy names

h2spec.config config file emitted a warning because the frontend name
has the same name as the backend.

(cherry picked from commit 588237ca6e6624f7c1162289a6a00cab3f10ac61)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 9368b370eace1e65497b79d0df05a405f25156ea)
Signed-off-by: Willy Tarreau <w@1wt.eu>

7 months agoTESTS: ist: fix wrong array size
William Lallemand [Tue, 4 Mar 2025 09:47:08 +0000 (10:47 +0100)]
TESTS: ist: fix wrong array size

test_istzero() and test_istpad() has the wrong array size buf[] which
lacks the space for the '\0';

Could be backported in every stable branches.

(cherry picked from commit ddd2c82a3521ad61398d24fa10f7483cd6518de8)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit df98c512bc927cd5c6a061a93f11bb01aceb265e)
Signed-off-by: Willy Tarreau <w@1wt.eu>

7 months agoBUG/MEDIUM: server: properly initialize PROXY v2 TLVs
Dragan Dosen [Wed, 26 Feb 2025 18:13:31 +0000 (19:13 +0100)]
BUG/MEDIUM: server: properly initialize PROXY v2 TLVs

The PROXY v2 TLVs were not properly initialized when defined with
"set-proxy-v2-tlv-fmt" keyword, which could have caused a crash when
validating the configuration or malfunction (e.g. when used in
combination with "server-template" and/or "default-server").

The issue was introduced with commit 6f4bfed3a ("MINOR: server: Add
parser support for set-proxy-v2-tlv-fmt").

This should be backported up to 2.9.

(cherry picked from commit 6838fe43a320cf090892451ee907967666b626e5)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 77b48b2ae0dba907115c304ea78187a0f77e60c0)
Signed-off-by: Willy Tarreau <w@1wt.eu>

7 months agoDOC: option redispatch should mention persist options
Lukas Tribus [Wed, 5 Feb 2025 07:42:15 +0000 (07:42 +0000)]
DOC: option redispatch should mention persist options

"option redispatch" remains vague in which cases a session would persist;
let's mention "option persist" and "force-persist" as an example so folks
don't draw the conclusion that this may be default.

Should be backported to stable branches.

(cherry picked from commit 5926fb78233ab4ba95b07db3815ee62f5cbc5082)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 6d1a374721b2813a2ad61bf0e88e0e58a2a0c8b0)
Signed-off-by: Willy Tarreau <w@1wt.eu>

7 months agoDOC: htx: clarify <mark> parameter for htx_xfer_blks()
William Lallemand [Fri, 31 Jan 2025 14:23:47 +0000 (15:23 +0100)]
DOC: htx: clarify <mark> parameter for htx_xfer_blks()

Clarify the fact that the first <mark> block is transferred before
stopping when using htx_xfer_blks()

(cherry picked from commit c17e02923256fef61f74b986045b7eb8d1e126c7)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 338edaa1e9aeb0ee87a58fc9d62e75548f11566f)
Signed-off-by: Willy Tarreau <w@1wt.eu>

7 months agoBUG/MEDIUM: htx: wrong count computation in htx_xfer_blks()
William Lallemand [Fri, 31 Jan 2025 13:41:28 +0000 (14:41 +0100)]
BUG/MEDIUM: htx: wrong count computation in htx_xfer_blks()

When transfering blocks from an src to another dst htx representation,
htx_xfer_blks() decreases the size of each block removed from the <count>
value passed in parameter, so it can't transfer more than <count>. The
size must also contains the metadata, represented by a simple
sizeof(struct htk_blk).

However, the code was doing a sizeof(dstblk) instead of a
sizeof(*dstblk) which as the consequence of removing only a size_t from
count. Fortunately htx_blk size is 64bits, so that does not provoke any
problem in 64bits. But on 32bits architecture, the count value is not
decreased correctly and the function could try to transfer more blocks
than allowed by the count parameter.

Must be backported in every stable release.

(cherry picked from commit c6390cdf9ce4e74540544207d6e3cfb31581eaea)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 3c47adcfdfbf1ba9231059773865df036ced8bea)
Signed-off-by: Willy Tarreau <w@1wt.eu>

7 months agoREGTESTS: Fix truncated.vtc to send 0-CRLF
Christopher Faulet [Tue, 18 Feb 2025 06:31:51 +0000 (07:31 +0100)]
REGTESTS: Fix truncated.vtc to send 0-CRLF

When a chunked messages is sent, the 0-CRLF must be explicitely sent. Since
the begining, it is missing. Just add it.

(cherry picked from commit b93e419750aa8921e76e1e58c57a9bf8eefea969)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit e08ed2d43e2871f9e9d696d35f61c6352703a4b6)
Signed-off-by: Willy Tarreau <w@1wt.eu>

7 months agoBUG/MEDIUM: fd: mark FD transferred to another process as FD_CLONED
Willy Tarreau [Wed, 12 Feb 2025 15:25:13 +0000 (16:25 +0100)]
BUG/MEDIUM: fd: mark FD transferred to another process as FD_CLONED

The crappy epoll API stroke again with reloads and transferred FDs.
Indeed, when listening sockets are retrieved by a new worker from a
previous one, and the old one finally stops listening on them, it
closes the FDs. But in this case, since the sockets themselves were
not closed, epoll will not unregister them and will continue to
report new activity for these in the old process, which can only
observe, count an fd_poll_drop event and not unregister them since
they're not reachable anymore.

The unfortunate effect is that long-lasting old processes are woken
up at the same rate as the new process when accepting new connections,
and can waste a lot of CPU. Accept rates divided by 8 were observed on
a small test involving a slow transfer on 10 connections facing a reload
every second so that 10 processes were busy dealing with them while
another process was hammering the service with new connections.

Fortunately, years ago we implemented a flag FD_CLONED exactly for
similar purposes. Let's simply mark transferred FDs with FD_CLONED
so that the process knows that these ones require special treatment
and have to be manually unregistered before being closed. This does
the job fine, now old processes correctly unregister the FD before
closing it and no longer receive accept events for the new process.

This needs to be backported to all stable versions. It only affects
epoll, as usual, and this time in combination with transferred FDs
(typically reloads in master-worker mode). Thanks to Damien Claisse
for providing all detailed measurements and statistics allowing to
understand and reproduce the problem.

(cherry picked from commit 561319bd1ccaab5dd69bac0c19387670b02d484f)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 12b7be07d9437639ee503873647cbf85d635dad4)
Signed-off-by: Willy Tarreau <w@1wt.eu>

8 months agoBUG/MEDIUM: spoe: Don't wakeup idle applets in loop during stopping
Christopher Faulet [Wed, 19 Feb 2025 10:39:59 +0000 (11:39 +0100)]
BUG/MEDIUM: spoe: Don't wakeup idle applets in loop during stopping

When HAproxy is stopping, idle applets are systematically woken up, to have a
change to close them. This was performed to properly handle the applets switched
to idle state after processing in synchronous mode. However, there was an issue
in async and pipelining modes. Applets remained idles while they had some free
slots to process messages. So these applets were not closed immediately and were
woken up in loop.

Now, we also take care to check that idle applets are not processing messages to
wake them up. The test is also moved after a processing attempt because it is
the right place to do so. There is no reason to perform this test systematically
at the end of the I/O handler.

This patch should be backported to all stable versions.

8 months agoBUG/MINOR: spoe: Allow applet creation when closing the last one during stopping
Christopher Faulet [Wed, 19 Feb 2025 10:33:02 +0000 (11:33 +0100)]
BUG/MINOR: spoe: Allow applet creation when closing the last one during stopping

When the last SPOE applet is closed, for any reason, if there are still some
pending messages, in the shared sending queueu or the shared waiting queue, a
new applet is automatically created to be able to pocess these messages. Howver,
this was not performed when HAProxy was stopping. But it was an error because it
was then possible to abort some messages and reported them on error with the
status code set to 256.

So, now, it is still possible to create applet, if necessary, when HAProxy is
stopping.

This patch should partially fix the issue #2868. It must be backported to all
stable versions.

8 months agoBUG/MINOR: spoe: Check the shared waiting queue to shut applets during stopping
Christopher Faulet [Wed, 19 Feb 2025 09:17:30 +0000 (10:17 +0100)]
BUG/MINOR: spoe: Check the shared waiting queue to shut applets during stopping

When HAProxy is stopped, we should take care to process all pending messages
and wait for ACK for already sent messages. However, the shared waiting
queue, used in async mode, was not checked. So it was possible to close some
applets too early and some messages could be reported on error, with the
status code set to 256.

So now, in stopping mode, before closing a SPOE applet, we check the shared
sending queue, the applet waiting queue and the shared waiting queue. If all
these queues are empty, the applet is closed.

The issue is minor. However, a simple workaround is to disable the async
mode by adding "no option async" in the spoe-agent section.

This patch should partially fix the issue #2868. It must be backported to
all stable versions.

8 months agoBUG/MINOR: mux-quic: handle closure of uni-stream
Amaury Denoyelle [Fri, 3 Jan 2025 15:25:14 +0000 (16:25 +0100)]
BUG/MINOR: mux-quic: handle closure of uni-stream

This commit is a direct follow-up to the previous one. As already
described, a previous fix was merged to prevent streamdesc attach
operation on already completed QCS instances scheduled for purging. This
was implemented by skipping app proto decoding.

However, this has a bad side-effect for remote uni-directional stream.
If receiving a FIN stream frame on such a stream, it will considered as
complete because streamdesc are never attached to a uni stream. Due to
the mentionned new fix, this prevent analysis of this last frame for
every uni stream.

To fix this, do not skip anymore app proto decoding for completed QCS.
Update instead qcs_attach_sc() to transform it as a noop function if QCS
is already fully closed before streamdesc instantiation. However,
success return value is still used to prevent an invalid decoding error
report.

The impact of this bug should be minor. Indeed, HTTP3 and QPACK uni
streams are never closed by the client as this is invalid due to the
spec. The only issue was that this prevented QUIC MUX to close the
connection with error H3_ERR_CLOSED_CRITICAL_STREAM.

This must be backported along the previous patch, at least to 3.1, and
eventually to 2.8 if mentionned patches are merged there.

(cherry picked from commit 801e39e1ccfc7404354d6870e779f83351593df0)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 85e3bf658163d97fd09ac63f6c997d230d32729b)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

8 months agoMINOR: mux-quic: change return value of qcs_attach_sc()
Amaury Denoyelle [Fri, 3 Jan 2025 15:16:45 +0000 (16:16 +0100)]
MINOR: mux-quic: change return value of qcs_attach_sc()

A recent fix was introduced to ensure that a streamdesc instance won't
be attached to an already completed QCS which is eligible to purging.
This was performed by skipping application protocol decoding if a QCS is
in such a state. Here is the patch responsible for this change.
  caf60ac696a29799631a76beb16d0072f65eef12
  BUG/MEDIUM: mux-quic: do not attach on already closed stream

However, this is too restrictive, in particular for unidirection stream
where no streamdesc is never attached. To fix this behavior, first
qcs_attach_sc() API has been modified. Instead of returning a streamdesc
instance, it returns either 0 on success or a negative error code.

There should be no functional changes with this patch. It is only to be
able to extend qcs_attach_sc() with the possibility of skipping
streamdesc instantiation while still keeping a success return value.

This should be backported wherever the above patch has been merged. For
the record, it was scheduled for immediate backport on 3.1, plus merging
on older releases up to 2.8 after a period of observation.

(cherry picked from commit af00be8e0f7fe3942fac93ac32bfe7ae7f5b78c9)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 1bcdf3c5f2528910930209dbbcfff2556d44c45a)
[cf: ctx adjt]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

8 months agoBUG/MEDIUM: mux-quic: do not attach on already closed stream
Amaury Denoyelle [Tue, 31 Dec 2024 15:06:32 +0000 (16:06 +0100)]
BUG/MEDIUM: mux-quic: do not attach on already closed stream

Due to QUIC packet reordering, a stream may be opened via a new
RESET_STREAM or STOP_SENDING frame. This would cause either Tx or Rx
channel to be immediately closed.

This can cause an issue with current QUIC MUX implementation with QCS
purging. QCS are inserted into QCC purge list when transfer could be
considered as completed. In most cases, this happens after full
request/response exchange. However, it can also happens after request
reception if RESET_STREAM/STOP_SENDING are received first.

A BUG_ON() crash will occur if a STREAM frame is received after. In this
case, streamdesc instance will be attached via qcs_attach_sc() to handle
the new request. However, QCS is already considered eligible to purging.
It could cause it to be released while its streamdesc instance remains.
A BUG_ON() crash detects this problem in qcc_purge_streams().

To fix this, extend qcc_decode_qcs() to skip app proto rcv_buf
invokation if QCS is considered completed. A similar condition was
already implemented when read was previously aborted after a
STOP_SENDING emission by QUIC MUX.

This crash was reproduced on haproxy.org. Here is the output of the
backtrace :
Core was generated by `./haproxy-dev -db -f /etc/haproxy/haproxy-current.cfg -sf 16495'.
Program terminated with signal SIGILL, Illegal instruction.
 #0  0x00000000004e442b in qcc_purge_streams (qcc=0x774cca0) at src/mux_quic.c:2661
2661                    BUG_ON_HOT(!qcs_is_completed(qcs));
[Current thread is 1 (LWP 1457)]
[ ## gdb ## ] bt
 #0  0x00000000004e442b in qcc_purge_streams (qcc=0x774cca0) at src/mux_quic.c:2661
 #1  0x00000000004e4db7 in qcc_io_process (qcc=0x774cca0) at src/mux_quic.c:2744
 #2  0x00000000004e5a54 in qcc_io_cb (t=0x7f71193940c0, ctx=0x774cca0, status=573504) at src/mux_quic.c:2886
 #3  0x0000000000b4f792 in run_tasks_from_lists (budgets=0x7ffdcea1e670) at src/task.c:603
 #4  0x0000000000b5012f in process_runnable_tasks () at src/task.c:883
 #5  0x00000000007de4a3 in run_poll_loop () at src/haproxy.c:2771
 #6  0x00000000007deb9f in run_thread_poll_loop (data=0x1335a00 <ha_thread_info>) at src/haproxy.c:2985
 #7  0x00000000007dfd8d in main (argc=6, argv=0x7ffdcea1e958) at src/haproxy.c:3570

This BUG_ON() crash can only happen since 3.1 refactoring. Indeed, purge
list was only implemented on this version. As such, please backport it
on 3.1 immediately. However, a logic issue remains for older version as
a stream could be attached on a fully closed QCS. Thus, it should be
backported up to 2.8, this time after a period of observation.

(cherry picked from commit caf60ac696a29799631a76beb16d0072f65eef12)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit ae6e84d0b238ddf91b53c86fafc047d11a93f93d)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

8 months agoBUG/MEDIUM: ssl: chosing correct certificate using RSA-PSS with TLSv1.3
William Lallemand [Fri, 7 Feb 2025 19:28:39 +0000 (20:28 +0100)]
BUG/MEDIUM: ssl: chosing correct certificate using RSA-PSS with TLSv1.3

The clienthello callback was written when TLSv1.3 was not yet out, and
signatures algorithm changed since then.

With TLSv1.2, the least significant byte was used to determine the
SignatureAlgorithm, which could be rsa(1), dsa(2), ecdsa(3).
https://datatracker.ietf.org/doc/html/rfc5246#section-7.4.1.4.1

This was used to chose which type of certificate to push to the client.

But TLSv1.3 changed that, and introduced new RSA-PSS algorithms that
does not have the least sinificant byte to 1.
https://datatracker.ietf.org/doc/html/rfc8446#section-4.2.3

This would result in chosing the wrong certificate when an RSA an ECDSA
ones are in the configuration for the same SNI or default entry.

This patch fixes the issue by parsing bothe hash and signature field to
check the RSA-PSS signature scheme.

This must fix issue #2852.

This must be backported in every stable versions. The code was moved
from ssl_sock.c to ssl_clienthello in recent versions.

(cherry picked from commit 64502715968fb12f446fcb6432a5431d803d8935)
[wla: moved the code in ssl_sock.c]
Signed-off-by: William Lallemand <wlallemand@haproxy.com>

8 months ago[RELEASE] Released version 3.0.8 v3.0.8
Christopher Faulet [Wed, 29 Jan 2025 14:38:32 +0000 (15:38 +0100)]
[RELEASE] Released version 3.0.8

Released version 3.0.8 with the following main changes :
    - BUG/MEDIUM: stconn: Don't forward shut for SC in connecting state
    - BUG/MINOR: stats: decrement srv refcount on stats-file release
    - MINOR: list: define a watcher type
    - BUG/MEDIUM: stats/server: use watcher to track server during stats dump
    - BUG/MEDIUM: clock: make sure now_ms cannot be TICK_ETERNITY
    - BUG/MINOR: cli: cli_snd_buf: preserve \r\n for payload lines
    - REGTESTS: ssl: add a PEM with mix of LF and CRLF line endings
    - BUG/MEDIUM: stconn: Only consider I/O timers to update stream's expiration date
    - BUG/MEDIUM: queues: Make sure we call process_srv_queue() when leaving
    - BUG/MEDIUM: queues: Do not use pendconn_grab_from_px().
    - DOC: config: add example for server "track" keyword
    - DOC: config: reorder "tune.lua.*" keywords by alphabetical order
    - DOC: config: add "tune.lua.burst-timeout" to the list of global parameters
    - BUG/MINOR: h2/rhttp: fix HTTP2 conn counters on reverse
    - BUG/MEDIUM: queue: Make process_srv_queue return the number of streams
    - BUG/MINOR: stats: fix segfault caused by uninitialized value in "show schema json"
    - DOC: config: add missing "track-sc0" in action keywords matrix
    - MINOR: config: Alert about extra arguments for errorfile and errorloc
    - BUG/MEDIUM: promex/resolvers: Don't dump metrics if no nameserver is defined
    - BUG/MEDIUM: h1-htx: Properly handle bodyless messages
    - MINOR: stconn: add a new pair of sf functions {bs,fs}.debug_str
    - MINOR: mux-h2: implement the debug string for logs
    - MINOR: mux-quic: define dump functions for QCC and QCS
    - MINOR: mux-quic: implement debug string for logs
    - MINOR: quic: dump quic_conn debug string for logs
    - MINOR: time: define tot_time structure
    - MINOR: mux-quic: measure QCS lifetime and its blocking state
    - MINOR: mux-h1: Add support of the debug string for logs
    - MINOR: sample: add the "when" converter to condition some expressions
    - MINOR: arg: add an argument type for identifier
    - MINOR: acl: export find_acl_default()
    - MINOR: sample: extend the "when" converter to support an ACL
    - CLEANUP: debug: make the BUG_ON() macros check the condition in the outer one
    - MEDIUM: debug: add match counters for BUG_ON/WARN_ON/CHECK_IF
    - MINOR: debug: add a new debug macro COUNT_IF()
    - MINOR: debug: add "debug dev counters" to list code counters
    - MINOR: debug/cli: replace "debug dev counters" with "debug counters"
    - DEV: sock: Add a debug counter to track strange flag on fd during connect()
    - MINOR: activity/memprofile: also monitor strdup() activity
    - MINOR: activity/memprofile: monitor non-portable calls as well
    - BUILD: activity/memprofile: fix a build warning in the posix_memalign handler
    - BUG/MINOR: stktable: fix big-endian compatiblity in smp_to_stkey()
    - BUG/MINOR: quic: reject NEW_TOKEN frames from clients
    - BUG/MEDIUM: stktable: fix missing lock on some table converters
    - BUG/MEDIUM: promex: Use right context pointers to dump backends extra-counters
    - BUG/MAJOR: quic: reject too large CRYPTO frames
    - BUG/MAJOR: log/sink: possible sink collision in sink_new_from_srv()
    - BUG/MINOR: init: set HAPROXY_STARTUP_VERSION from the variable, not the macro
    - BUG/MINOR: quic: ensure a detached coalesced packet can't access its neighbours
    - MINOR: quic: Add a BUG_ON() on quic_tx_packet refcount
    - BUILD: quic: Move an ASSUME_NONNULL() for variable which is not null
    - BUG/MEDIUM: mux-h1: Properly close H1C if an error is reported before sending data
    - BUG/MINOR: quic: do not increase congestion window if app limited
    - BUG/MINOR: ssl: put ssl_sock_load_ca under SSL_NO_GENERATE_CERTIFICATES
    - BUG/MINOR: stream: Properly handle "on-marked-up shutdown-backup-sessions"

8 months agoBUG/MINOR: stream: Properly handle "on-marked-up shutdown-backup-sessions"
Christopher Faulet [Mon, 27 Jan 2025 15:17:27 +0000 (16:17 +0100)]
BUG/MINOR: stream: Properly handle "on-marked-up shutdown-backup-sessions"

shutdown-backup-sessions action for on-marked-up directive does not work anymore
since the stream_shutdown() function was modified to be async-safe.

When stream_shutdown() was modified to be async-safe, dedicated task events were
added to map the reasons to shut a stream down. SF_ERR_DOWN was mapped to
TASK_F_EVT1 and SF_ERR_KILLED was mapped to TASK_F_EVT2. The reverse mapping was
performed by process_stream() to shut the stream with the appropriate reason.

However, SF_ERR_UP reason, used by shutdown-backup-sessions action to shut a
stream down because a preferred server became available, was not mapped in the
same way. So since commit b8e3b0a18d ("BUG/MEDIUM: stream: make
stream_shutdown() async-safe"), this action is ignored and does not work
anymore.

To fix an issue, and being able to bakcport the fix, a third task event was
added. TASK_F_EVT3 is now mapped on SF_ERR_UP.

This patch should fix the issue #2848. It must be backported as far as 2.6.

(cherry picked from commit 0a52a75ef7151a187b42cae6d49a75b79380f211)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 07a8dd887f412078842e16b016fbd8dd61063306)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

8 months agoBUG/MINOR: ssl: put ssl_sock_load_ca under SSL_NO_GENERATE_CERTIFICATES
Valentine Krasnobaeva [Thu, 23 Jan 2025 12:46:46 +0000 (13:46 +0100)]
BUG/MINOR: ssl: put ssl_sock_load_ca under SSL_NO_GENERATE_CERTIFICATES

ssl_sock_load_ca and ssl_sock_free_ca definitions are compiled only, if
SSL_NO_GENERATE_CERTIFICATES is not set. In case, when we set this define and
build haproxy, linker throws an error. So, let's fix this.

This should be backported in all stable versions.

(cherry picked from commit c987f30245023f3bdf4dbe5296ed39f2d8faa98b)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 5d3f2aba73c73329bc41a167ce931e8d03e5f28b)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

8 months agoBUG/MINOR: quic: do not increase congestion window if app limited
Amaury Denoyelle [Thu, 9 Jan 2025 16:26:37 +0000 (17:26 +0100)]
BUG/MINOR: quic: do not increase congestion window if app limited

Previously, congestion window was increased any time each time a new
acknowledge was received. However, it did not take into account the
window filling level. In a network condition with negligible loss, this
will cause the window to be incremented until the maximum value (by
default 480k), even though the application does not have enough data to
fill it.

In most cases, this issue is not noticeable. However, it may lead to
excessive memory consumption when a QUIC connection is suddendly
interrupted, as in this case haproxy will fill the window with
retransmission. It even has caused OOM crash when thousands of clients
were interrupted at once on a local network benchmark.

Fix this by first checking window level prior to every incrementation
via a new helper function quic_cwnd_may_increase(). It was arbitrarily
decided that the window must be at least 50% full when the ACK is
handled prior to increment it. This value is a good compromise to keep
window in check while still allowing fast increment when needed.

Note that this patch only concerns cubic and newreno algorithm. BBR has
already its notion of application limited which ensures the window is
only incremented when necessary.

This should be backported up to 2.6.

(cherry picked from commit bbaa7aef7ba25480b842cda274e57d820680bb57)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 2596916866366099c2cce22b26387598e995b724)
[cf: ctx adjt]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

8 months agoBUG/MEDIUM: mux-h1: Properly close H1C if an error is reported before sending data
Christopher Faulet [Thu, 23 Jan 2025 09:31:41 +0000 (10:31 +0100)]
BUG/MEDIUM: mux-h1: Properly close H1C if an error is reported before sending data

It is possible to have front H1 connections waiting for the client timeout
while they should be closed because a conneciton error was reported before
sebding an error message to the client. It is not a leak because the
connections are closed when the timeout expires but it is a waste of
ressources, especially if the client timeout is high.

When an early error message must be sent to the client, if an error was
already detected, no data are sent and the output buffer is released. At
this stage, the H1 connection is in CLOSING state and it must be
released. But because of a bug, this is not performed. The client timeout is
rearmed and the H1 connection is only closed when it expires.

To fix the issue, the condition to close a H1C must also be evaluated when
an error is detected before sending data.

It is only an issue with idle client connections, because there is no H1
stream in that case and the error message is generated by the mux itself.

This patch must be backported as far as 2.8.

(cherry picked from commit b18e988e0dabbb0c382d2dfb5b8701670a386dde)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit b833cbcf08ca7c9263e3be9112de8c7478a87ace)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

8 months agoBUILD: quic: Move an ASSUME_NONNULL() for variable which is not null
Frederic Lecaille [Tue, 21 Jan 2025 15:26:42 +0000 (16:26 +0100)]
BUILD: quic: Move an ASSUME_NONNULL() for variable which is not null

Some new compilers warn that <oldest_lost> variable can be null even this cannot be
the case as mentioned by the comment about an already present ASSUME_NONNULL()
call comment as follows:

src/quic_loss.c: In function â€˜qc_release_lost_pkts’:
src/quic_loss.c:307:86: error: potential null pointer dereference [-Werror=null-dereference]
  307 |   unsigned int period = newest_lost->time_sent_ms - oldest_lost->time_sent_ms;
      |                                                     ~~~~~~~~~~~^~~~~~~~~~~~~~

Move up this ASSUME_NONNULL() statement to please these compiler.

Must be backported as far as 2.6 to easy any further backport around this code part.

(cherry picked from commit 1f099db7e2ca978b467f0d524261af1d588d1d0a)
[cf: ALREADY_CHECKED() is moved because ASSUME_NONNULL() does not exist]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 0ef4f1bdbd7de903fc770b33da20eb741a5ac0a4)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

8 months agoMINOR: quic: Add a BUG_ON() on quic_tx_packet refcount
Frederic Lecaille [Tue, 21 Jan 2025 15:12:05 +0000 (16:12 +0100)]
MINOR: quic: Add a BUG_ON() on quic_tx_packet refcount

This is definitively a bug to call quic_tx_packet_refdec() to decrement the reference
counter of a TX packet calling quic_tx_packet_refdec(), and possibly to release its
memory when it is negative or null.

This counter is incremented when a TX frm is attached to it with some allocated memory
and when the packet is inserted into a data structure, if needed (list or tree).

Should be easily backported as far as 2.6 to ease any further backport around
this code part.

(cherry picked from commit 4f38c4bfd8c07331258b4b13111a07c7b3013021)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 1264b812bc10699d912d3bedcc73b0a1ac059cf0)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

8 months agoBUG/MINOR: quic: ensure a detached coalesced packet can't access its neighbours
Frederic Lecaille [Tue, 21 Jan 2025 14:49:51 +0000 (15:49 +0100)]
BUG/MINOR: quic: ensure a detached coalesced packet can't access its neighbours

Reset ->prev and ->next fields of a coalesced TX packet to ensure it cannot access
several times its neighbours after it is supposed to be detached from them calling
quic_tx_packet_dgram_detach().

There are two cases where a packet can be coalesced to another previous built one:
this is when it is built into the same datagrame without GSO (and flagged flag with
QUIC_FL_TX_PACKET_COALESCED) or when sent from the same sendto() syscall with GOS
(not flagged with QUIC_FL_TX_PACKET_COALESCED).

This fix may be in relation with GH #2839.

Must be backported as far as 2.6.

(cherry picked from commit cb729fb64d18815fd73e48d24f0d58a830aee63c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit d3eb23c684269d22a5d9059b38018c76c4926c3b)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

8 months agoBUG/MINOR: init: set HAPROXY_STARTUP_VERSION from the variable, not the macro
Willy Tarreau [Mon, 20 Jan 2025 16:21:19 +0000 (17:21 +0100)]
BUG/MINOR: init: set HAPROXY_STARTUP_VERSION from the variable, not the macro

This environment variable was added by commit d4c0be6b20 ("MINOR: startup:
HAPROXY_STARTUP_VERSION contains the version used to start"). However, it's
set from the macro that is passed during the build process instead of being
set from the variable that's kept up to date in version.c. The difference
is visible only during debugging/bisecting because only changed files and
version.o are rebuilt, but not necessarily haproxy.o, which is where the
environment variable is set. This means that the version exposed in the
environment is not necessarily the same as the one presented in
"haproxy -v" during such debugging sessions.

This should be backported to 2.8. It has no impact at all on regularly
built binaries.

(cherry picked from commit 9e61cf67909936036e8c22be5ea340b9d717cd88)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 46086b1a51850dbd10223188a96c5fbb21c27086)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

8 months agoBUG/MAJOR: log/sink: possible sink collision in sink_new_from_srv()
Aurelien DARRAGON [Mon, 20 Jan 2025 09:42:42 +0000 (10:42 +0100)]
BUG/MAJOR: log/sink: possible sink collision in sink_new_from_srv()

sink_new_from_srv() leverages sink_new_buf() with the server id as name,
sink_new_buf() then calls __sink_new() with the provided name.

Unfortunately sink_new() is designed in such a way that it will first look
up in the list of existing sinks to check if a sink already exists with
given name, in which case the existing sink is returned. While this
behavior may be error-prone, it is actually up to the caller to ensure
that the provided name is unique if it really expects a unique sink
pointer.

Due to this bug in sink_new_from_srv(), multiple tcp servers with the same
name defined in distinct log backends would end up sharing the same sink,
which means messages sent to one of the servers would also be forwarded to
all servers with the same name across all log backend sections defined in
the config, which is obviously an issue and could even raise security
concerns.

Example:

  defaults
    log backend@log-1 local0

  backend log-1
    mode log
    server s1 127.0.0.1:514
  backend log-2
    mode log
    server s1 127.0.0.1:5114

With the above config, logs sent to log-1/s1 would also end up being sent
to log-2/s1 due to server id "s1" being used for tcp servers in distinct
log backends.

To fix the issue, we now prefix the sink ame with the backend name:
back_name/srv_id combination is known to be unique (backend name
serves as a namespace)

This bug was reported by GH user @landon-lengyel under #2846.

UDP servers (with udp@ prefix before the address) are not affected as they
don't make use of the sink facility.

As a workaround, one should manually ensure that all tcp servers across
different log backends (backend with "mode log" enabled) use unique names

This bug was introduced in e58a9b4 ("MINOR: sink: add sink_new_from_srv()
function") thus it exists since the introduction of log backends in 2.9,
which means this patch should be backported up to 2.9.

(cherry picked from commit bfa493d4be6b390c1b0915971b42cb98418f0c78)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit bd6f3f2a33e80046121fd1d01279883724409bd5)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

8 months agoBUG/MAJOR: quic: reject too large CRYPTO frames
Amaury Denoyelle [Mon, 20 Jan 2025 09:15:12 +0000 (10:15 +0100)]
BUG/MAJOR: quic: reject too large CRYPTO frames

Received CRYPTO frames are inserted in a ncbuf to handle out-of-order
reception via ncb_add(). They are stored on the position relative to the
frame offset, minus a base offset which corresponds to the in-order data
length already handled.

Previouly, no check was implemented on the frame offset value prior to
ncb_add(), which could easily trigger a crash if relative offset was too
large. Fix this by ensuring first that the frame can be stored in the
buffer before ncb_add() invokation. If this is not the case, connection
is closed with error CRYPTO_BUFFER_EXCEEDED, as required by QUIC
specification.

This should fix github issue #2842.

This must be backported up to 2.6.

(cherry picked from commit c3a4a4d1660866457ffb83699bd091621aae680d)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit a43882ff98df536e73fcd3866866e03a933d3f11)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

8 months agoBUG/MEDIUM: promex: Use right context pointers to dump backends extra-counters
Christopher Faulet [Tue, 14 Jan 2025 06:39:48 +0000 (07:39 +0100)]
BUG/MEDIUM: promex: Use right context pointers to dump backends extra-counters

When backends extra counters are dumped, the wrong pointer was used in the
promex context to retrieve the stats module. p[1] must be used instead of
p[2]. Because of this typo, a infinite loop could be experienced if the
output buffer is full during this stage. But in all cases an overflow is
possible leading to a memory corruption.

This patch may be related to issue #2831. It must be backported as far as
3.0.

(cherry picked from commit 91578212d7cef7405f3631db43a086ac9f9ca162)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit ecd012d853de2ac8996bccd6cb8968d56a86ed84)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

8 months agoBUG/MEDIUM: stktable: fix missing lock on some table converters
Aurelien DARRAGON [Tue, 14 Jan 2025 10:18:24 +0000 (11:18 +0100)]
BUG/MEDIUM: stktable: fix missing lock on some table converters

In 819fc6f563
("MEDIUM: threads/stick-tables: handle multithreads on stick tables"),
sample fetch and action functions were properly guarded with stksess
read/write locks for read and write operations respectively, but the
sample_conv_table functions leveraged by "table_*" converters were
overlooked.

This bug was not known to cause issues in existing deployments yet (at
least it was not reported), but due to its nature it can theorically lead
to inconsistent values being reported by "table_*" converters if the value
is being updated by another thread in parallel.

It should be backported to all stable versions.

[ada: for versions < 3.0, glitch_cnt and glitch_rate samples should be
 ignored as they first appeared in 3.0]

(cherry picked from commit 8919a80da9391e348aa325b44fdae40a35f48dcf)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 9f7e76bb0d738a9af1bac614e153995a9ba97d5e)
[cf: There is no bytes rate factor on 3.0]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

8 months agoBUG/MINOR: quic: reject NEW_TOKEN frames from clients
Amaury Denoyelle [Tue, 7 Jan 2025 17:22:00 +0000 (18:22 +0100)]
BUG/MINOR: quic: reject NEW_TOKEN frames from clients

As specified by RFC 9000, reject NEW_TOKEN frames emitted by clients.
Close the connection with error code PROTOCOL_VIOLATION.

This must be backported up to 2.6.

(cherry picked from commit 4a5d82a97d9269eb17f9b92af6c8a9cd904705cd)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 21c807cc2233e5ca9f450d767311c8e12bbb9d70)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

8 months agoBUG/MINOR: stktable: fix big-endian compatiblity in smp_to_stkey()
Aurelien DARRAGON [Thu, 9 Jan 2025 08:05:43 +0000 (09:05 +0100)]
BUG/MINOR: stktable: fix big-endian compatiblity in smp_to_stkey()

When smp_to_stkey() deals with SINT samples, since stick-tables deals with
32 bits integers while SINT sample is 64 bit integer, inplace conversion
was done in smp_to_stkey. For that the 64 bit integer was truncated before
the key would point to it. Unfortunately this only works on little endian
architectures because with big endian ones, the key would point to the
wrong 32bit range.

To fix the issue and make the conversion endian-proof, let's re-assign
the sample as 32bit integer before the key points to it.

Thanks to Willy for having spotted the bug and suggesting the above fix.

It should be backported to all stable versions.

(cherry picked from commit b59d1fd911045c37b3fc88795a1f2c1947e6e59d)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 422e38bd828d1cddf55fe923ec7ebd269583d8ec)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoBUILD: activity/memprofile: fix a build warning in the posix_memalign handler
Willy Tarreau [Fri, 22 Nov 2024 08:41:02 +0000 (09:41 +0100)]
BUILD: activity/memprofile: fix a build warning in the posix_memalign handler

A "return NULL" statement was placed for error handling in the
posix_memalign() handler instead of an int errno value, by recent
commit 5ddc8b3ad4 ("MINOR: activity/memprofile: monitor non-portable
calls as well"). Surprisingly the warning only triggered on gcc-4.8.
Let's use ENOMEM instead. No backport needed.

(cherry picked from commit b30639848e502f79829abc28c8093aa0e34ba1f9)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoMINOR: activity/memprofile: monitor non-portable calls as well
Willy Tarreau [Thu, 21 Nov 2024 08:12:58 +0000 (09:12 +0100)]
MINOR: activity/memprofile: monitor non-portable calls as well

Some dependencies might very well rely on posix_memalign(), strndup()
or other less portable callsn making us miss them when chasing memory
leaks, resulting in negative global allocation counters. Let's provide
the handlers for the following functions:

  strndup()        // _POSIX_C_SOURCE >= 200809L || glibc >= 2.10
  valloc()         // _BSD_SOURCE || _XOPEN_SOURCE>=500 || glibc >= 2.12
  aligned_alloc()  // _ISOC11_SOURCE
  posix_memalign() // _POSIX_C_SOURCE >= 200112L
  memalign()       // obsolete
  pvalloc()        // obsolete

This time we don't fail if they're not found, we just silently forward
the calls.

(cherry picked from commit 5ddc8b3ad4190583aa6f0996a7401d6f2d4fe78e)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoMINOR: activity/memprofile: also monitor strdup() activity
Willy Tarreau [Thu, 21 Nov 2024 07:45:04 +0000 (08:45 +0100)]
MINOR: activity/memprofile: also monitor strdup() activity

Some memory profiling outputs have showed negative counters, very likely
due to some libs calling strdup(). Let's add it to the list of monitored
activities.

Actually even haproxy itself uses some. Having "profiling.memory on" in
the config reveals 35 call places.

(cherry picked from commit 33c0ce299da0a56a01ec1f59ebc78e8d59081acc)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoDEV: sock: Add a debug counter to track strange flag on fd during connect()
Christopher Faulet [Mon, 2 Dec 2024 07:43:06 +0000 (08:43 +0100)]
DEV: sock: Add a debug counter to track strange flag on fd during connect()

This counter was removed when the commit 56cd20cb53 ("BUG/MEDIUM: sock:
Remove FD_POLL_HUP during connect() if FD_POLL_ERR is not set") was
backported because the debug counters were not backported yet to the
3.0. Now de debug counters are available in 3.0, this counter can also be
added.

This commit is 3.0-specific. There is no upstream ID.

9 months agoMINOR: debug/cli: replace "debug dev counters" with "debug counters"
Willy Tarreau [Fri, 15 Nov 2024 15:19:05 +0000 (16:19 +0100)]
MINOR: debug/cli: replace "debug dev counters" with "debug counters"

"debug dev" commands are not meant to be used by end-users, and are
purposely not documented. Yet due to their usefulness in troubleshooting
sessions, users are increasingly invited by developers to use some of
them.

"debug dev counters" is one of them. Better move it to "debug counters"
and document it so that users can check them even if the output can look
cryptic at times. This, combined with DEBUG_GLITCHES, can be convenient
to observe suspcious activity. The doc however precises that the format
may change between versions and that new entries/types might appear
within a stable branch.

(cherry picked from commit 4420939fcd07f4ad0517b4bfa8a4788fefeb3f80)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoMINOR: debug: add "debug dev counters" to list code counters
Willy Tarreau [Mon, 21 Oct 2024 16:51:55 +0000 (18:51 +0200)]
MINOR: debug: add "debug dev counters" to list code counters

Issuing "debug dev counters" on the CLI will now scan all existing
counters, and report their count, type, location, function name, the
condition and an optional comment passed to the macro.

The command takes a number of arguments:
  - "show": this is the default, it will just list the counters
  - "reset": will reset the matching counters instead of listing them
  - "all": by default, only non-zero counters are listed. With "all",
     they are all listed
  - "bug": restrict the reset or dump to counters of type "BUG" (BUG_ON usually)
  - "chk": restrict the reset or dump to counters of type "CHK" (CHECK_IF)
  - "cnt": restrict the reset or dump to counters of type "CNT" (COUNT_IF)

The types may be cumulated, and the option entered in any order. Here's
an example of the output of "debug dev counters show all bug":

  Count     Type Location function(): "condition" [comment]
  0          BUG ring.h:114 ring_dup(): "max > ring_size(dst)"
  0          BUG vecpair.h:223 vp_getblk_ofs(): "ofs >= v1->len + v2->len"
  0          BUG buf.h:395 b_add(): "b->data + count > b->size"
  0          BUG buf.h:106 b_room(): "b->data > b->size"
  0          BUG task.h:328 _task_queue(): "(ulong)caller & 1"
  0          BUG task.h:324 _task_queue(): "task->tid != tid"
  0          BUG task.h:313 _task_queue(): "(ulong)caller & 1"
  (...)

This is expected to be convenient combined with the use and abuse of
COUNT_IF() at select locations.

(cherry picked from commit f2c415cec1861322d3745edca959d164955fcfe0)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoMINOR: debug: add a new debug macro COUNT_IF()
Willy Tarreau [Mon, 21 Oct 2024 16:34:21 +0000 (18:34 +0200)]
MINOR: debug: add a new debug macro COUNT_IF()

This macro works exactly like BUG_ON() except that it never logs anything
nor crashes, it only implements an atomic counter that is incremented on
every call. This can be used to count a number of unlikely events that are
worth checking at run time on setups showing unusual and unreproducible
behaviors.

(cherry picked from commit da66c42f657855ec13f918ce37c9431cb2922e8d)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoMEDIUM: debug: add match counters for BUG_ON/WARN_ON/CHECK_IF
Willy Tarreau [Mon, 21 Oct 2024 16:29:00 +0000 (18:29 +0200)]
MEDIUM: debug: add match counters for BUG_ON/WARN_ON/CHECK_IF

These macros do not always kill the process, and sometimes it would be
nice to know if some match or not, and how many times (especially for the
CHECK_IF one).

This commit adds a new section "dbg_cnt" made of structs that contain
function name, file name, line number, check type, condition and match
count. A newe macro __DBG_COUNT() adds one to the counter, and is placed
inside _BUG_ON() and _BUG_ON_ONCE(). It's worth noting that the exact
type of the check is not very precise but in practice we don't care,
as most checks will cause the process to die anyway unless they're of
type _BUG_ON_ONCE() (used by CHECK_IF by default).

All of this is limited to !defined(USE_OBSOLETE_LINKER) because we're
creating a section, thus we need a modern linker to be able to scan
this section later. Doing so adds ~50kB to the executable due to the
~1266 BUG_ON() and others placed there. That's not huge in comparison
to the visibility it can provide.

(cherry picked from commit 776fd03509cec5b8fe5a54aeac0370aabc15d313)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoCLEANUP: debug: make the BUG_ON() macros check the condition in the outer one
Willy Tarreau [Mon, 21 Oct 2024 16:17:25 +0000 (18:17 +0200)]
CLEANUP: debug: make the BUG_ON() macros check the condition in the outer one

The BUG_ON() macros are made of two levels so as to resolve the condition
to a string. However this doesn't offer much flexibility for performing
other operations when the condition is validated, so let's adjust them so
that the condition is checked in the outer macro and the operations are
performed in the inner one.

(cherry picked from commit 8844ed2009563d5976c42b6b2b7eb8f097344f25)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoMINOR: sample: extend the "when" converter to support an ACL
Willy Tarreau [Mon, 18 Nov 2024 14:27:28 +0000 (15:27 +0100)]
MINOR: sample: extend the "when" converter to support an ACL

Sometimes conditions to decide of an anomaly are not as easy to define
as just an error or a success. One example use case would be to monitor
the transfer time and fix a threshold.

An idea suggested by Tristan would be to make permit the "when"
converter to refer to a more variable or dynamic condition.

Here we make this possible by making "when" rely on a named ACL. The
ACL then needs to be specified in either the proxy or the defaults
section. Since it is evaluated inline, it may even refer to information
available at the end (at log time) such as the data transfer time. If
the ACL evalutates to true, the converter passes the data.

Example: log "dbg={-}" when fine, or "dbg={... debug info ...}" on slow
transfers:

  acl slow_xfer res.timer.data ge 10000   # more than 10s is slow
  log-format "$HAPROXY_HTTP_LOG_FMT                                \
              fsdbg={%[fs.debug_str,when(acl,slow_xfer)]}          \
              bsdbg={%[bs.debug_str,when(acl,slow_xfer)]}"

(cherry picked from commit 45f9e95f2276dd712b07a241e22ef15d5e65de95)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoMINOR: acl: export find_acl_default()
Willy Tarreau [Mon, 18 Nov 2024 14:15:54 +0000 (15:15 +0100)]
MINOR: acl: export find_acl_default()

It will be needed in a future patch, so let's export it (it was static).

(cherry picked from commit 00fcda1ff28c7826d230a4dd3b235b5be19cac07)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoMINOR: arg: add an argument type for identifier
Dragan Dosen [Thu, 17 Oct 2024 08:59:01 +0000 (10:59 +0200)]
MINOR: arg: add an argument type for identifier

The ARGT_ID argument type may now be used to set a custom resolve
function in order to help resolve the argument string value. If the
custom resolve function is not set, the behavior is the same as of
type ARGT_STR.

(cherry picked from commit f33e9079a93ce0a22954b9423657524e81d852e8)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoMINOR: sample: add the "when" converter to condition some expressions
Willy Tarreau [Tue, 22 Oct 2024 17:43:18 +0000 (19:43 +0200)]
MINOR: sample: add the "when" converter to condition some expressions

Sometimes it would be desirable to include some debugging output only
under certain conditions, but the end of the transfer is too late to
apply some rules.

Here we take the approach of making a converter ("when") that takes a
condition among an arbitrary list, and decides whether or not to let
the input sample pass through or not based on the condition. This
allows for example to log debugging information only when an error
was encountered during the processing (sort of an extension of
dontlog-normal). The conditions are quite limited (stopping, error,
normal, toapplet, forwarded, processed) and can be negated. The
converter can also be chained to use more complex conditions.

A suggested example will be:

    # log "dbg={-}" when fine, or "dbg={... debug info ...}" on error:
    log-format "$HAPROXY_HTTP_LOG_FMT dbg={%[bs.debug_str,when(!normal)]}"

(cherry picked from commit b74fb1325e60a2573f1ed8a460129bc6c0c9e53f)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoMINOR: mux-h1: Add support of the debug string for logs
Christopher Faulet [Tue, 22 Oct 2024 16:21:28 +0000 (18:21 +0200)]
MINOR: mux-h1: Add support of the debug string for logs

Now it is possible to have info about front and back H1 multiplexer. For instance:

    <134>Oct 22 18:10:46 haproxy[3841864]: 127.0.0.1:44280 [22/Oct/2024:18:10:43.265] front-http back-http/www 0/0/-1/-1/3082 503 217 - - SC-- 1/1/0/0/3 0/0 "GET / HTTP/1.1" fs=< h1s=0x13b6f10 h1s.flg=0x14010 .sd.flg=0x50404601 .req.state=MSG_DONE .res.state=MSG_DONE .meth=GET status=503 .sd.flg=0x50404601 .sc.flg=0x00034482 .sc.app=0x11e4c30 .subs=(nil) h1c.flg=0x0 .sub=0 .ibuf
=0@(nil)+0/0 .obuf=0@(nil)+0/0 .task=0x1337d10 .exp=<NEVER> conn.flg=0x80000300> bs=< h1s=0x13bb400 h1s.flg=0x100010 .sd.flg=0x10400001 .req.state=MSG_RQBEFORE .res.state=MSG_RPBEFORE .meth=UNKNOWN status=0 .sd.flg=0x10400001 .sc.flg=0x0003c007 .sc.app=0x11e4c30 .subs=(nil) h1c.flg=0x80000000 .sub=0 .ibuf=0@(nil)+0/0 .obuf=0@(nil)+0/0 .task=0x12ba610 .exp=<NEVER> conn.flg=0x5c0300>

The have this log message, the log-format must be set to:

  log-format "$HAPROXY_HTTP_LOG_FMT fs=<%[fs.debug_str]> bs=<%[bs.debug_str]>"

(cherry picked from commit ce314cfb395a407484e1b37d76e8af8007c1ff4c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoMINOR: mux-quic: measure QCS lifetime and its blocking state
Amaury Denoyelle [Wed, 31 Jul 2024 16:43:55 +0000 (18:43 +0200)]
MINOR: mux-quic: measure QCS lifetime and its blocking state

Reuse newly defined tot_time structure to measure various values related
to a QCS lifetime.

First, a timer is used to comptabilize the total QCS lifetime. Then, two
other timers are used to account the total time during which Tx from
stream layer to MUX is blocked, either on lack of buffer or due to
flow-control.

These three timers are reported in qmux_dump_qcs_info(). Thus, they are
available in traces and for QUIC MUX debug string sample.

(cherry picked from commit 9f829ea3f34fd15bee50ca91fac8526077f77142)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoMINOR: time: define tot_time structure
Amaury Denoyelle [Wed, 7 Aug 2024 12:50:26 +0000 (14:50 +0200)]
MINOR: time: define tot_time structure

Define a new utility type tot_time. Its purpose is to be able to account
elapsed time accross multiple periods. Functions are defined to easily
start and stop measures, and return the current value.

(cherry picked from commit a6e2523ca1f3dcc90b050d75af62bb867a2acc07)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoMINOR: quic: dump quic_conn debug string for logs
Amaury Denoyelle [Thu, 1 Aug 2024 09:35:04 +0000 (11:35 +0200)]
MINOR: quic: dump quic_conn debug string for logs

Define a new xprt_ops callback named dump_info. This can be used to
extend MUX debug string with infos from the lower layer.

Implement dump_info for QUIC stack. For now, only minimal info are
reported : bytes in flight and size of the sending window. This should
allow to detect if the congestion controller is fine. These info are
reported via QUIC MUX debug string sample.

(cherry picked from commit 663416b4ef2fafcffa61c04aa09056853d9674f7)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoMINOR: mux-quic: implement debug string for logs
Amaury Denoyelle [Wed, 31 Jul 2024 15:28:24 +0000 (17:28 +0200)]
MINOR: mux-quic: implement debug string for logs

Implement MUX_SCTL_DBG_STR for QUIC MUX. This returns info for the
current QCS and QCC instances, reusing qmux_dump_qc{c,s}_info functions
already used for traces, and the connection flags.

This stream operation is useful for debug string sample support.

(cherry picked from commit 630fa53c51ddfd3dbdf5e9d2495ca3fb45ff050e)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoMINOR: mux-quic: define dump functions for QCC and QCS
Amaury Denoyelle [Wed, 31 Jul 2024 15:27:33 +0000 (17:27 +0200)]
MINOR: mux-quic: define dump functions for QCC and QCS

Extract trace code to dump QCC and QCS instances into dedicated
functions named qmux_dump_qc{c,s}_info(). This will allow to easily
print QCC/QCS infos outside of traces.

(cherry picked from commit eb4dfa3b36ae949d4a55782417af08f4689096f4)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoMINOR: mux-h2: implement the debug string for logs
Willy Tarreau [Tue, 30 Jul 2024 17:33:07 +0000 (19:33 +0200)]
MINOR: mux-h2: implement the debug string for logs

Now it permits to have this for a front and a back:

<134>Jul 30 19:32:53 haproxy[24405]: 127.0.0.1:64860 [30/Jul/2024:19:32:53.732] test2 test2/s1 0/0/0/0/0 200 130 - - ---- 2/1/0/0/0 0/0 "GET /blah HTTP/2.0"  h2s.id=1 .st=CLO .flg=0x7003 .rxbuf=0@(nil)+0/0 .sc=0x1e03fb0(.flg=0x00034482 .app=0x1e04020) .sd=0x1e03f30(.flg=0x50405601) .subs=(nil) h2c.st0=FRH .err=0 .maxid=1 .lastid=-1 .flg=0x100e00 .nbst=0 .nbsc=1, .glitches=0 .fctl_cnt=0 .send_cnt=0 .tree_cnt=1 .orph_cnt=0 .sub=1 .dsi=1 .dbuf=0@(nil)+0/0 .mbuf=[1..1|32],h=[0@(nil)+0/0],t=[0@(nil)+0/0] .task=(nil) conn.flg=0x80000300
<134>Jul 30 19:32:53 haproxy[24405]: 127.0.0.1:65246 [30/Jul/2024:19:32:53.732] test1 test1/s1 0/0/0/0/0 200 130 - - ---- 2/1/0/0/0 0/0 "GET /blah HTTP/1.1"  h2s.id=1 .st=CLO .flg=0x7003 .rxbuf=0@(nil)+0/0 .sc=0x1dfc7b0(.flg=0x0006d01b .app=0x1c65fe0) .sd=0x1dfc820(.flg=0x1040ca01) .subs=(nil) h2c.st0=FRH .err=0 .maxid=1 .lastid=-1 .flg=0x108e00 .nbst=0 .nbsc=1, .glitches=0 .fctl_cnt=0 .send_cnt=0 .tree_cnt=1 .orph_cnt=0 .sub=1 .dsi=1 .dbuf=0@(nil)+0/0 .mbuf=[1..1|32],h=[0@(nil)+0/0],t=[0@(nil)+0/0] .task=(nil) conn.flg=0x000300

Just with this in the front and back proxies respectively:
  log-format "$HAPROXY_HTTP_LOG_FMT %[bs.debug_str(15)]"
  log-format "$HAPROXY_HTTP_LOG_FMT %[fs.debug_str(15)]"

For now the mux only implements muxs, muxc, conn. Xprt is ignored.

(cherry picked from commit 490cb16d3af03e735b25253a1ac3a641b51760b2)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoMINOR: stconn: add a new pair of sf functions {bs,fs}.debug_str
Willy Tarreau [Tue, 30 Jul 2024 15:57:37 +0000 (17:57 +0200)]
MINOR: stconn: add a new pair of sf functions {bs,fs}.debug_str

These are passed to the underlying mux to retrieve debug information
at the mux level (stream/connection) as a string that's meant to be
added to logs.

The API is quite complex just because we can't pass any info to the
bottom function. So we construct a union and pass the argument as an
int, and expect the callee to fill that with its buffer in return.

Most likely the mux->ctl and ->sctl API should be reworked before
the release to simplify this.

The functions take an optional argument that is a bit mask of the
layers to dump:
  muxs=1
  muxc=2
  xprt=4
  conn=8
  sock=16

The default (0) logs everything available.

(cherry picked from commit 921e04bf87a94b410287a85d2397a0502520348c)
[cf: The doc was adapted to move debug_str samples in the right section]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoBUG/MEDIUM: h1-htx: Properly handle bodyless messages
Christopher Faulet [Wed, 8 Jan 2025 16:42:44 +0000 (17:42 +0100)]
BUG/MEDIUM: h1-htx: Properly handle bodyless messages

During h1 parsing, there are some postparsing checks to detect bodyless
messages and switch the parsing in DONE state. However, a case was not
properly handled. Responses to HEAD requests with a "transfer-encoding"
header. The response parser remained blocked waiting for the response body.

To fix the issue, the postparsing was sliglty modified. Instead of trying to
handle bodyless messages in a common way between the request and the
response, it is now performed in the dedicated postparsing functions. It is
easier to enumerate all cases, especially because there is already a test
for responses to HEAD requests.

This patch should fix the issue #2836. It must be backported as far as 2.9.

(cherry picked from commit b9cc361b35e66c1f2d26a9b703f8759f70cbc03c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit becc475a16dea31037617baa91d1a5de01ccdd1e)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoBUG/MEDIUM: promex/resolvers: Don't dump metrics if no nameserver is defined
Christopher Faulet [Mon, 6 Jan 2025 07:41:57 +0000 (08:41 +0100)]
BUG/MEDIUM: promex/resolvers: Don't dump metrics if no nameserver is defined

A 'resolvers' section may be defined without any nameserver. In that case,
we must take care to not dump corresponding Prometheus metrics. However
there is an issue that could lead to a crash or a strange infinite loop
because we are looping on an empty list and, at some point, we are
dereferencing an invalid pointer.

There is an issue because the loop on the nameservers of a resolvers section
is performed via callback functions and not the standard list_for_each_entry
macro. So we must take care to properly detect end of the list and empty
lists for nameservers. But the fix is not so simple because resolvers
sections with and without nameservers may be mixed.

To fix the issue, in rslv_promex_start_ts() and rslv_promex_next_ts(), when the
next resolvers section must be evaluated, a loop is now used to properly skip
empty sections.

This patch is related to #2831. Not sure it fixes it. It must be backported
as far as 3.0.

(cherry picked from commit 892eb2bb2ceba2c0e80d51eed69dfc9494a56988)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 00f01f5e636351e1d91dbf62c80e1d0fea34172f)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoMINOR: config: Alert about extra arguments for errorfile and errorloc
Christopher Faulet [Fri, 3 Jan 2025 09:10:08 +0000 (10:10 +0100)]
MINOR: config: Alert about extra arguments for errorfile and errorloc

errorfile and errorloc directives expect excatly two arguments. But extra
arguments were just ignored while an error should be emitted. It is now
fixed.

This patch could be backported as far as 2.2 if necessary.

(cherry picked from commit f578811c4e8965f243cd16a6f569909a2a07498c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 03d6c648dfc3ab364149aa747c1574c847526193)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoDOC: config: add missing "track-sc0" in action keywords matrix
Aurelien DARRAGON [Tue, 24 Dec 2024 09:08:36 +0000 (10:08 +0100)]
DOC: config: add missing "track-sc0" in action keywords matrix

In d54e8f8107 ("DOC: config: reorganize actions into their own section"),
"track-sc0" keyword was properly documented but the keyword was not placed
in the action keywords matrix alongside other track-sc* statements. It
was probably overlooked, so let's fix that.

Could be backported up to 2.9 with d54e8f8107.

(cherry picked from commit f94c63021b9d46a5a77540b6b95cf6e60dcbf5e0)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 29b1710c9a86fb41f1c322f68a90d199298696f1)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoBUG/MINOR: stats: fix segfault caused by uninitialized value in "show schema json"
Aurelien DARRAGON [Mon, 23 Dec 2024 08:35:15 +0000 (09:35 +0100)]
BUG/MINOR: stats: fix segfault caused by uninitialized value in "show schema json"

Since b3d5708 ("MINOR: stats: remove implicit static trash_chunk usage")
a segfault can occur when issuing "show schema json" on the stats socket.

Indeed, now the dumping functions don't rely on trash_chunk anymore, but
instead they rely on the appctx->chunk buffer. However, unlike other
stats dumping commands, the "show schema json" only have an io handler,
and no parse function. With other command, the parse function is
responsible for pre-setting some data, including applet ctx reservation.

Thus due to "show schema json" lacking parsing function, the applet ctx is
used uninitialized, which is a bug obviously.

To fix the issue we simply add a parse function for "show schema json",
although all it does for now is calling applet_reserve_svcctx() for the
current applet ctx.

This issue was reported by @dsuch in GH #2825. It must be backported up
to 3.0.

(cherry picked from commit ac1f413590932d4eb3b78ca5a536845f162b7866)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit f3a3a8941561e0140ac9c76f7b82942b30ab63a0)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoBUG/MEDIUM: queue: Make process_srv_queue return the number of streams
Olivier Houchard [Mon, 23 Dec 2024 14:17:25 +0000 (14:17 +0000)]
BUG/MEDIUM: queue: Make process_srv_queue return the number of streams

Make process_srv_queue() return the number of streams unqueued, as
pendconn_grab_from_px() did, as that number is used by
srv_update_status() to generate logs.

This should be backported up to 2.6 with
111ea83ed4e13ac3ab028ed5e95201a1b4aa82b8

(cherry picked from commit 5b8899b6ccc7dab3a54a51dcb8ba1512bd0c886c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 70588a16903002709cf3c84255ad8ded73f8e584)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoBUG/MINOR: h2/rhttp: fix HTTP2 conn counters on reverse
Amaury Denoyelle [Mon, 16 Dec 2024 10:00:23 +0000 (11:00 +0100)]
BUG/MINOR: h2/rhttp: fix HTTP2 conn counters on reverse

Dedicated HTTP/2 stats proxy counters are available for current and
total number of HTTP/2 connection on both frontend and backend sides.
Both counters are simply incremented into h2_init().

This causes issues when using reverse HTTP. First, increment is not
performed on the expected side, as it is triggered before
h2_conn_reverse() which switches a connection from frontend to backend
or vice versa. For example on active revers side, h2_total_connections
is incremented on the backend only even after connection is reversed and
attached to a listener for the remainder of its lifetime.

h2_open_connections suffers from a similar but arguably worst behavior
as it is also decremented. If increment and decrement operations are not
performed on the same proxy side, which happens for every connection
which has been successfully reversed, it causes an invalid counter
value, possibly with an integer overflow.

To fix this, delay increment operations on reverse HTTP from h2_init()
to h2_conn_reverse(). Both counters are updated only after reverse has
completed, thus using the expected frontend or backend side.

To prevent overflow on h2_open_connections, ensure h2_release()
decrement is not performed if a connection is freed before achieving its
reversal, as in this case it would not have been accounted by H2
counters.

This should be backported up to 2.9.

This should fix github issue #2821.

(cherry picked from commit 8633446337cd322f010ce7b3ca9c257d1ce7848c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit aedc04be5a1fbf72392e68037c4e23b9aa034dc0)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoDOC: config: add "tune.lua.burst-timeout" to the list of global parameters
Aurelien DARRAGON [Thu, 19 Dec 2024 08:25:21 +0000 (09:25 +0100)]
DOC: config: add "tune.lua.burst-timeout" to the list of global parameters

"tune.lua.burst-timeout" was properly defined but not listed in the list
of global parameters as it was overlooked in 58e36e5b1 ("MEDIUM: hlua:
introduce tune.lua.burst-timeout")

(cherry picked from commit 67e3270c59f4cb997f2078d395d365e0189326ba)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 69a0ad6f781a374863af86d87b3abda0a1246773)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoDOC: config: reorder "tune.lua.*" keywords by alphabetical order
Aurelien DARRAGON [Thu, 19 Dec 2024 08:08:01 +0000 (09:08 +0100)]
DOC: config: reorder "tune.lua.*" keywords by alphabetical order

Effort was made to properly organize "tune.*" keywords by alphabetical
order, but "tune.lua" keywords didn't follow that rule with care.

Let's fix that.

(cherry picked from commit 985a45d9c77568798b5c0c77cae7ddba219f3b63)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit c2c2047b240a33519d57ef53806bd014cd45d6b2)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoDOC: config: add example for server "track" keyword
Aurelien DARRAGON [Tue, 17 Dec 2024 10:54:06 +0000 (11:54 +0100)]
DOC: config: add example for server "track" keyword

As requested on GH #2325, "track" server keyword could benefit from a
simple config example to show how to make use of it.

That's what we're doing in this commit, thanks to GH user @HAkmiller
for the suggestion.

(cherry picked from commit 48545113f4db86ad43a99ba01742a9811410e6dc)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit d270bc5864a3cec0b5faf40ae8ca9db0a365da81)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoBUG/MEDIUM: queues: Do not use pendconn_grab_from_px().
Olivier Houchard [Tue, 17 Dec 2024 14:39:21 +0000 (15:39 +0100)]
BUG/MEDIUM: queues: Do not use pendconn_grab_from_px().

pendconn_grab_from_px() was called when a server was brought back up, to
get some streams waiting in the proxy's queue and get them to run on the
newly available server. It is very similar to process_srv_queue(),
except it only goes through the proxy's queue, which can be a problem,
because there is a small race condition that could lead us to add more
streams to the server queue just as it's going down. If that happens,
the server would just be ignored when back up by new streams, as its
queue is not empty, and it would never try to process its queue.
The other problem with pendconn_grab_from_px() is that it is very
liberal with how it dequeues streams, and it is not very good at
enforcing maxconn, it could lead to having 3*maxconn connections.
For both those reasons, just get rid of pendconn_grab_from_px(), and
just use process_srv_queue().
Both problems are easy to reproduce, especially on a 64 threads machine,
set a maxconn to 100, inject in H2 with 1000 concurrent connections
containing up to 100 streams each, and after a few seconds/minutes the
max number of concurrent output streams will be much higher than
maxconn, and eventually the server will stop processing connections.

It may be related to github issue #2744. Note that it doesn't totally
fix the problem, we can occasionally see a few more connections than
maxconn, but the max that have been observed is 4 more connections, we
no longer get multiple times maxconn.

have more outgoing connections than maxconn,
This should be backported up to 2.6.

(cherry picked from commit 111ea83ed4e13ac3ab028ed5e95201a1b4aa82b8)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit ab4ff1b7a6c7685f28fbdea01b38caf7e816fddf)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoBUG/MEDIUM: queues: Make sure we call process_srv_queue() when leaving
Olivier Houchard [Fri, 13 Dec 2024 17:11:05 +0000 (17:11 +0000)]
BUG/MEDIUM: queues: Make sure we call process_srv_queue() when leaving

In stream_free(), make sure we call process_srv_queue() each time we
call sess_change_server(), otherwise a server may end up not dequeuing
any stream when it could do so. In some extreme cases it could lead to
an infinite loop, as the server would appear to be available, as its
"served" parameter would be < maxconn, but would end up not being used,
as there are elements still in its queue.

This should be backported up to 2.6.

(cherry picked from commit dc9ce9c26469e00ab71fe6387dbd13010d4930f0)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 1385e4ca16b3797b0091a959b626935cd7f29b38)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoBUG/MEDIUM: stconn: Only consider I/O timers to update stream's expiration date
Christopher Faulet [Mon, 16 Dec 2024 10:04:53 +0000 (11:04 +0100)]
BUG/MEDIUM: stconn: Only consider I/O timers to update stream's expiration date

In sc_notify(), it remained a case where it was possible to set an
expiration date on the stream in the past, leading to a crash because of a
BUG_ON(). This must never happen of course.

In sc_notify(), The stream's expiration may be updated in case no wakeup
conditions are encoutered. In that case, we must take care to never set an
expiration date in the past. However, it appeared there was still a
condition to do so. This code is based on an implicit postulate: the
stream's expiration date must always be set when we leave
process_stream(). It was true since the 2.9. But in 3.0, the buffer
allocation mechanism was improved and on an alloc failure in
process_stream(), the stream is inserted in a wait-list and its expiration
date is set to TICK_ETERNITY. With the good timing, and an analysis
expiration date set on a channel, it is possible to set the stream's
expiration date in past.

After analysis, it appeared that the proper way to fix the issue is to only
evaluate I/O timers (read and write timeout) and not stream's timers
(analase_exp or conn_exp) because only I/O timers may have changed since the
last process_stream() call.

This patch must be backported as far as 3.0 to fix the issue. But it is
probably a good idea to also backported it as far as 2.8.

(cherry picked from commit 4f32d03360e42a3e7fe4ed71012ec90339b9731d)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 0510c41910778257fd398309045c9d09bf34ec49)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoREGTESTS: ssl: add a PEM with mix of LF and CRLF line endings
Valentine Krasnobaeva [Fri, 13 Dec 2024 11:02:14 +0000 (12:02 +0100)]
REGTESTS: ssl: add a PEM with mix of LF and CRLF line endings

User tried to update a PEM, generated automatically. Part of this PEM has LF
line endings, and another part (CA certificate), added by some API, has CRLF
line endings. This has revealed a bug in cli_snd_buf(), see more
details in issue GitHUB #2818. So, let's add an example of such PEM in our
SSL regtest.

(cherry picked from commit ea4a148a7d9ead2132757bffd4699146e80da2df)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 296cbefc98f76862111170b8a7b3aeb8d88d107d)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoBUG/MINOR: cli: cli_snd_buf: preserve \r\n for payload lines
Valentine Krasnobaeva [Thu, 12 Dec 2024 18:38:38 +0000 (19:38 +0100)]
BUG/MINOR: cli: cli_snd_buf: preserve \r\n for payload lines

cli_snd_buf() analyzez input line by line. Before this patch it has always
scanned a given line for the presence of '\r' followed by '\n'.

This is only needed for strings, that contain the commands itself like
"show ssl cert\n", "set ssl cert test.pem <<\n".

In case of strings, which contain the command's payload, like
"-----BEGIN CERTIFICATE-----\r\n", '\r\n' should be preserved
as is.

This patch fixes the GitHub issue #2818.

This patch should be backported in v3.1 and in v3.0.

(cherry picked from commit d60c893991699af1103bcdcd33c915885c056559)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit 09ac38f57bb773a05cb4947e85d9adb93856d8b2)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoBUG/MEDIUM: clock: make sure now_ms cannot be TICK_ETERNITY
Willy Tarreau [Fri, 15 Nov 2024 14:52:17 +0000 (15:52 +0100)]
BUG/MEDIUM: clock: make sure now_ms cannot be TICK_ETERNITY

In clock ticks, 0 is TICK_ETERNITY. Long ago we used to make sure now_ms
couldn't be zero so that it could be assigned to expiration timers, but
it has long changed after functions like tick_add() were instrumented to
make the check. The problem is that aside the rare few accidental direct
assignments to expiration dates, it's also used to mark the beginning of
an event that's later checked against TICK_ETERNITY to know if it has
already struck. The problem in this case is that certain events may just
be replaced or dropped just because they apparently never appeared. It's
probably the case for stconn's "lra" and "fsb" fields, just like it is
for all those involving tick_add_ifset(), like h2c->idle_start.

The right approach would be to change the type of now_ms to something
else that cannot take direct computations and that represents a timestamp,
forcing to always use the conversion functions. The variables holding such
timestamps would also be distinguished from intervals. At first glance we
could have for timestamps:
  - 0 = never happened (for the past), eternity (for the future)
  - X = date
and for intervals:
  - 0 = not set
  - X = interval

However this requires significant changes. Instead for now, let's just
make sure again that now_ms is never 0 by setting it to 1 when this
happens (1 / 4 billion times, or 1ms every 49.7 days).

This will need to be carefully backported to older versions. Note that
with this patch backported, the previous ones fixing the zero date are
not strictly needed.

(cherry picked from commit 5a3735a155d47786c20774a1492f3ed20cfe4df3)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoBUG/MEDIUM: stats/server: use watcher to track server during stats dump
Amaury Denoyelle [Wed, 23 Oct 2024 09:33:34 +0000 (11:33 +0200)]
BUG/MEDIUM: stats/server: use watcher to track server during stats dump

If a server A is deleted while a stats dump is currently on it, deletion
is delayed thanks to reference counting. Server A is nonetheless removed
from the proxy list. However, this list is a single linked list. If the
next server B is deleted and freed immediately, server A would still
point to it. This problem has been solved by the prev_deleted list in
servers.

This model seems correct, but it is difficult to ensure completely its
validity. In particular, it implies when stats dump is resumed, server A
elements will be accessed despite the server being in a half-deleted
state.

Thus, it has been decided to completely ditch the refcount mechanism for
stats dump. Instead, use the watcher element to register every stats
dump currently tracking a server instance. Each time a server is deleted
on the CLI, each stats dump element which may points to it are updated
to access the next server instance, or NULL if this is the last server.
This ensures that a server which was deleted via CLI but not completely
freed is never accessed on stats dump resumption.

Currently, no race condition related to dynamic servers and stats dump
is known. However, as described above, the previous model is deemed too
fragile, as such this patch is labelled as bug-fix. It should be
backported up to 2.6, after a reasonable period of observation. It
relies on the following patch :
  MINOR: list: define a watcher type

(cherry picked from commit 071ae8ce3d1a318d2227fad2ebf63e78a05815f0)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 4e932f79bea653157516a4c41ee26b32f22c470c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoMINOR: list: define a watcher type
Amaury Denoyelle [Wed, 23 Oct 2024 09:31:44 +0000 (11:31 +0200)]
MINOR: list: define a watcher type

Define a new watcher type into list module. This type is similar to bref
and can be used to register an element which is currently tracking a
dynamic target. Contrary to bref, if the target is freed, every watcher
element are updated to point to a next valid entry or NULL.

This type will simplify handling of dynamic servers deletion, in
particular while stats dump are performed.

This patch is not a bug-fix. However, it is mandatory to fix a race
condition in dynamic servers. Thus, it should be backported along the
next commit up to 2.6.

(cherry picked from commit eafa8a32bb19ee567cb0a60a4042a8ef436e1e94)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit b4501697a6321e990a94243c0274b4294dcbbd6b)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

9 months agoBUG/MINOR: stats: decrement srv refcount on stats-file release
Amaury Denoyelle [Tue, 10 Dec 2024 14:57:17 +0000 (15:57 +0100)]
BUG/MINOR: stats: decrement srv refcount on stats-file release

Servers instance may be removed at runtime. This can occurs during a
stat dump which currently references this server instance. This case is
protected by server refcount to prevent the server immediate release.

CLI output may be interrupted prior to stats dump completion, for
example if client CLI has been disconnected before the full response
transfer. As such, srv_drop() must be called in every stats dump release
callback.

srv_drop() was missing for stats-file dump release callback. This could
cause a race condition which would prevent a server instance to be fully
removed. Fix this by adding srv_drop() invokation into
cli_io_handler_release_dump_stat_file().

This should be backported up to 3.0.

(cherry picked from commit 2199179461957c86f57e39c4d97c37f3e4d10935)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 41df9cac1f2092473347ee9c2f51739bf7ad0013)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoBUG/MEDIUM: stconn: Don't forward shut for SC in connecting state
Christopher Faulet [Wed, 30 Oct 2024 15:41:39 +0000 (16:41 +0100)]
BUG/MEDIUM: stconn: Don't forward shut for SC in connecting state

In connecting state, shutdown must not be forwarded or scheduled because
otherwise this will prevent any connection retries. Indeed, if a EOS is
reported by the mux during the connection establishment, this should be
handled by the stream to eventually retries. If the write side is closed
first, this will not be possible because the stconn will be switched in DIS
state. If the shut is scheduled because pending data are blocked, the same
may happen, depending on the abort-on-close option.

This patch should be slowly be backported as far as 2.4. But an observation
period is mandatory. On 2.4, the patch must be adapted to use the
stream-interface API.

(cherry picked from commit 72e529829b8eedebc1891970cf8ac64eaf26d006)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months ago[RELEASE] Released version 3.0.7 v3.0.7
Christopher Faulet [Thu, 12 Dec 2024 12:49:49 +0000 (13:49 +0100)]
[RELEASE] Released version 3.0.7

Released version 3.0.7 with the following main changes :
    - BUG/MEDIUM: pattern: prevent uninitialized reads in pat_match_{str,beg}
    - MINOR: quic: notify connection layer on handshake completion
    - BUG/MINOR: stream: unblock stream on wait-for-handshake completion
    - BUG/MEDIUM: quic: support wait-for-handshake
    - MINOR: quic: simplify qc_parse_pkt_frms() return path
    - MINOR: quic: use dynamically allocated frame on parsing
    - MINOR: quic: extend return value of CRYPTO parsing
    - BUG/MINOR: quic: repeat packet parsing to deal with fragmented CRYPTO
    - CLEANUP: guid: remove global tree export
    - BUG/MINOR: guid/server: ensure thread-safety on GUID insert/delete
    - BUG/MEDIUM: quic: prevent crash due to CRYPTO parsing error
    - BUG/MINOR: cli: don't show sockpairs in HAPROXY_CLI and HAPROXY_MASTER_CLI
    - BUG/MEDIUM: resolvers: Insert a non-executed resulution in front of the wait list
    - BUG/MEDIUM: mux-h2: Don't send RST_STREAM frame for streams with no ID
    - BUG/MINOR: Don't report early srv aborts on request forwarding in DONE state
    - BUG/MEDIUM: checks: make sure to always apply offsets to now_ms in expiration
    - BUG/MEDIUM: mailers: make sure to always apply offsets to now_ms in expiration
    - BUG/MINOR: mux_quic: make sure to always apply offsets to now_ms in expiration
    - BUG/MINOR: peers: make sure to always apply offsets to now_ms in expiration
    - DOC: config: A a space before ':' for {bs,fs}.aborted and {bs,fs}.rst_code
    - DOC: config: Fix a typo in "1.3.1. The Request line"
    - BUG/MINOR: http_ana: Report -1 for %Tr for invalid response only
    - DOC: config: Slightly improve the %Tr documentation
    - DOC: config: Move wait_end in section about internal samples
    - DOC: config: Move fs.* and bs.* in section about L5 samples
    - DOC: lua: fix yield-dependent methods expected contexts
    - DOC: configuration: explain quotes and spaces in conditional blocks
    - DOC: configuration: wrap long line for "strstr()" conditional expression
    - BUG/MINOR: http-ana: Adjust the server status before the L7 retries
    - BUG/MEDIUM: mux-h2: Increase max number of headers when encoding HEADERS frames
    - BUG/MEDIUM: mux-h2: Check the number of headers in HEADERS frame after decoding
    - BUG/MEDIUM: h3: Properly limit the number of headers received
    - BUG/MEDIUM: h3: Increase max number of headers when sending headers
    - DOC: config: Improve documentation of tune.http.maxhdr directive
    - BUG/MEDIUM: debug: don't set the STUCK flag from debug_handler()
    - BUG/MEDIUM: wdt: fix the stuck detection for warnings
    - BUG/MINOR: activity/memprofile: reinitialize the free calls on DSO summary
    - MINOR: activity/memprofile: offer a function to unregister stale info
    - BUG/MEDIUM: pools/memprofile: always clean stale pool info on pool_destroy()
    - BUG/MAJOR: mux-h1: Properly handle wrapping on obuf when dumping the first-line
    - BUG/MAJOR: quic: fix wrong packet building due to already acked frames
    - DEV: lags/show-sess-to-flags: Properly handle fd state on server side
    - BUG/MEDIUM: http-ana: Don't release too early the L7 buffer
    - BUG/MEDIUM: sock: Remove FD_POLL_HUP during connect() if FD_POLL_ERR is not set
    - MINOR: mux-quic: Don't send an emtpy H3 DATA frame during zero-copy forwarding
    - BUG/MINOR: log: fix lf_text() behavior with empty string
    - BUG/MEDIUM: event_hdl: fix uninitialized value in async mode when no data is provided
    - BUG/MEDIUM: http-ana: Reset request flag about data sent to perform a L7 retry
    - BUG/MINOR: h1-htx: Use default reason if not set when formatting the response
    - BUG/MINOR: signal: register default handler for SIGINT in signal_init()
    - BUG/MINOR: quic: remove startup alert if conn socket-owner unsupported
    - MINOR: mux-h2/traces: add a missing trace on negative initial window size
    - CLEANUP: mux-h2/traces: reword certain ambiguous traces
    - BUG/MINOR: server-state: Fix expiration date of srvrq_check tasks

10 months agoBUG/MINOR: server-state: Fix expiration date of srvrq_check tasks
Christopher Faulet [Wed, 11 Dec 2024 08:23:28 +0000 (09:23 +0100)]
BUG/MINOR: server-state: Fix expiration date of srvrq_check tasks

"hold.timeout" was used as expiration date for srvrq_check tasks. But it is
not accurrate. The expiration date must be based on the resolution timeouts
instead (resolve and retry).

The purpose of srvrq_check task is to clean up the server resolution status
when outdated info are inherited from the state file. Using "hold.timeout"
is not accurrate here because hold timeouts concern the resolution response
items not the resolution status of servers. It may be set to a huge value or
0. The expiration date of these tasks must be based on the resolution
timeouts instead.

So now the ("timeout resolve" + resolve_retries * "timeout retry") value is
used.

This patch should fix the issue #2816. It must be backported to all stable
versions.

(cherry picked from commit 647a2906626e9c3d9c3349d338a35798325496f2)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 3746a7d0639ced74bb9f7cff79181be9a0f18e56)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoCLEANUP: mux-h2/traces: reword certain ambiguous traces
Willy Tarreau [Fri, 6 Dec 2024 17:24:38 +0000 (18:24 +0100)]
CLEANUP: mux-h2/traces: reword certain ambiguous traces

Some h2 traces were not very clear, let's reword them a bit.

(cherry picked from commit 7c8e9420a23584c7f366aacbfeb308d949f5c7b3)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit ff64cbe2092bd5a6a2874d7b44afe322f6348e41)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoMINOR: mux-h2/traces: add a missing trace on negative initial window size
Willy Tarreau [Fri, 6 Dec 2024 16:30:05 +0000 (17:30 +0100)]
MINOR: mux-h2/traces: add a missing trace on negative initial window size

When a negative initial windows size is reported, we're going to close
the connection, so it's important to report a trace to explain why!
This should be backported at least to 3.1 and possibly 3.0 (adapting the
context since there's no glitches there).

(cherry picked from commit 86823c828f983bd986b150542c7a6482d60b291d)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 35bb79b797b85f8b89c87b8b925bc50f6f25865c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoBUG/MINOR: quic: remove startup alert if conn socket-owner unsupported
Amaury Denoyelle [Wed, 4 Dec 2024 15:25:03 +0000 (16:25 +0100)]
BUG/MINOR: quic: remove startup alert if conn socket-owner unsupported

QUIC relies on several advanced network API features from the kernel to
perform optimally. Checks are performed during startup to ensure that
these features are supported. A fallback is automatically performed for
every incompatible feature.

Besides the automatic fallback mechanism, a message is also reported to
the user at the same time. Previously, alert level was used, but it is
incorrect as it is reserved for unrecoverable errors which should
prevent haproxy to start. Warning level could be used, but this can
annoy users running with zero-warning mode.

This patch removes the alert message when 'socket-owner connection' mode
cannot be activated. Convert the message to a diag level. This allows
users to start without forcing configuration modification to hide a
warning. Besides, several feature fallback such as the polling mechanism
does not emit any warning either, so it's better to adopt a similar
behavior for QUIC features.

This must be backported up to 2.8.

(cherry picked from commit 6fed219fd786f3fdca155f686cf2fa2f9e572697)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 24fa1cc97310e436607f64aa1ce3fd4330a26597)
[cf: ctx adjt]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoBUG/MINOR: signal: register default handler for SIGINT in signal_init()
Valentine Krasnobaeva [Mon, 2 Dec 2024 13:47:17 +0000 (14:47 +0100)]
BUG/MINOR: signal: register default handler for SIGINT in signal_init()

When haproxy is launched in a background and in a subshell (see example below),
according to POSIX standard (2.11. Signals and Error Handling), it inherits
from the subshell SIG_IGN signal handler for SIGINT and SIGQUIT.

$ (./haproxy -f env4.cfg &)

So, when haproxy is lanched like this, it doesn't stop upon receiving
the SIGINT. This can be a root cause of some unexpected timeouts, when haproxy
is started under VTest, as VTest sends to the process SIGINT in order to
terminate it. To fix this, let's explicitly register the default signal
handler for the SIGINT in signal_init() initcall.

This should be backported in all stable versions.

(cherry picked from commit d3c20b02469dea6f46369bb91965d8b4924bb2b7)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit a78b02f37d70366ef5afd308de48e8e2c4b54b4a)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoBUG/MINOR: h1-htx: Use default reason if not set when formatting the response
Christopher Faulet [Fri, 29 Nov 2024 13:31:21 +0000 (14:31 +0100)]
BUG/MINOR: h1-htx: Use default reason if not set when formatting the response

When the response status line is formatted before sending it to the client,
if there is no reason set, HAProxy should add one that matches the status
code, as stated in the configuration manual. However it is not performed.

It is possible to hit this bug when the response comes from a H2 server,
because there is no reason field in HTTP/2 and above.

This patch should fix the issue #2798. It should be backported to all stable
versions.

(cherry picked from commit 37487ada739fc86e3acb46c9949196f4f15cc9b1)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 736d4e2c3550dc9c56e5f05778457466b3ce13d9)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoBUG/MEDIUM: http-ana: Reset request flag about data sent to perform a L7 retry
Christopher Faulet [Thu, 28 Nov 2024 09:01:41 +0000 (10:01 +0100)]
BUG/MEDIUM: http-ana: Reset request flag about data sent to perform a L7 retry

It is possible to loose the request after several L7 retries, leading to
crashes, because the request channel flag stating some data were sent is not
properly reset.

When a L7 retry is performed, some flags on different entities must be reset
to be sure a new connection will be properly retried, just like it was the
first one, mainly because there was no connection establishment failure. One
of them, on the request channel, is not reset. The flag stating some data
were already sent. It is annoying because this flag is used during the
connection establishment to know if an error is triggered at the connection
level or at the data level. In the last case, the error must be handled by
the HTTP response analyzer, to eventually perform another L7 retry.

Because CF_WROTE_DATA flag is not removed when a L7 retry is performed, a
subsequent connection establishment error may be handled as a L7 error while
in fact the request was never sent. It also means the request was never
saved in the buffer used to performed L7 retries. Thus, on the next L7
retires, the request is just lost. This forecefully leads to a bunch of
undefined behavior. One of them is a crash, when the request is used to
perform the load-balancing.

This patch should fix issue #2793. It must be backported to all stable
versions.

(cherry picked from commit 62f37801c881f68060cedb7a74b5b8cb5fcfec81)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit d0129d2c2a408a9dabd486ee129f3ec8b0199270)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoBUG/MEDIUM: event_hdl: fix uninitialized value in async mode when no data is provided
Aurelien DARRAGON [Fri, 29 Nov 2024 07:42:01 +0000 (08:42 +0100)]
BUG/MEDIUM: event_hdl: fix uninitialized value in async mode when no data is provided

In _event_hdl_publish(), when we prepare the asynchronous event and no
<data> was provided (set to NULL), we forgot to initialize the _data
event_hdl_async_event struct member to NULL, which leads to uninitialized
reads in event_hdl_async_free_event() when the event is freed:

==1002331== Conditional jump or move depends on uninitialised value(s)
==1002331==    at 0x35D9D1: event_hdl_async_free_event (event_hdl.c:224)
==1002331==    by 0x1CC8EC: hlua_event_runner (hlua.c:9917)
==1002331==    by 0x39AD3F: run_tasks_from_lists (task.c:641)
==1002331==    by 0x39B7B4: process_runnable_tasks (task.c:883)
==1002331==    by 0x314B48: run_poll_loop (haproxy.c:2976)
==1002331==    by 0x315218: run_thread_poll_loop (haproxy.c:3190)
==1002331==    by 0x18061D: main (haproxy.c:3747)

The bug severity was set to MEDIUM because of its nature, and it's best
if this patch can be backported up to 2.8. But in practise it can only be
triggered with events that don't provide optional data: since PAT_REF
events are the first native events making use of this feature, this bug
shouldn't be an issue before f72a66e ("MINOR: pattern: publish event_hdl
events on pat_ref updates")

(cherry picked from commit dd56616067d19060425940f6906cefe6efcd1955)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit 5b4381c19fbe87ad2972110330c59e1f231449ba)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoBUG/MINOR: log: fix lf_text() behavior with empty string
Aurelien DARRAGON [Thu, 28 Nov 2024 11:03:17 +0000 (12:03 +0100)]
BUG/MINOR: log: fix lf_text() behavior with empty string

As reported by Baptiste in GH #2797, if a logformat alias leveraging
lf_text() ends up printing nothing (empty string), the whole logformat
evaluation stops, leading garbage log message.

This bug was introduced during 3.0 cycle in fcb7e4b ("MINOR: log: add
lf_rawtext{_len}() functions"). At that time I genuinely thought that
if strlcpy2() returned 0, it was due to a lack of space, actually
forgetting that the function may simply be called with an empty string.

Because of that, lf_text() would return NULL if called with an empty
string, and since all lf_*() helpers are expected to return NULL on
error, this explains why the logformat evaluation immediately stops in
this case.

To fix the issue, let's simply consider that strlcpy2() returning 0 is
not an error, like it was already the case before.

It should be backported in 3.1 and 3.0 with fcb7e4b.

(cherry picked from commit 3e470471b7e0ec113807f6981699fda9538e7ffc)
Signed-off-by: Willy Tarreau <w@1wt.eu>
(cherry picked from commit ef8324f124f1ba0a98648edd49723ee2b8819bbe)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoMINOR: mux-quic: Don't send an emtpy H3 DATA frame during zero-copy forwarding
Christopher Faulet [Tue, 4 Jun 2024 17:01:18 +0000 (19:01 +0200)]
MINOR: mux-quic: Don't send an emtpy H3 DATA frame during zero-copy forwarding

It may only happens when there is no data to forward but a last stream frame
must be sent with the FIN bit. It is not invalid, but it is useless to send
an empty H3 DATA frame in that case.

(cherry picked from commit 6697e87ae5e1f569dc87cf690b5ecfc049c4aab0)
[ad: This patch was merely considered as an optimization. However, it
 is in fact mandatory as it fixes a bug on QUIC zero-copy
 implementation. As such, it must be backported up to 2.9.

This bug can happen when iobuf data is null in done_ff, indicating that
no data were transferred. Despite this, qcc_send_stream() was always
called with data incorrectly incremented to iobuf offset, which is equal
to HTTP/3 frame header length. This could cause garbage data emission by
QUIC MUX. The most visible effect is that it provokes a BUG_ON() crash
when QCS instance is released due to Tx offsets desynchronization.

This bug is related to github issue #2678.]

Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>

10 months agoBUG/MEDIUM: sock: Remove FD_POLL_HUP during connect() if FD_POLL_ERR is not set
Christopher Faulet [Wed, 27 Nov 2024 09:04:45 +0000 (10:04 +0100)]
BUG/MEDIUM: sock: Remove FD_POLL_HUP during connect() if FD_POLL_ERR is not set

epoll_wait() may return EPOLLUP and/or EPOLLRDHUP after an asynchronous
connect(), to indicate that the peer accepted the connection then
immediately closed before epoll_wait() returned. When this happens,
sock_conn_check() is called to check whether or not the connection correctly
established, and after that the receive channel of the socket is assumed to
already be closed. This lets haproxy send the request at best (if RDHUP and
not HUP) then immediately close.

Over the last two years, there were a few reports about this spuriously
happening on connections where network captures proved that the server had
not closed at all (and sometimes even received the request and responded to
it after haproxy had closed). The logs show that a successful connection is
immediately reported on error after the request was sent. After
investigations, it appeared that a EPOLLUP, or eventually a EPOLLRDHUP, can
be reported by epool_wait() during the connect() but in sock_conn_check(),
the connect() reports a success. So the connection is validated but the HUP
is handled on the first receive and an error is reported.

The same behavior could be observed on health-checks, leading HAProxy to
consider the server as DOWN while it is not.

The only explanation at this point is that it is a kernel bug, notably
because it does not even match the documentation for connect() nor epoll. In
addition for now it was only observed with Ubuntu kernels 5.4 and 5.15 and
was never reproduced on any other one.

We have no reproducer but here is the typical strace observed:

socket(AF_INET, SOCK_STREAM, IPPROTO_IP) = 114
fcntl(114, F_SETFL, O_RDONLY|O_NONBLOCK) = 0
setsockopt(114, SOL_TCP, TCP_NODELAY, [1], 4) = 0
connect(114, {sa_family=AF_INET, sin_port=htons(11000), sin_addr=inet_addr("A.B.C.D")}, 16) = -1 EINPROGRESS (Operation now in progress)
epoll_ctl(19, EPOLL_CTL_ADD, 114, {events=EPOLLIN|EPOLLOUT|EPOLLRDHUP, data={u32=114, u64=114}}) = 0
epoll_wait(19, [{events=EPOLLIN, data={u32=15, u64=15}}, {events=EPOLLIN, data={u32=151, u64=151}}, {events=EPOLLIN, data={u32=59, u64=59}}, {events=EPOLLIN|EPOLLRDHUP, data={u32=114, u64=114}}], 200, 0) = 4
epoll_ctl(19, EPOLL_CTL_MOD, 114, {events=EPOLLOUT, data={u32=114, u64=114}}) = 0
epoll_wait(19, [{events=EPOLLOUT, data={u32=114, u64=114}}, {events=EPOLLIN, data={u32=15, u64=15}}, {events=EPOLLIN, data={u32=10, u64=10}}, {events=EPOLLIN, data={u32=165, u64=165}}], 200, 0) = 4
connect(114, {sa_family=AF_INET, sin_port=htons(11000), sin_addr=inet_addr("A.B.C.D")}, 16) = 0
sendto(114, "POST "..., 1009, MSG_DONTWAIT|MSG_NOSIGNAL, NULL, 0) = 1009
close(114)                              = 0

Some ressources about this issue:
  - https://www.spinics.net/lists/netdev/msg876470.html
  - https://github.com/haproxy/haproxy/issues/1863
  - https://github.com/haproxy/haproxy/issues/2368

So, to workaround the issue, we have decided to remove FD_POLL_HUP flag on
the FD during the connection establishement if FD_POLL_ERR is not reported
too in sock_conn_check(). This way, the call to connect() is able to
validate or reject the connection. At the end, if the HUP or RDHUP flags
were valid, either connect() would report the error itself, or the next
recv() would return 0 confirming the closure that the poller tried to
report. EPOLL_RDHUP is only an optimization to save a syscall anyway, and
this pattern is so rare that nobody will ever notice the extra call to
recv().

Please note that at least one reporter confirmed that using poll() instead
of epoll() also addressed the problem, so that can also be a temporary
workaround for those discovering the problem without the ability to
immediately upgrade.

The event is accounted via a COUNT_IF(), to be able to spot it in future
issue. Just in case.

This patch should fix the issue #1863 and #2368. It may be related
to #2751. It should be backported as far as 2.4. In 3.0 and below, the
COUNT_IF() must be removed.

(cherry picked from commit 7262433183f590377ace31ff96b1fafa4525b7c2)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
(cherry picked from commit b369bdcddfab9627cc3bacc0e75c9e94ac3b24fa)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoBUG/MEDIUM: http-ana: Don't release too early the L7 buffer
Christopher Faulet [Mon, 25 Nov 2024 21:05:27 +0000 (22:05 +0100)]
BUG/MEDIUM: http-ana: Don't release too early the L7 buffer

In some cases, the buffer used to store the request to be able to perform a
L7 retry is released released too early, leading to a crash because a retry
is performed with an empty request.

First, there is a test on invalid 101 responses that may be caught by the
"junk-response" retry policy. Then, it is possible to get an error
(empty-response, bad status code...) after an interim response. In both
cases, the L7 buffer is already released while it should not.

To fix the issue, the L7 buffer is now released at the end of the
AN_RES_WAIT_HTTP analyser, but only when a response was successfully
received and processed. In all error cases, the stream is quickly released,
with the L7 buffer. So there is no leak and it is safer this way.

This patch may fix the issue #2793. It must be as far as 2.4.

(cherry picked from commit dc15581c02171eeb49ef3ffbab0f583f38482b4c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoDEV: lags/show-sess-to-flags: Properly handle fd state on server side
Christopher Faulet [Mon, 25 Nov 2024 20:57:27 +0000 (21:57 +0100)]
DEV: lags/show-sess-to-flags: Properly handle fd state on server side

It must be handled as an hexadecimal value.

(cherry picked from commit ceb80aed579bab9d8db38aa87790bc04b5c9767a)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoBUG/MAJOR: quic: fix wrong packet building due to already acked frames
Frederic Lecaille [Mon, 25 Nov 2024 10:14:20 +0000 (11:14 +0100)]
BUG/MAJOR: quic: fix wrong packet building due to already acked frames

If a packet build was asked to probe the peer with frames which have just
been acked, the frames build run by qc_build_frms() could be cancelled  by
qc_stream_frm_is_acked() whose aim is to check that current frames to
be built have not been already acknowledged. In this case the packet build run
by qc_do_build_pkt() is not interrupted, leading to the build of an empty packet
which should be ack-eliciting.

This is a bug detected by the BUG_ON() statement in qc_do_build_pk():

    BUG_ON(qel->pktns->tx.pto_probe &&
           !(pkt->flags & QUIC_FL_TX_PACKET_ACK_ELICITING));

Thank you to @Tristan971 for having reported this issue in GH #2709

This is an old bug which must be backported as far as 2.6.

(cherry picked from commit 96b2641fc8ce58eb1875e7b525c57e58e4b794c3)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoBUG/MAJOR: mux-h1: Properly handle wrapping on obuf when dumping the first-line
Christopher Faulet [Thu, 21 Nov 2024 21:01:12 +0000 (22:01 +0100)]
BUG/MAJOR: mux-h1: Properly handle wrapping on obuf when dumping the first-line

The formatting of the first-line, for a request or a response, does not
properly handle the wrapping of the output buffer. This may lead to a data
corruption for the current response or eventually for the previous one.

Utility functions used to format the first-line of the request or the
response rely on the chunk API. So it is not expected to pass a buffer that
wraps. Unfortunatly, because of a change performed during the 2.9 dev cycle,
the output buffer was direclty used instead of a non-wrapping buffer created
from it with b_make() function. It is not an issue for the request because
its start-line is always the first block formatted in the output buffer. But
for the response, the output may be not empty and may wrap. In that case,
the response start-line is dumped at a random position in the buffer,
corrupting data. AFAIK, it is only an issue if the HTTP request pipelining
is used.

To fix the issue, we now take care to create a non-wapping buffer from the
output buffer.

This patch should fix issues #2779 and #2996. It must be backported as far as
2.9.

(cherry picked from commit b150ae46dd97caa5050d8abefc1d9b619ab5ab9a)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoBUG/MEDIUM: pools/memprofile: always clean stale pool info on pool_destroy()
Willy Tarreau [Thu, 21 Nov 2024 10:30:03 +0000 (11:30 +0100)]
BUG/MEDIUM: pools/memprofile: always clean stale pool info on pool_destroy()

There's actually a problem with memprofiles: the pool pointer is stored
in ->info but some pools are replaced during startup, such as the trash
pool, leaving a dangling pointer there, that may randomly report crap or
even crash during "show profile memory".

Let's make pool_destroy() call memprof_remove_stale_info() added
by previous patch so that these entries are properly unregistered.

This must be backported along with the previous patch (MINOR:
activity/memprofile: offer a function to unregister stale info) as
far as 2.8.

(cherry picked from commit ed3ed358676edf058663bde7ec6098b51f8bc745)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoMINOR: activity/memprofile: offer a function to unregister stale info
Willy Tarreau [Thu, 21 Nov 2024 10:27:52 +0000 (11:27 +0100)]
MINOR: activity/memprofile: offer a function to unregister stale info

There's actually a problem with memprofiles: the pool pointer is stored
in ->info but some pools are replaced during startup, such as the trash
pool, leaving a dangling pointer there.

Let's complete the API with a new function memprof_remove_stale_info()
that will remove all stale references to this info pointer. It's also
present when USE_MEMORY_PROFILING is not set so as to ease the job on
callers.

(cherry picked from commit 859341c1ec583c586ef36db0b63cd84f3843bfab)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoBUG/MINOR: activity/memprofile: reinitialize the free calls on DSO summary
Willy Tarreau [Thu, 21 Nov 2024 14:26:23 +0000 (15:26 +0100)]
BUG/MINOR: activity/memprofile: reinitialize the free calls on DSO summary

In commit 401fb0e87a ("MINOR: activity/memprofile: show per-DSO stats")
we added a summary per DSO. However the free calls/tot were not initialized
when creating a new entry because initially they were applied to any entry,
but since we don't update free calls for non-free capable callers, we still
need to reinitialize these entries when reassigning one. Because of this
bug, a "show profiling memory" output can randomly show highly negative
values on the DSO lines if it turns out that the DSO entry was created on
an alloc instead of a realloc/free.

Since the commit above was backported to 2.9, this one must go there as
well.

(cherry picked from commit c42a2b8c945d1b45672a2b1715dfa586daaec657)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoBUG/MEDIUM: wdt: fix the stuck detection for warnings
Willy Tarreau [Thu, 21 Nov 2024 18:11:18 +0000 (19:11 +0100)]
BUG/MEDIUM: wdt: fix the stuck detection for warnings

If two slow tasks trigger one warning even a few seconds apart, the
watchdog code will mistakenly take this for a definite stuck task and
kill the process. The reason is that since commit 148eb5875f ("DEBUG:
wdt: better detect apparently locked up threads and warn about them")
the updated ctxsw count is not the correct one, instead of updating
the private counter it resets the public one, preventing it from making
progress and making the wdt believe that no progress was made. In
addition the initial value was read from [tid] instead of [thr].

Please note that another fix is needed in debug_handler() otherwise the
watchdog will fire early after the first warning or thread dump.

A simple test for this is to issue several of these commands back-to-back
on the CLI, which crashes an unfixed 3.1 very quickly:

  $ socat /tmp/sock1 - <<< "expert-mode on; debug dev loop 1000"

This needs to be backported to 2.9 since the fix above was backported
there. The impact on 3.0 and 2.9 is almost inexistent since the watchdog
there doesn't apply the shorter warning delay, so the first call already
indicates that the thread is stuck.

(cherry picked from commit 24ce001771a7609b2a3902fc1f851668ef176c59)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoBUG/MEDIUM: debug: don't set the STUCK flag from debug_handler()
Willy Tarreau [Thu, 21 Nov 2024 18:19:46 +0000 (19:19 +0100)]
BUG/MEDIUM: debug: don't set the STUCK flag from debug_handler()

Since 2.0 with commit e6a02fa65a ("MINOR: threads: add a "stuck" flag
to the thread_info struct"), the TH_FL_STUCK flag was set by the
debugger to flag that a thread was stuck and report it in the output.

However, two commits later (2bfefdbaef "MAJOR: watchdog: implement a
thread lockup detection mechanism"), this flag was used to detect that
a thread had already been reported as stuck. The problem is that it
seldom happens that a "show threads" command instantly crashes because
it calls debug_handler(), which sets the flag, and if the watchdog timer
was about to trigger before going back to the scheduler, the watchdog
believes that the thread has been stuck for a while and will kill the
process.

The issue was magnified in 3.1 with the lower-delay warning, because
it's possible for a thread to die on the next wakeup after the first
warning (which calls debug_handler() hence sets the STUCK flag).

One good approach would have been to use two distinct flags, one for
"stuck" as reported by the debug handler, and one for "stuck" as seen
by the watchdog. However, one could also argue that since the second
commit, given that the wdt monitors the threads, there's no point any
more for the debug handler to set the flag itself. Removing this code
means that two consecutive "show threads" will not report "stuck" until
the watchdog sets it, which aligns better with expectations.

This can be backported to all stable releases. This code has changed a
bit over time, the "if" block and the harmless variables just need to
be removed.

(cherry picked from commit 1151fe68186cf862882f147de208c509c25d525e)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoDOC: config: Improve documentation of tune.http.maxhdr directive
Christopher Faulet [Wed, 20 Nov 2024 17:02:35 +0000 (18:02 +0100)]
DOC: config: Improve documentation of tune.http.maxhdr directive

The description was inproved to clrealy mentionned it is applied on received
requests and responses. In addition, a comment was added about HTTP/2 and
HTTP/3 limitation when messages are encoded to be sent.

(cherry picked from commit e863d8d6814224961724157c605c77ddab85cbae)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoBUG/MEDIUM: h3: Increase max number of headers when sending headers
Christopher Faulet [Wed, 20 Nov 2024 16:14:56 +0000 (17:14 +0100)]
BUG/MEDIUM: h3: Increase max number of headers when sending headers

In the same way than for the H2, the maximum number of headers that can be
encoded when headers are sent must be increased to match the limit imposed
when they are received.

Reasons are the sames. On receive path, the maximum number of headers
accepted must be higher than the configured limit to be able to handle
pseudo headers and cookies headers. On the sending path, the same limit must
be applied because the pseudo headers will consume some extra slots and the
cookie header could be splitted.

This patch should be backported as far as 2.6.

(cherry picked from commit 3bd9a9e7d7a8d7869015eaf041b3ae7a0761c1d4)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoBUG/MEDIUM: h3: Properly limit the number of headers received
Christopher Faulet [Wed, 20 Nov 2024 16:20:05 +0000 (17:20 +0100)]
BUG/MEDIUM: h3: Properly limit the number of headers received

The number of headers are limited before the decoding but pseudo headers and
cookie headers consume extra slots. In practice, this lowers the maximum number
of headers that can be received.

To workaround this issue, the limit is doubled during the frame decoding to be
sure to have enough extra slots. And the number of headers is tested against the
configured limit after the HTX message was created to be able to report an
error. Unfortunatly no parsing error are reported because the QUIC multiplexer
is not able to do so for now.

The same is performed on trailers to be consistent with H2.

This patch should be backported as far as 2.6.

(cherry picked from commit 785e63335374a6db8ef35205cdb36ea726710061)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>

10 months agoBUG/MEDIUM: mux-h2: Check the number of headers in HEADERS frame after decoding
Christopher Faulet [Wed, 20 Nov 2024 15:27:34 +0000 (16:27 +0100)]
BUG/MEDIUM: mux-h2: Check the number of headers in HEADERS frame after decoding

There is no explicit test on the number of headers when a HEADERS frame is
received. It is implicitely limited by the size of the header list. But it
is twice the configured limit to be sure to decode the frame.

So now, a check is performed after the HTX message was created. This way, we
are sure to not exceed the configured limit after the decoding stage. If
there are too many headers, a parsing error is reported.

Note the same is performed on the trailers.

This patch should patially address the issue #2685. It should be backported
to all stable versions.

(cherry picked from commit 63d2760dfa679bea4b7a61a1a8702af23cf26e75)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>