Commit Graph

18386 Commits

Author SHA1 Message Date
Remi Tricot-Le Breton
6f57618877 BUG/MINOR: ssl: Memory leak of DH BIGNUM fields
When running HAProxy with OpenSSLv3, the two BIGNUMs used to build our
own DH parameters are not freed. It was not necessary previously because
ownership of those parameters was transferred to OpenSSL through the
DH_set0_pqg call.

This patch should be backported to 2.6.

(cherry picked from commit a2c21db155)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-11-17 16:41:50 +01:00
Miroslav Zagorac
b0c7490d4e BUG/MINOR: httpclient: fixed memory allocation for the SSL ca_file
The memory for the SSL ca_file was allocated only once (in the function
httpclient_create_proxy()) and that pointer was assigned to each created
proxy that the HTTP client uses.  This would not be a problem if this
memory was not freed in each individual proxy when it was deinitialized
in the function ssl_sock_free_srv_ctx().

  Memory allocation:
    src/http_client.c, function httpclient_create_proxy():
      1277:	if (!httpclient_ssl_ca_file)
      1278:		httpclient_ssl_ca_file = strdup("@system-ca");
      1280:	srv_ssl->ssl_ctx.ca_file = httpclient_ssl_ca_file;

  Memory deallocation:
    src/ssl_sock.c, function ssl_sock_free_srv_ctx():
      5613:	ha_free(&srv->ssl_ctx.ca_file);

This should be backported to version 2.6.

(cherry picked from commit a2ec192de3)
[cf: Context adjustment]
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-11-17 16:40:47 +01:00
Ilya Shipitsin
b055edbeeb CI: add monthly gcc cross compile jobs
Build only gcc cross compile jobs are added with monthly run to catch
rare errors, mostly 32bit <--> 64bit

(cherry picked from commit 5526f922af)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-11-17 16:36:02 +01:00
Amaury Denoyelle
eaff3aebc6 BUG/MINOR: quic: fix race condition on datagram purging
Each datagram is received by a random thread and dispatch to its
destination thread linked to the connection. Then, the datagram is
handled by the connection thread. Once this is done, datagram buffer
pointer is atomically set to NULL to mark it as consumed.

Consumed datagrams are purged before recvfrom() invocation on random
receiver threads. The check for NULL buffer must thus be done
atomically. This was not the case before this patch, which may have
triggered race conditions.

This bug has been introduced by commit
  91b2305ad7
  MINOR: quic: implement datagram cleanup for quic_receiver_buf

This should be backported up to 2.6 after previously mentionned commit.

(cherry picked from commit 0b13e94071)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-11-17 16:35:41 +01:00
Aurelien DARRAGON
55f37aa5e5 BUG/MINOR: log: fixing bug in tcp syslog_io_handler Octet-Counting
syslog_io_handler does specific treatment to handle syslog tcp octet
counting:

Logic was good, but a sneaky mistake prevented
rfc-6587 octet counting from working properly.

trash.area was used as an input buffer.
It does not make sense here since it is uninitialized.
Compilation was unaffected because trash is a thread
local "global" variable.

buf->area should definitely be used instead.

This should be backported as far as 2.4.

(cherry picked from commit 7faffdc6ab)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-11-17 16:34:42 +01:00
Amaury Denoyelle
d4c880d649 BUG/MINOR: quic: fix subscribe operation
Subscribing was not properly designed between quic-conn and quic MUX
layers. Align this as with in other haproxy components : <subs> field is
moved from the MUX to the quic-conn structure. All mention of qcc MUX is
cleaned up in quic_conn_subscribe()/quic_conn_unsubscribe().

Thanks to this change, ACK reception notification has been simplified.
It's now unnecessary to check for the MUX existence before waking it.
Instead, if <subs> quic-conn field is set, just wake-up the upper layer
tasklet without mentionning MUX. This should probably be extended to
other part in quic-conn code.

This should be backported up to 2.6.

(cherry picked from commit bbb1c68508)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-11-17 16:34:22 +01:00
Amaury Denoyelle
bf2a4b1203 MINOR: quic: remove unnecessary quic_session_accept()
A specialized listener accept was previously used for QUIC. This is now
unneeded and we can revert to the default one session_accept_fd().

One change of importance is that the call order between
conn_xprt_start() and conn_complete_session() is now reverted to the
default one. This means that MUX instance is now NULL during
qc_xprt_start() and its app-ops layer cannot be set here. This operation
has been delayed to qc_init() to prevent a segfault.

