We used to respond a connection error in case we received a trailers
frame on a closed stream, but it's a problem to do this if the error
was caused by a reset because the sender has not yet received it and
is just a victim of the timing. Thus we must not close the connection
in this case.
This patch may be backported to 1.9 but then it requires the following
previous ones :
MINOR: h2: add a generic frame checker
MEDIUM: mux-h2: check the frame validity before considering the stream state
CLEANUP: mux-h2: remove stream ID and frame length checks from the frame parsers
It's not convenient to have such structural checks mixed with the ones
related to the stream state. Let's remove all these basic tests that are
already covered once for all when reading the frame header.
There are some uneasy situation where it's difficult to validate a frame's
format without being in an appropriate state. This patch makes sure that
each frame passes through h2_frame_check() before being checked in the
context of the stream's state. This makes sure we can always return a GOAWAY
for protocol violations even if we can't process the frame.
RFC7540#5.1 states that these are the only states allowing any frame
type. For response HEADERS frames, we cannot accept that they are
delivered on idle streams of course, so we're left with these two
states only. It is important to test this so that we can remove the
generic CLOSE_STREAM test for such frames in the main loop.
This must be backported to 1.9 (1.8 doesn't have response HEADERS).
If a response HEADERS frame arrives on a closed connection (due to a
client abort sending an RST_STREAM), it's currently immediately rejected
with an RST_STREAM, like any other frame. This is incorrect, as HEADERS
frames must first be decoded to keep the HPACK decoder synchronized,
possibly breaking subsequent responses.
This patch excludes HEADERS/CONTINUATION/PUSH_PROMISE frames from the
central closed state test and leaves to the respective frame parsers
the responsibility to decode the frame then send RST_STREAM.
This fix must be backported to 1.9. 1.8 is not directly impacted since
it doesn't have response HEADERS nor trailers thus cannot recover from
such situations anyway.
The H2 spec requires to send GOAWAY when the client sends a frame after
it has already closed using END_STREAM. Here the corresponding case was
the fallback of a series of tests on the stream state, but it unfortunately
also catches old closed streams which we don't know anymore. Thus any late
packet after we've sent an RST_STREAM will trigger this GOAWAY and break
other streams on the connection.
This can happen when launching two tabs in a browser targetting the same
slow page through an H2-to-H2 proxy, and pressing Escape to stop one of
them. The other one gets an error when the page finally responds (and it
generally retries), and the logs in the middle indicate SD-- flags since
the late response was cancelled.
This patch takes care to only send GOAWAY on streams we still know.
It must be backported to 1.9 and 1.8.
When receiving a HEADERS or DATA frame with END_STREAM set, we would
inconditionally switch to half-closed(remote). This is wrong because we
could already have been in half-closed(local) and need to switch to closed.
This happens in the following situations :
- receipt of the end of a client upload after we've already responded
(e.g. redirects to POST requests)
- receipt of a response on the backend side after we've already finished
sending the request (most common case).
This may possibly have caused some streams to stay longer than needed
at the end of a transfer, though this is not apparent in tests.
This must be backported to 1.9 and 1.8.
When a settings frame updates the initial window, all affected streams's
window is updated as well. However the streams are not put back into the
send list if they were already blocked on flow control. The effect is that
such a stream will only be woken up by a WINDOW_UPDATE message but not by
a SETTINGS changing the initial window size. This can be verified with
h2spec's test http2/6.9.2/1 which occasionally fails without this patch.
It is unclear whether this situation is really met in field, but the
fix is trivial, it consists in adding each unblocked streams to the
wait list as is done for the window updates.
This fix must be backported to 1.9. For 1.8 the patch needs quite
a few adaptations. It's better to copy-paste the code block from
h2c_handle_window_update() adding the stream to the send_list when
its mws is > 0.
The WINDOW_UPDATE and DATA frame handlers used to still have a check on
h2s to return either h2s_error() or h2c_error(). This is a leftover from
the early code. The h2s cannot be null there anymore as it has already
been dereferenced before reaching these locations.
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.
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.
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.
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.
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.
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).
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.
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.
When data are pushed in the channel's buffer, in h2_rcv_buf(), the mux-h2 must
respect the reserve if the flag CO_RFL_KEEP_RSV is set. In HTX, because the
stream-interface always sees the buffer as full, there is no other way to know
the reserve must be respected.
This patch must be backported to 1.9.
Tim Dsterhus reported a possible crash in the H2 HEADERS frame decoder
when the PRIORITY flag is present. A check is missing to ensure the 5
extra bytes needed with this flag are actually part of the frame. As per
RFC7540#4.2, let's return a connection error with code FRAME_SIZE_ERROR.
Many thanks to Tim for responsibly reporting this issue with a working
config and reproducer. This issue was assigned CVE-2018-20615.
This fix must be backported to 1.9 and 1.8.
Now the H2 mux will parse and encode the HTX trailers blocks and send
the corresponding HEADERS frame. Since these blocks contain pure H1
trailers which may be fragmented on line boundaries, if first needs
to collect all of them, parse them using the H1 parser, build a list
and finally encode all of them at once once the EOM is met. Note that
this HEADERS frame always carries the end-of-headers and end-of-stream
flags.
This was tested using the helloworld examples from the grpc project,
as well as with the h2c tools. It doesn't seem possible at the moment
to test tailers using varnishtest though.
We want to make sure we won't emit another empty DATA frame if we meet
HTX_BLK_EOM after and end of stream was already sent. For now it cannot
happen as far as HTX is respected, but with trailers it may become
ambiguous.
When forwarding an H2 request to an H1 server, if the request doesn't
have a content-length header field, it is chunked. In this case it is
possible to send trailers to the server, which is what this patch does.
If the transfer is performed without chunking, then the trailers are
silently discarded.
This is not exactly a bug but a long-time design limitation. We used not
to decode trailers in H2, resulting in broken connections each time a
trailer was sent, since it was impossible to keep the HPACK decompressor
synchronized. Now that the sequencing of operations permits it, we must
make sure to at least properly decode them.
What we try to do is to identify if a HEADERS frame was already seen and
use this indication to know if it's a headers or a trailers. For this,
h2c_decode_headers() checks if the stream indicates that a HEADERS frame
was already received. If so, it decodes it and emits the trailing
0 CRLF CRLF in case of H1, or the HTX_EOD + HTX_EOM blocks in case of HTX,
to terminate the data stream.
The trailers contents are still deleted for now but the request works, and
the connection remains synchronized and usable for subsequent streams.
The correctness may be tested using a simple config and h2spec :
h2spec -o 1000 -v -t -S -k -h 127.0.0.1 -p 4443 generic/4/4
This should definitely be backported to 1.9 given the low impact for the
benefit. However it cannot be backported to 1.8 since the operations cannot
be resumed. The following patches are also needed with this one :
MINOR: mux-h2: make h2c_decode_headers() return a status, not a count
MINOR: mux-h2: add a new dummy stream : h2_error_stream
MEDIUM: mux-h2: make h2c_decode_headers() support recoverable errors
BUG/MINOR: mux-h2: detect when the HTX EOM block cannot be added after headers
MINOR: mux-h2: check for too many streams only for idle streams
MINOR: mux-h2: set H2_SF_HEADERS_RCVD when a HEADERS frame was decoded
The HEADERS frame parser checks if we still have too many streams, but
this should only be done for idle streams, otherwise it would prevent
us from processing trailer frames.
In h2c_frt_handle_headers() and h2c_bck_handle_headers() we have an unused
error path made of the strm_err label, while send_rst is used to emit an
RST upon stream error after forcing the stream to h2_refused_stream. Let's
remove this unused strm_err block now.
In h2c_frt_handle_headers(), we test the stream for SS_ERROR just after
setting it to SS_OPEN, this makes no sense and creates confusion in the
error path. Remove this misleading test.
In case we receive a very large HEADERS frame which doesn't leave enough
room to place the EOM block after the decoded headers, we must fail the
stream. This test was missing, resulting in the loss of the EOM, possibly
leaving the stream waiting for a time-out.
Note that we also clear h2c->dfl here so that we don't attempt to clear
it twice when going back to the demux.
If this is backported to 1.9, it also requires that the following patches
are backported as well :
MINOR: mux-h2: make h2c_decode_headers() return a status, not a count
MINOR: mux-h2: add a new dummy stream : h2_error_stream
MEDIUM: mux-h2: make h2c_decode_headers() support recoverable errors
When a decoding error is recoverable, we should emit a stream error and
not a connection error. This patch does this by carefully checking the
connection state before deciding to send a connection error. If only the
stream is in error, an RST_STREAM is sent.
This function used to return a byte count for the output produced, or
zero on failure. Not only this value is not used differently than a
boolean, but it prevents us from returning stream errors when a frame
cannot be extracted because it's too large, or from parsing a frame
and producing nothing on output.
This patch modifies its API to return <0 on errors, 0 on inability to
proceed, or >0 on success, irrelevant to the amount of output data.
In h2c_decode_headers() we update the buffer's length according to the
amount of data produced (outlen). But in case of HTX this outlen value
is not a quantity, just an indicator of success, resulting in the buffer
being added one extra byte and temporarily showing .data > .size, which
is wrong. Fortunately this is overridden when leaving the function by
htx_to_buf() so the impact only exists in step-by-step debugging, but
it definitely needs to be fixed.
This must be backported to 1.9.
When dealing with a server's H2 response, we used to set the
end-of-stream flag on the conn_stream and the stream before parsing
the response, which is incorrect since we can fail to process this
response by lack of room, buffer or anything. The extend of this problem
is still limited to a few rare cases, but with trailers it will cause a
systematic failure.
This fix must be backported to 1.9.
This function handles response HEADERS frames, it is not responsible
for creating new streams thus it must not check if we've reached the
stream count limit, otherwise it could lead to some undesired pauses
which bring no benefit.
This must be backported to 1.9.
If we exit this function because some data are pending in the rxbuf, we
currently don't indicate any blocking flag, which will prevent the operation
from being attempted again. Let's set H2_CF_DEM_SFULL in this case to indicate
there's not enough room in the stream buffer so that the operation may be
attempted again once we make room. It seems that this issue cannot be
triggered right now but it definitely will with trailers.
This fix should be backported to 1.9 for completeness.
h2c_restart_reading() is used at various place to resume processing of
demux data, but this one refrains from doing so if the mux is already
subscribed for receiving. It just happens that even if some incoming
frame processing is interrupted, the mux is always subscribed for
receiving, so this condition alone is not enough, it must be combined
with the fact that the demux buffer is empty, otherwise some resume
events are lost. This typically happens when we refrain from processing
some incoming data due to missing room in the stream's rxbuf, and want
to resume in h2c_rcv_buf(). It will become even more visible with trailers
since these ones want to have an empty rxbuf before proceeding.
This must be backported to 1.9.
In h2c_decode_headers() we mistakenly check for H2_F_DATA_END_STREAM
while we should check for H2_F_HEADERS_END_STREAM. Both have the same
value (1) but better stick to the correct flag.
When a session adds a connection to its connection list, we used to remove
connections for an another server if there were not enough room for our
server. This can't work, because those lists are now the list of connections
we're responsible for, not just the idle connections.
To fix this, allow for an unlimited number of servers, instead of using
an array, we're now using a linked list.
In h2_detach(), don't add the connection to the idle list if nb_streams
is at the max. This can happen if we already closed that stream before, so
its slot became available and was used by another stream.
This should be backported to 1.9.
Now that the HEADERS frame decoding is retryable, we can safely try to
fold CONTINUATION frames into a HEADERS frame when the END_OF_HEADERS
flag is missing. In order to do this, h2c_decode_headers() moves the
frames payloads in-situ and leaves a hole that is plugged when leaving
the function. There is no limit to the number of CONTINUATION frames
handled this way provided that all of them fit into the buffer. The
error reported when meeting isolated CONTINUATION frames has now changed
from INTERNAL_ERROR to PROTOCOL_ERROR.
Now there is only one (unrelated) remaining failure in h2spec.
