Commit Graph

22487 Commits

Author SHA1 Message Date
William Lallemand
aeb5cbdb23 DOC: configuration: add details about crt-store in bind "crt" keyword
Add some details about the certificate storage cache system in the "crt"
bind keyword.

This should be backported to 3.0. Fix issue #2618.

(cherry picked from commit ba37ad41b26a6ba83581821c13426a7fbe4d2494)
Signed-off-by: William Lallemand <wlallemand@haproxy.com>
2024-07-02 16:49:08 +02:00
Christopher Faulet
933f35fe26 BUG/MEDIUM: stick-table: Decrement the ref count inside lock to kill a session
When we try to kill a session, the shard must be locked before decrementing
the ref count on the session. Otherwise, the ref count can fall to 0 and a
purge task (stktable_trash_oldest or process_table_expire) may release the
session before we have the opportunity to acquire the lock on the shard to
effectively kill the session. This could lead to a double free.

Here is the scenario:

    Thread 1                                 Thread 2

  sktsess_kill(ts)
    if (ATOMIC_DEC(&ts->ref_cnt) != 0)
        return
                   /* here the ref count is 0 */

                                       stktable_trash_oldest()
                                          LOCK(&sh_lock)
                                          if (!ATOMIC_LOAD(&ts->ref_cnf))
                                              __stksess_free(ts)
                                          UNLOCK(&sh_lock)

                  /* here the session was released */
    LOCK(&sh_lock)
    __stksess_free(ts)  <--- double free
    UNLOCK(&sh_lock)

The bug was introduced in 2.9 by the commit 7968fe3889 ("MEDIUM:
stick-table: change the ref_cnt atomically"). The ref count must be
decremented inside the lock for stksess_kill() and sktsess_kill_if_expired()
function.

This patch should fix the issue #2611. It must be backported as far as 2.9. On
the 2.9, there is no sharding. All the table is locked. The patch will have to
be adapted.

(cherry picked from commit 9357873641c5de29b169848fc1c808747818a1eb)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-26 15:16:25 +02:00
Aurelien DARRAGON
08cc27085e BUG/MINOR: hlua: report proper context upon error in hlua_cli_io_handler_fct()
As a result of copy pasting, hlua_cli_io_handler_fct() used to report lua
exceptions like E_ETMOUT as "Lua converter" instead of "Lua cli".

Let's fix that.

It could be backported to all stable versions.

[ada: for older versions, HLUA_E_BTMOUT case didn't exist so it has to be
 skipped]

(cherry picked from commit 185d230e2c615ee723270c81e4eb1eec20181918)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-26 15:16:24 +02:00
Willy Tarreau
d5c8676f4c DEV: flags/show-fd-to-flags: adapt to recent versions
The script hadn't been updated since it was introduced, and the
hard-coded field 12 doesn't match anymore (it's 16 now). Let's just
use "grep -o cflg..." to extract the desired part more flexibly.
This can be backported at least to 3.0, probably further, but it
will need to be tested prior to this. Better not bring it too far,
it's only used when debugging.

(cherry picked from commit a14c7d194ad27f9f84c9d42aab953a162999252a)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-26 15:16:24 +02:00
Amaury Denoyelle
078cb85b89 BUG/MINOR: quic: fix BUG_ON() on Tx pkt alloc failure
On quic_tx_packet allocation failure, it is possible to trigger BUG_ON()
crash on INITIAL packet building. This statement is responsible to
ensure INITIAL packets are padded to 1.200 bytes as required. If a
packet on higher encryption level allocation fails, PADDING frame cannot
properly encoded, despite the INITIAL packet properly built.

This crash happens due to qc_txb_store() invokation after quic_tx_packet
allocation failure to validate already built packets. However, this
statement is unneeded as qc_purge_tx_buf() is called just after. Simply
remove qc_txb_store() to fix this issue.

This was detected using -dMfail.

This should be backported up to 2.6.

(cherry picked from commit d5376b7a874776b4d5d79f9b746d4654df796f85)
[cf: ctx adjt]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-26 15:16:24 +02:00
Amaury Denoyelle
83bd975406 BUG/MINOR: h3: fix BUG_ON() crash on control stream alloc failure
BUG_ON() from qcc_set_error() is triggered on HTTP/3 control stream
allocation failure. This is caused because both h3_finalize() and
qcc_init_stream_local() call qcc_set_error() which is forbidden to
prevent error code erasure.

Fix this by removing qcc_set_error() invocation from h3_finalize() on
allocation failure. Note that this function is still responsible to use
it on SETTING frame emission failure.

This was detected using -dMfail.

This must be backported up to 3.0.

(cherry picked from commit 5718c67c19766c87bb68b7624e1873a887fbbaf1)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-26 15:16:24 +02:00
Amaury Denoyelle
96c254fd3e BUG/MINOR: mux-quic: fix crash on qcs SD alloc failure
Since the following commit, sedesc are created since QCS instantiation
in qcs_new().
  086e51017e
  BUG/MEDIUM: mux-quic: Create sedesc in same time of the QUIC stream

However, sedesc is initialized before other QCS mandatory fields. If
sedesc allocation fails, a crash would occur on qcs_free() invocation
for QCS early release. To fix this, delay sedesc allocation until
function end.

This bug was detected using -dMfail.

This should be backported up to 2.6.

(cherry picked from commit 3aded1d3752a12af9b8e48f445218230e6967a06)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-26 15:16:24 +02:00
Amaury Denoyelle
5ab8106639 BUG/MINOR: h3: fix crash on STOP_SENDING receive after GOAWAY emission
After emitting a HTTP/3 GOAWAY frame, opening of streams higher than
advertised ID was prevented. h3_attach operation would return success
but without allocating H3S stream context for QCS. In addition, the
stream would be immediately scheduled for RESET_STREAM emission.