This should be backported up to 2.6.

(cherry picked from commit 0aba11e9e7)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-11-17 16:31:21 +01:00
Willy Tarreau
cf533a19e5 BUG/MEDIUM: stick-table: fix a race condition when updating the expiration task
Pierre Cheynier reported a rare crash that can affect stick-tables. When
a entry is created, the stick-table's expiration date is updated. But if
at exactly the same time the expiration task runs, it finishes by updating
its expiration timer without any protection, which may collide with the
call to task_queue() in another thread. In this case, it sometimes happens
that the first test for TICK_ETERNITY in task_queue() passes, then the
"expire" field is reset, then the BUG_ON() triggers, like below:

  FATAL: bug condition "task->expire == 0" matched at src/task.c:279
    call trace(13):
    |       0x649d86 [c6 04 25 01 00 00 00 00]: __task_queue+0xc6/0xce
    |       0x596bef [eb 90 ba 03 00 00 00 be]: stktable_requeue_exp+0x1ef/0x258
    |       0x596c87 [48 83 bb 90 00 00 00 00]: stktable_touch_with_exp+0x27/0x312
    |       0x563698 [48 8b 4c 24 18 4c 8b 4c]: stream_process_counters+0x3a8/0x6a2
    |       0x569344 [49 8b 87 f8 00 00 00 48]: process_stream+0x3964/0x3b4f
    |       0x64a80b [49 89 c7 e9 23 ff ff ff]: run_tasks_from_lists+0x3ab/0x566
    |       0x64ad66 [29 44 24 14 8b 7c 24 14]: process_runnable_tasks+0x396/0x71e
    |       0x6184b2 [83 3d 47 b3 a6 00 01 0f]: run_poll_loop+0x92/0x4ff
    |       0x618acf [48 8b 1d aa 20 7d 00 48]: main+0x1877ef
    | 0x7fc7d6ec1e45 [64 48 89 04 25 30 06 00]: libpthread:+0x7e45
    | 0x7fc7d6c9e4af [48 89 c7 b8 3c 00 00 00]: libc:clone+0x3f/0x5a

This one is extremely difficult to reproduce in practice, but adding a
printf() in process_table_expire() before assigning the value, while
running with an expire delay of 1ms helps a lot and may trigger the
crash in less than one minute on a 8-thread machine. Interestingly,
depending on the sequencing, this bug could also have made a table fail
to expire if the expire field got reset after the last update but before
the call to task_queue(). It would require to be quite unlucky so that
the table is never touched anymore after the race though.

The solution taken by this patch is to take the table's lock when
updating its expire value in stktable_requeue_exp(), enclosing the call
to task_queue(), and to update the task->expire field while still under
the lock in process_table_expire(). Note that thanks to previous changes,
taking the table's lock for the update in stktable_requeue_exp() costs
almost nothing since we now have the guarantee that this is not done more
than 1000 times a second.

Since process_table_expire() sets the timeout after returning from
stktable_trash_expired() which just released the lock, the two functions
were merged so that the task's expire field is updated while still under
the lock. Note that this heavily depends on the two previous patches
below:

  CLEANUP: stick-table: remove the unused table->exp_next
  OPTIM: stick-table: avoid atomic ops in stktable_requeue_exp() when possible

This is a bit complicated due to the fact that in 2.7 some parts were
made lockless. In 2.6 and older, the second part (the merge of the
two functions) will be sufficient since the task_queue() call was
already performed under the table's lock, and the patches above are
not needed.

This needs to be backported as far as 1.8 scrupulously following
instructions above.

