A subtle bug was introduced with H2 on the backend. RFC7540 states that
an attempt to create a stream on an ID not higher than the max known is
a connection error. This was translated into rejecting HEADERS frames
for closed streams. But with H2 on the backend, if the client aborts
and causes an RST_STREAM to be emitted, the stream is effectively closed,
and if/once the server responds, it starts by emitting a HEADERS frame
with this ID thus it is interpreted as a connection error.
This test must of course consider the side the mux is installed on and
not take this for a connection error on responses.
The effect is that an aborted stream on an outgoing H2 connection, for
example due to a client stopping a transfer with option abortonclose
set, would lead to an abort of all other streams. In the logs, this
appears as one or several CD-- line(s) followed by one or several SD--
lines which are victims.
Thanks to Luke Seelenbinder for reporting this problem and providing
enough elements to help understanding how to reproduce it.
This fix must be backported to 1.9.
A new warning appears when building at -O0 since commit 3f0fb9df6 ("MINOR:
peers: move "hello" message treatment code to reduce the size of the I/O
handler."), it is related to the fact that proto_len is initialized from
strlen() which is not a constant. Let's replace it with sizeof-1 instead
and also mark the variable as static since it's useless outside of the file.
The error handling code was extremely repetitive and error-prone due
to the numerous copy-pastes, some involving unlocks or free. Let's
factor this out. The code could be further simplified, but 12 locations
were already cleaned without taking risks.
Implements two new functions to init peer flags and other stuff after
having accepted or connected them with the peer I/O handler so that
to reduce its size.
May be backported as far as 1.5.
This patch implements three functions to read and parse the three
line of a "hello" peer protocol message so that to call them from the
peer I/O handler and reduce its size.
May be backported as far as 1.5.
When implementing peer_recv_msg() we added the statements reached with
a "goto imcomplete" at the end of this function. This statements
are executed only when co_getblk() returns something <0. So they
are useless for now on, and may be safely removed. The following
section wich was responsible of sending any peer protocol messages
were reached only when co_getblk() returned 0 (no more message to
read). In this case we replace the "goto impcomplete" statement by
a "goto send_msgs" to reach this only when peer_recv_msg() returns 0.
May be backported as far as 1.5.
This patch extracts the code responsible of sending peer protocol
messages from the peer I/O handler to create a new function and to
reduce the size of this handler.
May be backported as far as 1.5.
Extract the code of the peer I/O handler responsible of treating
any peer protocol message to create peer_treat_awaited_msg() function.
Also rename peer_recv_updatemsg() to peer_treat_updatemsg() as this
function only parse a stick-table update message already received
by peer_recv_msg().
May be backported as far as 1.5.
Implement three new functions to treat peer acks, switch and
definition messages extracting the code from the big swich-case
of the peer I/O handler to give more chances to this latter to be
readable.
May be backported as far as 1.5.
This patch implements a new function to treat the stick-table
update messages so that to reduce the size of the peer I/O handler
by ~200 lines.
May be backported as far as 1.5.
This patch reduces the size of the peer I/O handler implementing
a new function named peer_send_updatemsg() which uses the already
implement peer_prepare_updatemsg(), then ci_putblk().
Reuse the code used to implement peer_send_(ack|swith)msg() function
especially the more generic function peer_send_msg().
May be backported as far as 1.5.
Implements peer_send_*msg() functions for switch and ack messages which call the
already defined peer_prepare_*msg() before calling ci_putblk().
These two new functions are used at three places in the peer_io_handler().
May be backported as far as 1.5.
In case an asynchronous connection (ALPN) succeeds but the mux fails to
attach, we must release the stream interface's endpoint, otherwise we
leave the stream interface with an endpoint pointing to a freed connection
with si_ops == si_conn_ops, and sess_update_st_cer() calls si_shutw() on
it, causing it to crash.
This must be backported to 1.9 only.
In connect_server(), if the previous connection failed, but had an alpn, no
mux was created, and thus the stream_interface's endpoint would be the
connection. In this case, instead of forgetting about it, and overriding
the stream_interface's endpoint later, try to reuse the connection, or the
connection will still be in the session's connection list, and will reference
to a stream that was probably destroyed.
This should be backported to 1.9.
There's a very small but existing uncertainty window when waking another
thread up where it is possible for task_wakeup() not to wake the other
task up because it's still running while this once is in the process of
finishing and loses its TASK_RUNNING flag. In this case the wakeup will
be missed.
The problem is that we have a single flag to store 3 states, since the
transition from running to sleeping isn't atomic. Thus we need to have
another flag to cover this part. This patch introduces TASK_QUEUED to
mention that the task is already in the run queue, running or not. This
bit will be removed while TASK_RUNNING is kept once dequeued, and will
be used when removing TASK_RUNNING to check if the task has been requeued.
It might be possible to slightly improve this but the occurrence rate
is quite low and we don't really need to complexify the scheduler to
optimize for a rare case.
The impact with the current code is very low since we have few inter-
thread wakeups. Most of them are caused by checks killing sessions.
This must be backported to 1.9.
The calculation of available outgoing H2 streams was improved by commit
d64a3ebe6 ("BUG/MINOR: mux-h2: always check the stream ID limit in
h2_avail_streams()"), but it still is incorrect because RFC7540#6.8
specifically forbids the creation of new streams after a GOAWAY frame
was received. Thus we must not mark the connection as available anymore
in order to be able to handle a graceful shutdown.
This needs to be backported to 1.9.
The source address was not set but passed down the chain to the upper
layer's accept() calls. Let's initialize it like other UNIX sockets in
this case. At the moment it should not have any impact since socketpairs
are only usable for the master CLI.
This should be backported to 1.9.
It was mentioned when releasing 1.8 but early bugs have long been
addressed and this comment discourages some users from using threads.
This should be backported to 1.9 and 1.8 now.
There's some value in being able to limit MAX_THREADS, either to save
precious resources in embedded environments, or to protect certain
deployments against accidently incorrect settings.
With this patch, if MAX_THREADS is defined at build time, it will be
used. However, given that LONGBITS is not a macro but is defined
according to sizeof(long), we can't check the value range at build
time and instead we need to perform the check at early boot time.
However, the compiler is able to optimize away the constant comparisons
and doesn't even emit the check code when values are correct.
The output message regarding threading support was improved to report
the number of threads.
This is mandated by RFC7541#8.1.2.6. Till now we didn't have a copy of
the content-length header field. But now that it's already parsed, it's
easy to add the check.
The reg-test was updated to match the new behaviour as the previous one
expected unadvertised data to be silently discarded.
This should be backported to 1.9 along with previous patch (MEDIUM: h2:
always parse and deduplicate the content-length header) after it has got
a bit more exposure.
The header used to be parsed only in HTX but not in legacy. And even in
HTX mode, the value was dropped. Let's always parse it and report the
parsed value back so that we'll be able to store it in the streams.
When a connection reuse fails, we must not wait before retrying, as most
likely the issue is related to the reused connection and not to the server
itself.
This should be backported to 1.9, though it depends on previous patches
dealing with SI_ST_CON for connection reuse.
Before the first send() attempt, we should be in SI_ST_CON, not
SI_ST_EST, since we have not yet attempted to send and we are
allowed to retry. This is particularly important with complex
outgoing muxes which can fail during the first send attempt (e.g.
failed stream ID allocation).
It only requires that sess_update_st_con_tcp() knows about this
possibility, as we must not forcefully close a reused connection
when facing an error in this case, this will be handled later.
This may be backported to 1.9 with care after some observation period.
This parameter allows to limit the number of successive requests sent
on a connection. Let's compare it to the number of streams already sent
on the connection to decide if the connection may still appear in the
idle list or not. This may be used to help certain servers work around
resource leaks, and also helps dealing with the issue of the GOAWAY in
flight which requires to set a usage limit on the client to be reliable.
This must be backported to 1.9.
Some servers may wish to limit the total number of requests they execute
over a connection because some of their components might leak resources.
In HTTP/1 it was easy, they just had to emit a "connection: close" header
field with the last response. In HTTP/2, it's less easy because the info
is not always shared with the component dealing with the H2 protocol and
it could be harder to advertise a GOAWAY with a stream limit.
This patch provides a solution to this by adding a new "max-reuse" parameter
to the server keyword. This parameter indicates how many times an idle
connection may be reused for new requests. The information is made available
and the underlying muxes will be able to use it at will.
This patch should be backported to 1.9.
The code dealing with idle connections used to check the number of streams
available on the connection only to unlink the connection from the idle
list. But this still resulted in too many streams reusing the same connection
when they were already attached to it.
We must detect that there is no more room and refrain from using this
connection at all, and instead fall back to the no-reuse case. Ideally
we should try to search among other idle connections, but for a backport
let's stay safe.
This must be backported to 1.9.
One of the reasons for the excessive number of aborted requests when a
server sets a limit on the highest stream ID is that we don't check
this limit while allocating a new stream.
This patch does this at two locations :
- when a backend stream is allocated, we verify that there are still
IDs left ;
- when the ID is assigned, we verify that it's not higher than the
advertised limit.
This should be backported to 1.9.
This function is used to decide whether to put an idle connection back
into the idle pool. While it considers the limit in number of concurrent
requests, it does not consider the limit in number of streams, so if a
server announces a low limit in a GOAWAY frame, it will be ignored.
However there is a caveat : since we assign the stream IDs when sending
them, we have a number of allocated streams which max_id doesn't take
care of. This can be addressed by adding a new nb_reserved count on each
connection to keep track of the ID-less streams.
This patch makes sure we take care of the remaining number of streams
if such a limit was announced, or of the number of streams before the
highest ID. Now it is possible to accurately know how many streams
can be allocated, and the number of failed outgoing streams has dropped
in half.
This must be backported to 1.9.
We currently detect a number of situations where we have to immediately
deal with a state change, but we failed to consider the case of the
synchronous error reported on the stream-interface. We definitely do not
want to have to wait for a timeout to handle this one, especially at the
beginning of the connection when it can lead to an immediate retry.
This should be backported to 1.9.
The keyword parser doesn't check the value range, but supported values are
-1 and positive values, thus we should check it.
This can be backported to 1.9.
RFC7541#6.3 mandates that an error is reported when a dynamic table size
update announces a size larger than the one configured with settings. This
is tested by h2spec using test "hpack/6.3/1".
This must be backported to 1.9 and possibly 1.8 as well.
When sending RST_STREAM in response to a frame delivered on an already
closed stream, we used not to be able to update the error code and
deliver an RST_STREAM with a wrong code (e.g. H2_ERR_CANCEL). Let's
always allow to update the code so that RST_STREAM is always sent
with the appropriate error code (most often H2_ERR_STREAM_CLOSED).
This should be backported to 1.9 and possibly to 1.8.
There are incompatible MUST statements in the HTTP/2 specification. Some
require a stream error and others a connection error for the same situation.
As discussed in the thread below, let's always apply the connection error
when relevant (headers-like frame in half-closed(remote)) :
https://mailarchive.ietf.org/arch/msg/httpbisa/pOIWRBRBdQrw5TDHODZXp8iblcE
This must be backported to 1.9, possibly to 1.8 as well.
Since we now support CONTINUATION frames, we must take care of properly
aborting the connection when they are sent on a closed stream. By default
we'd get a stream error which is not sufficient since the compression
context is modified and unrecoverable.
More info in this discussion :
https://mailarchive.ietf.org/arch/msg/httpbisa/azZ1jiOkvM3xrpH4jX-Q72KoH00
This needs to be backported to 1.9 and possibly to 1.8 (less important there).
This patch adds H2_FT_HDR_MASK to group all frame types carrying headers
information, and H2_FT_LATE_MASK to group frame types allowed to arrive
after a stream was closed.
There was an incomplete test in h2c_frt_handle_headers() resulting
in negative return values from h2c_decode_headers() not being taken
as errors. The effect is that the stream is then aborted on timeout
only.
This fix must be backported to 1.9.
The current test consists in removing muxes which report that they're going
to assign their last available stream, but a mux may already be saturated
without having passed in this situation at all. This is what happens with
mux_h2 when receiving a GOAWAY frame informing the mux about the ID of the
last stream the other end is willing to process. The limit suddenly changes
from near infinite to 0. Currently what happens is that such a mux remains
in the idle list for a long time and refuses all new streams. Now at least
it will only fail a single stream in a retryable way. A future improvement
should consist in trying to pick another connection from the idle list.
This fix must be backported to 1.9.
In case we cannot allocate a stream ID for an outgoing stream, the stream
will be aborted. The problem is that we also release it and it will be
destroyed again by the application detecting the error, leading to a NULL
dereference in h2_shutr() and h2_shutw(). Let's only mark the error on the
CS and let the rest of the code handle the close.
This should be backported to 1.9.
