MINOR: stconn: Always report READ/WRITE event on shutr/shutw
It was done by hand by callers when a shutdown for read or write was
performed. It is now always handled by the functions performing the
shutdown. This way the callers don't take care of it. This will avoid some
bugs.
MINOR: stream: Use relative expiration date in trace messages
Expiration dates in trace messages are now relative to now_ms. It is fairly
easier to read traces this way. And an expired date is now negative. So, it
is also easy to detect when a timeout was reached.
MINOR: stream: Report rex/wex value using the sedesc date in trace messages
Becasue read and write timeout are now detected using .lra and .fsb fields
of the stream-endpoint descriptor, it is better to also use these fields to
report read and write expiration date in trace messages. Especially because
old rex and wex fields will be removed.
MAJOR: stream: Use SE descriptor date to detect read/write timeouts
We stop to use the channel's expiration dates to detect read and write
timeouts on the channels. We now rely on the stream-endpoint descriptor to
do so. All the stuff is handled in process_stream().
The stream relies on 2 helper functions to know if the receives or sends may
expire: sc_rcv_may_expire() and sc_snd_may_expire().
MINOR: applet/stconn: Add a SE flag to specify an endpoint does not expect data
An endpoint should now set SE_FL_EXP_NO_DATA flag if it does not expect any
data from the opposite endpoint. This way, the stream will be able to
disable any read timeout on the opposite endpoint. Applets should use
applet_expect_no_data() and applet_expect_data() functions to set or clear
the flag. For now, only dns and sink forwarder applets are concerned.
MEDIUM: stconn: Add two date to track successful reads and blocked sends
The stream endpoint descriptor now owns two date, lra (last read activity) and
fsb (first send blocked).
The first one is updated every time a read activity is reported, including data
received from the endpoint, successful connect, end of input and shutdown for
reads. A read activity is also reported when receives are unblocked. It will be
used to detect read timeouts.
The other one is updated when no data can be sent to the endpoint and reset
when some data are sent. It is the date of the first send blocked by the
endpoint. It will be used to detect write timeouts.
Helper functions are added to report read/send activity and to retrieve lra/fsb
date.
MEDIUM: stconn: Replace read and write timeouts by a unique I/O timeout
Read and write timeouts (.rto and .wto) are now replaced by an unique
timeout, call .ioto. Since the recent refactoring on channel's timeouts,
both use the same value, the client timeout on client side and the server
timeout on the server side. Thus, this part may be simplified. Now it
represents the I/O timeout.
MEDIUM: stconn: Don't requeue the stream's task after I/O
After I/O handling, in sc_notify(), the stream's task is no longer
requeue. The stream may be woken up. But its task is not requeue. It is
useless nowadays and only avoids a call to process_stream() for edge
cases. It is not really a big deal if the stream is woken up for nothing
because its task expired. At worst, it will be responsible to compute its
new expiration date.
MEDIUM: channel/stconn: Move rex/wex timer from the channel to the sedesc
These timers are related to the I/O. Thus it is logical to move them into
the SE descriptor. The patch is a bit huge but it is just a
replacement. However it is error-prone.
From the stconn or the stream, helper functions are used to get, set or
reset these timers. This simplify the timers manipulations.
MINOR: channel/stconn: Move rto/wto from the channel to the stconn
Read and write timeouts concerns the I/O. Thus, it is logical to move it into
the stconn. At the end, the stream is responsible to detect the timeouts. So
it is logcial to have these values in the stconn and not in the SE
descriptor. But it may change depending on the recfactoring.
So, now:
* scf->rto is used instead of req->rto
* scf->wto is used instead of res->wto
* scb->rto is used instead of res->rto
* scb->wto is used instead of req->wto
MAJOR: channel: Remove flags to report READ or WRITE errors
This patch removes CF_READ_ERROR and CF_WRITE_ERROR flags. We now rely on
SE_FL_ERR_PENDING and SE_FL_ERROR flags. SE_FL_ERR_PENDING is used for write
errors and SE_FL_ERROR for read or unrecoverable errors.
When a connection error is reported, SE_FL_ERROR and SE_FL_EOS are now set and a
read event and a write event are reported to be sure the stream will properly
process the error. At the stream-connector level, it is similar. When an error
is reported during a send, a write event is triggered. On the read side, nothing
more is performed because an error at this stage is enough to wake the stream
up.
A major change is brought with this patch. We stop to check flags of the
ooposite channel to report abort or timeout. It also means when an read or
write error is reported on a side, we no longer update the other side. Thus
a read error on the server side does no long lead to a write error on the
client side. This should ease errors report.
This flag was introduced in 1.3 to fix a design issue. It was untouch since
then but there is no reason to still have this trick. Note it could be good
to review what happens in HTTP with the server is waiting for the end of the
request. It could be good to be sure a client timeout is always reported.
BUG/MEDIUM: httpclient/lua: fix a race between lua GC and hlua_ctx_destroy
In bb581423b ("BUG/MEDIUM: httpclient/lua: crash when the lua task timeout
before the httpclient"), a new logic was implemented to make sure that
when a lua ctx destroyed, related httpclients are correctly destroyed too
to prevent a such httpclients from being resuscitated on a destroyed lua ctx.
This was implemented by adding a list of httpclients within the lua ctx,
and a new function, hlua_httpclient_destroy_all(), that is called under
hlua_ctx_destroy() and runs through the httpclients list in the lua context
to properly terminate them.
This was done with the assumption that no concurrent Lua garbage collection
cycles could occur on the same ressources, which seems OK since the "lua"
context is about to be freed and is not explicitly being used by other threads.
But when 'lua-load' is used, the main lua stack is shared between multiple
OS threads, which means that all lua ctx in the process are linked to the
same parent stack.
Yet it seems that lua GC, which can be triggered automatically under
lua_resume() or manually through lua_gc(), does not limit itself to the
"coroutine" stack (the stack referenced in lua->T) when performing the cleanup,
but is able to perform some cleanup on the main stack plus coroutines stacks
that were created under the same main stack (via lua_newthread()) as well.
This can be explained by the fact that lua_newthread() coroutines are not meant
to be thread-safe by design.
Source: http://lua-users.org/lists/lua-l/2011-07/msg00072.html (lua co-author)
It did not cause other issues so far because most of the time when using
'lua-load', the global lua lock is taken when performing critical operations
that are known to interfere with the main stack.
But here in hlua_httpclient_destroy_all(), we don't run under the global lock.
Now that we properly understand the issue, the fix is pretty trivial:
We could simply guard the hlua_httpclient_destroy_all() under the global
lua lock, this would work but it could increase the contention over the
global lock.
Instead, we switched 'lua->hc_list' which was introduced with bb581423b
from simple list to mt_list so that concurrent accesses between
hlua_httpclient_destroy_all and hlua_httpclient_gc() are properly handled.
The issue was reported by @Mark11122 on Github #2037.
This must be backported with bb581423b ("BUG/MEDIUM: httpclient/lua: crash
when the lua task timeout before the httpclient") as far as 2.5.
BUG/MINOR: lua/httpclient: missing free in hlua_httpclient_send()
In hlua_httpclient_send(), we replace hc->req.url with a new url.
But we forgot to free the original url that was allocated in
hlua_httpclient_new() or in the previous httpclient_send() call.
Because of this, each httpclient request performed under lua scripts would
result in a small leak. When stress-testing a lua action which uses httpclient,
the leak is clearly visible since we're leaking severals Mbytes per minute.
This bug was discovered by chance when trying to reproduce GH issue #2037.
Willy Tarreau [Mon, 20 Feb 2023 18:18:47 +0000 (19:18 +0100)]
MINOR: compiler: add a TOSTR() macro to turn a value into a string
Pretty often we have to emit a value (setting, limit etc) in an error
message, and this value is known at compile-time, and just doing this
forces to use a printf format such as "%d". Let's have a simple macro
to turn any other macro or value into a string that can be concatenated
with the rest of the string around. This simplifies error messages
production on the CLI for example.
BUG/MINOR: cache: Check cache entry is complete in case of Vary
Before looking for a secondary cache entry for a given request we
checked that the first entry was complete, which might prevent us from
using a valid entry if the first one with the same primary key is not
full yet.
Likewise, if the primary entry is complete but not the secondary entry
we try to use, we might end up using a partial entry from the cache as
a response.
This bug was raised in GitHub #2048.
It can be backported up to branch 2.4.
BUG/MINOR: cache: Cache response even if request has "no-cache" directive
Since commit cc9bf2e5f "MEDIUM: cache: Change caching conditions"
responses that do not have an explicit expiration time are not cached
anymore. But this mechanism wrongly used the TX_CACHE_IGNORE flag
instead of the TX_CACHEABLE one. The effect this had is that a cacheable
response that corresponded to a request having a "Cache-Control:
no-cache" for instance would not be cached.
Contrary to what was said in the other commit message, the "checkcache"
option should not be impacted by the use of the TX_CACHEABLE flag
instead of the TX_CACHE_IGNORE one. The response is indeed considered as
not cacheable if it has no expiration time, regardless of the presence
of a cookie in the response.
This should fix GitHub issue #2048.
This patch can be backported up to branch 2.4.
MINOR: startup: HAPROXY_STARTUP_VERSION contains the version used to start
HAPROXY_STARTUP_VERSION: contains the version used to start, in
master-worker mode this is the version which was used to start the
master, even after updating the binary and reloading.
This patch could be backported in every version since it is useful when
debugging.
BUG/MEDIUM: mworker: prevent inconsistent reload when upgrading from old versions
Previous versions ( < 1.9 ) of the master-worker process didn't had the
"HAPROXY_PROCESSES" environment variable which contains the list of
processes, fd etc.
The part which describes the master is created at first startup so if
you started the master with an old version you would never have
it.
Since patch 68836740 ("MINOR: mworker: implement a reload failure
counter"), the failedreloads member of the proc_self structure for the
master is set to 0. However if this structure does not exist, it will
result in a NULL dereference and crash the master.
This patch fixes the issue by creating the proc_self structure for the
master when it does not exist. It also shows a warning which states to
restart the master if that is the case, because we can't guarantee that
it will be working correctly.
This MUST be backported as far as 2.5, and could be backported in every
other stable branches.
BUG/MINOR: mworker: stop doing strtok directly from the env
When parsing the HAPROXY_PROCESSES environement variable, strtok was
done directly from the ptr resulting from getenv(), which replaces the ;
by \0, showing confusing environment variables when debugging in /proc
or in a corefile.
REGTESTS: Fix ssl_errors.vtc script to wait for connections close
In this scripts, several clients perform a requests and exit because an SSL
error is expected and thus no response is sent. However, we must explicitly
wait for the connection close, via an "expect_close" statement. Otherwise,
depending on the timing, HAProxy may detect the client abort before any
connection attempt on the server side and no SSL error is reported, making
the script to fail.
REGTESTS: Skip http_splicing.vtc script if fast-forward is disabled
If "-dF" command line argument is passed to haproxy to execute the script,
by sepcifying HAPROXY_ARGS variable, http_splicing.vtc is now skipped.
Without this patch, the script fails when the fast-forward is disabled.
These options are those that can be disabled on the command line. This way
it is possible, from a reg-test for instance, to know if a feature is
supported or not :
BUG/MINOR: haproxy: Fix option to disable the fast-forward
The option was renamed to only permit to disable the fast-forward. First
there is no reason to enable it because it is the default behavior. Then it
introduced a bug because there is no way to be sure the command line has
precedence over the configuration this way. So, the option is now named
"tune.disable-fast-forward" and does not support any argument. And of
course, the commande line option "-dF" has now precedence over the
configuration.
MINOR: proxy: Only consider backend httpclose option for server connections
For server connections, both the frontend and backend were considered to
enable the httpclose option. However, it is ambiguous because on client side
only the frontend is considerd. In addition for 2 frontends, one with the
option enabled and not for the other, the HTTP connection mode may differ
while it is a backend setting.
Thus, now, for the server side, only the backend is considered. Of course,
if the option is set for a listener, the option will be enabled if the
listener is the backend's connection.
DOC: config: Fix description of options about HTTP connection modes
Since the HTX, the decription of options about HTTP connection modes is
wrong. In fact, it is worst, all the documentation about HTTP connection
mode is wrong. But only options will be updated for now to be backported.
So, documentation of "option httpclose", "option "http-keep-alive", "option
http-server-close" and "option "http-pretend-keepalive" was reviewed. First,
it is specify these options only concern HTT/1.x connections. Then, the
descriptions were updated to reflect the HTX implementation.
The main changes concerns the fact that server connections are no longer
attached to client connections. The connection mode on one side does not
affect the connection mode on the other side. It is especially true for
t"option httpclose". For client connections, only the frontend option is
considered and for server ones, both frontend and backend options are
considered.
DEBUG: stream: Add a BUG_ON to never exit process_stream with an expired task
We must never exit for the stream processing function with an expired
task. Otherwise, we are pretty sure this will ends with a spinning loop. It
is really better to abort as far as possible and with the original buggy
state. This will ease the debug sessions.
BUG/MEDIUM: quic: Missing TX buffer draining from qc_send_ppkts()
If the TX buffer (->tx.buf) attached to the connection is not drained, there
are chances that this will be detected by qc_txb_release() which triggers
a BUG_ON_HOT() when this is the case as follows
Willy Tarreau [Mon, 20 Feb 2023 16:05:10 +0000 (17:05 +0100)]
MINOR: mux-h2/traces: add a missing TRACE_LEAVE() in h2s_frt_handle_headers()
Traces from this function would miss a TRACE_LEAVE() on the success path,
which had for consequences, 1) that it was difficult to figure where the
function was left, and 2) that we never had the allocated stream ID
clearly visible (actually the one returned by h2c_frt_stream_new() is
the right one but it's not obvious).
Willy Tarreau [Mon, 20 Feb 2023 15:57:47 +0000 (16:57 +0100)]
MINOR: mux-h2/traces: do not log h2s pointer for dummy streams
Functions which are called with dummy streams pass it down the traces
and that leads to somewhat confusing "h2s=0x1234568(0,IDL)" for example
while the nature of the called function makes this stream useless at that
place. Better not report a random pointer, especially since it always
requires to look at the code before remembering how this should be
interpreted.
Now what we're doing is that the idle stream only prints "h2s=IDL" which
is shorter and doesn't report a pointer, closed stream do not report
anything since the stream ID 0 already implies it, and other ones are
reported normally.
This could be backported to 2.7 and 2.6 as it improves traces legibility.
MEDIUM: quic: trigger fast connection closing on process stopping
With previous commit, quic-conn are now handled as jobs to prevent the
termination of haproxy process. This ensures that QUIC connections are
closed when all data are acknowledged by the client and there is no more
active streams.
The quic-conn layer emits a CONNECTION_CLOSE once the MUX has been
released and all streams are acknowledged. Then, the timer is scheduled
to definitely free the connection after the idle timeout period. This
allows to treat late-arriving packets.
Adjust this procedure to deactivate this timer when process stopping is
in progress. In this case, quic-conn timer is set to expire immediately
to free the quic-conn instance as soon as possible. This allows to
quickly close haproxy process.
MINOR: quic: mark quic-conn as jobs on socket allocation
To prevent data loss for QUIC connections, haproxy global variable jobs
is incremented each time a quic-conn socket is allocated. This allows
the QUIC connection to terminate all its transfer operation during proxy
soft-stop. Without this patch, the process will be terminated without
waiting for QUIC connections.
Note that this is done in qc_alloc_fd(). This means only QUIC connection
with their owned socket will properly support soft-stop. In the other
case, the connection will be interrupted abruptly as before. Similarly,
jobs decrement is conducted in qc_release_fd().
Amaury Denoyelle [Tue, 24 Jan 2023 17:20:28 +0000 (18:20 +0100)]
MEDIUM: mux-quic: properly implement soft-stop
Properly implement support for haproxy soft-stop on QUIC MUX. This code
is similar to H2 MUX :
* on timeout refresh, if stop-stop in progress, schedule the timeout to
expire with regards to the close-spread-end window.
* after input/output processing, if soft-stop in progress, shutdown the
connection. This is randomly spread by close-spread-end window. In the
case of H3 connection, a GOAWAY is emitted and the connection is kept
until all data are sent for opened streams. If the client tries to use
new streams, they are rejected in conformance with the GOAWAY
specification.
This ensures that MUX is able to forward all content properly before
closing the connection. The lower quic-conn layer is then responsible
for retransmission and should be closed when all data are acknowledged.
This will be implemented in the next commit to fully support soft-stop
for QUIC connections.
Implement client-fin timeout for MUX quic. This timeout is used once an
applicative layer shutdown has been called. In HTTP/3, this corresponds
to the emission of a GOAWAY.
Amaury Denoyelle [Tue, 24 Jan 2023 17:19:47 +0000 (18:19 +0100)]
MINOR: mux-quic: define qc_process()
Define a new function qc_process(). This function will regroup several
internal operation which should be called both on I/O tasklet and wake()
callback. For the moment, only streams purge is conducted there.
This patch is useful to support haproxy soft stop. This should be
backported up to 2.7.
Amaury Denoyelle [Tue, 24 Jan 2023 17:18:23 +0000 (18:18 +0100)]
MINOR: mux-quic: define qc_shutdown()
Factorize shutdown operation in a dedicated function qc_shutdown(). This
will allow to call it from multiple places. A new flag QC_CF_APP_SHUT is
also defined to ensure it will only be executed once even if called
multiple times per connection.
This commit will be useful to properly support haproxy soft stop.
This should be backported up to 2.7.
Amaury Denoyelle [Tue, 24 Jan 2023 16:42:21 +0000 (17:42 +0100)]
MEDIUM: h3: enforce GOAWAY by resetting higher unhandled stream
When a GOAWAY has been emitted, an ID is announced to represent handled
streams. H3 RFC suggests that higher streams should be resetted with the
error code H3_REQUEST_CANCELLED. This allows the peer to replay requests
on another connection.
For the moment, the impact of this change is limitted as GOAWAY is only
used on connection shutdown just before the MUX is freed. However, for
soft-stop support, a GOAWAY can be emitted in anticipation while keeping
the MUX to finish the active streams. In this case, new streams opened
by the client are resetted.
As a consequence of this change, app_ops.attach() operation has been
delayed at the very end of qcs_new(). This ensure that all qcs members
are initialized to support RESET_STREAM sending.
Amaury Denoyelle [Thu, 26 Jan 2023 15:03:45 +0000 (16:03 +0100)]
BUG/MINOR: h3: prevent hypothetical demux failure on int overflow
h3s stores the current demux frame type and length as a state info. It
should be big enough to store a QUIC variable-length integer which is
the maximum H3 frame type and size.
Without this patch, there is a risk of integer overflow if H3 frame size
is bigger than INT_MAX. This can typically causes demux state mismatch
and demux frame error. However, no occurence has been found yet of this
bug with the current implementation.
Amaury Denoyelle [Mon, 20 Feb 2023 09:32:16 +0000 (10:32 +0100)]
BUG/MINOR: quic: acknowledge STREAM frame even if MUX is released
When the MUX is freed, the quic-conn layer may stay active until all
streams acknowledgment are processed. In this interval, if a new stream
is opened by the client, the quic-conn is thus now responsible to handle
it. This is done by the emission of a STOP_SENDING + RESET_STREAM.
Prior to this patch, the received packet was not acknowledged. This is
undesirable if the quic-conn is able to properly reject the request as
this can lead to unneeded retransmission from the client.
Amaury Denoyelle [Mon, 20 Feb 2023 09:31:27 +0000 (10:31 +0100)]
BUG/MINOR: quic: also send RESET_STREAM if MUX released
When the MUX is freed, the quic-conn layer may stay active until all
streams acknowledgment are processed. In this interval, if a new stream
is opened by the client, the quic-conn is thus now responsible to handle
it. This is done by the emission of a STOP_SENDING.
This process has been completed to also emit a RESET_STREAM with the
same error code H3_REQUEST_REJECTED. This is done to conform with the H3
specification to invite the client to retry its request on a new
connection.
MINOR: quic: adjust request reject when MUX is already freed
When the MUX is freed, the quic-conn layer may stay active until all
streams acknowledgment are processed. In this interval, if a new stream
is opened by the client, the quic-conn is thus now responsible to handle
it. This is done by the emission of a STOP_SENDING.
This process is closely related to HTTP/3 protocol despite being handled
by the quic-conn layer. This highlights a flaw in our QUIC architecture
which should be adjusted. To reflect this situation, the function
qc_stop_sending_frm_enqueue() is renamed qc_h3_request_reject(). Also,
internal H3 treatment such as uni-directional bypass has been moved
inside the function.
This commit is only a refactor. However, bug fix on next patches will
rely on it so it should be backported up to 2.6.
BUG/MINOR: quic: Missing padding for short packets
This was revealed by Amaury when setting tune.quic.frontend.max-streams-bidi to 8
and asking a client to open 12 streams. haproxy has to send short packets
with little MAX_STREAMS frames encoded with 2 bytes. In addition to a packet number
encoded with only one byte. In the case <len_frms> is the length of the encoded
frames to be added to the packet plus the length of the packet number.
Ensure the length of the packet is at least QUIC_PACKET_PN_MAXLEN adding a PADDING
frame wich (QUIC_PACKET_PN_MAXLEN - <len_frms>) as size. For instance with
a two bytes MAX_STREAMS frames and a one byte packet number length, this adds
one byte of padding.
See https://datatracker.ietf.org/doc/html/rfc9001#name-header-protection-sample.
BUG/MINOR: quic: Do not drop too small datagrams with Initial packets
When receiving an Initial packet a peer must drop it if the datagram is smaller
than 1200. Before this patch, this is the entire datagram which was dropped.
In such a case, drop the packet after having parsed its length.
BUG/MINOR: quic: Wrong initialization for io_cb_wakeup boolean
This bug arrives with this commit: 982896961 MINOR: quic: split and rename qc_lstnr_pkt_rcv()
The first block of code consists in possibly setting this variable to true.
But it was already initialized to true before entering this code section.
Should be initialized to false.
Also take the opportunity to remove an unused "err" label.
BUG/MINOR: quic: Do not probe with too little Initial packets
Before probing the Initial packet number space, verify that we can at least
sent 1200 bytes by datagram. This may not be the case due to the amplification limit.
MINOR: quic: Add a trace to identify connections which sent Initial packet.
This should help in diagnosing issues revealed by the interop runner which counts
the number of handshakes from the number of Initial packets sent by the server.
BUG/MINOR: quic: Missing call to task_queue() in qc_idle_timer_do_rearm()
The aim of this function is to rearm the idle timer. The ->expire
field of the timer task was updated without being requeued.
Some connection could be unexpectedly terminated.
MINOR: quic: Make qc_dgrams_retransmit() return a status.
This is very helpful during retranmission when receiving ICMP port unreachable
errors after the peer has left. This is the unique case at prevent where
qc_send_hdshk_pkts() or qc_send_app_probing() may fail (when they call
qc_send_ppkts() which fails with ECONNREFUSED as errno).
Also make the callers qc_dgrams_retransmit() stop their packet process. This
is the case of quic_conn_app_io_cb() and quic_conn_io_cb().
This modifications stops definitively any packet processing when receiving
ICMP port unreachable errors.
MINOR: quic: Move code to wakeup the timer task to avoid anti-amplication deadlock
This code was there because the timer task was not running on the same thread
as the one which parse the QUIC packets. Now that this is no more the case,
we can wake up this task directly.
MINOR: quic: Add new traces about by connection RX buffer handling
Move quic_rx_pkts_del() out of quic_conn.h to make it benefit from the TRACE API.
Add traces which already already helped in diagnosing an issue encountered with
ngtcp2 which sent too much 1RTT packets before the handshake completion. This
has been fixed here after having discussed with Tasuhiro on QUIC dev slack:
BUG/MINOR: quic: Possible unexpected counter incrementation on send*() errors
Some counters could potentially be incremented even if send*() syscall returned
no error when ret >= 0 and ret != sz. This could be the case for instance if
a first call to send*() returned -1 with errno set to EINTR (or any previous syscall
which set errno to a non-null value) and if the next call to send*() returned
something positive and smaller than <sz>.
Amaury Denoyelle [Fri, 17 Feb 2023 14:56:06 +0000 (15:56 +0100)]
MINOR: h3: add traces on decode_qcs callback
Add traces inside h3_decode_qcs(). Every error path has now its
dedicated trace which should simplify debugging. Each early returns has
been converted to a goto invocation.
To complete the demux tracing, demux frame type and length are now
printed using the h3s instance whenever its possible on trace
invocation. A new internal value H3_FT_UNINIT is used as a frame type to
mark demuxing as inactive.
BUG/MINOR: mworker: prevent incorrect values in uptime
Since the recent changes on the clocks, now.tv_sec is not to be used
between processes because it's a clock which is local to the process and
does not contain a real unix timestamp. This patch fixes the issue by
using "data.tv_sec" which is the wall clock instead of "now.tv_sec'.
It prevents having incoherent timestamps.
It also introduces some checks on negatives values in order to never
displays a netative value if it was computed from a wrong value set by a
previous haproxy version.
Amaury Denoyelle [Tue, 14 Feb 2023 14:36:36 +0000 (15:36 +0100)]
BUG/MINOR: mux-quic: transfer FIN on empty STREAM frame
Implement support for clients that emit the stream FIN with an empty
STREAM frame. For that, qcc_recv() offset comparison has been adjusted.
If offset has already been received but the FIN bit is now transmitted,
do not skip the rest of the function and call application layer
decode_qcs() callback.
Without this, streams will be kept open forever as HTX EOM is never
transfered to the upper stream layer.
This notably caused the interop multiplexing test to fail as unclosed
streams on haproxy side prevented the emission of new MAX_STREAMS frame
to the client.
This shoud be backported up to 2.6. It also relies on previous commit : 381d8137e31d941c9143a1dc8b5760d29f388fef
MINOR: h3/hq-interop: handle no data in decode_qcs() with FIN set
Amaury Denoyelle [Fri, 17 Feb 2023 08:51:20 +0000 (09:51 +0100)]
MINOR: h3/hq-interop: handle no data in decode_qcs() with FIN set
Properly handle a STREAM frame with no data but the FIN bit set at the
application layer. H3 and hq-interop decode_qcs() callback have been
adjusted to not return early in this case.
If the FIN bit is accepted, a HTX EOM must be inserted for the upper
stream layer. If the FIN is rejected because the stream cannot be
closed, a proper CONNECTION_CLOSE error will be triggered.
A new utility function qcs_http_handle_standalone_fin() has been
implemented in the qmux_http module. This allows to simply add the HTX
EOM on qcs HTX buffer. If the HTX buffer is empty, a EOT is first added
to ensure it will be transmitted above.
This commit will allow to properly handle FIN notify through an empty
STREAM frame. However, it is not sufficient as currently qcc_recv() skip
the decode_qcs() invocation when the offset is already received. This
will be fixed in the next commit.
This should be backported up to 2.6 along with the next patch.
Willy Tarreau [Fri, 17 Feb 2023 07:36:42 +0000 (08:36 +0100)]
MINOR: threads: add flags to know if a thread is started and/or running
Several times during debugging it has been difficult to find a way to
reliably indicate if a thread had been started and if it was still
running. It's really not easy because the elements we look at are not
necessarily reliable (e.g. harmless bit or idle bit might not reflect
what we think during a signal). And such notions can be subjective
anyway.
Here we define two thread flags, TH_FL_STARTED which is set as soon as
a thread enters run_thread_poll_loop() and drops the idle bit, and
another one, TH_FL_IN_LOOP, which is set when entering run_poll_loop()
and cleared when leaving it. This should help init/deinit code know
whether it's called from a non-initialized thread (i.e. tid must not
be trusted), or shared functions know if they're being called from a
running thread or from init/deinit code outside of the polling loop.
Willy Tarreau [Thu, 16 Feb 2023 08:19:21 +0000 (09:19 +0100)]
BUG/MEDIUM: sched: allow a bit more TASK_HEAVY to be processed when needed
As reported in github issue #1881, there are situations where an excess
of TLS handshakes can cause a livelock. What's happening is that normally
we process at most one TLS handshake per loop iteration to maintain the
latency low. This is done by tagging them with TASK_HEAVY, queuing these
tasklets in the TL_HEAVY queue. But if something slows down the loop, such
as a connect() call when no more ports are available, we could end up
processing no more than a few hundred or thousands handshakes per second.
If the llmit becomes lower than the rate of incoming handshakes, we will
accumulate them and at some point users will get impatient and give up or
retry. Then a new problem happens: the queue fills up with even more
handshake attempts, only one of which will be handled per iteration, so
we can end up processing only outdated handshakes at a low rate, with
basically nothing else in the queue. This can for example happen in
parallel with health checks that don't require incoming handshakes to
succeed to continue to cause some activity that could maintain the high
latency stuff active.
Here we're taking a slightly different approach. First, instead of always
allowing only one handshake per loop (and usually it's critical for
latency), we take the current situation into account:
- if configured with tune.sched.low-latency, the limit remains 1
- if there are other non-heavy tasks, we set the limit to 1 + one
per 1024 tasks, so that a heavily loaded queue of 4k handshakes
per thread will be able to drain them at ~4 per loops with a
limited impact on latency
- if there are no other tasks, the limit grows to 1 + one per 128
tasks, so that a heavily loaded queue of 4k handshakes per thread
will be able to drain them at ~32 per loop with still a very
limited impact on latency since only I/O will get delayed.
It was verified on a 56-core Xeon-8480 that this did not degrade the
latency; all requests remained below 1ms end-to-end in full close+
handshake, and even 500us under low-lat + busy-polling.
Willy Tarreau [Thu, 16 Feb 2023 08:07:00 +0000 (09:07 +0100)]
BUG/MINOR: sched: properly report long_rq when tasks remain in the queue
There's a per-thread "long_rq" counter that is used to indicate how
often we leave the scheduler with tasks still present in the run queue.
The purpose is to know when tune.runqueue-depth served to limit latency,
due to a large number of tasks being runnable at once.
However there's a bug there, it's not always set: if after the first
run, one heavy task was processed and later only heavy tasks remain,
we'll loop back to not_done_yet where we try to pick more tasks, but
none are eligible (since heavy ones have already run) so we directly
return without incrementing the counter. This is what causes ultra-low
values on long_rq during massive SSL handshakes, that are confusing
because they make one believe that tl_class_mask doesn't have the HEAVY
flag anymore. Let's just fix that by not returning from the middle of
the function.
Willy Tarreau [Fri, 17 Feb 2023 13:55:41 +0000 (14:55 +0100)]
BUG/MEDIUM: wdt: fix wrong thread being checked for sleeping
In 2.7, the method used to check for a sleeping thread changed with
commit e7475c8e7 ("MEDIUM: tasks/fd: replace sleeping_thread_mask with
a TH_FL_SLEEPING flag"). Previously there was a global sleeping mask
and now there is a flag per thread. The commit above partially broke
the watchdog by looking at the current thread's flags via th_ctx
instead of the reported thread's flags, and using an AND condition
instead of an OR to update and leave. This can cause a wrong thread
to be killed when the load is uneven. For example, when enabling
busy polling and sending traffic over a single connection, all
threads have their run time grow, and if the one receiving the
signal is also processing some traffic, it will not match the
sleeping/harmless condition and will set the stuck flag, then die
upon next invocation. While it's reproducible in tests, it's unlikely
to be met in field.
REGTESTS: Remove unsupported feature command in http_splicing.vtc
A feature command was added to detect if infinite forward is disabled to be
able to skip the script. Unfortunately, it is no supported to evaluate such
expression. Thus remove it. For now, reg-tests must not be executed with
"-dF" option.
MINOR: haproxy: Add an command option to disable data fast-forward
The -dF option can now be used to disable data fast-forward. It does the
same than the global option "tune.fast-forward off". Some reg-tests may rely
on this optim. To detect the feature and skip such script, the following
vtest command must be used:
MINOR: global: Add an option to disable the data fast-forward
The new global option "tune.fast-forward" can be set to "off" to disable the
data fast-forward. It is an debug option, thus it is internally marked as
experimental. The directive "expose-experimental-directives" must be set
first to use this one. By default, the data fast-forward is enable.
It could be usefull to force to wake the stream up when data are
received. To be sure, evreything works fine in this case. The data
fast-forward is an optim. It must work without it. But some code may rely on
the fact the stream will not be woken up. With this option, it is possible
to spot some hidden bugs.
BUG/MEDIUM: stconn: Don't rearm the read expiration date if EOI was reached
At the stream level, the read expiration date is unset if a shutr was
received but not if the end of input was reached. If we know no more data
are excpected, there is no reason to let the read expiration date armed,
except to respect clientfin/serverfin timeout on some circumstances.
This patch could slowly be backported as far as 2.2.
BUG/MEDIUM: http-ana: Detect closed SC on opposite side during body forwarding
During the payload forwarding, since the commit f2b02cfd9 ("MAJOR: http-ana:
Review error handling during HTTP payload forwarding"), when an error
occurred on one side, we don't rely anymore on a specific HTTP message state
to detect it on the other side. However, nothing was added to detect the
error. Thus, when this happens, a spinning loop may be experienced and an
abort because of the watchdog.
To fix the bug, we must detect the opposite side is closed by checking the
opposite SC state. Concretly, in http_end_request() and http_end_response(),
we wait for the other side iff the HTTP message state is lower to
HTTP_MSG_DONE (the message is not finished) and the SC state is not
SC_ST_CLO (the opposite side is not closed). In these function, we don't
care if there was an error on the opposite side. We only take care to detect
when we must stop waiting the other side.
This patch should fix the issue #2042. No backport needed.
BUG/MINOR: config: crt-list keywords mistaken for bind ssl keywords
This patch fixes an issue in the "-dK" keywords dumper, which was
mistakenly displaying the "crt-list" keywords for "bind ssl" keywords.
The patch fixes the issue by dumping the "crt-list" keywords in its own
section, and dumping the "bind" keywords which are in the "SSL" scope
with a "bind ssl" prefix.
This commit depends on the previous "MINOR: ssl: rename confusing
ssl_bind_kws" commit.
Must be backported in 2.6.
Diff of the `./haproxy -dKall -q -c -f /dev/null` output before and
after the patch in 2.8-dev4:
Willy Tarreau [Tue, 14 Feb 2023 15:55:17 +0000 (16:55 +0100)]
[RELEASE] Released version 2.8-dev4
Released version 2.8-dev4 with the following main changes :
- BUG/MINOR: stats: fix source buffer size for http dump
- BUG/MEDIUM: stats: fix resolvers dump
- BUG/MINOR: stats: fix ctx->field update in stats_dump_proxy_to_buffer()
- BUG/MINOR: stats: fix show stats field ctx for servers
- BUG/MINOR: stats: fix STAT_STARTED behavior with full htx
- MINOR: quic: Update version_information transport parameter to draft-14
- BUG/MINOR: stats: Prevent HTTP "other sessions" counter underflows
- BUG/MEDIUM: thread: fix extraneous shift in the thread_set parser
- BUG/MEDIUM: listener/thread: bypass shards setting on failed thread resolution
- BUG/MINOR: ssl/crt-list: warn when a line is malformated
- BUG/MEDIUM: stick-table: do not leave entries in end of window during purge
- BUG/MINOR: clock: do not mix wall-clock and monotonic time in uptime calculation
- BUG/MEDIUM: cache: use the correct time reference when comparing dates
- MEDIUM: clock: force internal time to wrap early after boot
- BUILD: ssl/ocsp: ssl_ocsp-t.h depends on ssl_sock-t.h
- MINOR: ssl/ocsp: add a function to check the OCSP update configuration
- MINOR: cfgparse/server: move (min/max)conn postparsing logic into dedicated function
- BUG/MINOR: server/add: ensure minconn/maxconn consistency when adding server
- BUG/MEDIUM: stconn: Schedule a shutw on shutr if data must be sent first
- BUG/MEDIUM: quic: fix crash when "option nolinger" is set in the frontend
- MINOR: quic: implement a basic "show quic" CLI handler
- MINOR: quic: display CIDs and state in "show quic"
- MINOR: quic: display socket info on "show quic"
- MINOR: quic: display infos about various encryption level on "show quic"
- MINOR: quic: display Tx stream info on "show quic"
- MINOR: quic: filter closing conn on "show quic"
- BUG/MINOR: quic: fix filtering of closing connections on "show quic"
- BUG/MEDIUM: stconn: Don't needlessly wake the stream on send during fast-forward
- BUG/MINOR: quic: fix type bug on "show quic" for 32-bits arch
- BUG/MINOR: mworker: fix uptime for master process
- BUG/MINOR: clock/stats: also use start_time not start_date in HTML info
- BUG/MEDIUM: stconn: stop to enable/disable reads from streams via si_update_rx
- BUG/MEDIUM: quic: Buffer overflow when looking through QUIC CLI keyword list
- DOC: proxy-protocol: fix wrong byte in provided example
- MINOR: ssl-ckch: Stop to test CF_WRITE_ERROR to commit CA/CRL file
- MINOR: bwlim: Remove useless test on CF_READ_ERROR to detect the last packet
- BUG/MINOR: http-ana: Fix condition to set LAST termination flag
- BUG/MINOR: mux-h1: Don't report an H1C error on client timeout
- BUG/MEDIUM: spoe: Don't set the default traget for the SPOE agent frontend
- BUG/MINOR: quic: Wrong datagram dispatch because of qc_check_dcid()
- BUG/CRITICAL: http: properly reject empty http header field names
Willy Tarreau [Thu, 9 Feb 2023 20:36:54 +0000 (21:36 +0100)]
BUG/CRITICAL: http: properly reject empty http header field names
The HTTP header parsers surprizingly accepts empty header field names,
and this is a leftover from the original code that was agnostic to this.
When muxes were introduced, for H2 first, the HPACK decompressor needed
to feed headers lists, and since empty header names were strictly
forbidden by the protocol, the lists of headers were purposely designed
to be terminated by an empty header field name (a principle that is
similar to H1's empty line termination). This principle was preserved
and generalized to other protocols migrated to muxes (H1/FCGI/H3 etc)
without anyone ever noticing that the H1 parser was still able to deliver
empty header field names to this list. In addition to this it turns out
that the HPACK decompressor, despite a comment in the code, may
successfully decompress an empty header field name, and this mistake
was propagated to the QPACK decompressor as well.
The impact is that an empty header field name may be used to truncate
the list of headers and thus make some headers disappear. While for
H2/H3 the impact is limited as haproxy sees a request with missing
headers, and headers are not used to delimit messages, in the case of
HTTP/1, the impact is significant because the presence (and sometimes
contents) of certain sensitive headers is detected during the parsing.
Thus, some of these headers may be seen, marked as present, their value
extracted, but never delivered to upper layers and obviously not
forwarded to the other side either. This can have for consequence that
certain important header fields such as Connection, Upgrade, Host,
Content-length, Transfer-Encoding etc are possibly seen as different
between what haproxy uses to parse/forward/route and what is observed
in http-request rules and of course, forwarded. One direct consequence
is that it is possible to exploit this property in HTTP/1 to make
affected versions of haproxy forward more data than is advertised on
the other side, and bypass some access controls or routing rules by
crafting extraneous requests. Note, however, that responses to such
requests will normally not be passed back to the client, but this can
still cause some harm.
This specific risk can be mostly worked around in configuration using
the following rule that will rely on the bug's impact to precisely
detect the inconsistency between the known body size and the one
expected to be advertised to the server (the rule works from 2.0 to
2.8-dev):
http-request deny if { fc_http_major 1 } !{ req.body_size 0 } !{ req.hdr(content-length) -m found } !{ req.hdr(transfer-encoding) -m found } !{ method CONNECT }
This will exclusively block such carefully crafted requests delivered
over HTTP/1. HTTP/2 and HTTP/3 do not need content-length, and a body
that arrives without being announced with a content-length will be
forwarded using transfer-encoding, hence will not cause discrepancies.
In HAProxy 2.0 in legacy mode ("no option http-use-htx"), this rule will
simply have no effect but will not cause trouble either.
A clean solution would consist in modifying the loops iterating over
these headers lists to check the header name's pointer instead of its
length (since both are zero at the end of the list), but this requires
to touch tens of places and it's very easy to miss one. Functions such
as htx_add_header(), htx_add_trailer(), htx_add_all_headers() would be
good starting points for such a possible future change.
Instead the current fix focuses on blocking empty headers where they
are first inserted, hence in the H1/HPACK/QPACK decoders. One benefit
of the current solution (for H1) is that it allows "show errors" to
report a precise diagnostic when facing such invalid HTTP/1 requests,
with the exact location of the problem and the originating address:
I want to address sincere and warm thanks for their great work to the
team composed of the following security researchers who found the issue
together and reported it: Bahruz Jabiyev, Anthony Gavazzi, and Engin
Kirda from Northeastern University, Kaan Onarlioglu from Akamai
Technologies, Adi Peleg and Harvey Tuch from Google. And kudos to Amaury
Denoyelle from HAProxy Technologies for spotting that the HPACK and
QPACK decoders would let this pass despite the comment explicitly
saying otherwise.
This fix must be backported as far as 2.0. The QPACK changes can be
dropped before 2.6. In 2.0 there is also the equivalent code for legacy
mode, which doesn't suffer from the list truncation, but it would better
be fixed regardless.
BUG/MEDIUM: spoe: Don't set the default traget for the SPOE agent frontend
The commit d5983cef8 ("MINOR: listener: remove the useless ->default_target
field") revealed a bug in the SPOE. No default-target must be defined for
the SPOE agent frontend. SPOE applets are used on the frontend side and a
TCP connection is established on the backend side.
Because of this bug, since the commit above, the stream target is set to the
SPOE applet instead of the backend connection, leading to a spinning loop on
the applet when it is released because are unable to close the backend side.
This patch should fix the issue #2040. It only affects the 2.8-DEV but to
avoid any future bug, it should be backported to all stable versions.
BUG/MINOR: mux-h1: Don't report an H1C error on client timeout
When a client timeout is reported by the H1 mux, it is not an error but an
abort. Thus, H1C_F_ERROR flag must not be set. It is espacially important to
not inhibit the send. Because of this bug, a 408-Request-time-out is
reported in logs but the error message is not sent to the client.
BUG/MINOR: http-ana: Fix condition to set LAST termination flag
We should not report LAST data in log if the response is in TUNNEL mode on
client close/timeout because there is no way to be sure it is the last
data. It means, it can only be reported in DONE, CLOSING or CLOSE states.
MINOR: bwlim: Remove useless test on CF_READ_ERROR to detect the last packet
There is already a test on CF_EOI and CF_SHUTR. The last one is always set
when a read error is reported. Thus there is no reason to check
CF_READ_ERROR.
MINOR: ssl-ckch: Stop to test CF_WRITE_ERROR to commit CA/CRL file
This change was performed on all applet I/O handlers but one was missed. In
the CLI I/O handler used to commit a CA/CRL file, we can remove the test on
CF_WRITE_ERROR because there is already a test on CF_SHUTW.
Willy Tarreau [Sun, 12 Feb 2023 08:26:48 +0000 (09:26 +0100)]
DOC: proxy-protocol: fix wrong byte in provided example
There was a mistake in the example of proxy-proto frame
provided, it cannot end with 0x02 but only 0x20 or 0x21
since the version is in the upper 4 bits and the lower ones
are 0 for LOCAL or 1 for PROXY, hence the example should be:
BUG/MEDIUM: quic: Buffer overflow when looking through QUIC CLI keyword list
This has been detected by libasan as follows:
=================================================================
==3170559==ERROR: AddressSanitizer: global-buffer-overflow on address 0x55cf77faad08 at pc 0x55cf77a87370 bp 0x7ffc01bdba70 sp 0x7ffc01bdba68
READ of size 8 at 0x55cf77faad08 thread T0
#0 0x55cf77a8736f in cli_find_kw src/cli.c:335
#1 0x55cf77a8a9bb in cli_parse_request src/cli.c:792
#2 0x55cf77a8c385 in cli_io_handler src/cli.c:1024
#3 0x55cf77d19ca1 in task_run_applet src/applet.c:245
#4 0x55cf77c0b6ba in run_tasks_from_lists src/task.c:634
#5 0x55cf77c0cf16 in process_runnable_tasks src/task.c:861
#6 0x55cf77b48425 in run_poll_loop src/haproxy.c:2934
#7 0x55cf77b491cf in run_thread_poll_loop src/haproxy.c:3127
#8 0x55cf77b4bef2 in main src/haproxy.c:3783
#9 0x7fb8b0693d09 in __libc_start_main ../csu/libc-start.c:308
#10 0x55cf7764f4c9 in _start (/home/flecaille/src/haproxy-untouched/haproxy+0x1914c9)
0x55cf77faad08 is located 0 bytes to the right of global variable 'cli_kws' defined in 'src/quic_conn.c:7834:27' (0x55cf77faaca0) of size 104
SUMMARY: AddressSanitizer: global-buffer-overflow src/cli.c:335 in cli_find_kw
Shadow bytes around the buggy address:
According to cli_find_kw() code and cli_kw_list struct definition, the second
member of this structure ->kw[] must be a null-terminated array.
Add a last element with default initializers to <cli_kws> global variable which
is impacted by this bug.
This bug arrived with this commit: 15c74702d MINOR: quic: implement a basic "show quic" CLI handler
Must be backported to 2.7 where this previous commit has been already
backported.
BUG/MEDIUM: stconn: stop to enable/disable reads from streams via si_update_rx
It is not really a bug because it does not fix any known issue. And it is
flagged as MEDIUM because it is sensitive. But if there are some extra calls
to process_stream(), it can be an issue because, in si_update_rx(), we may
disable reading for the SC when outgoing data are blocked in the input
channel. But it is not really the process_stream() job to take care of
that. This may block data receipt.
It is an old code, mainly here to avoid wakeup in loop on the stats
applet. Today, it seems useless and can lead to bugs. An endpoint is
responsible to block the SC if it waits for some room and the opposite
endpoint is responsible to unblock it when some data are sent. The stream
should not interfere on this part.
This patch could be backported to 2.7 after a period of observation. And it
should only be backported to lower versions if an issue is reported.
Willy Tarreau [Fri, 10 Feb 2023 15:53:35 +0000 (16:53 +0100)]
BUG/MINOR: clock/stats: also use start_time not start_date in HTML info
For an unknown reason in the change of uptime calculation for the HTML
page didn't make it to commit 6093ba47c ("BUG/MINOR: clock: do not mix
wall-clock and monotonic time in uptime calculation"). Let's address it
as well otherwise the stats page will display an incorrect uptime.
No backport needed unless the patch above is backported.
Amaury Denoyelle [Fri, 10 Feb 2023 14:25:45 +0000 (15:25 +0100)]
BUG/MINOR: mworker: fix uptime for master process
Uptime calculation for master process was incorrect as it used
<start_date> as its timestamp base time. Fix this by using the scheduler
time <start_time> for this.
The impact of this bug is minor as timestamp base time is only used for
"show proc" CLI output. it was highlighted by the following commit.
which caused a negative value to be displayed for the master process
uptime on "show proc" output.
Amaury Denoyelle [Fri, 10 Feb 2023 08:25:22 +0000 (09:25 +0100)]
BUG/MINOR: quic: fix type bug on "show quic" for 32-bits arch
Incorrect printf format specifier "%lu" was used on "show quic" handler
for uint64_t. This breaks build on 32-bits architecture. To fix this
portability issue, force an explicit cast to unsigned long long with
"%llu" specifier.
BUG/MEDIUM: stconn: Don't needlessly wake the stream on send during fast-forward
With a connection, when data are received, if these data are sent to the
opposite side because the fast-forwarding is possible, the stream may be
woken up on some conditions (at the end of sc_app_chk_snd_conn()):
* The channel is shut for write
* The SC is not in the "established" state
* The stream must explicitly be woken up on write and all data was sent
* The connection was just established.
A bug on the last condition was introduced with the commit d89884153
("MEDIUM: channel: Use CF_WRITE_EVENT instead of CF_WRITE_PARTIAL"). The
stream is now woken up on any write events.
This patch fixes this issue and restores the original behavior. No backport
is needed.
BUG/MINOR: quic: fix filtering of closing connections on "show quic"
Filtering of closing/draining connections on "show quic" was not
properly implemented. This causes the extra argument "all" to display
all connections to be without effect. This patch fixes this and restores
the output of all connections.
Reduce default "show quic" output by masking connection on
closing/draing state due to a CONNECTION_CLOSE emission/reception. These
connections can still be displayed using the special argument "all".
MINOR: quic: display Tx stream info on "show quic"
Complete "show quic" handler by displaying information about
quic_stream_desc entries. These structures are used to emit stream data
and store them until acknowledgment is received.
MINOR: quic: display infos about various encryption level on "show quic"
Complete "show quic" handler by displaying various information related
to each encryption level and packet number space. Most notably, ack
ranges and bytes in flight are present to help debug retransmission
issues.
Complete "show quic" handler by displaying information related to the
quic_conn owned socket. First, the FD is printed, followed by the
address of the local and remote endpoint.
MINOR: quic: display CIDs and state in "show quic"
Complete "show quic" handler. Source and destination CIDs are printed
for every connection. This is complete by a state info to reflect if
handshake is completed and if a CONNECTION_CLOSE has been emitted or
received and the allocation status of the attached MUX. Finally the idle
timer expiration is also printed.
MINOR: quic: implement a basic "show quic" CLI handler
Implement a basic "show quic" CLI handler. This command will be useful
to display various information on all the active QUIC frontend
connections.
This work is heavily inspired by "show sess". Most notably, a global
list of quic_conn has been introduced to be able to loop over them. This
list is stored per thread in ha_thread_ctx.
Also add three CLI handlers for "show quic" in order to allocate and
free the command context. The dump handler runs on thread isolation.
Each quic_conn is referenced using a back-ref to handle deletion during
handler yielding.
For the moment, only a list of raw quic_conn pointers is displayed. The
handler will be completed over time with more information as needed.
Willy Tarreau [Thu, 9 Feb 2023 16:53:41 +0000 (17:53 +0100)]
BUG/MEDIUM: quic: fix crash when "option nolinger" is set in the frontend
Commit 0aba11e9e ("MINOR: quic: remove unnecessary quic_session_accept()")
overlooked one problem, in session_accept_fd() at the end, there's a bunch
of FD-specific stuff that either sets up or resets the socket at the TCP
level. The tests are mostly performed for AF_INET/AF_INET6 families but
they're only for one part (i.e. to avoid setting up TCP options on UNIX
sockets). Other pieces continue to configure the socket regardless of its
family. All of this directly acts on the FD, which is not correct since
the FD is not valid here, it corresponds to the QUIC handle. The issue
is much more visible when "option nolinger" is enabled in the frontend,
because the access to fdatb[cfd].state immediately crashes on the first
connection, as can be seen in github issue #2030.
This patch bypasses this setup for FD-less connections, such as QUIC.
However some of them could definitely be relevant to the QUIC stack, or
even to UNIX sockets sometimes. A better long-term solution would consist
in implementing a setsockopt() equivalent at the protocol layer that would
be used to configure the socket, either the FD or the QUIC conn depending
on the case. Some of them would not always be implemented but that would
allow to unify all this code.
This fix must be backported everywhere the commit above is backported,
namely 2.6 and 2.7.
Thanks to github user @twomoses for the nicely detailed report.
BUG/MEDIUM: stconn: Schedule a shutw on shutr if data must be sent first
The commit 7f59d68fe ("BUG/MEDIIM: stconn: Flush output data before
forwarding close to write side") introduced a regression. When the read side
is closed, the close is not forwarded to the write side if there are some
pending outgoind data. The idea is to foward data first and the close the
write side. However, when fast-forwarding is enabled and last data block is
received with the read0, the close is never forwarded.
We cannot revert the commit above because it really fix an issue. However,
we can schedule the shutdown for write by setting CF_SHUTW_NOW flag on the
write side. Indeed, it is the purpose of this flag.
To not replicate ugly and hardly maintainable code block at different places
in stconn.c, an helper function is used. Thus, sc_cond_forward_shutw() must
be called to know if the close can be fowarded or not. It returns 1 if it is
possible. In this case, the caller is responsible to forward the close to
the write side. Otherwise, if the close cannot be forwarded, 0 is
returned. It happens when it should not be performed at all. Or when it
should only be delayed, waiting for the input channel to be flushed. In this
last case, the CF_SHUTW_NOW flag is set in the output channel.
This patch should fix the issue #2033. It must be backported with the commit
above, thus at least as far as 2.2.
projects / thirdparty / haproxy.git / log