(cherry picked from commit fbb934da90)
[wt: only merged the two functions above to update task->expire under
 the table's lock; tested and confirmed to fix the problem]
Signed-off-by: Willy Tarreau <w@1wt.eu>
2022-11-14 18:31:24 +01:00
Christopher Faulet
4c629b75a3 BUG/MAJOR: stick-table: don't process store-response rules for applets
The commit bc7c207f74 ("BUG/MAJOR: stick-tables: do not try to index a
server name for applets") tried to catch applets case when we tried to index
the server name. However, there is still an issue. The applets are
unconditionally casted to servers and this bug exists since a while. it's
just luck if it doesn't crash.

Now, when store rules are processed, we skip the rule if the stream's target
is not a server or, of course, if it is a server but the "non-stick" option
is set. However, we still take care to release the sticky session.

This patch must be backported to all stable versions.

(cherry picked from commit b976640fe1)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 18:28:10 +02:00
William Lallemand
4f60fdd26e DOC: lua: add a note about compression w/ httpclient
Decompression is not supported by the httpclient.

(cherry picked from commit a9256194b8)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 18:25:56 +02:00
William Lallemand
a9ba03ae89 DOC: management: add forgotten "show startup-logs"
The keyword was never documented.

Must be backported in all maintained versions.

(cherry picked from commit f76b3b47ea)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 18:25:19 +02:00
Ilya Shipitsin
910d89986e CI: SSL: temporarily stick to LibreSSL=3.5.3
recently released 3.6.0 introduced some regression which must be
resolved first, let us use 3.5.3 notation instead of "latest"

(cherry picked from commit b65fd66666)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:54:29 +02:00
Ilya Shipitsin
b78d3e8595 CI: SSL: use proper version generating when "latest" semantic is used
both "OPENSSL_VERSION=latest" and "LIBRESSL_VERSION=latest" processing
introduced errors when build-ssl.sh script was invoked. that error
in turn led to skipping custom openssl build and haproxy was linked against
stock openssl, i.e. openssl-1.1.1

(cherry picked from commit 14711bdc9a)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:54:19 +02:00
Christopher Faulet
a7456b2d14 BUG/MINOR: sink: Set default connect/server timeout for implicit ring buffers
Ring buffers may be implicitly created from log declarations when "tcp@",
"tcp6@", "tcp4@" or "uxst@" prefixes are used. These ring buffers rely on
unconfigurable proxies. While connect and server timeouts should be defined for
explicit ring buffers, it is no possible for implicit ones. However, a default
value must be set and TICK_ETERNITY is not an acceptable one.

Thus, now "1s" is set for the connect timeout and "5s" is set for server one.

This patch may be backported as far as 2.4.

(cherry picked from commit d08a25b1f1)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:53:54 +02:00
Christopher Faulet
9c2c1e328f BUG/MINOR: sink: Only use backend capability for the sink proxies
When a ring section is parsed, a proxy is created. For now, it has the
frontend (PR_CAP_FE) and the internal (PR_CAP_INT) capabilities, in addition
to the expected backend capability (PR_CAP_BE).

PR_CAP_INT capability was added to silent warning triggered because of
PR_CAP_FE capability. Indeed, Because the proxy is declared as a frontend,
warnings about missing bind lines and missing client timeout should be
triggered during the configuration parsing. These warnings are inhibited
because PR_CAP_INT capability is set. It is an issue on the 2.4 because
PR_CAP_INT capability does not exist. So warnings are always emitted.

But the true bug is that these proxies should not have PR_CAP_FE and
PR_CAP_INT capabilities. Removing these capabilities is enough to remove any
warnings on the 2.4, with no regression on higher versions. However, it may
be a good idea to eval if a dedicated frontend for sinks should be added or
not. This way, a true frontend would be used to start the sink applets. In
addition, proxies capabilities/modes have to be reviewed to have a less
ambiguous API. For instance a dedicate mode for sinks (PR_MODE_SINK ?) may
be added. Finally, it could be very nice to have all proxies in the same
list, including internal ones.

This patch should fix the issue #1900. It must be backported as far as 2.4.

(cherry picked from commit 11a707ae52)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:53:51 +02:00
Amaury Denoyelle
9c15bd5d37 MINOR: quic: display unknown error sendto counter on stat page
This patch complete the previous incomplete commit. The new counter
sendto_err_unknown is now displayed on stats page/CLI show stats.

This is related to github issue #1903.

This should be backported up to 2.6.

(cherry picked from commit 7941ead3aa)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:52:33 +02:00
Amaury Denoyelle
934659e0ce MINOR: quic: do not crash on unhandled sendto error
Remove ABORT_NOW() statement on unhandled sendto error. Instead use a
dedicated counter sendto_err_unknown to report these cases.

If we detect increment of this counter, strace can be used to detect
errno value :
  $ strace -p $(pidof haproxy) -f -e trace=sendto -Z

This should be backported up to 2.6.

This should help to debug github issue #1903.

(cherry picked from commit 1d9f170edd)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:52:22 +02:00
Christopher Faulet
126f683a2c BUG/MEDIUM: compression: handle rewrite errors when updating response headers
When an HTTP response is compressed by HAProxy, the headers are updated.
However it is possible to encounter a rewrite error because the buffer is
full. In this case, the compression is aborted. Thus, we must be sure to
leave the response in a valid state.

For now, it is an issue because the "Content-Encoding" header is added
before all other headers manipulations. So if the compression is aborted on
error, the "Content-Encoding" header may remain while the payload is not
compressed.

So now, we take care to leave with a valid response on error by reordering
the headers manipulations. It is too painful to really rollback all changes,
especially for an edge case.

This patch should be backported as far as 2.0. Note that on the 2.0, the
legacy HTTP part is also concerned.

(cherry picked from commit 910b7577bc)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:50:59 +02:00
Amaury Denoyelle
9287b4047b BUG/MINOR: mux-quic: complete flow-control for uni streams
Max stream data was not enforced and respect for local/remote uni
streams. Previously, qcs instances incorrectly reused the limit defined
from bidirectional ones.

This is now fixed. Two fields are added in qcc structure connection :
* value for local flow control to enforce on remote uni streams
* value for remote flow control to respect on local uni streams

These two values can be reused to properly initialized msd field of a
qcs instance in qcs_new(). The rest of the code is similar.

This must be backported up to 2.6.

(cherry picked from commit 176174f7e4)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:50:54 +02:00
William Lallemand
784987a344 BUILD: Makefile: add "USE_SHM_OPEN" on the linux-musl target
The startup-logs with the shm works correctly with Alpine and Musl,
enable the feature by default for the linux-musl target.

(cherry picked from commit 83e9bcaa87)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:50:28 +02:00
William Lallemand
730251540b CI: github: dump the backtrace of coredumps in the alpine container
This patch allows to show the backtrace of a coredump produced in the
alpine/musl jobs.

It activates some option required by the containers to allow the
production of coredump, set a shared directory so the kernel could dump
the coredump within the container. Some debug packages were also added.

(cherry picked from commit 6435801d09)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:50:18 +02:00
William Lallemand
3e731f871c REGTESTS: httpclient/lua: test the lua task timeout with the httpclient
Test the httpclient when the lua action timeout. The lua timeout is
reached before the httpclient is ended. This test that the httpclient
are correctly cleaned when destroying the hlua context.

Must be backported as far as 2.5.

(cherry picked from commit 4ed0a3a883)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:50:06 +02:00
William Lallemand
b398f7368b BUG/MEDIUM: httpclient: check if the httpclient was released in the IO handler
Upon a applet_release(), the applet can be scheduled again and a call to
the IO handler is still possible. When the struct httpclient is already
free the IO handler could try to access it.

This patch fixes the issue by setting svcctx to NULL in the
applet_release, and checking its value in the IO handler.

Must be backported as far as 2.5.

(cherry picked from commit a93eac41f0)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:50:03 +02:00
William Lallemand
3fd456abc7 BUG/MEDIUM: httpclient/lua: crash when the lua task timeout before the httpclient
When the lua task finished  before the httpclient that are associated to
it, there is a risk that the httpclient try to task_wakeup() the lua
task which does not exist anymore.

To fix this issue the httpclient used in a lua task are stored in a
list, and the httpclient are destroyed at the end of the lua task.

Must be backported in 2.5 and 2.6.

(cherry picked from commit bb581423b3)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:49:59 +02:00
Christopher Faulet
244e18af77 BUG/MINOR: ring: Properly parse connect timeout
The connect timeout in a ring section was not properly parsed. Thus, it was
never set and the server timeout may be overwritten, depending on the
directives order. The first char of the keyword must be tested, not the
third one.

This patch is related to the issue #1900. But it does not fix the issue. It
must be backported as far as 2.4.

(cherry picked from commit 321d100cc8)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:49:55 +02:00
Christopher Faulet
64153b11e5 BUG/MINOR: log: Preserve message facility when the log target is a ring buffer
When a ring is used as log target, the original facility, if any, must be
preserved. The default facility must only be used if there no facility was
found in the incoming log message.

This patch should fix the issue #1901. It must be backported as far as 2.4.

(cherry picked from commit cc640e851a)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:49:48 +02:00
Amaury Denoyelle
05f9c5c266 MINOR: quic: extend Retry token check function
On Initial packet reception, token is checked for validity through
quic_retry_token_check() function. However, some related parts were left
in the parent function quic_rx_pkt_retrieve_conn(). Move this code
directly into quic_retry_token_check() to facilitate its call in various
context.

The API of quic_retry_token_check() has also been refactored. Instead of
working on a plain char* buffer, it now uses a quic_rx_packet instance.
This helps to reduce the number of parameters.

This change will allow to check Retry token even if data were received
with a FD-owned quic-conn socket. Indeed, in this case,
quic_rx_pkt_retrieve_conn() call will probably be skipped.

This should be backported up to 2.6.

(cherry picked from commit 9e3026c58d)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:49:43 +02:00
Amaury Denoyelle
8bcc6909f4 MINOR: quic: refactor packet drop on reception
Sometimes, a packet is dropped on reception. Several goto statements are
used, mostly to increment a proxy drop counter or drop silently the
packet. However, this labels are interleaved. Re-arrang goto labels to
simplify this process :
* drop label is used to drop a packet with counter incrementation. This
  is the default method.
* drop_silent is the next label which does the same thing but skip the
  counter incrementation. This is useful when we do not need to report
  the packet dropping operation.

This should be backported up to 2.6.

(cherry picked from commit 6e56a9e055)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:49:39 +02:00
Amaury Denoyelle
fa1e984fd0 MINOR: quic: split and rename qc_lstnr_pkt_rcv()
This change is the following of qc_lstnr_pkt_rcv() refactoring. This
function has finally been split into several ones.

The first half is renamed quic_rx_pkt_parse(). This function is
responsible to parse a QUIC packet header and calculate the packet
length.

QUIC connection retrieval has been extracted and is now called directly
by quic_lstnr_dghdlr().

The second half of qc_lstnr_pkt_rcv() is renamed to qc_rx_pkt_handle().
This function is responsible to copy a QUIC packet content to a
quic-conn receive buffer.

A third function named qc_rx_check_closing() is responsible to detect if
the connection is already in closing state. As this requires to drop the
whole datagram, it seems justified to be in a separate function.

This change has no functional impact. It is part of a refactoring series
on qc_lstnr_pkt_rcv(). The objective is to facilitate the integration of
FD-owned quic-conn socket patches.

This should be backported up to 2.6.

(cherry picked from commit 982896961c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:49:36 +02:00
Amaury Denoyelle
f7fd2a5d50 MINOR: quic: extract connection retrieval
Simplify qc_lstnr_pkt_rcv() by extracting code responsible to retrieve
the quic-conn instance. This code is put in a dedicated function named
quic_rx_pkt_retrieve_conn(). This new function could be skipped if a
FD-owned quic-conn socket is used.

The first traces of qc_lstnr_pkt_rcv() have been clean up as qc instance
is always NULL here : thus qc parameter can be removed without any
change.

This change has no functional impact. It is a part of a refactoring
series on qc_lstnr_pkt_rcv(). The objective is facilitate integration of
FD-owned socket patches.

This should be backported up to 2.6.

(cherry picked from commit 449b1a8f55)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:49:32 +02:00
Amaury Denoyelle
2b697ca18f MINOR: quic: define first packet flag
Received packets treatment has some difference regarding if this is the
first one or not of the encapsulating datagram. Previously, this was set
via a function argument. Simplify this by defining a new Rx packet flag
named QUIC_FL_RX_PACKET_DGRAM_FIRST.

This change does not have functional impact. It will simplify API when
qc_lstnr_pkt_rcv() is broken into several functions : their number of
arguments will be reduced thanks to this patch.

This should be backported up to 2.6.

(cherry picked from commit deb7c87f55)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:49:29 +02:00
Amaury Denoyelle
2f0d2c3197 MINOR: quic: extend pn_offset field from quic_rx_packet
pn_offset field was only set if header protection cannot be removed.
Extend the usage of this field : it is now set everytime on packet
parsing in qc_lstnr_pkt_rcv().

This change helps to clean up API of Rx functions by removing
unnecessary variables and function argument.

This change has no functional impact. It is a part of a refactoring
series on qc_lstnr_pkt_rcv(). The objective is facilitate integration of
FD-owned socket patches.

This should be backported up to 2.6.

(cherry picked from commit 845169da58)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:49:25 +02:00
Amaury Denoyelle
60abfa542e MINOR: quic: add version field on quic_rx_packet
Add a new field version on quic_rx_packet structure. This is set on
header parsing in qc_lstnr_pkt_rcv() function.

This change has no functional impact. It is a part of a refactoring
series on qc_lstnr_pkt_rcv(). The objective is facilitate integration of
FD-owned socket patches.

This should be backported up to 2.6.

(cherry picked from commit 0eae57273b)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:49:22 +02:00
Amaury Denoyelle
160edadb83 BUG/MINOR: quic: fix buffer overflow on retry token generation
When generating a Retry token, client CID is used as encryption input.
The client must reuse the same CID when emitting the token in a new
Initial packet.

A memory overflow can occur on quic_generate_retry_token() depending on
the size of client CID. This is because space reserved for <aad> only
accounted for QUIC_HAP_CID_LEN (size of haproxy owned generated CID).
However, the client CID size only depends on client parameter and is
instead limited to QUIC_CID_MAXLEN as specified in RFC9000.

This was reproduced with ngtcp2 and haproxy built with ASAN. Here is the error
log :
  ==14964==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fffee228cee at pc 0x7ffff785f427 bp 0x7fffee2289e0 sp 0x7fffee228188
  WRITE of size 17 at 0x7fffee228cee thread T5
      #0 0x7ffff785f426 in __interceptor_memcpy /usr/src/debug/gcc/libsanitizer/sanitizer_common/sanitizer_common_interceptors.inc:827
      #1 0x555555906ea7 in quic_generate_retry_token_aad src/quic_conn.c:5452
      #2 0x555555907e72 in quic_retry_token_check src/quic_conn.c:5577
      #3 0x55555590d01e in qc_lstnr_pkt_rcv src/quic_conn.c:6103
      #4 0x5555559190fa in quic_lstnr_dghdlr src/quic_conn.c:7179
      #5 0x555555eb0abf in run_tasks_from_lists src/task.c:590
      #6 0x555555eb285f in process_runnable_tasks src/task.c:855
      #7 0x555555d9118f in run_poll_loop src/haproxy.c:2853
      #8 0x555555d91f88 in run_thread_poll_loop src/haproxy.c:3042
      #9 0x7ffff709f8fc  (/usr/lib/libc.so.6+0x868fc)
      #10 0x7ffff7121a5f  (/usr/lib/libc.so.6+0x108a5f)

This must be backported up to 2.6.

(cherry picked from commit 6c940569f6)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:49:18 +02:00
Frédéric Lécaille
c387bf149b BUILD: quic: Fix build for m68k cross-compilation
Fix several warinings as this one:

src/qmux_trace.c:80:45: error: format ‘%lu’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘uint64_t’ {aka ‘const long long unsigned int’} [-Werror=format=]
   80 |    chunk_appendf(&trace_buf, " qcs=%p .id=%lu .st=%s",
      |                                           ~~^
      |                                             |
      |                                             long unsigned int
      |                                           %llu
   81 |                  qcs, qcs->id,
      |                       ~~~~~~~
      |                          |
      |                          uint64_t {aka const long long unsigned int}
compilation terminated due to -Wfatal-errors.

Cast remaining uint64_t variables as ullong with %llu as printf format and size_t
others as ulong with %lu as printf format.

Thank you to Ilya for having reported this issue in GH #1899.

Must be backported to 2.6

(cherry picked from commit ea492e3e47)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:49:14 +02:00
Amaury Denoyelle
8c26e397c2 BUILD: ssl_sock: fix null dereference for QUIC build
A previous commit tries to fix uninitialized GCC warning on ssl code for
QUIC build. See the fix here :
  48e46f98cc
  BUILD: ssl_sock: bind_conf uninitialized in ssl_sock_bind_verifycbk()

However, this is incomplete as it still reports possible NULL
dereference on ctx variable (GCC v12.2.0). Here is the compilation
result :

  src/ssl_sock.c: In function ‘ssl_sock_bind_verifycbk’:
  src/ssl_sock.c:1739:12: error: potential null pointer dereference [-Werror=null-dereference]
   1739 |         ctx->xprt_st |= SSL_SOCK_ST_FL_VERIFY_DONE;
        |

To fix this, remove check on qc which can also never happens and replace
it with a BUG_ON. This seems to satisfy GCC on my machine.

This must be backported up to 2.6.

(cherry picked from commit ba303deadc)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:49:07 +02:00
Thierry Fournier
c4706a113e BUG/MEDIUM: httpclient: segfault when the httpclient parser fails
If the uri is unexpected ("/" in place of "http://xxx/"), some parsing
function fails. The failure is not handled.

This patch handle these errors. Note: the return code is boolean, maybe
we can return more precise error for Lua reporting ?

Must be backported in 2.6.

(cherry picked from commit 74a9eb5216)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:49:03 +02:00
Frédéric Lécaille
c9aeed34a5 BUILD: quic: QUIC mux build fix for 32-bit build
Thank you to Ilya for having reported this issue in GH #1897

Must be backported to 2.6.

(cherry picked from commit 5a5d05c71b)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:47:53 +02:00
Tim Duesterhus
0855bad311 CI: Replace the deprecated ::set-output command by writing to $GITHUB_OUTPUT in workflow definition
See "CI: Replace the deprecated `::set-output` command by writing to
$GITHUB_OUTPUT in matrix.py" for the reasoning behind this commit.

(cherry picked from commit b87ecbb179)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:47:49 +02:00
Tim Duesterhus
7258191d11 CI: Replace the deprecated ::set-output command by writing to $GITHUB_OUTPUT in matrix.py
As announced in

https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/

the `::set-output` command is deprecated, because processes during the workflow
execution might output untrusted information that might include the
`::set-output` command, thus allowing these untrusted information to hijack the
build.

The replacement is writing to the file indicated by the `$GITHUB_OUTPUT`
environment variable.

(cherry picked from commit 8a03bf4052)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:47:43 +02:00
Christopher Faulet
252ac81019 MINOR: httpclient/lua: Don't set req_payload callback if body is empty
The HTTPclient callback req_payload callback is set when a request payload
must be streamed. In the lua, this callback is set when a body is passed as
argument in one of httpclient functions (head/get/post/put/delete). However,
there is no reason to set it if body string is empty.

This patch is related to the issue #1898. It may be backported as far as
2.5.

(cherry picked from commit 380ae9c3ff)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:47:11 +02:00
Christopher Faulet
bb170f4dff BUG/MEDIUM: httpclient: Don't set EOM flag on an empty HTX message
In the HTTP client, when the request body is streamed, at the end of the
payload, we must be sure to not set the EOM flag on an empty message.
Otherwise, because there is no data, the buffer is reset to be released and
the flag is lost. Thus, the HTTP client is never notified of the end of
payload for the request and the applet is blocked. If the HTTP client is
instanciated from a Lua script, it is even worse because we fall into a
wakeup loop between the lua script and the HTTP client applet. At the end,
HAProxy is killed because of the watchdog.

This patch should fix the issue #1898. It must be backported to 2.6.

(cherry picked from commit 48005de17c)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:47:05 +02:00
Frédéric Lécaille
80bb8f8ab5 BUILD: ssl_sock: bind_conf uninitialized in ssl_sock_bind_verifycbk()
Even if this cannot happen, ensure <bind_conf> is initialized in this
function to please some compilers.

Takes the opportunity of this patch to replace an ABORT_NOW() by
a BUG_ON() because if the variable values they test are not initialized,
this is really because there is a bug.

Must be backported to 2.6.

(cherry picked from commit 48e46f98cc)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:46:59 +02:00
Amaury Denoyelle
85c059a307 MINOR: quic: implement datagram cleanup for quic_receiver_buf
Each time data is read on QUIC receiver socket, we try to reuse the
first datagram of the currently used quic_receiver_buf instead of
allocating a new one. This algorithm is suboptimal if there is several
unused datagrams as only the first one is tested and its buffer removed
from quic_receiver_buf.

If QUIC traffic is quite substential, this can lead to an important
number of quic_dgram occurences allocated from pool_head_quic_dgram and
a lack of free space in allocated quic_receiver_buf buffers.

To improve this, each time we want to reuse a datagram, we pop elements
until a non-yet released datagram is found or the list is empty. All
intermediary elements are freed and the last found datagram can be
reused. This operation has been extracted in a dedicated function named
quic_rxbuf_purge_dgrams().

This should improve memory consumption incured by quic_dgram instances under heavy
QUIC traffic. Note that there is still room for improvement as if the
first datagram is still in use, it may block several unused datagram
after him. However this requires to support removal of datagrams out of
order which is currently not possible.

This should be backported up to 2.6.

(cherry picked from commit 91b2305ad7)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:46:15 +02:00
Amaury Denoyelle
6f2acccd90 CLEANUP: quic: improve naming for rxbuf/datagrams handling
QUIC datagrams are read from a random thread. They are then redispatch
to the connection thread according to the first packet DCID. These
operations are implemented through a special buffer designed to avoid
locking.

Refactor this code with the following changes :
* <rxbuf> type is renamed <quic_receiver_buf>. Its list element is also
  renamed to highligh its attach point to a receiver.
* <quic_dgram> and <quic_receiver_buf> definition are moved to
  quic_sock-t.h. This helps to reduce the size of quic_conn-t.h.
* <quic_dgram> list elements are renamed to highlight their attach point
  into a <quic_receiver_buf> and a <quic_dghdlr>.

This should be backported up to 2.6.

(cherry picked from commit 1cba8d60f3)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:46:11 +02:00
Amaury Denoyelle
94cf0e576d CLEANUP: quic: remove unused rxbufs member in receiver
rxbuf is the structure used to store QUIC datagrams and redispatch them
to the connection thread.

Each receiver manages a list of rxbuf. This was stored both as an array
and a mt_list. Currently, only mt_list is needed so removed <rxbufs>
member from receiver structure.

This should be backported up to 2.6.

(cherry picked from commit 8c4d062d25)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:46:08 +02:00
Frédéric Lécaille
118e94d0c0 MINOR: quic: Split the secrets key allocation in two parts
Implement quic_tls_secrets_keys_alloc()/quic_tls_secrets_keys_free() to allocate
the memory for only one direction (RX or TX).
Modify ha_quic_set_encryption_secrets() to call these functions for one of this
direction (or both). So, for now on we can rely on the value of the secret keys
to know if it was derived.
Remove QUIC_FL_TLS_SECRETS_SET flag which is no more useful.
Consequently, the secrets are dumped by the traces only if derived.

Must be backported to 2.6.

(cherry picked from commit e1a49cfd4d)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:46:04 +02:00
Frédéric Lécaille
e0212bcb47 BUG/MINOR: quic: Stalled 0RTT connections with big ClientHello TLS message
This issue was reproduced with -Q picoquic client option to split a big ClientHello
message into two Initial packets and haproxy as server without any knowledged of
any previous ORTT session (restarted after a firt 0RTT session). The ORTT received
packets were removed from their queue when the second Initial packet was parsed,
and the QUIC handshake state never progressed and remained at Initial state.

To avoid such situations, after having treated some Initial packets we always
check if there are ORTT packets to parse and we never remove them from their
queue. This will be done after the hanshake is completed or upon idle timeout
expiration.

Also add more traces to be able to analize the handshake progression.

Tested with ngtcp2 and picoquic

Must be backported to 2.6.

(cherry picked from commit 4aa7d8197a)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:46:01 +02:00
Frédéric Lécaille
c094639d65 MINOR: quic: Use a non-contiguous buffer for RX CRYPTO data
Implement quic_get_ncbuf() to dynamically allocate a new ncbuf to be attached to
any quic_cstream struct which needs such a buffer. Note that there is no quic_cstream
for 0RTT encryption level. quic_free_ncbuf() is added to release the memory
allocated for a non-contiguous buffer.

Modify qc_handle_crypto_frm() to call this function and allocate an ncbuf for
crypto data which are not received in order. The crypto data which are received in
order are not buffered but provide to the TLS stack (calling qc_provide_cdata()).

Modify qc_treat_rx_crypto_frms() which is called after having provided the
in order received crypto data to the TLS stack to provide again the remaining
crypto data which has been buffered, if possible (if they are in order). Each time
buffered CRYPTO data were consumed, we try to release the memory allocated for
the non-contiguous buffer (ncbuf).
Also move rx.crypto.offset quic_enc_level struct member to rx.offset quic_cstream
struct member.

Must be backported to 2.6.

(cherry picked from commit 9f9263ed13)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:45:58 +02:00
Frédéric Lécaille
f05bd4e5bd MINOR: quic: Extract CRYPTO frame parsing from qc_parse_pkt_frms()
Implement qc_handle_crypto_frm() to parse a CRYPTO frame.

Must be backported to 2.6.

(cherry picked from commit a20c93e6e2)
Signed-off-by: Christopher Faulet <cfaulet@haproxy.com>
2022-10-25 11:45:54 +02:00