The SHOW_TOT() and SHOW_AVG() macros used in cli_io_handler_show_activity()
produce a warning on gcc 4.7 on MIPS with threads disabled because the
compiler doesn't know that global.nbthread is necessarily non-null, hence
that at least one iteration is performed. Let's just change the loop for
a do {} while () that lets the compiler know it's always initialized. It
also has the tiny benefit of making the code shorter.
The shctx code relies on sensitive conditions that are hard to infer
from the code itself, let's add some BUG_ON() to verify them. They
helped spot the previous bugs.
In shctx_row_reserve_hot() we only leave if we've found the exact
requested size instead of at least as large, as is documented. This
results in extra lookups and free calls in the avail loop while it is
not needed, and participates to seeing a negative data_len early as
spotted in previous bugs.
It doesn't seem to have any other impact however, but it's better to
backport it to stable branches.
In shctx_row_reserve_hot(), a missing break allows the avail loop to
loop for a while after having allocated the required blocks, possibly
leading to the point where it could trigger the watchdog after checking
up to 2 million blocks. In addition, the extra iteration may leave one
block assigned with size zero at the head of the avail list, and mark
it as being an isolated chain of 1 block. It's unclear whether this
could have had other consequences.
There is a non-negligible chance that it addreses bugs #1451 and #1284,
as the pattern observed in the loop looks exactly the same as the one
reported there in the crashes.
It's only marked medium because it is extremely hard to trigger. Here
the conditions were reproduced when starting 4k connections at once
requesting objects of random sizes between 0 and 20k to store them into
a small 1MB cache. However the watchdog will never trigger in such a case
so one needs to instrument the functions.
Thanks to Sohaib Ahmad and @g0uZ for providing useful traces.
This will need to be backported to all stable branches.
The "show cache" command restarts from the previous node to look for a
duplicate key, but does this after having released the lock, so under
high write load, the node has many chances of having been reassigned
and the dereference of the node crashes after a few iterations. Since
the keys are unique anyway, there's no point looking for a dup, so
let's just continue from the next value.
This is only marked as medium as it seems to have been there for a
while, and discovering it that late simply means that nobody uses that
command, thus in practice it has a very limited impact on real users.
This should be backported to all stable versions.
A warning is triggered by gcc9 on this code path, which is the compiler
version used by ubuntu20.04 on the github CI.
This is linked to github issue #1445.
When receiving Initial packets for Version Negotiation, no quic_conn is
instantiated. Thus, on the final trace, the quic_conn dereferencement
must be tested before using it.
This simple patch add the parsing support for theses frames. But nothing is
done at this time about the streams or flow control concerned. This is only to
prevent some QUIC tracker or interop runner tests from failing for a reason
independant of their tested features.
When we have already received ACK frames with the same largest packet
number, this is not an error at all. In this case, we must continue
to parse the ACK current frame.
Add ->err member to quic_conn struct to store the connection errors.
This is the responsability of ->send_alert callback of SSL_QUIC_METHOD
struct to handle the TLS alert and consequently update ->err value.
At this time, when entering qc_build_pkt() we build a CONNECTION_CLOSE
frame close the connection when ->err value is not null.
When adding a range, if no "lower" range was present in the ack range root for
the packet number space concerned, we did not check if the new added range could
overlap the next one. This leaded haproxy to crash when encoding negative integer
when building ACK frames.
This bug was revealed thanks to "multi_packet_client_hello" QUIC tracker
test which makes a client send two first Initial packets out of order.
->qc (QUIC connection) member of packet structure were badly initialized
when received as second Initial packet (from picoquic -Q for instance).
This leaded to corrupt the quic_conn structure with random behaviors
as size effects. This bug came with this commit:
"MINOR: quic: Possible wrong connection identification"
If we want to run quic-tracker against haproxy, we must at least
support the draft version of the TLS extension for the QUIC transport
parameters (0xffa5). quic-tracker QUIC version is draft-29 at this time.
We select this depending on the QUIC version. If draft, we select the
draft TLS extension.
UDP datagrams with Initial packet were padded only for the clients (haproxy
servers). But such packets MUST also be padded for the servers (haproxy
listeners). Furthere, for servers, only UDP datagrams containing ack-eliciting
Initial packet must be padded.
A client may send several Initial packets. This is the case for picoquic
with -Q option. In this case we must identify the connection of incoming
Initial packets thanks to the original destination connection ID.
When allocating destination addresses for QUIC connections we did not set
this flag which denotes these addresses have been set. This had as side
effect to prevent the H3 request results from being returned to the QUIC clients.
Note that this bug was revealed by this commit:
"MEDIUM: backend: Rely on addresses at stream level to init server connection"
Thanks to Christopher for having found the real cause of this issue.
During 2.4-dev, an issue with partial frames was fixed with commit
3d4631fec ("BUG/MEDIUM: mux-h2: fix read0 handling on partial frames").
However this patch is not completely correct. It makes h2_recv() return
0 if the connection was shut for reads, but this not make h2_io_cb()
call h2_process(), so if there are any pending data left in the demux
buffer, they will never be processed, and the I/O callback will be
called in loops forever from the poller.
The correct return value there is 1, as is done at the end of the
function to report a pending read0.
This should definitely fix issue #1328. However even after a lot of
tests I couldn't manage to reproduce it, the conditions to enter that
situation are quite racy.
This must be backported to 2.0 since the fix above was merged into
2.0.21 and 2.2.9.
Since commit c2aae74 ("MEDIUM: ssl: Handle early data with OpenSSL
1.1.1"), the codepath of the clientHello callback changed, letting an
unknown SNI escape with a 'return 1' instead of passing through the
abort label.
An error was still emitted because the frontend continued the handshake
with the initial_ctx, which can't be used to achieve an handshake.
However, it had the ugly side effect of letting the request pass in the
case of a TLS resume. Which could be surprising when combining strict-sni
with the removing of a crt-list entry over the CLI for example. (like
its done in the ssl/new_del_ssl_crlfile.vtc reg-test).
This patch switches the code path of the allow_early and abort label, so
the default code path is the abort one, letting the clientHello returns
the correct SSL_AD_UNRECOGNIZED_NAME in case of errors.
Which means the client will now receive:
OpenSSL error[0x14094458] ssl3_read_bytes: tlsv1 unrecognized name
Instead of:
OpenSSL error[0x14094410] ssl3_read_bytes: sslv3 alert handshake failure
Which was the error emitted before HAProxy 1.8.
This patch must be carrefuly backported as far as 1.8 once we validated
its impact.
When establishing an outboud connection, haproxy checks if the cached
TLS session has the same SNI as the connection we are trying to
resume.
This test was done by calling SSL_get_servername() which in TLSv1.2
returned the SNI. With TLSv1.3 this is not the case anymore and this
function returns NULL, which invalidates any outboud connection we are
trying to resume if it uses the sni keyword on its server line.
This patch fixes the problem by storing the SNI in the "reused_sess"
structure beside the session itself.
The ssl_sock_set_servername() now has a RWLOCK because this session
cache entry could be accessed by the CLI when trying to update a
certificate on the backend.
This fix must be backported in every maintained version, however the
RWLOCK only exists since version 2.4.
Sometimes it is really useful to be able to specify a default value for
an optional environment variable, like the ${name-value} construct in
shell. In fact we're really missing this for a number of settings in
reg tests, starting with timeouts.
This commit simply adds support for the common syntax above. Other
common forms like '+' to replace existing variables, or ':-' and ':+'
to act on empty variables, were not implemented at this stage, as they
are less commonly needed.
The else is not for boringSSL but for the lack of Client Hello callback.
Should have been changed in 1fc44d4 ("BUILD: ssl: guard Client Hello
callbacks with HAVE_SSL_CLIENT_HELLO_CB macro instead of openssl
version").
Could be backported in 2.4.
Previously, the cleanup of the listeners was done in mworker_loop(),
which was called once the configuration file was parsed. HAProxy was
switching in wait mode when the configuration failed to load, so no
listeners where created.
Since the latest change on the mworker mode, HAProxy switch to wait mode
after successfuly loading the configuration, without cleaning its
listeners, because it was done in mworker_loop, resulting in the master
not closing its listeners and keeping them. The master needs its
configuration to know which listeners it need to close, so that must be
done before the exec().
This patch fixes the problem by cleaning the listeners in the
mworker_reexec() function.
No backport needeed.
If the client announced a QUIC version not supported by haproxy, emit a
Version Negotiation Packet, according to RFC9000 6. Version Negotiation.
This is required to be able to use the framework for QUIC interop
testing from https://github.com/marten-seemann/quic-interop-runner. The
simulator checks that the server is available by sending packets to
force the emission of a Version Negotiation Packet.
Implement a new app_ops layer for quic interop. This layer uses HTTP/0.9
on top of QUIC. Implementation is minimal, with the intent to be able to
pass interoperability test suite from
https://github.com/marten-seemann/quic-interop-runner.
It is instantiated if the negotiated ALPN is "hq-interop".
Remove the hardcoded initialization of h3 layer on mux init. Now the
ALPN is looked just after the SSL handshake. The app layer is then
installed if the ALPN negotiation returned a supported protocol.
This required to add a get_alpn on the ssl_quic layer which is just a
call to ssl_sock_get_alpn() from ssl_sock. This is mandatory to be able
to use conn_get_alpn().
This change is required to be able to use multiple app_ops layer on top
of QUIC. The stream-interface will now call the mux snd_buf which is
just a proxy to the app_ops snd_buf function.
The architecture may be simplified in the structure to install the
app_ops on the stream_interface and avoid the detour via the mux layer
on the sending path.
When receiving an unknown h3 frame type, the frame must be discarded
silently and the processing of the remaing frames must continue. This is
according to the HTTP/3 draft34.
This issue was detected when using the quiche client which uses GREASE
frame to test interoperability.
The commit a85c522d4 ("BUG/MINOR: mux-h1: Save shutdown mode if the shutdown
is delayed") revealed several hidden bugs in connection's shutdown
handling. One of them is about delayed silent shudown.
If outgoing data are not fully sent, we delayed the shutdown. However, in
h1_process(), only normal (or clean) shutdown are really detected. If a
silent (or dirty) shutdown is performed, the H1 connection is not
immediately released. Of course, in this situation, the client never
acknowledged the shutdown. Thus, the H1 connection remains open till the
client timeout.
This patch should fix the issues #1448 and #1453. It must be backported as
far as 2.0.
When a log message is emitted, The session's listener is always defined when
the session's owner is an inbound connection while it is undefined for a
health-check. It is not obvious. So, comments have been added to make it
clear.
This patch is related to the issue #1434.
When an ipv6 key is used to filter a CLI command on a stick table
(clear/set/show table ...), the return value of inet_pton() call must be
checked to be sure the key is valid.
This patch should fix the issue #1163. It should be backported to all
supported versions.
When haproxy is built with DEBUG_UAF=1, some particularly slow
allocation functions are used for each pool, and it was not uncommon
to see the watchdog trigger during performance tests. For this reason
the allocation functions were surrounded by a pair of thread_harmless
calls to mention that the function was waiting in slow syscalls. The
problem is that this also releases functions blocked in thread_isolate()
which can then start their work.
In order to protect against the accidental removal of a shared resource
in this situation, in 2.5-dev4 with commit ba3ab7907 ("MEDIUM: servers:
make the server deletion code run under full thread isolation") was added
thread_isolate_full() for functions which want to be totally protected
due to being manipulating some data.
But this is not sufficient, because there are still places where we
can allocate/free (thus sleep) under a lock, such as in long call
chains involving the release of an idle connection. In this case, if
one thread asks for isolation, one thread might hang in
pool_alloc_area_uaf() with a lock held (for example the conns_lock
when coming from conn_backend_get()->h1_takeover()->task_new()), with
another thread blocked on a lock waiting for that one to release it,
both keeping their bit clear in the thread_harmless mask, preventing
the first thread from being released, thus causing a deadlock.
In addition to this, it was already seen that the "show fd" CLI handler
could wake up during a pool_free_area_uaf() with an incompletely
released memory area while deleting a file descriptor, and be fooled
showing bad pointers, or during a pool_alloc() on another thread that
was in the process of registering a freshly allocated connection to a
new file descriptor.
One solution could consist in replacing all thread_isolate() calls by
thread_isolate_full() but then that makes thread_isolate() useless
and only shifts the problem by one slot.
A better approach could possibly consist in having a way to mark that
a thread is entering an extremely slow section. Such sections would
be timed so that this is not abused, and the bit would be used to
make the watchdog more patient. This would be acceptable as this would
only affect debugging.
The approach used here for now consists in removing the harmless bits
around the UAF allocator, thus essentially undoing commit 85b2cae63
("MINOR: pools: make the thread harmless during the mmap/munmap
syscalls").
This is marked as minor because nobody is expected to be running with
DEBUG_UAF outside of development or serious debugging, so this issue
cannot affect regular users. It must be backported to stable branches
that have thread_harmless_now() around the mmap() call.
The value for H2_CF_DEM_SHORT_READ flag is wrong. 2 bits are erroneously
set, 0x200 and 0x80000. It is not an issue because both bits are not used
anywhere else.
The typo was introduced in the commit b5f7b5296 ("BUG/MEDIUM: mux-h2: Handle
remaining read0 cases on partial frames"). Thus this patch must also be
backported as far a 2.0.
httpclient_new() sets the hc->req.uri ist without duplicating its
memory, which is a problem since the string in the ist could be
inaccessible at some point. The API was made to use a ist which was
allocated dynamically, but httpclient_new() didn't do that, which result
in a crash when calling istfree().
This patch fixes the problem by doing an istdup()
Fix issue #1452.
When in wait mode, the mworker-prog postparser is launched, but
unfortunately the child structure doesn't contain all required
information to be able to launch the test.
This test is only required when doing a configuration parsing.
Must be backported as far as 2.0.
Since the wait mode is always used once we successfuly loaded the
configuration, every processes were marked as old workers.
To fix this, the PROC_O_LEAVING flag is set only on the processes which
have a number of reloads greater than the current processes.
The ReloadFailed prompt in the master CLI is shown only when
failedreloads > 0. It was previously using a check on the wait mode, but
we always use the wait mode now.
Implement a reload failure counter which counts the number of failure
since the last success. This counter is available in 'show proc' over
the master CLI.
Clarify the startup and reload messages:
On a successful configuration load, haproxy will emit "Loading success."
after successfuly forked the children.
When it didn't success to load the configuration it will emit "Loading failure!".
When trying to reload the master process, it will emit "Reloading
HAProxy".
Use the waitpid mode after successfully loading the configuration, this
way the memory will be freed in the master, and will preserve the memory.
This will be useful when doing a reload with a configuration which has
large maps or a lot of SSL certificates, avoiding an OOM because too
much memory was allocated in the master.
nbproc was removed, it's time to remove any reference to the relative
PID in the master-worker, since there can be only 1 current haproxy
process.
This patch cleans up the alerts and warnings emitted during the exit of
a process, as well as the "show proc" output.
This reverts commit 597909f4e67866c4f3ecf77f95f2cd4556c0c638
http-after-response rules evaluation was changed to do the same that was
done for http-response, in the code. However, the opposite must be performed
instead. Only the rules of the current section must be stopped. Thus the
above commit is reverted and the http-response rules evaluation will be
fixed instead.
Note that only "allow" action is concerned. It is most probably an uncommon
action for an http-after-request rule.
This patch must be backported as far as 2.2 if the above commit was
backported.
