Some variables were only checked via BUG_ON macro. If compiling without
DEBUG_STRICT, this instruction is a noop. Fix this by using an explicit
condition + ABORT_NOW.
This should fix the github issue #1549.
qc_rx_strm_frm_cpy is unsafe because it updates the offset field of the
frame. This is not safe as the frame is inserted in the tree when
calling this function and offset serves as the key node.
To fix this, the API is modified so that qc_rx_strm_frm_cpy does not
update the frame parameter. The caller is responsible to update
offset/length in case of a partial copy.
The impact of this bug is not known. It can only happened with received
STREAM frames out-of-order. This might be triggered with large h3 POST
requests.
In si_cs_recv(), the mux must never set CS_FL_WANT_ROOM flag on the
conn-stream if the input buffer is empty and nothing was copied. It is
important because, there is nothing the app layer can do in this case to
make some room. If this happens, this will most probably lead to a ping-pong
loop between the mux and the stream.
With this BUG_ON(), it will be easier to spot such bugs.
If a parsing error is detected and the corresponding HTX flag is set
(HTX_FL_PARSING_ERROR), we must be sure to always report it to the app
layer. It is especially important when the error occurs during the response
parsing, on the server side. In this case, the RX buffer contains an empty
HTX message to carry the flag. And it remains in this state till the info is
reported to the app layer. This must be done otherwise, on the conn-stream,
the CS_FL_ERR_PENDING flag cannot be switched to CS_FL_ERROR and the
CS_FL_WANT_ROOM flag is always set when h2_rcv_buf() is called. The result
is a ping-pong loop between the mux and the stream.
Note that this patch fixes a bug. But it also reveals a design issue. The
error must not be reported at the HTX level. The error is already carried by
the conn-stream. There is no reason to duplicate it. In addition, it is
errorprone to have an empty HTX message only to report the error to the app
layer.
This patch should fix the issue #1561. It must be backported as far as 2.0
but the bug only affects HAProxy >= 2.4.
After sending some data, we try to wake the H1 stream to resume data
processing at the stream level, except if the output buffer is still
full. However we must also be sure the mux is not blocked because of an
allocation failure on this buffer. Otherwise, it may lead to a ping-pong
loop between the stream and the mux to send more data with an unallocated
output buffer.
Note there is a mechanism to queue buffers allocations when a failure
happens. However this mechanism is totally broken since the filters were
introducted in HAProxy 1.7. And it is worse now with the multiplexers. So
this patch fixes a possible loop needlessly consuming all the CPU. But
buffer allocation failures must remain pretty rare.
This patch must be backported as far as 2.0.
In htx_copy_msg(), if the destination buffer is empty, we perform a raw copy
of the message instead of a copy block per block. But we must be sure the
destianation buffer was really allocated. In other word, to perform a raw
copy, the HTX message must be empty _AND_ it must have some free space
available.
This function is only used to copy an HTTP reply (for instance, an error or
a redirect) in the buffer of the response channel. For now, we are sure the
buffer was allocated because it is a pre-requisite to call stream
analyzers. However, it may be a source of bug in future.
This patch may be backported as far as 2.3.
The qcc instance should be tested as it is implied by a previous test
that it may be NULL. In this case, qc_timeout_task can be stopped.
This should fix github issue #1559.
A bug was uncovered by commit fc5912914 ("MINOR: httpclient: Don't limit
data transfer to 1024 bytes"), it happens that callers of b_xfer() and
b_force_xfer() are expected to check for available room in the target
buffer. Previously it was unlikely to be full but now with full buffer-
sized transfers, it happens more often and in practice it is possible
to crash the process with the debug command "httpclient" on the CLI by
going beyond a the max buffer size. Other call places ought to be
rechecked by now and it might be time to rethink this API if it tends
to generalize.
This must be backported to 2.5.
The url2sa implementation is inconsitent when parsing an IPv4, indeed
url2sa() takes a <ulen> as a parameter where the call to url2ipv4() takes
a null terminated string. Which means url2ipv4 could try to read more
that it is supposed to.
This function is only used from a buffer so it never reach a unallocated
space. It can only cause an issue when used from the httpclient which
uses it with an ist.
This patch fixes the issue by copying everything in the trash and
null-terminated it.
Must be backported in all supported version.
When calling ssl_ocsp_response_print which is used to display an OCSP
response's details when calling the "show ssl ocsp-response" on the CLI,
we use the BIO_read function that copies an OpenSSL BIO into a trash.
The return value was not checked though, which could lead to some
crashes since BIO_read can return a negative value in case of error.
This patch should be backported to 2.5.
When calling the "show ssl ocsp-response" CLI command some OpenSSL
objects need to be created in order to get some information related to
the OCSP response and some of them were not freed.
It should be backported to 2.5.
The b_istput function called to append the last data block to the end of
an OCSP response's detailed output was not checked in
ssl_ocsp_response_print. The ssl_ocsp_response_print return value checks
were added as well since some of them were missing.
This error was raised by Coverity (CID 1469513).
This patch fixes GitHub issue #1541.
It can be backported to 2.5.
The 'dst' optionnal field on a httpclient request can be used to set an
alternative server address in the haproxy address format. Which means it
could be use with unix@, ipv6@ etc.
Should fix issue #1471.
When starting for the 2nd time a request from the same httpclient *hc
context, the flags are not reinitialized and the httpclient will stop
after the first call to the IO handler, because the END flag is always
present.
This patch also add a test before httpclient_start() to ensure we don't
start a client already started.
Must be backported in 2.5.
The idle connection delay calculation before a request is a bit tricky,
especially for multiplexed protocols. It changed between 2.3 and 2.4 by
the integration of the idle delay inside the session itself with these
commits:
dd78921c6 ("MINOR: logs: Use session idle duration when no stream is provided")
7a6c51324 ("MINOR: stream: Always get idle duration from the session")
and by then it was only set by the H1 mux. But over multiple changes, what
used to be a zero idle delay + a request delay for H2 became a bit odd, with
the idle time slipping into the request time measurement. The effect is that,
as reported in GH issue #1395, some H2 request times look huge.
This patch introduces the calculation of the session's idle time on the
H2 mux before creating the stream. This is made possible because the
stream_new() code immediately copies this value into the stream for use
at log time. Thus we don't care about changing something that will be
touched by every single request. The idle time is calculated as documented,
i.e. the delay from the previous request to the current one. This also
means that when a single stream is present on a connection, a part of
the server's response time may appear in the %Ti measurement, but this
reflects the reality since nothing would prevent the client from using
the connection to fetch more objects. In addition this shows how long
it takes a client to find references to objects in an HTML page and
start to fetch them.
A different approach could have consisted in counting from the last time
the connection was left without any request (i.e. really idle), but this
would at least require a documentation change and it's not certain this
would provide a more useful information.
Thanks to Bart Butler and Luke Seelenbinder for reporting enough elements
to diagnose this issue.
This should be backported to 2.4.
Sadly, despite particular care, commit 39a0a1e12 ("MEDIUM: h2/hpack: emit
a Dynamic Table Size Update after settings change") broke H2 when sending
DTSU. A missing negation on the flag caused the DTSU_EMITTED flag to be
lost and the DTSU to be sent again on the next stream, and possibly to
break flow control or a few other internal states.
This will have to be backported wherever the patch above was backported.
Thanks to Yves Lafon for notifying us with elements to reproduce the
issue!
tls_basic_sync_wo_stkt_backend fails once every 200 runs for me. This
seems to be because the startup delay doesn't always allow peers to
perform a simultaneous connect, close and new attempt. With 3s I can't
see it fail anymore. In addition the long "delay 0.2" are still way too
much since we do not really care about the startup order in practice.
Sometimes when sending commands to shut down a server, haproxy complains
that some connections remain, this is because the server-side connection
might not always be completely released at the moment the client leaves
and the operation is emitted. While shutting down server sessions work,
it seems cleaner to just use "option httpclose" which releases the server
earlier and avoids the race.
This can be backported to 2.5.
There's a bug in spoe_release_appctx() which checks the presence of items
in the wrong list rt[tid].agents to run over rt[tid].waiting_queue and
zero their spoe_appctx. The effect is that these contexts are not zeroed
and if spoe_stop_processing() is called, "sa->cur_fpa--" will be applied
to one of these recently freed contexts and will corrupt random memory
locations, as found at least in bugs #1494 and #1525.
This must be backported to all stable versions.
Many thanks to Christian Ruppert from Babiel for exchanging so many
useful traces over the last two months, testing debugging code and
helping set up a similar environment to reproduce it!
Ensure calls to http_find_header() terminate. If a "Set-Cookie2"
header is found then the while(1) loop in
http_manage_server_side_cookies() will never terminate, resulting in
the watchdog firing and the process terminating via SIGABRT.
The while(1) loop becomes unbounded because an unmatched call to
http_find_header("Set-Cookie") will leave ctx->blk=NULL. Subsequent
calls to check for "Set-Cookie2" will now enumerate from the beginning
of all the blocks and will once again match on subsequent
passes (assuming a match first time around), hence the loop becoming
unbounded.
This issue was introduced with HTX and this fix should be backported
to all versions supporting HTX.
Many thanks to Grant Spence (gspence@redhat.com) for working through
this issue with me.
ist are not ended by '\0', leading to junk characters being displayed
when using %s for printing the HTTP start line.
Fix the issue by replacing %s by %.*s + istlen.
Must be backported in 2.5.
If the same filename was specified in multiple calls of the jwt_verify
converter, we would have parsed the contents of the file every time it
was used instead of checking if the entry already existed in the tree.
This lead to memory leaks because we would not insert the duplicated
entry and we would not free it (as well as the EVP_PKEY it referenced).
We now check the return value of ebst_insert and free the current entry
if it is a duplicate of an existing entry.
The order in which the tree insert and the pkey parsing happen was also
switched in order to avoid parsing key files in case of duplicates.
Should be backported to 2.5.
When emptying the jwt_cert_tree during deinit, the entries are freed but
not the EVP_PKEY reference they kept, leading in a memory leak.
Should be backported in 2.5.
The node pointer was not moving properly along the jwt_cert_tree during
the deinit which ended in a double free during cleanup (or when checking
a configuration that used the jwt_verify converter with an explicit
certificate specified).
This patch fixes GitHub issue #1533.
It should be backported to 2.5.
Inspect return code of HEADERS/DATA parsing functions and use a BUG_ON
to signal an error. The stream should be closed to handle the error
in a more clean fashion.
This is the same fixe as for this commit:
"BUILD: tree-wide: avoid warnings caused by redundant checks of obj_types"
Should fix CID 1469649 for GH #1546
Remove this server specific code section. It is useless, not tested. Furthermore
this is really not the good place to retrieve the peer transport parameters.
Add a new function h3_data_to_htx. This function is used to parse a H3
DATA frame and copy it in the mux stream HTX buffer. This is required to
support HTTP POST data.
Note that partial transfers if the HTX buffer is fulled is not properly
handle. This causes large DATA transfer to fail at the moment.
Move the HEADERS parsing code outside of generic h3_decode_qcs to a new
dedicated function h3_headers_to_htx. The benefit will be visible when
other H3 frames parsing will be implemented such as DATA.
Flags EOI/EOS must be set on conn-stream when transfering the last data
of a stream in rcv_buf. This is activated if qcs HTX buffer has the EOM
flag and has been fully transfered.
Implement the stream rcv_buf operation on QUIC mux.
A new buffer is stored in qcs structure named app_buf. This new buffer
will contains HTX and will be filled for example on H3 DATA frame
parsing.
The rcv_buf operation transfer as much as possible data from the HTX
from app_buf to the conn-stream buffer. This is mainly identical to
mux-h2. This is required to support HTTP POST data.
Adjust the method to detect that a H3 HEADERS frame is the last one of
the stream. If this is true, the flags EOM and BODYLESS must be set on
the HTX message.