Despite the immediate stream close, the current is not sufficient enough
and can cause crashes. When of this occurence can be found if
STOP_SENDING is the first frame received for a stream. A crash would
occur under qcc_recv_stop_sending() after h3_attach invokation, when
h3_close() is used which try to access to H3S context.

To fix this, change h3_attach API. In case of success, H3S stream
context is always allocated, even if the stream will be scheduled for
immediate close. This renders the code more reliable.

This crash should be extremely rare, as it can only happen after GOAWAY
emission, which is only used on soft-stop or reload.

This should solve the second crash occurence reported on GH #2607.

This must be backported up to 2.8.

(cherry picked from commit 85838822ba37a92b2dcc43205a07c2b33208b985)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-26 15:16:24 +02:00
Aurelien DARRAGON
75a9d54c67 DOC: api/event_hdl: small updates, fix an example and add some precisions
Fix an example suggesting that using EVENT_HDL_SUB_TYPE(x, y) with y being
0 was valid. Then add some notes to explain how to use
EVENT_HDL_SUB_FAMILY() and EVENT_HDL_SUB_TYPE() with valid values.

Also mention that the feature is available starting from 2.8 and not 2.7.
Finally, perform some purely cosmetic updates.

This could be backported in 2.8.

(cherry picked from commit 13e0972aeac275137b429163def950af88fecd46)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-26 15:16:24 +02:00
Amaury Denoyelle
a4c5cae3bb SCRIPTS: git-show-backports: do not truncate git-show output
git-show-backports lists a git-show command which can be used to inspect
all commits subject to backport. This command specifies formatting
option to reproduce default git-show output, especially for commit
messages indented with 4 spaces character. However, it also add wrapping
on message line longer than 72 characters. This reduce lisibility of
messages where large info are written such as backtraces.

Improve this by changing git-show format option. Use a limit value of 0
to disable wrapping while preserving indentation.

This could be backported to every stable version to simplify backporting
process.

(cherry picked from commit b27470fd1d06acd6dc33161e1fdb6743f72770df)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-26 15:16:24 +02:00
Amaury Denoyelle
9ecf24b43e BUG/MAJOR: quic: fix padding with short packets
QUIC sending functions were extended to be more flexible. Of all the
changes, they support now iterating over a variable instance of QEL
instance of only 2 previously. This change has rendered PADDING emission
less previsible, which was adjusted via the following patch :

  a60609f1aa3e5f61d2a2286fdb40ebf6936a80ee
  BUG/MINOR: quic: fix padding of INITIAL packets

Its main purpose was to ensure PADDING would only be generated for the
last iterated QEL instance, to avoid unnecessary padding. In parallel, a
BUG_ON() statement ensure that built INITIAL packets are always padded
to 1.200 bytes as necessary before emitted them.

This BUG_ON() statement caused crash in one particular occurence : when
building datagrams that mixed Initial long packets and 1-RTT short
packets. This last occurence type does not have a length field in its
header, contrary to Long packets. This caused a miscalculation for the
necessary padding size, with INITIAL packets not padded enough to reach
the necessary 1.200 bytes size.

This issue was detected on 3.0.2. It can be reproduced by using 0-RTT
combined with latency. Here are the used commands :

  $ ngtcp2-client --tp-file=/tmp/ngtcp2-tp.txt \
    --session-file=/tmp/ngtcp2-session.txt --exit-on-all-streams-close \
    127.0.0.1 20443 "https://[::]/?s=32o"
  $ sudo tc qdisc add dev lo root netem latency 500ms

Note that this issue cannot be reproduced on current dev version.
Indeed, it seems that the following patch introduce a slight change in
packet building ordering :

  cdfceb10ae136b02e51f9bb346321cf0045d58e0
  MINOR: quic: refactor qc_prep_pkts() loop

This must be backported to 3.0.

This should fix github issue #2609.

