]> git.ipfire.org Git - thirdparty/haproxy.git/log
thirdparty/haproxy.git
2 months agoBUG/MINOR: server: dont depend on proxy for server cleanup in srv_drop()
Aurelien DARRAGON [Mon, 12 May 2025 14:09:15 +0000 (16:09 +0200)] 
BUG/MINOR: server: dont depend on proxy for server cleanup in srv_drop()

In commit b5ee8bebfc ("MINOR: server: always call ssl->destroy_srv when
available"), we made it so srv_drop() doesn't depend on proxy to perform
server cleanup.

It turns out this is now mandatory, because during deinit, free_proxy()
can occur before the final srv_drop(). This is the case when using Lua
scripts for instance.

In 2a9436f96 ("MINOR: lbprm: Add method to deinit server and proxy") we
added a freeing check under srv_drop() that depends on the proxy.
Because of that UAF may occur during deinit when using a Lua script that
manipulate server objects.

To fix the issue, let's perform the lbprm server deinit logic under
free_proxy() directly, where the DEINIT server hooks are evaluated.

Also, to prevent similar bugs in the future, let's explicitly document
in srv_drop() that server cleanups should assume that the proxy may
already be freed.

No backport needed unless 2a9436f96 is.

2 months agoBUG/MINOR: cfgparse: improve the empty arg position report's robustness
Willy Tarreau [Mon, 12 May 2025 14:06:28 +0000 (16:06 +0200)] 
BUG/MINOR: cfgparse: improve the empty arg position report's robustness

OSS Fuzz found that the previous fix ebb19fb367 ("BUG/MINOR: cfgparse:
consider the special case of empty arg caused by \x00") was incomplete,
as the output can sometimes be larger than the input (due to variables
expansion) in which case the work around to try to report a bad arg will
fail. While the parse_line() function has been made more robust now in
order to avoid this condition, let's fix the handling of this special
case anyway by just pointing to the beginning of the line if the supposed
error location is out of the line's buffer.

All details here:
   https://oss-fuzz.com/testcase-detail/5202563081502720

No backport is needed unless the fix above is backported.

2 months agoBUG/MINOR: tools: improve parse_line()'s robustness against empty args
Willy Tarreau [Mon, 12 May 2025 14:01:27 +0000 (16:01 +0200)] 
BUG/MINOR: tools: improve parse_line()'s robustness against empty args

The fix in 10e6d0bd57 ("BUG/MINOR: tools: only fill first empty arg when
not out of range") was not that good. It focused on protecting against
<arg> becoming out of range to detect we haven't emitted anything, but
it's not the right way to detect this. We're always maintaining arg_start
as a copy of outpos, and that later one is incremented when emitting a
char, so instead of testing args[arg] against out+arg_start, we should
instead check outpos against arg_start, thereby eliminating the <out>
offset and the need to access args[]. This way we now always know if
we've emitted an empty arg without dereferencing args[].

There's no need to backport this unless the fix above is also backported.

2 months agoBUG/MINOR: threads: fix soft-stop without multithreading support
Aurelien DARRAGON [Mon, 12 May 2025 09:57:39 +0000 (11:57 +0200)] 
BUG/MINOR: threads: fix soft-stop without multithreading support

When thread support is disabled ("USE_THREAD=" or "USE_THREAD=0" when
building), soft-stop doesn't work as haproxy never ends after stopping
the proxies.

This used to work fine in the past but suddenly stopped working with
ef422ced91 ("MEDIUM: thread: make stopping_threads per-group and add
stopping_tgroups") because the "break;" instruction under the stopping
condition is never executed when support for multithreading is disabled.

To fix the issue, let's add an "else" block to run the "break;"
instruction when USE_THREAD is not defined.

It should be backported up to 2.8

2 months agoMINOR: ssl/ckch: warn when the same keyword was used twice
William Lallemand [Fri, 9 May 2025 17:18:38 +0000 (19:18 +0200)] 
MINOR: ssl/ckch: warn when the same keyword was used twice

When using a crt-list or a crt-store, keywords mentionned twice on the
same line overwritte the previous value.

This patch emits a warning when the same keyword is found another time
on the same line.

2 months agoBUG/MINOR: ssl/ckch: always ha_freearray() the previous entry during parsing
William Lallemand [Fri, 9 May 2025 17:16:02 +0000 (19:16 +0200)] 
BUG/MINOR: ssl/ckch: always ha_freearray() the previous entry during parsing

The ckch_conf_parse() function is the generic function which parses
crt-store keywords from the crt-store section, and also from a
crt-list.

When having multiple time the same keyword, a leak of the previous
value happens. This patch ensure that the previous value is always
freed before overwriting it.

This is the same problem as the previous "BUG/MINOR: ssl/ckch: always
free() the previous entry during parsing" patch, however this one
applies on PARSE_TYPE_ARRAY_SUBSTR.

No backport needed.

2 months agoMINOR: tools: ha_freearray() frees an array of string
William Lallemand [Fri, 9 May 2025 17:09:12 +0000 (19:09 +0200)] 
MINOR: tools: ha_freearray() frees an array of string

ha_freearray() is a new function which free() an array of strings
terminated by a NULL entry.

The pointer to the array will be free and set to NULL.

2 months agoBUG/MINOR: ssl/ckch: always free() the previous entry during parsing
William Lallemand [Fri, 9 May 2025 17:01:28 +0000 (19:01 +0200)] 
BUG/MINOR: ssl/ckch: always free() the previous entry during parsing

The ckch_conf_parse() function is the generic function which parses
crt-store keywords from the crt-store section, and also from a crt-list.

When having multiple time the same keyword, a leak of the previous value
happens. This patch ensure that the previous value is always freed
before overwriting it.

This patch should be backported as far as 3.0.

2 months agoBUG/MINOR: ssl: prevent multiple 'crt' on the same ssl-f-use line
William Lallemand [Fri, 9 May 2025 16:52:09 +0000 (18:52 +0200)] 
BUG/MINOR: ssl: prevent multiple 'crt' on the same ssl-f-use line

The 'ssl-f-use' implementation doesn't prevent to have multiple time the
'crt' keyword, which overwrite the previous value. Letting users think
that is it possible to use multiple certificates on the same line, which
is not the case.

This patch emits an alert when setting the 'crt' keyword multiple times
on the same ssl-f-use line.

Should fix issue #2966.

No backport needed.

2 months agoBUG/MINOR: ssl: doesn't fill conf->crt with first arg
William Lallemand [Fri, 9 May 2025 16:23:06 +0000 (18:23 +0200)] 
BUG/MINOR: ssl: doesn't fill conf->crt with first arg

Commit c7f29afc ("MEDIUM: ssl: replace "crt" lines by "ssl-f-use"
lines") forgot to remove an the allocation of the crt field which was
done with the first argument.

Since ssl-f-use takes keywords, this would put the first keyword in
"crt" instead of the certificate name.

2 months agoMEDIUM: sock-inet: re-check IPv6 connectivity every 30s
Willy Tarreau [Fri, 9 May 2025 13:23:10 +0000 (15:23 +0200)] 
MEDIUM: sock-inet: re-check IPv6 connectivity every 30s

IPv6 connectivity might start off (e.g. network not fully up when
haproxy starts), so for features like resolvers, it would be nice to
periodically recheck.

With this change, instead of having the resolvers code rely on a variable
indicating connectivity, it will now call a function that will check for
how long a connectivity check hasn't been run, and will perform a new one
if needed. The age was set to 30s which seems reasonable considering that
the DNS will cache results anyway. There's no saving in spacing it more
since the syscall is very check (just a connect() without any packet being
emitted).

The variables remain exported so that we could present them in show info
or anywhere else.

This way, "dns-accept-family auto" will now stay up to date. Warning
though, it does perform some caching so even with a refreshed IPv6
connectivity, an older record may be returned anyway.

2 months agoDEBUG: pools: add a new integrity mode "backup" to copy the released area
Willy Tarreau [Thu, 8 May 2025 13:28:18 +0000 (15:28 +0200)] 
DEBUG: pools: add a new integrity mode "backup" to copy the released area

This way we can preserve the entire contents of the released area for
later inspection. This automatically enables comparison at reallocation
time as well (like "integrity" does). If used in combination with
integrity, the comparison is disabled but the check of non-corruption
of the area mangled by integrity is still operated.

2 months agoMINOR: acme: add the global option 'acme.scheduler'
William Lallemand [Fri, 9 May 2025 11:45:48 +0000 (13:45 +0200)] 
MINOR: acme: add the global option 'acme.scheduler'

The automatic scheduler is useful but sometimes you don't want to use,
or schedule manually.

This patch adds an 'acme.scheduler' option in the global section, which
can be set to either 'auto' or 'off'. (auto is the default value)

This also change the ouput of the 'acme status' command so it does not
shows scheduled values. The state will be 'Stopped' instead of
'Scheduled'.

2 months agoDEBUG: pool: permit per-pool UAF configuration
Willy Tarreau [Thu, 8 May 2025 17:52:16 +0000 (19:52 +0200)] 
DEBUG: pool: permit per-pool UAF configuration

The new MEM_F_UAF flag can be set just after a pool's creation to make
this pool UAF for debugging purposes. This allows to maintain a better
overall performance required to reproduce issues while still having a
chance to catch UAF. It will only be used by developers who will manually
add it to areas worth being inspected, though.

2 months agoBUG/MEDIUM: mux-quic: fix crash on invalid fctl frame dereference
Amaury Denoyelle [Fri, 9 May 2025 08:54:40 +0000 (10:54 +0200)] 
BUG/MEDIUM: mux-quic: fix crash on invalid fctl frame dereference

Emission of flow-control frames have been recently modified. Now, each
frame is sent one by one, via a single entry list. If a failure occurs,
emission is interrupted and frame is reinserted into the original
<qcc.lfctl.frms> list.

This code is incorrect as it only checks if qcc_send_frames() returns an
error code to perform the reinsert operation. However, an error here
does not always mean that the frame was not properly emitted by lower
quic-conn layer. As such, an extra test LIST_ISEMPTY() must be performed
prior to reinsert the frame.

This bug would cause a heap overflow. Indeed, the reinsert frame would
be a random value. A crash would occur as soon as it would be
dereferenced via <qcc.lfctl.frms> list.

This was reproduced by issuing a POST with a big file and interrupt it
after just a few seconds. This results in a crash in about a third of
the tests. Here is an example command using ngtcp2 :

 $ ngtcp2-client -q --no-quic-dump --no-http-dump \
   -m POST -d ~/infra/html/1g 127.0.0.1 20443 "http://127.0.0.1:20443/post"

Heap overflow was detected via a BUG_ON() statement from qc_frm_free()
via qcc_release() caller :

  FATAL: bug condition "!((&((*frm)->reflist))->n == (&((*frm)->reflist)))" matched at src/quic_frame.c:1270

This does not need to be backported.

2 months ago[RELEASE] Released version 3.2-dev15 v3.2-dev15
Willy Tarreau [Fri, 9 May 2025 08:51:30 +0000 (10:51 +0200)] 
[RELEASE] Released version 3.2-dev15

Released version 3.2-dev15 with the following main changes :
    - BUG/MEDIUM: stktable: fix sc_*(<ctr>) BUG_ON() regression with ctx > 9
    - BUG/MINOR: acme/cli: don't output error on success
    - BUG/MINOR: tools: do not create an empty arg from trailing spaces
    - MEDIUM: config: warn about the consequences of empty arguments on a config line
    - MINOR: tools: make parse_line() provide hints about empty args
    - MINOR: cfgparse: visually show the input line on empty args
    - BUG/MINOR: tools: always terminate empty lines
    - BUG/MINOR: tools: make parseline report the required space for the trailing 0
    - DEBUG: threads: don't keep lock label "OTHER" in the per-thread history
    - DEBUG: threads: merge successive idempotent lock operations in history
    - DEBUG: threads: display held locks in threads dumps
    - BUG/MINOR: proxy: only use proxy_inc_fe_cum_sess_ver_ctr() with frontends
    - Revert "BUG/MEDIUM: mux-spop: Handle CLOSING state and wait for AGENT DISCONNECT frame"
    - MINOR: acme/cli: 'acme status' show the status acme-configured certificates
    - MEDIUM: acme/ssl: remove 'acme ps' in favor of 'acme status'
    - DOC: configuration: add "acme" section to the keywords list
    - DOC: configuration: add the "crt-store" keyword
    - BUG/MAJOR: queue: lock around the call to pendconn_process_next_strm()
    - MINOR: ssl: add filename and linenum for ssl-f-use errors
    - BUG/MINOR: ssl: can't use crt-store some certificates in ssl-f-use
    - BUG/MINOR: tools: only fill first empty arg when not out of range
    - MINOR: debug: bump the dump buffer to 8kB
    - MINOR: stick-tables: add "ipv4" as an alias for the "ip" type
    - MINOR: quic: extend return value during TP parsing
    - BUG/MINOR: quic: use proper error code on missing CID in TPs
    - BUG/MINOR: quic: use proper error code on invalid server TP
    - BUG/MINOR: quic: reject retry_source_cid TP on server side
    - BUG/MINOR: quic: use proper error code on invalid received TP value
    - BUG/MINOR: quic: fix TP reject on invalid max-ack-delay
    - BUG/MINOR: quic: reject invalid max_udp_payload size
    - BUG/MEDIUM: peers: hold the refcnt until updating ts->seen
    - BUG/MEDIUM: stick-tables: close a tiny race in __stksess_kill()
    - BUG/MINOR: cli: fix too many args detection for commands
    - MINOR: server: ensure server postparse tasks are run for dynamic servers
    - BUG/MEDIUM: stick-table: always remove update before adding a new one
    - BUG/MEDIUM: quic: free stream_desc on all data acked
    - BUG/MINOR: cfgparse: consider the special case of empty arg caused by \x00
    - DOC: config: recommend disabling libc-based resolution with resolvers

2 months agoDOC: config: recommend disabling libc-based resolution with resolvers
Willy Tarreau [Fri, 9 May 2025 08:30:30 +0000 (10:30 +0200)] 
DOC: config: recommend disabling libc-based resolution with resolvers

Using both libc and haproxy resolvers can lead to hard to diagnose issues
when their bevahiour diverges; recommend using only one type of resolver.

Should be backported to stable versions.

Link: https://www.mail-archive.com/haproxy@formilux.org/msg45663.html
Co-authored-by: Lukas Tribus <lukas@ltri.eu>
2 months agoBUG/MINOR: cfgparse: consider the special case of empty arg caused by \x00
Willy Tarreau [Fri, 9 May 2025 07:55:39 +0000 (09:55 +0200)] 
BUG/MINOR: cfgparse: consider the special case of empty arg caused by \x00

The reporting of the empty arg location added with commit 08d3caf30
("MINOR: cfgparse: visually show the input line on empty args") falls
victim of a special case detected by OSS Fuzz:

     https://issues.oss-fuzz.com/issues/415850462

In short, making an argument start with "\x00" doesn't make it empty for
the parser, but still emits an empty string which is detected and
displayed. Unfortunately in this case the error pointer is not set so
the sanitization function crashes.

What we're doing in this case is that we fall back to the position of
the output argument as an estimate of where it was located in the input.
It's clearly inexact (quoting etc) but will still help the user locate
the problem.

No backport is needed unless the commit above is backported.

2 months agoBUG/MEDIUM: quic: free stream_desc on all data acked
Amaury Denoyelle [Wed, 7 May 2025 15:08:42 +0000 (17:08 +0200)] 
BUG/MEDIUM: quic: free stream_desc on all data acked

The following patch simplifies qc_stream_desc_ack(). The qc_stream_desc
instance is not freed anymore, even if all data were acknowledged. As
implies by the commit message, the caller is responsible to perform this
cleaning operation.
  f4a83fbb14bdd14ed94752a2280a2f40c1b690d2
  MINOR: quic: do not remove qc_stream_desc automatically on ACK handling

However, despite the commit instruction, qc_stream_desc_free()
invokation was not moved in the caller. This commit fixes this by adding
it after stream ACK handling. This is performed only when a transfer is
completed : all data is acknowledged and qc_stream_desc has been
released by its MUX stream instance counterpart.

This bug may cause a significant increase in memory usage when dealing
with long running connection. However, there is no memory leak, as every
qc_stream_desc attached to a connection are finally freed when quic_conn
instance is released.

This must be backported up to 3.1.

2 months agoBUG/MEDIUM: stick-table: always remove update before adding a new one
Willy Tarreau [Thu, 8 May 2025 21:21:25 +0000 (23:21 +0200)] 
BUG/MEDIUM: stick-table: always remove update before adding a new one

Since commit 388539faa ("MEDIUM: stick-tables: defer adding updates to a
tasklet"), between the entry creation and its arrival in the updates tree,
there is time for scheduling, and it now becomes possible for an stksess
entry to be requeued into the list while it's still in the tree as a remote
one. Only local updates were removed prior to being inserted. In this case
we would re-insert the entry, causing it to appear as the parent of two
distinct nodes or leaves, and to be visited from the first leaf during a
delete() after having already been removed and freed, causing a crash,
as Christian reported in issue #2959.

There's no reason to backport this as this appeared with the commit above
in 3.2-dev13.

2 months agoMINOR: server: ensure server postparse tasks are run for dynamic servers
Aurelien DARRAGON [Wed, 7 May 2025 21:50:46 +0000 (23:50 +0200)] 
MINOR: server: ensure server postparse tasks are run for dynamic servers

commit 29b76cae4 ("BUG/MEDIUM: server/log: "mode log" after server
keyword causes crash") introduced some postparsing checks/tasks for
server

Initially they were mainly meant for "mode log" servers postparsing, but
we already have a check dedicated to "tcp/http" servers (ie: only tcp
proto supported)

However when dynamic servers are added they bypass _srv_postparse() since
the REGISTER_POST_SERVER_CHECK() is only executed for servers defined in
the configuration.

To ensure consistency between dynamic and static servers, and ensure no
post-check init routine is missed, let's manually invoke _srv_postparse()
after creating a dynamic server added via the cli.

2 months agoBUG/MINOR: cli: fix too many args detection for commands
Aurelien DARRAGON [Wed, 7 May 2025 23:01:28 +0000 (01:01 +0200)] 
BUG/MINOR: cli: fix too many args detection for commands

d3f928944 ("BUG/MINOR: cli: Issue an error when too many args are passed
for a command") added a new check to prevent the command to run when
too many arguments are provided. In this case an error is reported.

However it turns out this check (despite marked for backports) was
ineffective prior to 20ec1de21 ("MAJOR: cli: Refacor parsing and
execution of pipelined commands") as 'p' pointer was reset to the end of
the buffer before the check was executed.

Now since 20ec1de21, the check works, but we have another issue: we may
read past initialized bytes in the buffer because 'p' pointer is always
incremented in a while loop without checking if we increment it past 'end'
(This was detected using valgrind)

To fix the issue introduced by 20ec1de21, let's only increment 'p' pointer
if p < end.

For 3.2 this is it, now for older versions, since d3f928944 was marked for
backport, a sligthly different approach is needed:

 - conditional p increment must be done in the loop (as in this patch)
 - max arg check must moved above "fill unused slots" comment where p is
   assigned to the end of the buffer

This patch should be backported with d3f928944.

2 months agoBUG/MEDIUM: stick-tables: close a tiny race in __stksess_kill()
Willy Tarreau [Wed, 7 May 2025 16:43:57 +0000 (18:43 +0200)] 
BUG/MEDIUM: stick-tables: close a tiny race in __stksess_kill()

It might be possible not to see the element in the tree, then not to
see it in the update list, thus not to take the lock before deleting.
But an element in the list could have moved to the tree during the
check, and be removed later without the updt_lock.

Let's delete prior to checking the presence in the tree to avoid
this situation. No backport is needed since this arrived in -dev13
with the update list.

2 months agoBUG/MEDIUM: peers: hold the refcnt until updating ts->seen
Willy Tarreau [Tue, 6 May 2025 09:32:34 +0000 (11:32 +0200)] 
BUG/MEDIUM: peers: hold the refcnt until updating ts->seen

In peer_treat_updatemsg(), we call stktable_touch_remote() after
releasing the write lock on the TS, asking it to decrement the
refcnt, then we update ts->seen. Unfortunately this is racy and
causes the issue that Christian reported in issue #2959.

The sequence of events is very hard to trigger manually, but what happens
is the following:

 T1.  stktable_touch_remote(table, ts, 1);
      -> at this point the entry is in the mt_list, and the refcnt is zero.

      T2.  stktable_trash_oldest() or process_table_expire()
           -> these can run, because the refcnt is now zero.
              The entry is cleanly deleted and freed.

 T1.  HA_ATOMIC_STORE(&ts->seen, 1)
      -> we dereference freed memory.

A first attempt at a fix was made by keeping the refcnt held during
all the time the entry is in the mt_list, but this is expensive as
such entries cannot be purged, causing lots of skips during
trash_oldest_data(). This managed to trigger watchdogs, and was only
hiding the real cause of the problem.

The correct approach clearly is to maintain the ref_cnt until we
touch ->seen. That's what this patch does. It does not decrement
the refcnt, while calling stktable_touch_remote(), and does it
manually after touching ->seen. With this the problem is gone.

Note that a reproducer involves the following:
  - a config with 10 stick-ctr tracking the same table with a
    random key between 10M and 100M depending on the machine.
  - the expiration should be between 10 and 20s. http_req_cnt
    is stored and shared with the peers.
  - 4 total processes with such a config on the local machine,
    each corresponding to a different peer. 3 of the peers are
    bound to half of the cores (all threads) and share the same
    threads; the last process is bound to the other half with
    its own threads.
  - injecting at full load, ~256 conn, on the shared listening
    port. After ~2x expiration time to 1 minute the lone process
    should segfault in pools code due to a corrupted by_lru list.

This problem already exists in earlier versions but the race looks
narrower. Given how difficult it is to trigger on a given machine
in its current form, it's likely that it only happens once in a
while on stable branches. The fix must be backported wherever the
code is similar, and there's no hope to reproduce it to validate
the backport.

Thanks again to Christian for his amazing help!

2 months agoBUG/MINOR: quic: reject invalid max_udp_payload size
Amaury Denoyelle [Tue, 6 May 2025 16:01:32 +0000 (18:01 +0200)] 
BUG/MINOR: quic: reject invalid max_udp_payload size

Add a checks on received max_udp_payload transport parameters. As
defined per RFC 9000, values below 1200 are invalid, and thus the
connection must be closed with TRANSPORT_PARAMETER_ERROR code.

Prior to this patch, an invalid value was silently ignored.

This should be backported up to 2.6. Note that is relies on previous
patch "MINOR: quic: extend return value on TP parsing".

2 months agoBUG/MINOR: quic: fix TP reject on invalid max-ack-delay
Amaury Denoyelle [Tue, 6 May 2025 16:01:09 +0000 (18:01 +0200)] 
BUG/MINOR: quic: fix TP reject on invalid max-ack-delay

Checks are implemented on some received transport parameter values,
to reject invalid ones defined per RFC 9000. This is the case for
max_ack_delay parameter.

The check was not properly implemented as it only reject values strictly
greater than the limit set to 2^14. Fix this by rejecting values of 2^14
and above. Also, the proper error code TRANSPORT_PARAMETER_ERROR is now
set.

This should be backported up to 2.6. Note that is relies on previous
patch "MINOR: quic: extend return value on TP parsing".

2 months agoBUG/MINOR: quic: use proper error code on invalid received TP value
Amaury Denoyelle [Tue, 6 May 2025 16:00:43 +0000 (18:00 +0200)] 
BUG/MINOR: quic: use proper error code on invalid received TP value

As per RFC 9000, checks must be implemented to reject invalid values for
received transport parameters. Such values are dependent on the
parameter type.

Checks were already implemented for ack_delay_exponent and
active_connection_id_limit, accordingly with the QUIC specification.
However, the connection was closed with an incorrect error code. Fix
this to ensure that TRANSPORT_PARAMETER_ERROR code is used as expected.

This should be backported up to 2.6. Note that is relies on previous
patch "MINOR: quic: extend return value on TP parsing".

2 months agoBUG/MINOR: quic: reject retry_source_cid TP on server side
Amaury Denoyelle [Tue, 6 May 2025 15:59:37 +0000 (17:59 +0200)] 
BUG/MINOR: quic: reject retry_source_cid TP on server side

Close the connection on error if retry_source_connection_id transport
parameter is received. This is specified by RFC 9000 as this parameter
must not be emitted by a client. Previously, it was silently ignored.

This should be backported up to 2.6. Note that is relies on previous
patch "MINOR: quic: extend return value on TP parsing".

2 months agoBUG/MINOR: quic: use proper error code on invalid server TP
Amaury Denoyelle [Tue, 6 May 2025 15:59:21 +0000 (17:59 +0200)] 
BUG/MINOR: quic: use proper error code on invalid server TP

This commit is similar to the previous one. It fixes the error code
reported when dealing with invalid received transport parameters. This
time, it handles reception of original_destination_connection_id,
preferred_address and stateless_reset_token which must not be emitted by
the client.

This should be backported up to 2.6. Note that is relies on previous
patch "MINOR: quic: extend return value on TP parsing".

2 months agoBUG/MINOR: quic: use proper error code on missing CID in TPs
Amaury Denoyelle [Tue, 6 May 2025 14:45:23 +0000 (16:45 +0200)] 
BUG/MINOR: quic: use proper error code on missing CID in TPs

Handle missing received transport parameter value
initial_source_connection_id / original_destination_connection_id.
Previously, such case would result in an error reported via
quic_transport_params_store(), which triggers a TLS alert converted as
expected as a CONNECTION_CLOSE. The issue is that the error code
reported in the frame was incorrect.

Fix this by returning QUIC_TP_DEC_ERR_INVAL for such conditions. This is
directly handled via quic_transport_params_store() which set the proper
TRANSPORT_PARAMETER_ERROR code for the CONNECTION_CLOSE. However, no
error is reported so the SSL handshake is properly terminated without a
TLS alert. This is enough to ensure that the CONNECTION_CLOSE frame will
be emitted as expected.

This should be backported up to 2.6. Note that is relies on previous
patch "MINOR: quic: extend return value on TP parsing".

2 months agoMINOR: quic: extend return value during TP parsing
Amaury Denoyelle [Tue, 6 May 2025 16:10:27 +0000 (18:10 +0200)] 
MINOR: quic: extend return value during TP parsing

Extend API used for QUIC transport parameter decoding. This is done via
the introduction of a dedicated enum to report the various error
condition detected. No functional change should occur with this patch,
as the only returned code is QUIC_TP_DEC_ERR_TRUNC, which results in the
connection closure via a TLS alert.

This patch will be necessary to properly reject transport parameters
with the proper CONNECTION_CLOSE error code. As such, it should be
backported up to 2.6 with the following series.

2 months agoMINOR: stick-tables: add "ipv4" as an alias for the "ip" type
Willy Tarreau [Tue, 6 May 2025 08:54:48 +0000 (10:54 +0200)] 
MINOR: stick-tables: add "ipv4" as an alias for the "ip" type

However the doc purposely says the opposite, to encourage migrating away
from "ip". The goal is that in the future we change "ip" to mean "ipv6",
which seems to be what most users naturally expect. But we cannot break
configurations in the LTS version so for now "ipv4" is the alias.

The reason for not changing it in the table is that the type name is
used at a few places (look for "].kw"):
  - dumps
  - promex

We'd rather not change that output for 3.2, but only do it in 3.3.
This way, 3.2 can be made future-proof by using "ipv4" in the config
without any other side effect.

Please see github issue #2962 for updates on this transition.

2 months agoMINOR: debug: bump the dump buffer to 8kB
Willy Tarreau [Wed, 7 May 2025 08:00:08 +0000 (10:00 +0200)] 
MINOR: debug: bump the dump buffer to 8kB

Now with the improved backtraces, the lock history and details in the
mux layers, some dumps appear truncated or with some chars alone at
the beginning of the line. The issue is in fact caused by the limited
dump buffer size (2kB for stderr, 4kB for warning), that cannot hold
a complete trace anymore.

Let's jump bump them to 8kB, this will be plenty for a long time.

2 months agoBUG/MINOR: tools: only fill first empty arg when not out of range
Willy Tarreau [Wed, 7 May 2025 05:22:24 +0000 (07:22 +0200)] 
BUG/MINOR: tools: only fill first empty arg when not out of range

In commit 3f2c8af313 ("MINOR: tools: make parse_line() provide hints
about empty args") we've added the ability to record the position of
the first empty arg in parse_line(), but that check requires to
access the args[] array for the current arg, which is not valid in
case we stopped on too large an argument count. Let's just check the
arg's validity before doing so.

This was reported by OSS Fuzz:
  https://issues.oss-fuzz.com/issues/415850462

No backport is needed since this was in the latest dev branch.

2 months agoBUG/MINOR: ssl: can't use crt-store some certificates in ssl-f-use
William Lallemand [Tue, 6 May 2025 19:16:20 +0000 (21:16 +0200)] 
BUG/MINOR: ssl: can't use crt-store some certificates in ssl-f-use

When declaring a certificate via the crt-store section, this certificate
can then be used 2 ways in a crt-list:
- only by using its name, without any crt-store options
- or by using the exact set of crt-list option that was defined in the
  crt-store

Since ssl-f-use is generating a crt-list, this is suppose to behave the
same. To achieve this, ckch_conf_parse() will parse the keywords related
to the ckch_conf on the ssl-f-use line and use ckch_conf_cmp() to
compare it to the previous declaration from the crt-store. This
comparaison is only done when any ckch_conf keyword are present.

However, ckch_conf_parse() was done for the crt-list, and the crt-list
does not use the "crt" parameter to declare the name of the certificate,
since it's the first element of the line. So when used with ssl-f-use,
ckch_conf_parse() will always see a "crt" keyword which is a ckch_conf
one, and consider that it will always need to have the exact same set of
paremeters when using the same crt in a crt-store and an ssl-f-use line.

So a simple configuration like this:

   crt-store web
     load crt "foo.com.crt" key "foo.com.key" alias "foo"

   frontend mysite
     bind :443 ssl
     ssl-f-use crt "@web/foo" ssl-min-ver TLSv1.2

Would lead to an error like this:

    config : '@web/foo' in crt-list '(null)' line 0, is already defined with incompatible parameters:
    - different parameter 'key' : previously 'foo.com.key' vs '(null)'

In order to fix the issue, this patch parses the "crt" parameter itself
for ssl-f-use instead of using ckch_conf_parse(), so the keyword would
never be considered as a ckch_conf keyword to compare.

This patch also take care of setting the CKCH_CONF_SET_CRTLIST flag only
if a ckch_conf keyword was found. This flag is used by ckch_conf_cmp()
to know if it has to compare or not.

No backport needed.

2 months agoMINOR: ssl: add filename and linenum for ssl-f-use errors
William Lallemand [Tue, 6 May 2025 18:13:15 +0000 (20:13 +0200)] 
MINOR: ssl: add filename and linenum for ssl-f-use errors

Fill cfg_crt_node with a filename and linenum so the post_section
callback can use it to emit errors.

This way the errors are emitted with the right filename and linenum
where ssl-f-use is used instead of (null):0

2 months agoBUG/MAJOR: queue: lock around the call to pendconn_process_next_strm()
Willy Tarreau [Tue, 6 May 2025 16:55:04 +0000 (18:55 +0200)] 
BUG/MAJOR: queue: lock around the call to pendconn_process_next_strm()

The extra call to pendconn_process_next_strm() made in commit cda7275ef5
("MEDIUM: queue: Handle the race condition between queue and dequeue
differently") was performed after releasing the server queue's lock,
which is incompatible with the calling convention for this function.
The result is random corruption of the server's streams list likely
due to picking old or incorrect pendconns from the queue, and in the
end infinitely looping on apparently already locked mt_list objects.
Just adding the lock fixes the problem.

It's very difficult to reproduce, it requires low maxconn values on
servers, stickiness on the servers (cookie), a long enough slowstart
(e.g. 10s), and regularly flipping servers up/down to re-trigger the
slowstart.

No backport is needed as this was only in 3.2.

2 months agoDOC: configuration: add the "crt-store" keyword
William Lallemand [Tue, 6 May 2025 14:07:29 +0000 (16:07 +0200)] 
DOC: configuration: add the "crt-store" keyword

Add the "crt-store" keyword with its argument in the "3.12" section, so
this could be detected by haproxy-dconv has a keyword and put in the
keywords list.

Must be backported as far as 3.0

2 months agoDOC: configuration: add "acme" section to the keywords list
William Lallemand [Tue, 6 May 2025 13:34:39 +0000 (15:34 +0200)] 
DOC: configuration: add "acme" section to the keywords list

Add the "acme" keyword with its argument in the "3.13" section, so this
could be detected by haproxy-dconv has a keyword and put in the keywords
list.

2 months agoMEDIUM: acme/ssl: remove 'acme ps' in favor of 'acme status'
William Lallemand [Tue, 6 May 2025 13:20:05 +0000 (15:20 +0200)] 
MEDIUM: acme/ssl: remove 'acme ps' in favor of 'acme status'

Remove the 'acme ps' command which does not seem useful anymore with the
'acme status' command.

The big difference with the 'acme status' command is that it was only
displaying the running tasks instead of the status of all certificate.

2 months agoMINOR: acme/cli: 'acme status' show the status acme-configured certificates
William Lallemand [Tue, 6 May 2025 12:56:38 +0000 (14:56 +0200)] 
MINOR: acme/cli: 'acme status' show the status acme-configured certificates

The "acme status" command, shows the status of every certificates
configured with ACME, not only the running task like "acme ps".

The IO handler loops on the ckch_store tree and outputs a line for each
ckch_store which has an acme section set. This is still done under the
ckch_store lock and doesn't support resuming when the buffer is full,
but we need to change that in the future.

2 months agoRevert "BUG/MEDIUM: mux-spop: Handle CLOSING state and wait for AGENT DISCONNECT...
Christopher Faulet [Tue, 6 May 2025 11:43:59 +0000 (13:43 +0200)] 
Revert "BUG/MEDIUM: mux-spop: Handle CLOSING state and wait for AGENT DISCONNECT frame"

This reverts commit 53c3046898633e56f74f7f05fb38cabeea1c87a1.

This patch introduced a regression leading to a loop on the frames
demultiplexing because a frame may be ignore but not consumed.

But outside this regression that can be fixed, there is a design issue that
was not totally fixed by the patch above. The SPOP connection state is mixed
with the status of the frames demultiplexer and this needlessly complexify
the connection management. Instead of fixing the fix, a better solution is
to revert it to work a a proper solution.

For the record, the idea is to deal with the spop connection state onlu
using 'state' field and to introduce a new field to handle the frames
demultiplexer state. This should ease the closing state management.

Another issue that must be fixed. We must take care to not abort a SPOP
stream when an error is detected on a SPOP connection or when the connection
is closed, if the ACK frame was already received for this stream. It is not
a common case, but it can be solved by saving the last known stream ID that
recieved a ACK.

This patch must be backported if the commit above is backported.

2 months agoBUG/MINOR: proxy: only use proxy_inc_fe_cum_sess_ver_ctr() with frontends
Aurelien DARRAGON [Fri, 2 May 2025 17:06:44 +0000 (19:06 +0200)] 
BUG/MINOR: proxy: only use proxy_inc_fe_cum_sess_ver_ctr() with frontends

proxy_inc_fe_cum_sess_ver_ctr() was implemented in 9969adbc
("MINOR: stats: add by HTTP version cumulated number of sessions and
requests")

As its name suggests, it is meant to be called for frontends, not backends

Also, in 9969adbc, when used under h1_init(), a precaution is taken to
ensure that the function is only called with frontends.

However, this precaution was not applied in h2_init() and qc_init().

Due to this, it remains possible to have proxy_inc_fe_cum_sess_ver_ctr()
being called with a backend proxy as parameter. While it did not cause
known issues so far, it is not expected and could result in bugs in the
future. Better fix this by ensuring the function is only called with
frontends.

It may be backported up to 2.8

2 months agoDEBUG: threads: display held locks in threads dumps
Willy Tarreau [Tue, 6 May 2025 03:11:56 +0000 (05:11 +0200)] 
DEBUG: threads: display held locks in threads dumps

Based on the lock history, we can spot some locks that are still held
by checking the last operation that happened on them: if it's not an
unlock, then we know the lock is held. In this case we append the list
after "locked:" with their label and state like below:

  U:QUEUE S:IDLE_CONNS U:IDLE_CONNS R:TASK_WQ U:TASK_WQ S:QUEUE S:QUEUE S:QUEUE locked: QUEUE(S)
  S:IDLE_CONNS U:IDLE_CONNS S:TASK_RQ U:TASK_RQ S:QUEUE U:QUEUE S:IDLE_CONNS locked: IDLE_CONNS(S)
  R:TASK_WQ S:TASK_WQ R:TASK_WQ S:TASK_WQ R:TASK_WQ S:TASK_WQ R:TASK_WQ locked: TASK_WQ(R)
  W:STK_TABLE W:STK_TABLE_UPDT U:STK_TABLE_UPDT W:STK_TABLE W:STK_TABLE_UPDT U:STK_TABLE_UPDT W:STK_TABLE W:STK_TABLE_UPDT locked: STK_TABLE(W) STK_TABLE_UPDT(W)

The format is slightly different (label(status)) so as to easily
differentiate them visually from the history.

2 months agoDEBUG: threads: merge successive idempotent lock operations in history
Willy Tarreau [Mon, 5 May 2025 16:24:40 +0000 (18:24 +0200)] 
DEBUG: threads: merge successive idempotent lock operations in history

In order to make the lock history a bit more useful, let's try to merge
adjacent lock/unlock sequences that don't change anything for other
threads. For this we can replace the last unlock with the new operation
on the same label, and even just not store it if it was the same as the
one before the unlock, since in the end it's the same as if the unlock
had not been done.

Now loops that used to be filled with "R:LISTENER U:LISTENER" show more
useful info such as:

  S:IDLE_CONNS U:IDLE_CONNS S:PEER U:PEER S:IDLE_CONNS U:IDLE_CONNS R:LISTENER U:LISTENER
  U:STK_TABLE W:STK_SESS U:STK_SESS R:STK_TABLE U:STK_TABLE W:STK_SESS U:STK_SESS R:STK_TABLE
  R:STK_TABLE U:STK_TABLE W:STK_SESS U:STK_SESS W:STK_TABLE_UPDT U:STK_TABLE_UPDT S:PEER

It's worth noting that it can sometimes induce confusion when recursive
locks of the same label are used (a few exist on peers or stick-tables),
as in such a case the two operations would be needed. However these ones
are already undebuggable, so instead they will just have to be renamed
to make sure they use a distinct label.

2 months agoDEBUG: threads: don't keep lock label "OTHER" in the per-thread history
Willy Tarreau [Mon, 5 May 2025 16:10:57 +0000 (18:10 +0200)] 
DEBUG: threads: don't keep lock label "OTHER" in the per-thread history

Most threads are filled with "R:OTHER U:OTHER" in their history. Since
anything non-important can use other it's not observable but it pollutes
the history. Let's just drop OTHER entirely during the recording.

2 months agoBUG/MINOR: tools: make parseline report the required space for the trailing 0
Willy Tarreau [Mon, 5 May 2025 15:58:04 +0000 (17:58 +0200)] 
BUG/MINOR: tools: make parseline report the required space for the trailing 0

The fix in commit 09a325a4de ("BUG/MINOR: tools: always terminate empty
lines") is insufficient. While it properly addresses the lack of trailing
zero, it doesn't account for it in the returned outlen that is used to
allocate a larger line. This happens at boot if the very first line of
the test file is exactly a sharp with nothing else. In this case it will
return a length 0 and the caller (parse_cfg()) will try to re-allocate an
entry of size zero and will fail, bailing out a lack of memory. This time
it should really be OK.

It doesn't need to be backported, unless the patch above would be.

2 months agoBUG/MINOR: tools: always terminate empty lines
Willy Tarreau [Mon, 5 May 2025 15:33:22 +0000 (17:33 +0200)] 
BUG/MINOR: tools: always terminate empty lines

Since latest commit 7e4a2f39ef ("BUG/MINOR: tools: do not create an empty
arg from trailing spaces"), an empty line will no longer produce an arg
and no longer append a trailing zero to them. This was not visible because
one is already present in the input string, however all the trailing args
are set to out+outpos-1, which now points one char before the buffer since
nothing was emitted, and was noticed by ASAN, and/or when parsing garbage.
Let's make sure to always emit the zero for empty lines as well to address
this issue. No backport is needed unless the patch above gets backported.

2 months agoMINOR: cfgparse: visually show the input line on empty args
Willy Tarreau [Mon, 5 May 2025 14:13:33 +0000 (16:13 +0200)] 
MINOR: cfgparse: visually show the input line on empty args

Now when an empty arg is found on a line, we emit the sanitized
input line and the position of the first empty arg so as to help
the user figure the cause (likely an empty environment variable).

Co-authored-by: Valentine Krasnobaeva <vkrasnobaeva@haproxy.com>
2 months agoMINOR: tools: make parse_line() provide hints about empty args
Willy Tarreau [Mon, 5 May 2025 14:11:42 +0000 (16:11 +0200)] 
MINOR: tools: make parse_line() provide hints about empty args

In order to help parse_line() callers report the position of empty
args to the user, let's decide that if no error is emitted, then
we'll stuff the errptr with the position of the first empty arg
without affecting the return value.

Co-authored-by: Valentine Krasnobaeva <vkrasnobaeva@haproxy.com>
2 months agoMEDIUM: config: warn about the consequences of empty arguments on a config line
Willy Tarreau [Fri, 2 May 2025 13:47:41 +0000 (15:47 +0200)] 
MEDIUM: config: warn about the consequences of empty arguments on a config line

For historical reasons, the config parser relies on the trailing '\0'
to detect the end of the line being parsed. When the lines started to be
tokenized into arguments, this principle has been preserved, and now all
the parsers rely on *args[arg]='\0' to detect the end of a line. But as
reported in issue #2944, while most of the time it breaks the parsing
like below:

     http-request deny if { path_dir '' }

it can also cause some elements to be silently ignored like below:

     acl bad_path path_sub '%2E' '' '%2F'

This may also subtly happen with environment variables that don't exist
or which are empty:

     acl bad_path path_sub '%2E' "$BAD_PATTERN" '%2F'

Fortunately, parse_line() returns the number of arguments found, so it's
easy from the callers to verify if any was empty. The goal of this commit
is not to perform sensitive changes, it's only to mention when parsing a
line that an empty argument was found and alert about its consequences
using a warning. Most of the time when this happens, the config does not
parse. But for examples as the ACLs above, there could be consequences
that are better detected early.

This patch depends on this previous fix:
   BUG/MINOR: tools: do not create an empty arg from trailing spaces

Co-authored-by: Valentine Krasnobaeva <vkrasnobaeva@haproxy.com>
2 months agoBUG/MINOR: tools: do not create an empty arg from trailing spaces
Willy Tarreau [Fri, 2 May 2025 13:46:18 +0000 (15:46 +0200)] 
BUG/MINOR: tools: do not create an empty arg from trailing spaces

Trailing spaces on the lines of the config file create an empty arg
which makes it complicated to detect really empty args. Let's first
address this. Note that it is not user-visible but prevents from
fixing user-visible issues. No backport is needed.

The initial issue was introduced with this fix that already tried to
address it:

    8a6767d266 ("BUG/MINOR: config: don't count trailing spaces as empty arg (v2)")

The current patch properly addresses leading and trailing spaces by
only counting arguments if non-lws chars were found on the line. LWS
do not cause a transition to a new arg anymore but they complete the
current one. The whole new code relies on a state machine to detect
when to create an arg (!in_arg->in_arg), and when to close the current
arg. A special care was taken for word expansion in the form of
"${ARGS[*]}" which still continue to emit individual arguments past
the first LWS. This example works fine:

    ARGS="100 check inter 1000"
    server name 192.168.1."${ARGS[*]}"

It properly results in 6 args:

    "server", "name", "192.168.1.100", "check", "inter", "1000"

This fix should not have any visible user impact and is a bit tricky,
so it's best not to backport it, at least for a while.

Co-authored-by: Valentine Krasnobaeva <vkrasnobaeva@haproxy.com>
2 months agoBUG/MINOR: acme/cli: don't output error on success
William Lallemand [Fri, 2 May 2025 19:15:45 +0000 (21:15 +0200)] 
BUG/MINOR: acme/cli: don't output error on success

Previous patch 7251c13c7 ("MINOR: acme: move the acme task init in a dedicated
function") mistakenly returned the wrong error code when "acme renew" parsing
was successful, and tried to emit an error message.

This patch fixes the issue by returning 0 when the acme task was correctly
scheduled to start.

No backport needed.

2 months agoBUG/MEDIUM: stktable: fix sc_*(<ctr>) BUG_ON() regression with ctx > 9
Aurelien DARRAGON [Fri, 2 May 2025 14:25:57 +0000 (16:25 +0200)] 
BUG/MEDIUM: stktable: fix sc_*(<ctr>) BUG_ON() regression with ctx > 9

As reported in GH #2958, commit 6c9b315 caused a regression with sc_*
fetches and tracked counter id > 9.

As such, the below configuration would cause a BUG_ON() to be triggered:

  global
    log stdout format raw local0
    tune.stick-counters 11

  defaults
    log global
    mode http

  frontend www
    bind *:8080

    acl track_me bool(true)
    http-request set-var(txn.track_var) str("a")
    http-request track-sc10 var(txn.track_var) table rate_table if track_me
    http-request set-var(txn.track_var_rate) sc_gpc_rate(0,10,rate_table)
    http-request return status 200

  backend rate_table
      stick-table type string size 1k expire 5m store gpc_rate(1,1m)

While in 6c9b315 the src_fetch logic was removed from
smp_fetch_sc_stkctr(), num > 9 is indeed not expected anymore as
original num value. But what we didn't consider is that num is effectively
re-assigned for generic sc_* variant.

Thus the BUG_ON() is misplaced as it should only be evaluated for
non-generic fetches. It explains why it triggers with valid configurations

Thanks to GH user @tkjaer for his detailed report and bug analysis

No backport needed, this bug is specific to 3.2.

2 months ago[RELEASE] Released version 3.2-dev14 v3.2-dev14
Willy Tarreau [Fri, 2 May 2025 14:23:28 +0000 (16:23 +0200)] 
[RELEASE] Released version 3.2-dev14

Released version 3.2-dev14 with the following main changes :
    - MINOR: acme: retry label always do a request
    - MINOR: acme: does not leave task for next request
    - BUG/MINOR: acme: reinit the retries only at next request
    - MINOR: acme: change the default max retries to 5
    - MINOR: acme: allow a delay after a valid response
    - MINOR: acme: wait 5s before checking the challenges results
    - MINOR: acme: emit a log when starting
    - MINOR: acme: delay of 5s after the finalize
    - BUG/MEDIUM: quic: Let it be known if the tasklet has been released.
    - BUG/MAJOR: tasks: fix task accounting when killed
    - CLEANUP: tasks: use the local state, not t->state, to check for tasklets
    - DOC: acme: external account binding is not supported
    - MINOR: hlua: ignore "tune.lua.bool-sample-conversion" if set after "lua-load"
    - MEDIUM: peers: Give up if we fail to take locks in hot path
    - MEDIUM: stick-tables: defer adding updates to a tasklet
    - MEDIUM: stick-tables: Limit the number of old entries we remove
    - MEDIUM: stick-tables: Limit the number of entries we expire
    - MINOR: cfgparse-global: add explicit error messages in cfg_parse_global_env_opts
    - MINOR: ssl: add function to extract X509 notBefore date in time_t
    - BUILD: acme: need HAVE_ASN1_TIME_TO_TM
    - MINOR: acme: move the acme task init in a dedicated function
    - MEDIUM: acme: add a basic scheduler
    - MINOR: acme: emit a log when the scheduler can't start the task

2 months agoMINOR: acme: emit a log when the scheduler can't start the task
William Lallemand [Fri, 2 May 2025 14:12:41 +0000 (16:12 +0200)] 
MINOR: acme: emit a log when the scheduler can't start the task

Emit an error log when the renewal scheduler can't start the task.

2 months agoMEDIUM: acme: add a basic scheduler
William Lallemand [Fri, 2 May 2025 12:55:08 +0000 (14:55 +0200)] 
MEDIUM: acme: add a basic scheduler

This patch implements a very basic scheduler for the ACME tasks.

The scheduler is a task which is started from the postparser function
when at least one acme section was configured.

The scheduler will loop over the certificates in the ckchs_tree, and for
each certificate will start an ACME task if the notAfter date is past
curtime + (notAfter - notBefore) / 12, or 7 days if notBefore is not
available.

Once the lookup over all certificates is terminated, the task will sleep
and will wakeup after 12 hours.

2 months agoMINOR: acme: move the acme task init in a dedicated function
William Lallemand [Fri, 2 May 2025 12:47:03 +0000 (14:47 +0200)] 
MINOR: acme: move the acme task init in a dedicated function

acme_start_task() is a dedicated function which starts an acme task
for a specified <store> certificate.

The initialization code was move from the "acme renew" command parser to
this function, in order to be called from a scheduler.

2 months agoBUILD: acme: need HAVE_ASN1_TIME_TO_TM
William Lallemand [Fri, 2 May 2025 13:57:31 +0000 (15:57 +0200)] 
BUILD: acme: need HAVE_ASN1_TIME_TO_TM

Restrict the build of the ACME feature to libraries which provide
ASN1_TIME_to_tm() function.

2 months agoMINOR: ssl: add function to extract X509 notBefore date in time_t
William Lallemand [Fri, 2 May 2025 12:42:28 +0000 (14:42 +0200)] 
MINOR: ssl: add function to extract X509 notBefore date in time_t

Add x509_get_notbefore_time_t() which returns the notBefore date in
time_t format.

2 months agoMINOR: cfgparse-global: add explicit error messages in cfg_parse_global_env_opts
Valentine Krasnobaeva [Mon, 28 Apr 2025 21:00:17 +0000 (23:00 +0200)] 
MINOR: cfgparse-global: add explicit error messages in cfg_parse_global_env_opts

When env variable name or value are not provided for setenv/presetenv it's not
clear from the old error message shown at stderr, what exactly is missed. User
needs to search in it's configuration.

Let's add more explicit error messages about these inconsistencies.

No need to be backported.

2 months agoMEDIUM: stick-tables: Limit the number of entries we expire
Olivier Houchard [Fri, 2 May 2025 12:07:53 +0000 (12:07 +0000)] 
MEDIUM: stick-tables: Limit the number of entries we expire

In process_table_expire(), limit the number of entries we remove in one
call, and just reschedule the task if there's more to do. Removing
entries require to use the heavily contended update write lock, and we
don't want to hold it for too long.
This helps getting stick tables perform better under heavy load.

2 months agoMEDIUM: stick-tables: Limit the number of old entries we remove
Olivier Houchard [Fri, 2 May 2025 12:05:37 +0000 (12:05 +0000)] 
MEDIUM: stick-tables: Limit the number of old entries we remove

Limit the number of old entries we remove in one call of
stktable_trash_oldest(), as we do so while holding the heavily contended
update write lock, so we'd rather not hold it for too long.
This helps getting stick tables perform better under heavy load.

2 months agoMEDIUM: stick-tables: defer adding updates to a tasklet
Olivier Houchard [Fri, 2 May 2025 11:46:54 +0000 (11:46 +0000)] 
MEDIUM: stick-tables: defer adding updates to a tasklet

There is a lot of contention trying to add updates to the tree. So
instead of trying to add the updates to the tree right away, just add
them to a mt-list (with one mt-list per thread group, so that the
mt-list does not become the new point of contention that much), and
create a tasklet dedicated to adding updates to the tree, in batchs, to
avoid keeping the update lock for too long.
This helps getting stick tables perform better under heavy load.

2 months agoMEDIUM: peers: Give up if we fail to take locks in hot path
Olivier Houchard [Fri, 2 May 2025 11:29:05 +0000 (11:29 +0000)] 
MEDIUM: peers: Give up if we fail to take locks in hot path

In peer_send_msgs(), give up in order to retry later if we failed at
getting the update read lock.
Similarly, in __process_running_peer_sync(), give up and just reschedule
the task if we failed to get the peer lock. There is an heavy contention
on both those locks, so we could spend a lot of time trying to get them.
This helps getting peers perform better under heavy load.

2 months agoMINOR: hlua: ignore "tune.lua.bool-sample-conversion" if set after "lua-load"
Aurelien DARRAGON [Fri, 2 May 2025 11:56:08 +0000 (13:56 +0200)] 
MINOR: hlua: ignore "tune.lua.bool-sample-conversion" if set after "lua-load"

tune.lua.bool-sample-conversion must be set before any lua-load or
lua-load-per-thread is used for it to be considered. Indeed, lua-load
directives are parsed on the fly and will cause some parts of the scripts
to be executed during init already (script body/init contexts).

As such, we cannot afford to have "tune.lua.bool-sample-conversion" set
after some Lua code was loaded, because it would mean that the setting
would be handled differently for Lua's code executed during or after
config parsing.

To avoid ambiguities, the documentation now states that the setting must
be set before any lua-load(-per-thread) directive, and if the setting
is met after some Lua was already loaded, the directive is ignored and
a warning informs about that.

It should fix GH #2957

It may be backported with 29b6d8af16 ("MINOR: hlua: rename
"tune.lua.preserve-smp-bool" to "tune.lua.bool-sample-conversion"")

2 months agoDOC: acme: external account binding is not supported
William Lallemand [Fri, 2 May 2025 10:03:21 +0000 (12:03 +0200)] 
DOC: acme: external account binding is not supported

Add a note on external account binding in the ACME section.

2 months agoCLEANUP: tasks: use the local state, not t->state, to check for tasklets
Willy Tarreau [Fri, 2 May 2025 08:55:43 +0000 (10:55 +0200)] 
CLEANUP: tasks: use the local state, not t->state, to check for tasklets

There's no point reading t->state to check for a tasklet after we've
atomically read the state into the local "state" variable. Not only it's
more expensive, it's also less clear whether that state is supposed to
be atomic or not. And in any case, tasks and tasklets have their type
forever and the one reflected in state is correct and stable.

2 months agoBUG/MAJOR: tasks: fix task accounting when killed
Willy Tarreau [Fri, 2 May 2025 08:34:16 +0000 (10:34 +0200)] 
BUG/MAJOR: tasks: fix task accounting when killed

After recent commit b81c9390f ("MEDIUM: tasks: Mutualize the TASK_KILLED
code between tasks and tasklets"), the task accounting was no longer
correct for killed tasks due to the decrement of tasks in list that was
no longer done, resulting in infinite loops in process_runnable_tasks().
This just illustrates that this code remains complex and should be further
cleaned up. No backport is needed, as this was in 3.2.

2 months agoBUG/MEDIUM: quic: Let it be known if the tasklet has been released.
Olivier Houchard [Thu, 1 May 2025 12:39:39 +0000 (14:39 +0200)] 
BUG/MEDIUM: quic: Let it be known if the tasklet has been released.

quic_conn_release() may, or may not, free the tasklet associated with
the connection. So make it return 1 if it was, and 0 otherwise, so that
if it was called from the tasklet handler itself, the said handler can
act accordingly and return NULL if the tasklet was destroyed.
This should be backported if 9240cd4a2771245fae4d0d69ef025104b14bfc23
is backported.

2 months agoMINOR: acme: delay of 5s after the finalize
William Lallemand [Fri, 2 May 2025 08:34:48 +0000 (10:34 +0200)] 
MINOR: acme: delay of 5s after the finalize

Let 5 seconds by default to the server after the finalize to generate
the certificate. Some servers would not send a Retry-After during
processing.

2 months agoMINOR: acme: emit a log when starting
William Lallemand [Fri, 2 May 2025 08:23:42 +0000 (10:23 +0200)] 
MINOR: acme: emit a log when starting

Emit a administrative log when starting the ACME client for a
certificate.

2 months agoMINOR: acme: wait 5s before checking the challenges results
William Lallemand [Fri, 2 May 2025 08:18:24 +0000 (10:18 +0200)] 
MINOR: acme: wait 5s before checking the challenges results

Wait 5 seconds before trying to check if the challenges are ready, so it
let time to server to execute the challenges.

2 months agoMINOR: acme: allow a delay after a valid response
William Lallemand [Fri, 2 May 2025 08:16:12 +0000 (10:16 +0200)] 
MINOR: acme: allow a delay after a valid response

Use the retryafter value to set a delay before doing the next request
when the previous response was valid.

2 months agoMINOR: acme: change the default max retries to 5
William Lallemand [Fri, 2 May 2025 07:40:12 +0000 (09:40 +0200)] 
MINOR: acme: change the default max retries to 5

Change the default max retries constant to 5 instead of 3.
Some servers can be be a bit long to execute the challenge.

2 months agoBUG/MINOR: acme: reinit the retries only at next request
William Lallemand [Fri, 2 May 2025 07:27:15 +0000 (09:27 +0200)] 
BUG/MINOR: acme: reinit the retries only at next request

The retries were reinitialized incorrectly, it must be reinit only
when we didn't retry. So any valid response would reinit the retries
number.

2 months agoMINOR: acme: does not leave task for next request
William Lallemand [Fri, 2 May 2025 07:22:23 +0000 (09:22 +0200)] 
MINOR: acme: does not leave task for next request

The next request was always leaving the task befor initializing the
httpclient. This patch optimize it by jumping to the next step at the
end of the current one. This way, only the httpclient is doing a
task_wakeup() to handle the response. But transiting from response to
the next request does not leave the task.

2 months agoMINOR: acme: retry label always do a request
William Lallemand [Fri, 2 May 2025 07:15:07 +0000 (09:15 +0200)] 
MINOR: acme: retry label always do a request

Doing a retry always result in initializing a request again, set
ACME_HTTP_REQ directly in the label instead of doing it for each step.

2 months ago[RELEASE] Released version 3.2-dev13 v3.2-dev13
Willy Tarreau [Wed, 30 Apr 2025 16:25:28 +0000 (18:25 +0200)] 
[RELEASE] Released version 3.2-dev13

Released version 3.2-dev13 with the following main changes :
    - MEDIUM: checks: Make sure we return the tasklet from srv_chk_io_cb
    - MEDIUM: listener: Make sure w ereturn the tasklet from accept_queue_process
    - MEDIUM: mux_fcgi: Make sure we return the tasklet from fcgi_deferred_shut
    - MEDIUM: quic: Make sure we return the tasklet from qcc_io_cb
    - MEDIUM: quic: Make sure we return NULL in quic_conn_app_io_cb if needed
    - MEDIUM: quic: Make sure we return the tasklet from quic_accept_run
    - BUG/MAJOR: tasklets: Make sure he tasklet can't run twice
    - BUG/MAJOR: listeners: transfer connection accounting when switching listeners
    - MINOR: ssl/cli: add a '-t' option to 'show ssl sni'
    - DOC: config: fix ACME paragraph rendering issue
    - DOC: config: clarify log-forward "host" option
    - MINOR: promex: expose ST_I_PX_RATE (current_session_rate)
    - BUILD: acme: use my_strndup() instead of strndup()
    - BUILD: leastconn: fix build warning when building without threads on old machines
    - MINOR: threads: prepare DEBUG_THREAD to receive more values
    - MINOR: threads: turn the full lock debugging to DEBUG_THREAD=2
    - MEDIUM: threads: keep history of taken locks with DEBUG_THREAD > 0
    - MINOR: threads/cli: display the lock history on "show threads"
    - MEDIUM: thread: set DEBUG_THREAD to 1 by default
    - BUG/MINOR: ssl/acme: free EVP_PKEY upon error
    - MINOR: acme: separate the code generating private keys
    - MINOR: acme: failure when no directory is specified
    - MEDIUM: acme: generate the account file when not found
    - MEDIUM: acme: use 'crt-base' to load the account key
    - MINOR: compiler: add more macros to detect macro definitions
    - MINOR: cli: split APPCTX_CLI_ST1_PROMPT into two distinct flags
    - MEDIUM: cli: make the prompt mode configurable between n/i/p
    - MEDIUM: mcli: make the prompt mode configurable between i/p
    - MEDIUM: mcli: replicate the current mode when enterin the worker process
    - DOC: configuration: acme account key are auto generated
    - CLEANUP: acme: remove old TODO for account key
    - DOC: configuration: add quic4 to the ssl-f-use example
    - BUG/MINOR: acme: does not try to unlock after a failed trylock
    - BUG/MINOR: mux-h2: fix the offset of the pattern for the ping frame
    - MINOR: tcp: add support for setting TCP_NOTSENT_LOWAT on both sides
    - BUG/MINOR: acme: creating an account should not end the task
    - MINOR: quic: rename min/max fields for congestion window algo
    - MINOR: quic: refactor BBR API
    - BUG/MINOR: quic: ensure cwnd limits are always enforced
    - MINOR: thread: define cshared type
    - MINOR: quic: account for global congestion window
    - MEDIUM: quic: limit global Tx memory
    - MEDIUM: acme: use a map to store tokens and thumbprints
    - BUG/MINOR: acme: remove references to virt@acme
    - MINOR: applet: add appctx_schedule() macro
    - BUG/MINOR: dns: add tempo between 2 connection attempts for dns servers
    - CLEANUP: dns: remove unused dns_stream_server struct member
    - BUG/MINOR: dns: prevent ds accumulation within dss
    - CLEANUP: proxy: mention that px->conn_retries isn't relevant in some cases
    - DOC: ring: refer to newer RFC5424
    - MINOR: tools: make my_strndup() take a size_t len instead of and int
    - MINOR: Add "sigalg" to "sigalg name" helper function
    - MINOR: ssl: Add traces to ssl init/close functions
    - MINOR: ssl: Add traces to recv/send functions
    - MINOR: ssl: Add traces to ssl_sock_io_cb function
    - MINOR: ssl: Add traces around SSL_do_handshake call
    - MINOR: ssl: Add traces to verify callback
    - MINOR: ssl: Add ocsp stapling callback traces
    - MINOR: ssl: Add traces to the switchctx callback
    - MINOR: ssl: Add traces about sigalg extension parsing in clientHello callback
    - MINOR: Add 'conn' param to ssl_sock_chose_sni_ctx
    - BUG/MEDIUM: mux-spop: Wait end of handshake to declare a spop connection ready
    - BUG/MEDIUM: mux-spop: Handle CLOSING state and wait for AGENT DISCONNECT frame
    - BUG/MINOR: mux-h1: Don't pretend connection was released for TCP>H1>H2 upgrade
    - BUG/MINOR: mux-h1: Fix trace message in h1_detroy() to not relay on connection
    - BUILD: ssl: Fix wolfssl build
    - BUG/MINOR: mux-spop: Use the right bitwise operator in spop_ctl()
    - MEDIUM: mux-quic: increase flow-control on each bufsize
    - MINOR: mux-quic: limit emitted MSD frames count per qcs
    - MINOR: add hlua_yield_asap() helper
    - MINOR: hlua_fcn: enforce yield after *_get_stats() methods
    - DOC: config: restore default values for resolvers hold directive
    - MINOR: ssl/cli: "acme ps" shows the acme tasks
    - MINOR: acme: acme_ctx_destroy() returns upon NULL
    - MINOR: acme: use acme_ctx_destroy() upon error
    - MEDIUM: tasks: Mutualize code between tasks and tasklets.
    - MEDIUM: tasks: More code factorization
    - MEDIUM: tasks: Remove TASK_IN_LIST and use TASK_QUEUED instead.
    - MINOR: tasks: Remove unused tasklet_remove_from_tasklet_list
    - MEDIUM: tasks: Mutualize the TASK_KILLED code between tasks and tasklets
    - BUG/MEDIUM: connections: Report connection closing in conn_create_mux()
    - BUILD/MEDIUM: quic: Make sure we build with recent changes

2 months agoBUILD/MEDIUM: quic: Make sure we build with recent changes
Olivier Houchard [Wed, 30 Apr 2025 16:22:46 +0000 (18:22 +0200)] 
BUILD/MEDIUM: quic: Make sure we build with recent changes

TASK_IN_LIST has been changed to TASK_QUEUED, but one was missed in
quic_conn.c, so fix that.

2 months agoBUG/MEDIUM: connections: Report connection closing in conn_create_mux()
Olivier Houchard [Wed, 30 Apr 2025 11:19:38 +0000 (13:19 +0200)] 
BUG/MEDIUM: connections: Report connection closing in conn_create_mux()

Add an extra parametre to conn_create_mux(), "closed_connection".
If a pointer is provided, then let it know if the connection was closed.
Callers have no way to determine that otherwise, and we need to know
that, at least in ssl_sock_io_cb(), as if the connection was closed we
need to return NULL, as the tasklet was free'd, otherwise that can lead
to memory corruption and crashes.
This should be backported if 9240cd4a2771245fae4d0d69ef025104b14bfc23
is backported too.

2 months agoMEDIUM: tasks: Mutualize the TASK_KILLED code between tasks and tasklets
Olivier Houchard [Tue, 29 Apr 2025 13:46:20 +0000 (15:46 +0200)] 
MEDIUM: tasks: Mutualize the TASK_KILLED code between tasks and tasklets

The code to handle a task/tasklet when it's been killed before it were
to run is mostly identical, so move it outside of task and tasklet
specific code, and inside the common code.

This commit is just cosmetic, and should have no impact.

2 months agoMINOR: tasks: Remove unused tasklet_remove_from_tasklet_list
Olivier Houchard [Tue, 29 Apr 2025 13:25:53 +0000 (15:25 +0200)] 
MINOR: tasks: Remove unused tasklet_remove_from_tasklet_list

Remove tasklet_remove_from_tasklet_list, as the function hasn't been
used for a long time, and there is little reason to keep it.

2 months agoMEDIUM: tasks: Remove TASK_IN_LIST and use TASK_QUEUED instead.
Olivier Houchard [Tue, 29 Apr 2025 13:24:54 +0000 (15:24 +0200)] 
MEDIUM: tasks: Remove TASK_IN_LIST and use TASK_QUEUED instead.

TASK_QUEUED was used to mean "the task has been scheduled to run",
TASK_IN_LIST was used to mean "the tasklet has been scheduled to run",
remove TASK_IN_LIST and just use TASK_QUEUED for tasklets instead.

This commit is just cosmetic, and should not have any impact.

2 months agoMEDIUM: tasks: More code factorization
Olivier Houchard [Tue, 29 Apr 2025 13:15:27 +0000 (15:15 +0200)] 
MEDIUM: tasks: More code factorization

There is some code that should run no matter if the task was killed or
not, and was needlessly duplicated, so only use one instance.
This also fixes a small bug when a tasklet that got killed before it
could run would still count as a tasklet that ran, when it should not,
which just means that we'd run one less useful task before going back to
the poller.
This commit is mostly cosmetic, and should not have any impact.

2 months agoMEDIUM: tasks: Mutualize code between tasks and tasklets.
Olivier Houchard [Tue, 29 Apr 2025 13:15:01 +0000 (15:15 +0200)] 
MEDIUM: tasks: Mutualize code between tasks and tasklets.

The code that checks if we're currently running, and waits if so, was
identical between tasks and tasklets, so move it in code common to tasks
and tasklets.
This commit is just cosmetic, and should not have any impact.

2 months agoMINOR: acme: use acme_ctx_destroy() upon error
William Lallemand [Wed, 30 Apr 2025 15:18:46 +0000 (17:18 +0200)] 
MINOR: acme: use acme_ctx_destroy() upon error

Use acme_ctx_destroy() instead of a simple free() upon error in the
"acme renew" error handling.

It's better to use this function to be sure than everything has been
been freed.

2 months agoMINOR: acme: acme_ctx_destroy() returns upon NULL
William Lallemand [Wed, 30 Apr 2025 15:17:58 +0000 (17:17 +0200)] 
MINOR: acme: acme_ctx_destroy() returns upon NULL

acme_ctx_destroy() returns when its argument is NULL.

2 months agoMINOR: ssl/cli: "acme ps" shows the acme tasks
William Lallemand [Wed, 30 Apr 2025 13:49:53 +0000 (15:49 +0200)] 
MINOR: ssl/cli: "acme ps" shows the acme tasks

Implement a way to display the running acme tasks over the CLI.

It currently only displays a "Running" status with the certificate name
and the acme section from the configuration.

The displayed running tasks are limited to the size of a buffer for now,
it will require a backref list later to be called multiple times to
resume the list.

2 months agoDOC: config: restore default values for resolvers hold directive
Aurelien DARRAGON [Wed, 30 Apr 2025 14:56:00 +0000 (16:56 +0200)] 
DOC: config: restore default values for resolvers hold directive

Default values for hold directive (resolver context) used to be documented
but this was lost when the keyword description was reworked in 24b319b
("Default value is 10s for "valid", 0s for "obsolete" and 30s for
others.")

Restoring the part that describes the default value.

It may be backported to all stable versions with 24b319b

2 months agoMINOR: hlua_fcn: enforce yield after *_get_stats() methods
Aurelien DARRAGON [Wed, 30 Apr 2025 14:41:16 +0000 (16:41 +0200)] 
MINOR: hlua_fcn: enforce yield after *_get_stats() methods

{listener,proxy,server}_get_stats() methods are know to be expensive,
expecially if used under an iteration. Indeed, while automatic yield
is performed every X lua instructions (defaults to 10k), computing an
object's stats 10K times in a single cpu loop is not desirable and
could create contention.

In this patch we leverage hlua_yield_asap() at the end of *_get_stats()
methods in order to force the automatic yield to occur ASAP after the
method returns. Hopefully this should help in similar scenarios as the
one described in GH #2903

2 months agoMINOR: add hlua_yield_asap() helper
Aurelien DARRAGON [Wed, 30 Apr 2025 14:37:56 +0000 (16:37 +0200)] 
MINOR: add hlua_yield_asap() helper

When called, this function will try to enforce a yield (if available) as
soon as possible. Indeed, automatic yield is already enforced every X
Lua instructions. However, there may be some cases where we know after
running heavy operation that we should yield already to avoid taking too
much CPU at once.

This is what this function offers, instead of asking the user to manually
yield using "core.yield()" from Lua itself after using an expensive
Lua method offered by haproxy, we can directly enforce the yield without
the need to do it in the Lua script.

2 months agoMINOR: mux-quic: limit emitted MSD frames count per qcs
Amaury Denoyelle [Mon, 28 Apr 2025 13:36:44 +0000 (15:36 +0200)] 
MINOR: mux-quic: limit emitted MSD frames count per qcs

The previous commit has implemented a new calcul method for
MAX_STREAM_DATA frame emission. Now, a frame may be emitted as soon as a
buffer was consumed by a QCS instance.

This will probably increase the number of MAX_STREAM_DATA frame
emission. It may even cause a series of frame emitted for the same
stream with increasing values under high load, which is completely
unnecessary.

To improve this, limit the number of MAX_STREAM_DATA frames built to one
per QCS instance. This is implemented by storing a reference to this
frame in QCS structure via a new member <tx.msd_frm>.

Note that to properly reset QCS msd_frm member, emission of flow-control
frames have been changed. Now, each frame is emitted individually. On
one side, it is better as it prevent to emit frames related to different
streams in a single datagram, which is not desirable in case of packet
loss. However, this can also increase sendto() syscall invocation.

2 months agoMEDIUM: mux-quic: increase flow-control on each bufsize
Amaury Denoyelle [Wed, 19 Mar 2025 15:09:08 +0000 (16:09 +0100)] 
MEDIUM: mux-quic: increase flow-control on each bufsize

Recently, QCS Rx allocation buffer method has been improved. It is now
possible to allocate multiple buffers per QCS instances, which was
necessary to improve HTTP/3 POST throughput.

However, a limitation remained related to the emission of
MAX_STREAM_DATA. These frames are only emitted once at least half of the
receive capacity has been consumed by its QCS instance. This may be too
restrictive when a client need to upload a large payload.

Improve this by adjusting MAX_STREAM_DATA allocation. If QCS capacity is
still limited to 1 or 2 buffers max, the old calcul is still used. This
is necessary when user has limited upload throughput via their
configuration. If QCS capacity is more than 2 buffers, a new frame is
emitted if at least a buffer was consumed.

This patch has reduced number of STREAM_DATA_BLOCKED frames received in
POST tests with some specific clients.

2 months agoBUG/MINOR: mux-spop: Use the right bitwise operator in spop_ctl()
Christopher Faulet [Wed, 30 Apr 2025 13:58:53 +0000 (15:58 +0200)] 
BUG/MINOR: mux-spop: Use the right bitwise operator in spop_ctl()

Becaues of a typo, '||' was used instead of '|' to test the SPOP conneciton
flags and decide if the mux is ready or not. The regression was introduced
in the commit fd7ebf117 ("BUG/MEDIUM: mux-spop: Wait end of handshake to
declare a spop connection ready").

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

2 months agoBUILD: ssl: Fix wolfssl build
Remi Tricot-Le Breton [Wed, 30 Apr 2025 13:26:30 +0000 (15:26 +0200)] 
BUILD: ssl: Fix wolfssl build

The newly added SSL traces require an extra 'conn' parameter to
ssl_sock_chose_sni_ctx which was added in the "regular" code but not in
the wolfssl specific one.
Wolfssl also has a different prototype for some getter functions
(SSL_get_servername for instance), which do not expect a const SSL while
openssl version does.

2 months agoBUG/MINOR: mux-h1: Fix trace message in h1_detroy() to not relay on connection
Christopher Faulet [Wed, 30 Apr 2025 12:32:16 +0000 (14:32 +0200)] 
BUG/MINOR: mux-h1: Fix trace message in h1_detroy() to not relay on connection

h1_destroy() may be called to release a H1C after a multiplexer upgrade. In
that case, the connection is no longer attached to the H1C. It must not be
used in the h1 trace message because the connection context is no longer a H1C.

Because of this bug, when a H1>H2 upgrade is performed, a crash may be
experienced if the H1 traces are enabled.

This patch must be backport to all stable versions.

2 months agoBUG/MINOR: mux-h1: Don't pretend connection was released for TCP>H1>H2 upgrade
Christopher Faulet [Wed, 30 Apr 2025 12:16:42 +0000 (14:16 +0200)] 
BUG/MINOR: mux-h1: Don't pretend connection was released for TCP>H1>H2 upgrade

When an applicative upgrade of the H1 multiplexer is performed, we must not
pretend the connection was released.  Indeed, in that case, a H1 stream is
still their with a stream connector attached on it. It must be detached
first before releasing the H1 connection and the underlying connection. So
it is important to not pretend the connection was already released.

Concretely, in that case h1_process() must return 0 instead of -1. It is
minor error because, AFAIK, it is harmless. But it is not correct. So let's
fix it to avoid futur bugs.

To be clear, this happens when a TCP connection is upgraded to H1 connection
and a H2 preface is detected, leading to a second upgrade from H1 to H2.

This patch may be backport to all stable versions.

2 months agoBUG/MEDIUM: mux-spop: Handle CLOSING state and wait for AGENT DISCONNECT frame
Christopher Faulet [Mon, 28 Apr 2025 06:08:06 +0000 (08:08 +0200)] 
BUG/MEDIUM: mux-spop: Handle CLOSING state and wait for AGENT DISCONNECT frame

In the SPOE specification, when an error occurred on the SPOP connection,
HAProxy must send a DISCONNECT frame and wait for the agent DISCONNECT frame
in return before trully closing the connection.

However, this part was not properly handled by the SPOP multiplexer. In this
case, the SPOP connection should be in the CLOSING state. But this state was
not used at all. Depending on when the error was encountered, the connection
could be closed immediately, without sending any DISCONNECT frame. It was
the case when an early error was detected during the AGENT-HELLO frame
parsing. Or it could be moved from ERROR to FRAME_H state, as if no error
were detected. This case was less dramatic than it seemed because some flags
were also set to prevent any problem. But it was not obvious.

So now, the SPOP connection is properly switch to CLOSING state when an
DISCONNECT is sent to the agent to be able to wait for its DISCONNECT in
reply. spop_process_demux() was updated to parse frames in that state and
some validity checks was added.

This patch must be backport to 3.1.

2 months agoBUG/MEDIUM: mux-spop: Wait end of handshake to declare a spop connection ready
Christopher Faulet [Mon, 28 Apr 2025 06:01:40 +0000 (08:01 +0200)] 
BUG/MEDIUM: mux-spop: Wait end of handshake to declare a spop connection ready

A SPOP connection must not be considered as ready while the hello handshake
is not finished with success. In addition, no error or shutdown must have
been reported for the underlying connection. Otherwise a freshly openned
spop connexion may be reused while it is in fact dead, leading to a
connection retry.

This patch must be backported to 3.1.