Originally with "option accept-invalid-http-request", we couldn't really
edit the request on the fly to remove offending headers. But since we
have HTX and the headers are indexed one at a time, it has become
trivial. A non-negligible number of violations are conditioned by the
now renamed "option accept-unsafe-violations-in-http-request", and a
controversial one could definitely be reporting and passing invalid
header names containing control chars or spaces. The option was placed
so as not to block requests/responses containing them, but there's no
point in passing them to the other side. Most of the time it will be
totally harmless since the other side will reject them. But in case
haproxy is placed in front of a non-compliant server, it would fail
to protect it.
This patch implements a name check for all headers when a parsing
error was detected. It's cheap enough (especially since only done
after an error), and will skip the header if its name is invalid.
This may also remove some possibilities of confusion in logs, or
when encoding headers names for example.
This should be backported at least till the last LTS.
In 3.3 with commit fda6dc959 ("MINOR: regex: use a thread-local match
pointer for pcre2") we got a thread-local match that saves us from having
to allocate a match array with each match. However something was clearly
overlooked or misunderstood in the pcre2 API because the local match
array was initialized via pcre2_match_data_create() for MAX_MATCH-1
entries instead of MAX_MATCH, despite the commit message mentioning
MAX_MATCH entries. It was possibly confused with an index. Due to this
there is a risk of crash when matching more than 9 groups in a regex.
This fix must be backported to 3.3.
When not set, the cluster secret is randomly generated by two
consecutive calls to ha_random64(). However, the random64 PRNG may be
partially observed on a fully idle machine (QUIC retry tokens, UUID,
WS key), and it could be rolled back to the initial call that produced
the secret. This is purely theoretical as a normally loaded system
wouldn't reveal meaningful sequences, but better address this while
it's still easy.
The first here consists in isolating the cluster_secret from the PRNG
sequence. When RAND_bytes() is available and works, it's used. Otherwise
ha_random64() is mixed with uncorrelated bits from random().
This could be backported to stable releases.
When a primary cache hit has a Vary secondary_key_signature, the code calls
retain_entry() and shctx_row_detach() before performing the secondary lookup.
If get_secondary_entry() returns NULL (no stored variant matches), res is set
to NULL and the function falls through to return ACT_RET_CONT without calling
release_entry() or shctx_row_reattach(). Each such request leaks one refcount
and pins one shctx row permanently, eventually exhausting the cache.
In order to fix this we must do like when no secondary key could be built
and release everything. We only reattach to the row if we previously
detached.
The issue was introduced in 2.4 with commit 1785f3dd9 ("MEDIUM: cache: Add
the Vary header support"). The code changed a bit in 2.9 with commit
48f81ec09 ("MAJOR: cache: Delay cache entry delete in reserve_hot function"),
so in order to backport to 2.8 and older, the patch will have to be manually
applied (no test on detached).
In dict_insert(), when ebis_insert() returns an existing node n indicating
that another thread inserted the same key concurrently, the code freed its
own newly-allocated entry and returned the winner without bumping its
refcount. Both callers then held a reference with refcount=1 instead of 2,
so when one expires the other becomes a use-after-free or double-free.
The bug likely comes from the fact that new_dict_entry() creates an entry
with a refcount preset to 1 (saves an atomic op) and that because of this
there is no refcount increment upon a successful insertion in the tree,
resulting in requiring different code paths for collision and normal
insertion.
A simple fix consists in bumping the refcount under the lock and unlocking
only at the end, but this would mean performing two free() calls under a
lock, which we always try to avoid. The code was slightly rearranged so
that we can now bump the existing entry's refcount under the lock in case
of duplicate, or unlock immediately in the common case, so that the free()
call is done out of the lock.
The probably of the race is very low (at peers connection setup only),
reason why it's marked low. This should be backported to all versions.
In parse_log_message(), the first loop looks for '>' that finishes the
priority field, and unfortunately it stops once it has checked the first
byte after the end of the buffer. This means that a priority made only
of digits for the whole buffer would read one extra byte. In practice
since pools have a tag at the end this is only detectable when using ASAN,
but this should be fixed nevertheless.
This can be backported to all versions.
It's worth noting that RFC5424 now says that the PRI field is 1..3
digits only, so maybe at some point we could seriously limit the
length as well.
In 2.3, in preparation for log forwarding, commit 546488559 ("MEDIUM:
log/sink: re-work and merge of build message API.") extended the log
send API to be able to use metadata from an existing header. However
the month number is parsed from the passed meta-data and compared
against 11 but there's no check for negative values which could in
theory cause a negative monthname[] index.
It can be a problem when the date is received as RFC5424 and forced
to RFC3164 because certain characters in the month field could result
in a negative month value. Let's fix it by turning the month to unsigned
to make sure we only accept months 0..11.
This should be backported to all branches.
In 2.5, commit da0264a96 ("MINOR: sample: Add be2hex converter")
introduced the be2hex() converter, which reads input data of a given
chunk size, processes it as a big endian block and turns it to hex
output.
There's an issue if the configured chunk_size (2nd argument) is larger
than tune.bufsize/2, because the max_size calculation will underflow,
and the later loop will always match since it compares a size_t to an
int (BTW, compilers love to annoy us with useless warnings but I never
found how to see some for these ones). This can result in overflowing
the output trash if the input sample is at least as large as half a
buffer.
Let's add an explicit check for this, and change the max_size type to
size_t so that the comparison is always right. While we're at it, let's
ask the trash buffer to be twice as large, just like bin2hex() does, as
it may result in offering a larger buffer in 3.4. thanks to the large
buffers support.
Despite the risk, this is marked as minor because a config with that
large an argument in the converter makes absolutely no sense.
This should be backported to 2.6. The *2 for the trash allocation will
conflict and have to be dropped in stable versions, which is safe.
When the PRIORITY flag is present on a HEADERS frame, the frame must
contain a stream dependency and a weight, for a total of 5 bytes. The
length is checked after reading the stream dep field so theoretically
such a frame could cause up to 4-byte OOB read at the end of the buffer,
though in practice buffers allocated from pools never end on a page
boundary (one extra word at the end) and the anomaly is still detected
after reading the stream ID and the connection aborted with the glitch
count incremented. Thus while not technically correct, practically
speaking it's harmless.
This should be backported to all stable releases.
The previous fix 75f72c2eb ("BUG/MEDIUM: resolvers: Fix test on dn label
size in resolv_dn_label_to_str()") may still leave garbage from the input
buffer into the response: if a component length is passed as zero, it
should mark the end, but instead a dot will be emitted, and whatever
follows it in the input buffer would continue to be appended as extra
components. While having no direct consequences beyond the domain not
being properly decoded, it could at least complicate troubleshooting.
This should be backported where the fix above is backported.
The previous fix 75f72c2eb ("BUG/MEDIUM: resolvers: Fix test on dn label
size in resolv_dn_label_to_str()") can still be fooled by an input exactly
the size of str_len, in which case the trailing zero appended at the end
was not being accounted for. Let's add 1 to the condition to prepare for
it.
This needs to be backported wherever the fix above is backported.
When lf_expr_compile() fails in cfg_parse_log_profile, the code leaves
without freeing the previously strdup()'d strings in target_lf->str and
target_lf->conf.file. Let's add a call to lf_expr_deinit() there to
release it.
It was harmless anyway since the startup will abort when this happens,
but better clean it because with increasingly dynamic setups, one day
it could become a runtime leak.
No backport is needed.
In resolv_validate_dns_response(), when matching an additional A/AAAA
record to an SRV record, the code checked tmp_record->ar_item == NULL
then called pool_free(resolv_answer_item_pool, tmp_record->ar_item).
This is a copy-paste mistake from similar patterns elsewhere since
the pointer is confirmed to be NULL a few lines above, so let's just
drop the confusing pool_free.
Both boundary checks in the authority record parsing loop of
resolv_validate_dns_response() use >= bufend where they should use
> bufend, causing valid DNS responses with exactly enough bytes to be
rejected as invalid.
The first one, "reader + offset + 10 >= bufend" is too strict since it
prevents 10-byte responses from being accepted as valid while they
are. The second one, "reader + len >= bufend" has the same issue, when
exactly len bytes remain, the check rejects it even though dns_max_name()
already validated it. It may be backported though it is unlikely to ever
be noticed.
In resolv_validate_dns_response(), the second DNS record parsing path
manually constructs a 32-bit big-endian TTL value from four individual
bytes using the expression:
reader[0] * 16777216 + reader[1] * 65536 + reader[2] * 256 + reader[3]
We have read_n32() to do this, and it's more robust against unexpected
signedness surprises (which should not happen right here since reader is
unsigned char and we use -fwrapv so the result is defined). Also, let's
make the ttl an uint instead of an int. The TTL is only retrieved and not
used for now, so better clean it now.
acme_res_certificate() passes the httpclient response buffer to
ssl_sock_load_pem_into_ckch(), which will then call BIO_new_mem_buf(buf, -1).
The "-1" flag will make the OpenSSL PEM parser determine the length by
using strlen(). However, the httpclient populates the response buffer with
__b_putblk() without writing a trailing NUL to it. The byte at area[data]
is whatever data previously resided there in the memory pool.
Thus, a malicious or compromised ACME CA can perform an arbitrary-length
out-of-bounds read until hitting the first NULL byte past the response
body. The OpenSSL PEM loader will try to iterate to load the chain
certificates, thus the PEM-looking garbage found in freed memory chunks
can be erroneously loaded as additional intermediate certificates. The
presence of a single NUL inside the valid response body will result in
silent truncation of the certificate.
Make sure that the area[data] contains a terminating NULL before passing
the buffer to the parser. Fail on insufficient room for the NUL terminator.
No backport required: The ACME client has been added in 3.x and this
code path didn't exist in 2.x.
When the dedidacted buffer to store the command payload was added (c5ae0da62
"MEDIUM: cli: Make a buffer for the command payload"), an bug was
introduced. When the pattern finishing the command payload is found, it is
removed from the buffer. A NULL-bytes is added before it, skipping the
previous newline character.
It worked well in all cases before the commit above, because the commandline
was already parsed and was placed at the beginning of the cmdline buffer.
So, there is always a line before the payload.
Now, the payload is stored in a dedicated buffer. So there is nothing
preceeding it in a buffer. If the payload is empty, we cannot rewind to the
previous line to set the NULL-byte character. We must handle this case to
avoid integer underflow on the payload buffer length.
It is a 3.4-specific bug. No backport needed.
In hlua_socket_receive_yield(), when we try to get a line, the trailing CRLF is
stripped by decrementing the block length. The '\n' is first skipped, then,
possible a preceeding '\r'. But the block lenght is never checked. If an empty
line is returned, this leads to an integer underflow and most probably to a
crash because this length is used to copy data into a LUA string.
To fix the issue, the block length is now properly tested against 0 before
decrementing it.
This patch must be backported to all stable versions.
When parsing the agent-check reply, we first loop on the response to find
the newline character, to add a NULL-byte at the end of the line. However,
this loop is not bounded to the data available in the buffer. So it is
possible to read bytes outside the buffer and eventually write a NULL-byte
ouside the buffer.
So let's check for the end of the buffer when looping on the agent-check
reply.
This patch must be backported to all stable versions.
In dict_entry_unref(), the write lock on d->rwlock was only acquired after
decrementing the refcount. However, between the decrement and the lock,
another thread could increment it by calling dict_insert(). That could lead
to a UAF.
To fix the issue, the call to HA_ATOMIC_SUB_FETCH is moved inside the write
lock.
This patch must be backported to all stable versions.
In haproxy, when an Initial packet is received, a new connection may be
created and a DCID must be attributed. This CID is derived from the
original DCID used by the client in its first packet. This is an
optimization to avoid storing two CIDs values in the CID tree.
On CID lookup, if the DCID used is not found, derivation is performed
again. This should permit to retrieve the DCID node. However, this
operation is not performed as expected in quic_get_cid_tid(), as the
wrong value is used on the second lookup. Fix this function by using
derive CID for it. Note that retrieve_qc_conn_from_cid() performs the
same lookup but the bug was not present there.
The impact of this bug is relatively low as most clients send a single
Initial packet. Even in case of multiple packets in a single datagram,
this does not cause any issue as the current thread is assigned as
default.
This should be backported up to 2.8.
In ssl_sock_generate_certificate(), if the LRU cache for generated
certificates is used, the LRU tree is not unlocked on cache miss if the
certificate generation failed. So let's unlock it on error path.
The bug was introduced by the commit fbc98ebcd ("BUG/MEDIUM: ssl: fix error
path on generate-certificates"). So this patch must be backported with the
commit above, so to all stable versions.
In resolv_dn_label_to_str(), size for a dn label was stored into an integer
from a signed char without a cast to unsigned. So dn label with a size of
128 bytes or more become negative, skipping this way the copy loop and
desynchronizing input vs output.
In addition, the size of the destination string was only checked at the
begining, against the dn string length. But it must also be checked for
every dn label, to be sure. The dn string can be forged to copied more bytes
than expected.
This patch must be backported to all stable versions.
when appctx_rcv_buf() function was called to get data from the applet, but
to get zero bytes, nothing was performed and the function early
returned. However, we must at least take care to set SE_FL_WANT_ROOM if
necessary. Otherwise, if data are still blocked in the applet's output
buffer while the EOI/EOS are pending, the information can be reported to the
upper layer and remaining data can be lost.
Indeed, in such case, SE_FL_WANT_ROOM flag is here to specify the applet has
more data to deliver. Thanks to this flag, the stream will wait before
closing. But when appctx_rcv_buf() function is called, this flag is removed by
the stconn. It is the function responsibility to set it again when necessary.
This patch should fix second part of the issue #3366. It must be backported
to 3.0.
Completes qmux_parse_frm() to ensure every frames allowed by QMux
protocol are listed. For now, nothing is implemented except a CHECK_IF()
to report such events.
This is necessary to prevent a crash on abort. Frames not supported by
QMux should already have been rejected prior via qmux_is_frm_valid().
Handle reception of a MAX_STREAMS frame for unidirectional stream usage
when using QMux. This simply consists in using qcc_recv_max_streams() as
with QUIC protocol.
Ensure reception of STOP_SENDING via QMux protocol is properly handled.
This simply consists in using qcc_recv_stop_sending() which will update
the associated QCS if found.
The check on the OCSP response expire time is based on the "Next Update"
field of the response, converted by my_timegm function that returns a
time_t (signed long). It is then stored in the 'expire' field of the
certificate_ocsp structure which is typed as a signed long.
When loading an OCSP response, if the "Next Update" time is too far in
the future and we are running on a 32 bits machine, we might end up with
negative times ireturned by my_timegm, which make the comparison with
the current date fail and raises the "OCSP single response: no longer
valid." error message.
This problem typically happens in the ocsp_auto_update.vtc regtest since
the loaded OCSP response have a "Next Update" field in 2050.
This patch simply changes the type of the expire field to an unsigned
long since the 'my_timegm' function does not return '-1' in case of
error, contrary to the standard 'timegm' one.
Ths patch can be backported to all stable branches.
Use xprt_add_l6hs() at the end of connect_server() if selected MUX layer
relies on a temporary handshake prior to its initialization. This
functions is noop is SSL layer is active.
This change is necessary to support clear QMux on the backend side.
Recently defined <init_xprt> from mux_proto_list is used to render the
code as generic as possible.
Activates xprt_qmux layer if necessary via session_accept_fd(). This is
necessary to be able to support QMux in clear. This operation is noop if
SSL is active, as in this case xprt_qmux will be activated after the SSL
handshake completion.
To ensure MUX init is delayed when running with clear QMux, mask
CO_FL_WAIT_XPRT_L6 is added to test if the embryonic task must be
started instead.
Define a new connection flag mask CO_FL_WAIT_XPRT_L6. This will be used
to indicate that a XPRT layer is running on top of layer 6. For now,
only xprt_qmux implements this method of operation.
Extend get_alpn() for xprt_qmux layer. If lower layer does not implement
ALPN negotiation, return a statically default protocol value. Currently
this is set to "h3".
This change is required to support QMux in clear without SSL. In the
future, it could be useful to configure the default protocol, for
example by extending the syntax for the "proto" keyword.
When QMux protocol is used, xprt_qmux layer is setup after SSL handshake
completion but prior to the MUX initialization. Once transport
parameters exchange is successful, the layer is removed and the MUX is
started.
The layer setup operation was performed directly on ssl_sock_io_cb().
Simplify the code by extracting it in a dedicated function
xprt_add_l6hs(). The function is generic so the requested XPRT layer
must be passed as argument.
The code is mostly identical. One difference is that a check is
performed to ensure no SSL handshake is pending. If this is the case,
the function is a noop. This will become useful to support QMux
transparently both in clear or on top of SSL.
Another minor addition is that CO_FL_XPRT_READY flag is automatically
resetted by xprt_add_l6hs(). This allows the code to use
conn_xprt_start() standard function after XPRT init.
A recent patch has introduced <init_xprt> mux_proto_list member. This
allows to activate QMux on SSL handshake completion without explicit
"proto qmux" setting.
Thanks to this change, on SSL handshake completion it is not necessary
anymore to check for CO_FL_QMUX_* flags.
The various tcp_option_* converters rely on tcp_fullhdr_find_opt() to
find the option. However, the same bug as fixed in commit dbf471f99a
("BUG/MAJOR: net_helper: ip.fp infinite loop on malformed tcp options")
was also present there, by which an option of length 0 could be looped
over indefinitely. In practice this does not happen since such options
are not valid, but if passed encoded in an HTTP header for example, it
could possibly be passed.
While fixing it, let's check for length >1 in all 3 locations insteead
of only non-zero, since there's no point processing a malformed option
that wouldn't even be properly skipped.
This fix doesn't need to be backported, unless the ip.fp series is.
Thanks to @Vincent55 for reporting this issue.
When threads are disabled, "static __decl_spinlock(foo);" ends up as
"static;", causing a build warning when threads are disabled. We don't
need it to be static so let's drop "static" here. No backport is needed,
this is 3.4-only.
Released version 3.4-dev13 with the following main changes :
- BUG/MINOR: backend: correct parameter value validation in get_server_ph_post()
- BUG/MINOR: config/dns: properly fail on duplicate nameserver name detection
- BUG/MEDIUM: dns: fix long loops in additional records parse on name failure
- BUG/MEDIUM: resolvers: fix name compression pointer validation in resolv_read_name()
- BUG/MEDIUM: dns: fix memory leak of sockaddr in dns_session_init() error path
- CLEANUP: proxy: fix tiny mistakes in parse error messages
- CLEANUP: dns: fix misleading error messages in dns_stream_init()
- BUG/MINOR: server: better handling of OOM in srv_set_fqdn()
- BUG/MINOR: servers: use proper source of pool_conn_name in srv_settings_cpy()
- BUG/MEDIUM: server/cli: unlock server lock on failure in cli_parse_set_server
- BUG/MINOR: resolvers: fix dangling list pointer in resolvers_new() error paths
- BUG/MINOR: dns: fix dangling dgram pointer on dns_dgram_init() failure path
- BUG/MINOR: proxy: use proxy_drop() in parse_new_proxy() error path
- CLEANUP: resolvers: properly initialize the sample in resolv_action_do_resolve()
- BUG/MINOR: resolvers: report the expression error in the do-resolve() action parser
- BUG/MINOR: resolvers: fix leaked dgram and dns_ring struct in parse_resolve_conf()
- BUG/MINOR: resolvers: fix leaked fields on cfg_parse_resolvers() error paths
- BUG/MINOR: resolvers: fix missing task_idle destruction in resolvers_destroy()
- CLEANUP: proxy: fix duplicate declaration of cli_find_frontend in proxy.h
- CLEANUP: address a few typos and copy-paste errors in httpclient and dns
- DOC: internal: add a few rules about internal core principles
- BUG/MINOR: session/trace: use distinct flags for SESS_EV_END and _ERR
- CLEANUP: stick-table: uniformize the different action_inc_gpc*()
- REGTESTS: do not run quic/tls13_ssl_crt-list_filters in quic openssl compat mode
- REGTESTS: quic/issuers_chain_path: do not forget to enable QUIC compat mode
- BUG/MINOR: sock: store the connection error status
- BUG/MINOR: check: properly report errno in chk_report_conn_err()
- CLEANUP: tcpcheck: mention that we're a bit far for a sync errno
- BUG/MINOR: jwt: fix possible memory leak in convert_ecdsa_sig() error path
- CLEANUP: jwe: fix theoretical overflow in AAD length calculation
- DOC: config: further clarify that resolvers "default" exists
- MINOR: proxy: remove the experimental status on dynamic backends
- BUG/MEDIUM: limits: properly account for global.maxpipes in compute_ideal_maxconn()
- BUG/MINOR: jws: fix OpenSSL 3.0 version check from > to >=
- BUG/MINOR: jws: Add missing return value check (EVP_PKEY_get_bn_param)
- BUG/MINOR: server: Properly handle init-state value during haproxy startup
- BUG/MINOR: httpclient-cli: Destroy http-client context if failing to start it
- BUG/MEDIUM: h1: Skip all h2c values from Upgrade headers during parsing
- BUG/MINOR: h1: Don't mask websocket protocol if multiple protocols used
- MINOR: haterm: Don't init haterm master pipe if not used
- CLEANUP: haterm: Remove "(too old kernel)" from warning message during init
- BUG/MINOR: httpclient-cli: fix uninit variable in error label
- MINOR: mux: Rename the "token" from mux_proto_list to mux_proto
- MEDIUM: connections: Use both mux_proto and alpn to pick a mux
- MINOR: connection: define conn_select_mux_fe()
- MINOR: connection: define conn_select_mux_be()
- MINOR: connection/mux_quic: add MUX <init_xprt> field for QMux handshake
- MINOR: proxy/server: reject TCP ALPN h3 without experimental
- MEDIUM: ssl: allow h3/QMux negotiation without explicit proto
- BUG/MINOR: server: accept server IDs above 2^31 and clarify error message
- BUG/MINOR: backend: fix balance hash calculation when using hash-type none
- MINOR: server: support hash-key id32 for a cleaner distribution
- MINOR: backend: support hash-key guid for a stabler distribution
- MINOR: startup: support unprivileged chroot if possible
- MEDIUM: startup: add automatic chroot feature
- MINOR: h2: explain committed_extra_streams dec on h2_init() error
- OPTIM: h2: do not update committed streams if elasticity disabled
- MINOR: mux_quic: implement basic committed_extra_streams accounting
- MINOR: quic: use stream elasticity value for initial advertisement
- MINOR: mux_quic: define ms_bidi_rel QCC member
- MAJOR: mux_quic: support stream elasticity during connection lifetime
- BUG/MEDIUM: servers: Store the connection hash with the parameter cache
- BUG/MINOR: prevent conn leak in case of xprt_qmux init failure
- BUILD: traces: set a few __maybe_unused on vars used only for traces
- BUILD: traces: add USE_TRACE allowing to disable traces
- MINOR: startup: do not execute chroot() when "/"
- MEDIUM: startup: warn when chroot is not set for root
- BUG/MEDIUM: servers: Don't forget to set srv_hash when needed
- DOC: fix typo on QUIC stream.max-concurrent reference
- BUG/MINOR: mux_quic: do not exceed stream.max-concurrent on backend side
- BUG/MINOR: htx: Fix value of HTX_XFER_HDRS_ONLY flag
- MEDIUM: htx: Improve htx_xfer API to not count HTX meta-data
- BUG/MEDIUM: applet: Fix transfer of HTX data to the applet
- BUG/MEDIUM: htx: Alloc a chunk of right size in htx_replace_blk_value()
- MEDIUM: stick-tables: Avoid freeing elements while holding a lock
- MINOR: intops: add a multiply overflow detection for ulong and size_t
- CLEANUP: tree-wide: use array_size_or_fail() in array size for allocations
- DOC: update supported gcc and openssl versions in INSTALL
Instead of relying on malloc(n*size), we now pass array_size_or_fail(n,m)
so that it becomes possible to detect overflow. This is particularly
interesting for global settings that might be set large enough to cause
overflows on 32-bit systems for example, resulting in small values that
then cause trouble. Now the overflow will be detected at allocation time.
Around 25 locations were updated.
Sometimes we'd like to know if some products overflow, so let's add a
pair of functions for this, for ulong and for size_t. For recent enough
compilers (gcc >= 5, clang >= 3.4) we just use __builtin_mul_overflow()
otherwise we rely on a division and a comparison before performing the
operation.
A third function, array_size_or_fail() computes the size of an array
of m elements of n bytes each, and returns the total size if it fits
in a size_t, otherwise ~0 if it does not so that passing this to
malloc() or any other variant would fail by trying to exhaust the
entire memory space.
In stksess_trash_oldest(), and process_tables_expire(), avoid freeing
elements while holding two locks, as it could be very costly.
Instead, build a linked list of elements to be free'd, and do so once we
no longer hold any lock.
This may help with github issue #3380, and may be backported to 3.3.
Since support for large buffers was added, we must be careful when chunks
are allocated. Indeed, depending on the context a large chunks may be
required if data are copied from a large buffer.
In htx_replace_blk_value() function, when a defragmentation is necessary,
the data to be replaced are copied to a chunk before the
defragmentation. However, I forgot to get large chunk when necessary by
calling alloc_trash_chunk_sz() instead of alloc_trash_chunk(). Because of
this issue, it is possible to copy data to a too small chunk, leading to a
crash.
So let's fix the issue.
Thanks to Vincent55 for finding and reporting this.
No backport needed.
appctx_htx_snd_buf() function is relying on htx_xfer() function to transfer
HTX blocks when a swap of buffers is not possible. However, it was not
properly using this function.
Indeed, originally htx_xfer() was designed to transfer blocks with a limit,
the <count> parameter, which included the blocks payload and the
meta-data. It was aligned with all calls, except for the transfer of HTX
data to the applet, in appctx_htx_snd_buf() function. In that case, the
<count> parameter is the amount of data forwarded by the stream to the
applet. So meta-data are not included.
Thanks to the previous commit ("MEDIUM: htx: Improve htx_xfer API to not count
HTX meta-data"), it is now possible to instruct htx_xfer() function that
<count> parameter does not include the meta-data.
Because of this bug, crashes can be experienced when transferring HTX data
to an applet. At first glance, lua HTTP applets and the http client are
concerned.
Stable versions from 3.3 to 3.0 are also affected. But this patch cannot be
backported as is because htx_xfer() function does not exist on these
versions.
Thaks to Yon Harlicaj for finding and reporting this.
(https://x.com/nvmb3r - https://www.linkedin.com/in/eljon-harlicaj/)
This patch add the ability to the htx_xfer() function to transfer data
without acounting the meta-data. By default, the <count> variable includes
the meta-data. But by setting the flag HTX_XFER_NO_METADATA, It is possible
to transfer HTX blocks without count meta-data. In that case, <count> will
not contain the blocks meta-data and the return value will not include them.
HTX_XFER_* flags must be declared as a bitfield. However, value of
HTX_XFER_HDRS_ONLY was set of 0x03 while it should be 0x04. So let's fix it.
This patch must be backported where the htx_xfer() function was backported
(5ead611cc "MEDIUM: htx: Add htx_xfer function to replace htx_xfer_blks").
Fix usage of stream.max-concurrent QUIC setting on the backend side.
Contrary to frontend connections, this limit must be enforced by QUIC
MUX directly. This is necessary as the peer may allow a larger number of
concurrent streams via its flow control.
First, QUIC TP initial max bidi streams value is now set to 0. This is
fine as only the HTTP/3 client is expected to open bidirectional
streams.
The most important changes is performed in qcm_avail_streams(). The
value first depends on the peer flow control. Now, it is further reduced
if necessary to not exceed the configured BE stream.max-concurrent.
Note that this new behavior may further increases current limitation on
QUIC BE reuse when a QCS instance is kept while its upper stream layer
is detached. In this case there is a risk that the connection is not
reinserted in the correct server pool, as an idle or avail one.
This is a breaking change as BE stream.max-concurrent keyword setting
meaning is changed in effect. However, this does not necessitate extra
warnings as the previous usage was in effect useless. Furthermore, QUIC
on the backend side is still considered as experimental.
This can be backported up to 3.3.
Commit 8aa854ab26a7daa613a17548f1fe1d0adb8cf61b made it so we'd store
the hash corresponding to the server parameters, so that we could detect
if we're still talking to the same server, and not use those parameters
if not.
However, when updating those parameters, we forgot to store the new
hash, which would result in the new parameters never be used, and
breakling 0RTT.
Fix that by properly update the hash when needed.
This should be backported when 8aa854ab26a7daa613a17548f1fe1d0adb8cf61b
is backported.