(cherry picked from commit c714b6bb55e34c7cd2cb3ff7dbed374e6b6eae65)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-26 15:16:24 +02:00
Aurelien DARRAGON
acb50d3e0e DOC: management: document ptr lookup for table commands
Add missing documentation and examples for the optional ptr lookup method
for table {show,set,clear} commands introduced in commit 9b2717e7 ("MINOR:
stktable: use {show,set,clear} table with ptr"), as initially described in
GH #2118.

It may be backported in 3.0.

(cherry picked from commit 7422f16da3b84829f2ecf3ff393584b5c5682e06)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-26 15:16:24 +02:00
William Lallemand
f06071283a DOC: configuration: fix alphabetical order of bind options
Put the curves, ecdhe, severity-output, v4v6 and v6only keyword at the
right place.

Fix issue #2594.

Could be backported in every stable versions.

(cherry picked from commit 0cc2913aec965dabc579cd90a3d91a440f29967c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-26 15:16:24 +02:00
Aurelien DARRAGON
e8df66a4fc BUG/MEDIUM: proxy: fix email-alert invalid free
In fa90a7d3 ("BUG/MINOR: proxy: fix email-alert leak on deinit()"), I
tried to fix email-alert deinit() leak the simple way by leveraging
existing free_email_alert() helper function which was already used for
freeing email alert settings used in a default section.

However, as described in GH #2608, there is a subtelty that makes
free_email_alert() not suitable for use from free_proxy().

Indeed, proxy 'mailers.name' hint shares the same memory space than the
pointer to the corresponding mailers section (once the proxy is resolved,
name hint is replaced by the pointer to the section). However, since both
values share the same space (through union), we have to take care of not
freeing `mailers.name` once init_email_alert() was called on the proxy.

Unfortunately, free_email_alert() isn't protected against that, causing
double free() during deinit when mailers section is referenced from
multiple proxy sections. Since there is no easy fix, and that the leak in
itself isn't a big deal (fa90a7d3 was simply an opportunistic fix rather
than a must-have given that the leak only occurs during deinit and not
during runtime), let's actually revert the fix to restore legacy behavior
and prevent deinit errors.

Thanks to @snetat for having reported the issue on Github as well as
providing relevant infos to pinpoint the bug.

It should be backported everywhere fa90a7d3 was backported.
[ada: for versions prior to 3.0, simply revert the offending commit using
'git revert' as proxy_free_common() first appears in 3.0]

(cherry picked from commit 8e226682be904a6774f65e90bac0b674888cc293)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-26 15:16:24 +02:00
William Lallemand
98da55645c REGTESTS: ssl: fix some regtests 'feature cmd' start condition
Since patch fde517b ("REGTESTS: wolfssl: temporarly disable some failing
reg-tests") some 'feature cmd' lines have an extra quotation mark, so
they were disable in every cases.

Must be backported to 2.9.

(cherry picked from commit 6da0879083749d5f098b8b2f4d459a70260491d2)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-26 15:16:24 +02:00
Aurelien DARRAGON
4a744814ed DEBUG: hlua: distinguish burst timeout errors from exec timeout errors
hlua burst timeout was introduced in 58e36e5b1 ("MEDIUM: hlua: introduce
tune.lua.burst-timeout").

It is a safety measure that allows to detect when too much time is spent
on a single lua execution (between 2 interruptions/yields), meaning that
the current thread is not able to perform other tasks. Such scenario
should be avoided because it will cause thread contention which may have
negative performance impact and could cause the watchdog to trigger. When
the burst timeout is exceeded, the current Lua execution is aborted and a
timeout error is reported to the user.

Unfortunately, the same error is currently being reported for cumulative
(AKA execution) timeout and for burst timeout, which may be confusing to
the user.

Indeed, "execution timeout" error historically results from the current
hlua context exceeding the total (cumulative) time it's allowed to run.
It is set per lua context using the dedicated tunables:
 - tune.lua.session-timeout
 - tune.lua.task-timeout
 - tune.lua.service-timeout

We've already faced an user report where the user was able to trigger the
burst timeout and got "Lua task: execution timeout." error while the user
didn't set cumulative timeout. Thus the error was actually confusing
because it was indeed the burst timeout which was causing it due to the
use of cpu-intensive call from within the task without sufficient manual
"yield" keypoints around the cpu-intensive call to ensure it runs on a
dedicated scheduler cycle.

In this patch we make it so burst timeout related errors are reported as
"burst timeout" errors instead of "execution timeout" errors (which
in fact became the generic timeout errors catchall with 58e36e5b1).

To do this, hlua_timer_check() now returns a different value depending if
the exeeded timeout is the burst one or the cumulative one, which allows
us to return either HLUA_E_ETMOUT or HLUA_E_BTMOUT in hlua_ctx_resume().

It should improve the situation described in GH #2356 and may possibly be
backported with 58e36e5b1 to improve error reporting if it applies without
resistance.

(cherry picked from commit 983513d901bb7511ea6b1e8c3bb00d58a9d432f2)
[cf: No reason to backport further]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-26 15:16:24 +02:00
Aurelien DARRAGON
8a110626bf BUG/MINOR: log: fix broken '+bin' logformat node option
In 12d08cf912 ("BUG/MEDIUM: log: don't ignore disabled node's options"),
while trying to restore historical node option inheritance behavior, I
broke the '+bin' logformat node option recently introduced in b7c3d8c87c
("MINOR: log: add +bin logformat node option").

Indeed, because of 12d08cf912, LOG_OPT_BIN is not set anymore on
individual nodes even if it was set globally, making the feature unusable.
('+bin' is also used for binary cbor encoding)

What I should have done instead is include LOG_OPT_BIN in the options
inherited from global ones. This is what's being done in this commit.
Misleading comment was adjusted.

It must be backported in 3.0 with 12d08cf912.

(cherry picked from commit 0030f722a2fa574d1e7d90e6f242e4b6a5ace355)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-26 15:16:24 +02:00
Christopher Faulet
a45a8e6235 [RELEASE] Released version 3.0.2
Released version 3.0.2 with the following main changes :
    - MINOR: log: fix "http-send-name-header" ignore warning message
    - BUG/MINOR: proxy: fix server_id_hdr_name leak on deinit()
    - BUG/MINOR: proxy: fix log_tag leak on deinit()
    - BUG/MINOR: proxy: fix email-alert leak on deinit()
    - BUG/MINOR: proxy: fix check_{command,path} leak on deinit()
    - BUG/MINOR: proxy: fix dyncookie_key leak on deinit()
    - BUG/MINOR: proxy: fix source interface and usesrc leaks on deinit()
    - BUG/MINOR: proxy: fix header_unique_id leak on deinit()
    - BUG/MEDIUM: log: fix lf_expr_postcheck() behavior with default section
    - DOC: config: move "hash-key" from proxy to server options
    - DOC: config: add missing section hint for "guid" proxy keyword
    - DOC: config: add missing context hint for new server and proxy keywords
    - BUG/MINOR: promex: Skip resolvers metrics when there is no resolver section
    - MINOR: proxy: add proxy_free_common() helper function
    - BUG/MEDIUM: proxy: fix UAF with {tcp,http}checks logformat expressions
    - CLEANUP: log/proxy: fix comment in proxy_free_common()
    - BUG/MAJOR: mux-h1: Prevent any UAF on H1 connection after draining a request
    - BUG/MINOR: quic: fix padding of INITIAL packets
    - DOC/MINOR: management: add missed -dR and -dv options
    - DOC/MINOR: management: add -dZ option
    - DOC: management: rename show stats domain cli "dns" to "resolvers"
2024-06-14 15:00:35 +02:00
Aurelien DARRAGON
91c27aa227 DOC: management: rename show stats domain cli "dns" to "resolvers"
In commit f8642ee82 ("MEDIUM: resolvers: rename dns extra counters to
resolvers extra counters"), we renamed "dns" counters to "resolvers", but
we forgot to update the documentation accordingly.

This may be backported to all stable versions.

(cherry picked from commit cf913c2f9019c2264986f38da67bed7bed191a24)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-14 11:22:32 +02:00
Valentine Krasnobaeva
42b8781915 DOC/MINOR: management: add -dZ option
Add some description for missed -dZ command line option in
the "3. Starting HAProxy" chapter.

Need to be backported until 2.9.

(cherry picked from commit 61d66a3d061cfb302f1519e5a774eb7e82f57ab9)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-14 11:22:22 +02:00
Valentine Krasnobaeva
b7c0f49d51 DOC/MINOR: management: add missed -dR and -dv options
Add some description for missed -dR and -dv command line options in
the "3. Starting HAProxy" chapter.

Need to be backported in every stable version.

(cherry picked from commit 27623d8393a3187ca827f752efc1956cbb89cef5)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-14 11:22:18 +02:00
Amaury Denoyelle
275d064212 BUG/MINOR: quic: fix padding of INITIAL packets
API for sending has been extended to support emission on more than 2 QEL
instances. However, this has rendered the PADDING emission for INITIAL
packets less previsible. Indeed, if qc_send() is used with empty QEL
instances, a padding frame may be generated before handling the last QEL
registered, which could cause unnecessary padding to be emitted.

This commit simplify PADDING by only activating it for the last QEL
registered. This ensures that no superfluous padding is generated as if
the minimal INITIAL datagram length is reached, padding is resetted
before handling last QEL instance.

This bug is labelled as minor as haproxy already emit big enough INITIAL
packets coalesced with HANDSHAKE one without needing padding. This
however render the padding code difficult to test. Thus, it may be
useful to force emission on INITIAL qel only without coalescing
HANDSHAKE packet. Here is a sample to reproduce it :

--- a/src/quic_conn.c
+++ b/src/quic_conn.c
@@ -794,6 +794,14 @@ struct task *quic_conn_io_cb(struct task *t, void *context, unsigned int state)
                }
        }

+       if (qc->iel && qel_need_sending(qc->iel, qc)) {
+               struct list empty = LIST_HEAD_INIT(empty);
+               qel_register_send(&send_list, qc->iel, &qc->iel->pktns->tx.frms);
+               if (qc->hel)
+                       qel_register_send(&send_list, qc->hel, &empty);
+               qc_send(qc, 0, &send_list);
+       }
+
        /* Insert each QEL into sending list if needed. */
        list_for_each_entry(qel, &qc->qel_list, list) {
                if (qel_need_sending(qel, qc))

This should be backported up to 3.0.

(cherry picked from commit a60609f1aa3e5f61d2a2286fdb40ebf6936a80ee)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-14 11:21:57 +02:00
Christopher Faulet
c9b7482365 BUG/MAJOR: mux-h1: Prevent any UAF on H1 connection after draining a request
Since 2.9, it is possible to drain the request payload from the H1
multiplexer in case of early reply. When this happens, the upper stream is
detached but the H1 stream is not destroyed. Once the whole request is
drained, the end of the detach stage is finished. So the H1 stream is
destroyed and the H1 connection is ready to be reused, if possible,
otherwise it is released.

And here is the issue. If some data of the next request are received with
last bytes of the drained one, parsing of the next request is immediately
started. The previous H1 stream is destroyed and a new one is created to
handle the parsing.  At this stage the H1 connection may be released, for
instance because of a parsing error. This case was not properly handled.
Instead of immediately exiting the mux, it was still possible to access the
released H1 connection to refresh its timeouts, leading to a UAF issue.

Many thanks to Annika for her invaluable help on this issue.

The patch should fix the issue #2602. It must be backported as far as 2.9.

(cherry picked from commit 0e09cce0fdf104994b37a492e256d3bc37880ddc)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-12 16:14:06 +02:00
Aurelien DARRAGON
34a0a7cc6b CLEANUP: log/proxy: fix comment in proxy_free_common()
Thanks to previous commit, logformat expressions for default proxies are
also postchecked, adjusting a comment that suggests it's not the case.

(cherry picked from commit c6931a4f01a29cb4f36e0b70900a6c97a5a2bdda)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-12 09:31:43 +02:00
Aurelien DARRAGON
a13b459311 BUG/MEDIUM: proxy: fix UAF with {tcp,http}checks logformat expressions
When parsing a logformat expression using parse_logformat_string(), the
caller passes the proxy under which the expression is found as argument.

This information allows the logformat expression API to check if the
expression is compatible with the proxy settings.

Since 7a21c3a ("MAJOR: log: implement proper postparsing for logformat
expressions"), the proxy compatibilty checks are postponed after the proxy
is fully parsed to ensure proxy properties are fully resolved for checks
consistency.

The way it works, is that each time parse_logformat_string() is called for
a given expression and proxy, it schedules the expression for postchecking
by appending the expression to the list of pending expression checks on
the proxy (lf_checks struct). Then, when the proxy is called with the
REGISTER_POST_PROXY_CHECK() hook, it iterates over unchecked expressions
and performs the check, then it removes the expression from its list.

However, I overlooked a special case: if a logformat expression is used
on a proxy that is disabled or a default proxy:
REGISTER_POST_PROXY_CHECK() hook is never called. Because of that, lf
expressions may still point to the proxy after the proxy is freed.

For most logformat expressions, this isn't an issue because they are
stored within the proxy itself, but this isn't the case with
{tcp,http}checks logformat expressions: during deinit() sequence, all
proxies are first cleaned up, and only then shared checks are freed.

Because of that, the below config will trigger UAF since 7a21c3a:

uaf.conf:
  listen dummy
    bind localhost:2222

  backend testback
    disabled
    mode http
    option httpchk
    http-check send hdr test "test"
    http-check expect status 200

haproxy -f uaf.conf -c:

==152096== Invalid write of size 8
==152096==    at 0x21C317: lf_expr_deinit (log.c:3491)
==152096==    by 0x2334A3: free_tcpcheck_http_hdr (tcpcheck.c:84)
==152096==    by 0x2334A3: free_tcpcheck_http_hdr (tcpcheck.c:79)
==152096==    by 0x2334A3: free_tcpcheck_http_hdrs (tcpcheck.c:98)
==152096==    by 0x23365A: free_tcpcheck.part.0 (tcpcheck.c:130)
==152096==    by 0x2338B1: free_tcpcheck (tcpcheck.c:108)
==152096==    by 0x2338B1: deinit_tcpchecks (tcpcheck.c:3780)
==152096==    by 0x2CF9A4: deinit (haproxy.c:2949)
==152096==    by 0x2D0065: deinit_and_exit (haproxy.c:3052)
==152096==    by 0x169BC0: main (haproxy.c:3996)
==152096==  Address 0x52a8df8 is 6,968 bytes inside a block of size 7,168 free'd
==152096==    at 0x484B27F: free (vg_replace_malloc.c:872)
==152096==    by 0x2CF8AD: deinit (haproxy.c:2906)
==152096==    by 0x2D0065: deinit_and_exit (haproxy.c:3052)
==152096==    by 0x169BC0: main (haproxy.c:3996)

To fix the issue, let's ensure in proxy_free_common() that no unchecked
expressions may still point to the proxy after the proxy is freed by
purging the list (DEL_INIT is used to reset list items).

Special thanks to GH user @mhameed who filed a comprehensive issue with
all the relevant information required to reproduce the bug (see GH #2597),
after having first reported the issue on the alpine project bug tracker.

(cherry picked from commit 318c290ff2779c6a8ddf773fb00035266c1f74d4)
[cf: must be backported to 3.0 only]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-12 09:29:33 +02:00
Aurelien DARRAGON
3bb95e2697 MINOR: proxy: add proxy_free_common() helper function
As shown by previous patch series, having to free some common proxy
struct members twice (in free_proxy() and proxy_free_defaults()) is
error-prone: we often overlook one of the two free locations when
adding new features.

To prevent such bugs from being introduced in the future, and also avoid
code duplication, we now have a proxy_free_common() function to free all
proxy struct members that are common to all proxy types (either regular or
default ones).

This should greatly improve code maintenance related to proxy freeing
logic.

(cherry picked from commit 005e4ba715cf54204ea2fc752ba4d44a23eb61bd)
[cf: Required in 3.0 to ease backport of a bug fix]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-12 09:28:39 +02:00
Christopher Faulet
ebd0e8f699 BUG/MINOR: promex: Skip resolvers metrics when there is no resolver section
By default, there is always at least on resolver section, the default one,
based on "/etc/resolv.conf" content. However, it is possible to have no
resolver at all if the file is empty or if any error occurred. Errors are
silently ignored at this stage.

In that case, there was a bug in the Prometheus exporter leading to a crash
because the resolver section list is empty. An invalid resolver entity was
used. To fix the issue we must only take care to not dump resolvers metrics
when there is no resolver.

Thanks to Aurelien to have spotted the offending commit.

This patch should fix the issue #2604. It must be backported to 3.0.

(cherry picked from commit 91fe085943cea52d0c3d04e81f8ecb6a51668b09)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-12 09:13:19 +02:00
Aurelien DARRAGON
abecd956e5 DOC: config: add missing context hint for new server and proxy keywords
To stay consistent with the work started in 54627f991 ("DOC: config: add
context hint for proxy keywords") and 3d4e1e682 ("DOC: config: add context
hint for server keywords"), we add missing context hint for "guid" (both
proxy and server) keyword and "hash-key" server keyword that were added
during 3.0 development.

This may be backported in 3.0.

(cherry picked from commit c157894ba97a40f40f777344041841e423f99c2c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-12 09:13:15 +02:00
Aurelien DARRAGON
1c04c95b83 DOC: config: add missing section hint for "guid" proxy keyword
"guid" proxy keyword added in da754b45 ("MINOR: proxy: implement GUID
support") was lacking the section hint in the keyword description, let's
fix that.

It could be backported in 3.0 with da754b45.

(cherry picked from commit aec02320bdb4628839525c0704a327a812db64a4)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-12 09:12:59 +02:00
Aurelien DARRAGON
56373d1d7c DOC: config: move "hash-key" from proxy to server options
As reported by Ashley Morris, "hash-key" keyword which was introduced in
commit faa8c3e0 ("MEDIUM: lb-chash: Deterministic node hashes based on
server address") doesn't belong to proxy keywords and should be found in
5.2 "Server and default-server options" instead.

It should be backported in 3.0 with faa8c3e0

(cherry picked from commit cdf1d20e8a8eb1db0141a33ea18227d28abd5026)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-12 09:12:40 +02:00
Aurelien DARRAGON
d1a7330086 BUG/MEDIUM: log: fix lf_expr_postcheck() behavior with default section
Since 7a21c3a4ef ("MAJOR: log: implement proper postparsing for logformat
expressions"), logformat expressions stored in a default section are not
postchecked anymore. This is because the REGISTER_POST_PROXY_CHECK() only
evaluates regular proxies. Because of this, proxy options which are
automatically enabled on the proxy depending on the logformat expression
features in use are not set on the default proxy, which means such options
are not passed to the regular proxies that inherit from it (proxies that
and will actually be running the logformat expression during runtime).

Because of that, a logformat expression stored inside a default section
and executed by a regular proxy may not behave properly. Also, since
03ca16f38b ("OPTIM: log: resolve logformat options during postparsing"),
it's even worse because logformat node options postresoving is also
skipped, which may also alter logformat expression encoding feature.

To fix the issue, let's add a special case for default proxies in
parse_logformat_string() and lf_expr_postcheck() so that default proxies
are postchecked on the fly during parsing time in a "relaxed" way as we
cannot assume that the features involved in the logformat expression won't
be compatible with the proxy actually running it since we may have
different types of proxies inheriting from the same default section.

This bug was discovered while trying to address GH #2597.

It should be backported to 3.0 with 7a21c3a4ef and 03ca16f38b.

(cherry picked from commit e4f122f3f46ff7b6e28f3599e8ec2006e2caac37)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-12 09:12:23 +02:00
Aurelien DARRAGON
310c0dd14e BUG/MINOR: proxy: fix header_unique_id leak on deinit()
proxy header_unique_id wasn't cleaned up in proxy_free_defaults(),
resulting in small memory leak if "unique-id-header" was used on a
default proxy section.

It may be backported to all stable versions.

(cherry picked from commit 847c406b9a040193c374daad3269d646dda7dbcc)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-12 09:07:09 +02:00
Aurelien DARRAGON
e724bd3cd8 BUG/MINOR: proxy: fix source interface and usesrc leaks on deinit()
proxy conn_src.iface_name was only freed in proxy_free_defaults(), whereas
proxy conn_src.bind_hdr_name was only freed in free_proxy().

Because of that, using "source usesrc hdr_ip()" in a default proxy, or
"source interface" in a regular or default proxy would cause memory leaks
during deinit.

It may be backported to all stable versions.

(cherry picked from commit 1aa219078dbaeef402b43af24538078fdd875790)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-12 09:06:59 +02:00
Aurelien DARRAGON
6b16cfe38b BUG/MINOR: proxy: fix dyncookie_key leak on deinit()
proxy dyncookie_key wasn't cleaned up in free_proxy(), resulting in small
memory leak if "dynamic-cookie-key" was used on a regular or default
proxy.

It may be backported to all stable versions.

(cherry picked from commit 6f53df3fcfe3dc7220cff862d3ded1601f642931)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-12 09:06:55 +02:00
Aurelien DARRAGON
7680c52b16 BUG/MINOR: proxy: fix check_{command,path} leak on deinit()
proxy check_{command,path} members (used for "external-check" feature)
weren't cleaned up in free_proxy(), resulting in small memory leak if
"external-check command" or "external-check path" were used on a regular
or default proxy.

It may be backported to all stable versions.

(cherry picked from commit 62d0465a96ac847f40e95a9474d8971dac062114)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-12 09:06:52 +02:00
Aurelien DARRAGON
28d69a218e BUG/MINOR: proxy: fix email-alert leak on deinit()
proxy email-alert settings weren't cleaned up in free_proxy(), resulting
in small memory leak if "email-alert to" or "email-alert from" were used
on a regular or default proxy.

It may be backported to all stable versions.

(cherry picked from commit fa90a7d313703cfc4e1b41d258d0d6d470ffe967)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-12 09:06:48 +02:00
Aurelien DARRAGON
cc722a3e5b BUG/MINOR: proxy: fix log_tag leak on deinit()
proxy log_tag wasn't cleaned up in free_proxy(), resulting in small
memory leak if "log-tag" was used on a regular or default proxy.

It may be backported to all stable versions.

(cherry picked from commit 77b192ea3682a2ad56418d2e4379bb763e0b427e)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-12 09:06:45 +02:00
Aurelien DARRAGON
67d4a9507d BUG/MINOR: proxy: fix server_id_hdr_name leak on deinit()
proxy server_id_hdr_name member (used for "http-send-name-header" option)
wasn't cleaned up in free_proxy(), resulting in small memory leak if
"http-send-name-header" was used on a regular or default proxy.

This may be backported to all stable versions.

(cherry picked from commit 99f340958211881dc6b48b6dc960d126179c4bcc)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-12 09:06:39 +02:00
Aurelien DARRAGON
948a83f4e9 MINOR: log: fix "http-send-name-header" ignore warning message
Warning message to indicate that the "http-send-name-header" option is
ignored for backend in "mode log" was referenced using its internal
struct wording instead of public name (as seen in the documentation).

Let's fix that.

It may be backported with c7783fb ("MINOR: log/backend: prevent
"http-send-name-header" use with LOG mode") in 2.9.

(cherry picked from commit e5ccfda9d31db54dfafe618404e79f09883b3ea5)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-12 09:06:32 +02:00
Christopher Faulet
471a1b2f11 [RELEASE] Released version 3.0.1
Released version 3.0.1 with the following main changes :
    - BUG/MINOR: cfgparse: remove the correct option on httpcheck send-state warning
    - BUG/MINOR: tcpcheck: report correct error in tcp-check rule parser
    - BUG/MINOR: tools: fix possible null-deref in env_expand() on out-of-memory
    - DOC: configuration: add an example for keywords from crt-store
    - BUG/MINOR: hlua: use CertCache.set() from various hlua contexts
    - BUG/MEDIUM: h1-htx: Don't state interim responses are bodyless
    - MEDIUM: stconn: Be able to unblock zero-copy data forwarding from done_fastfwd
    - BUG/MEDIUM: mux-quic: Unblock zero-copy forwarding if the txbuf can be released
    - BUG/MINOR: quic: prevent crash on qc_kill_conn()
    - CLEANUP: hlua: use hlua_pusherror() where relevant
    - BUG/MINOR: hlua: don't use lua_pushfstring() when we don't expect LJMP
    - BUG/MINOR: hlua: fix unsafe hlua_pusherror() usage
    - BUG/MINOR: hlua: prevent LJMP in hlua_traceback()
    - BUG/MINOR: hlua: fix leak in hlua_ckch_set() error path
    - CLEANUP: hlua: simplify ambiguous lua_insert() usage in hlua_ctx_resume()
    - BUG/MEDIUM: mux-quic: Don't unblock zero-copy fwding if blocked during nego
    - BUG/MEDIUM: ssl: wrong priority whem limiting ECDSA ciphers in ECDSA+RSA configuration
    - BUG/MEDIUM: ssl: bad auth selection with TLS1.2 and WolfSSL
    - BUG/MINOR: quic: fix computed length of emitted STREAM frames
    - BUG/MINOR: quic: ensure Tx buf is always purged
    - BUG/MEDIUM: stconn/mux-h1: Fix suspect change causing timeouts
    - BUG/MAJOR: mux-h1:  Properly copy chunked input data during zero-copy nego
    - BUG/MINOR: mux-h1: Use the right variable to set NEGO_FF_FL_EXACT_SIZE flag
2024-06-10 16:15:30 +02:00
Christopher Faulet
200734201f BUG/MINOR: mux-h1: Use the right variable to set NEGO_FF_FL_EXACT_SIZE flag
Instead of setting this flag on the ones used for the zero-copy negociation,
it is set on the connection flags used for xprt->rcv_buf()
call. Fortunately, there is no real consequence. The only visible effect is
the chunk size that is written on 8 bytes for no reason.

This patch is related to issue #2598. It must be backported to 3.0.

(cherry picked from commit 7bff576ebb5e9cc8d081f0a7f7b9cf657e5a3d13)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-10 14:48:23 +02:00
Christopher Faulet
e4e79659f8 BUG/MAJOR: mux-h1: Properly copy chunked input data during zero-copy nego
When data are transfered via zero-copy data forwarding, if some data were
already received, we try to immediately tranfer it during the negociation
step. If data are chunked and the chunk size is unknown, 10 bytes are reserved
to write the chunk size during the done step. However, when input data are
finally transferred, the offset is ignored. Data are copied into the output
buffer. But the first 10 bytes are then crushed by the chunk size. Thus the
chunk is truncated leading to a malformed message.

This patch should fix the issue #2598. It must be backported to 3.0.

(cherry picked from commit e8cc8a60be614c1cf978233b0b97771c9cc8fa20)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-10 14:38:48 +02:00
William Manley
913b7b672f BUG/MEDIUM: stconn/mux-h1: Fix suspect change causing timeouts
This fixes an issue I've had where if a connection was idle for ~23s
it would get in a bad state.  I don't understand this code, so I'm
not sure exactly why it was failing.

I discovered this by bisecting to identify the commit that caused the
regression between 2.9 and 3.0.  The commit is
d2c3f8dde7: "MINOR: stconn/connection:
Move shut modes at the SE descriptor level" - a part of v3.0-dev8.
It seems to be an innocent renaming, so I looked through it and this
stood out as suspect:

    -        if (mode != CO_SHW_NORMAL)
    +        if (mode & SE_SHW_NORMAL)

It looks like the not went missing here, so this patch reverses that
condition.  It fixes my test.

I don't quite understand what this is doing or is for so I can't write
a regression test or decent commit message.  Hopefully someone else
will be able to pick this up from where I've left it.

[CF: This inverts the condition to perform clean shutdowns. This means no
     clean shutdown are performed when it should do. This patch must be
     backported to 3.0]

(cherry picked from commit 52eb6b23f81d8663ebc6b2fe5d9996916c05ed8f)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-10 14:38:45 +02:00
Amaury Denoyelle
0a360df9de BUG/MINOR: quic: ensure Tx buf is always purged
quic_conn API for sending was recently refactored. The main objective
was to regroup the different functions present for both handshake and
application emission.

After this refactoring, an optimization was introduced to avoid calling
qc_send() if there was nothing new to emit. However, this prevent the Tx
buffer to be purged if previous sending was interrupted, until new
frames are finally available.

To fix this, simply remove the optimization. qc_send() is thus now
always called in quic_conn IO handlers.

The impact of this bug should be minimal as it happens only on sending
temporary error. However in this case, this could cause extra latency or
even a complete sending freeze in the worst scenario.

This must be backported up to 3.0.

(cherry picked from commit 0ef94e2dff873c0755584d0797c81e1b2c697e52)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-10 14:38:41 +02:00
Amaury Denoyelle
8728d680e1 BUG/MINOR: quic: fix computed length of emitted STREAM frames
qc_build_frms() is responsible to encode multiple frames in a single
QUIC packet. It accounts for room left in the buffer packet for each
newly encded frame.

An incorrect computation was performed when encoding a STREAM frame in a
single packet. Frame length was accounted twice which would reduce in
excess the buffer packet room. This caused the remaining built frames to
be reduced with the resulting packet not able to fill the whole MTU.

The impact of this bug should be minimal. It is only present when
multiple frames are encoded in a single packet after a STREAM. However
in this case datagrams built are smaller than expecting, which is
suboptimal for bandwith.

This should be backported up to 2.6.

(cherry picked from commit 50470a5181b6105ad1914c0d4e67794a2f69c80a)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-10 14:38:37 +02:00
William Lallemand
be979dd127 BUG/MEDIUM: ssl: bad auth selection with TLS1.2 and WolfSSL
The ClientHello callback for WolfSSL introduced in haproxy 2.9, seems
not to behave correctly with TLSv1.2.

In TLSv1.2, this is the cipher that is used to chose the authentication algorithm
(ECDSA or RSA), however an SSL client can send a signature algorithm.

In TLSv1.3, the authentication is not part of the ciphersuites, and
is selected using the signature algorithm.

The mistake in the code is that the signature algorithm in TLSv1.2 are
overwritting the auth that was selected using the ciphers.

This must be backported as far as 2.9.

(cherry picked from commit 711338e1ceb061db0a5c832acdea8edbeafa712f)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2024-06-10 14:38:34 +02:00
William Lallemand
3738434b22 BUG/MEDIUM: ssl: wrong priority whem limiting ECDSA ciphers in ECDSA+RSA configuration
The ClientHello Callback which is used for certificate selection uses
both the signature algorithms and the ciphers sent by the client.

However, when a client is announcing both ECDSA and RSA capabilities
with ECSDA ciphers that are not available on haproxy side and RSA
ciphers that are compatibles, the ECDSA certificate will still be used
but this will result in a "no shared cipher" error, instead of a
fallback on the RSA certificate.

For example, a client could send
'ECDHE-ECDSA-AES128-CCM:ECDHE-RSA-AES256-SHA and HAProxy could be
configured with only 'ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES256-SHA'.

This patch fixes the issue by validating that at least one ECDSA cipher
is available on both side before chosing the ECDSA certificate.

This must be backported on all stable versions.

(cherry picked from commit 93cc23a35561cd89b353143d20962dd86aa82a9c)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
2024-06-06 14:16:07 +02:00
Christopher Faulet
96aef291cb BUG/MEDIUM: mux-quic: Don't unblock zero-copy fwding if blocked during nego
The previous fix (792a645ec2 ["BUG/MEDIUM: mux-quic: Unblock zero-copy
forwarding if the txbuf can be released"]) introduced a regression. The
zero-copy data forwarding must only be unblocked if it was blocked by the
producer, after a successful negotiation.

It is important because during a negotiation, the consumer may be blocked
for another reason. Because of the flow control for instance. In that case,
there is not necessarily a TX buffer. And it unexpected to try to release an
unallocated TX buf.

In addition, the same may happen while a TX buf is still in-use. In that
case, it must also not be released. So testing the TX buffer is not the
right solution.

To fix the issue, a new IOBUF flag was added (IOBUF_FL_FF_WANT_ROOM). It
must be set by the producer if it is blocked after a sucessful negotiation
because it needs more room. In that case, we know a buffer was provided by
the consummer. In done_fastfwd() callback function, it is then possible to
safely unblock the zero-copy data forwarding if this flag is set.

This patch must be backported to 3.0 with the commit above.

(cherry picked from commit 9748df29ff655a9626d6e75ea9db79bb9afa2a50)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
2024-06-06 14:15:51 +02:00
Aurelien DARRAGON
f2384ec8fa CLEANUP: hlua: simplify ambiguous lua_insert() usage in hlua_ctx_resume()
'lua_insert(lua->T, -lua_gettop(lua->T))' is actually used to rotate the
top value with the bottom one, thus the code was overkill and the comment
was actually misleading, let's fix that by using explicit equivalent form
(absolute index).

It may be backported with 5508db9a2 ("BUG/MINOR: hlua: fix unsafe
lua_tostring() usage with empty stack") to all stable versions to ease
code maintenance.

(cherry picked from commit 2bde0d64ddf0e32257444f14e69adea8f899b74b)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
2024-06-06 14:15:39 +02:00
Aurelien DARRAGON
8b8fb7776e BUG/MINOR: hlua: fix leak in hlua_ckch_set() error path
in hlua_ckch_commit_yield() and hlua_ckch_set(), when an error occurs,
we enter the error path and try to raise an error from the <err> msg
pointer which must be freed afterwards.

However, the fact that luaL_error() never returns was overlooked, because
of that <err> msg is never freed in such case.

To fix the issue, let's use hlua_pushfstring_safe() helper to push the
err on the lua stack and then free it before throwing the error using
lua_error().

It should be backported up to 2.6 with 30fcca18 ("MINOR: ssl/lua:
CertCache.set() allows to update an SSL certificate file")

(cherry picked from commit 755c2daf0f88885fd6825c55ae59198726c4905e)
Signed-off-by: Amaury Denoyelle <adenoyelle@haproxy.com>
2024-06-06 14:15:27 +02:00