Commit Graph

476 Commits

Author SHA1 Message Date
Bryce Anderson
8f01259833 HpackDecoder treats invalid pseudo-headers as stream level errors
Motivation:

The HTTP/2 spec dictates that invalid pseudo-headers should cause the
request/response to be treated as malformed (8.1.2.1), and the recourse
for that is to treat the situation as a stream error of type
PROTOCOL_ERROR (8.1.2.6). However, we're treating them as a connection
error with the connection being immediately torn down and the HPACK
state potentially being corrupted.

Modifications:

The HpackDecoder now throws a StreamException for validation failures
and throwing is deffered until the end of of the decode phase to ensure
that the HPACK state isn't corrupted by returning early.

Result:

Behavior more closely aligned with the HTTP/2 spec.

Fixes #8043.
2018-06-26 13:53:14 +02:00
Bryce Anderson
8687e1eeed Don't fail the deregistration promise in Http2MultiplexCodec
Motivation:

We deviate from the AbstractChannel implementation on deregistration by
failing the provided promise if the channel is already deregistered. In
contrast, AbstractChannel will always set the promise to successfully
done.

Modification:

Change the
Http2MultiplexCodec.DefaultHttp2StreamChannel.Http2ChannelUnsafe to
always set the promise provided to deregister as done as is the
case in AbstractChannel.
2018-06-21 10:20:54 +02:00
Bryce Anderson
c7c8e6a3ec Defer channelInactive and channelUnregistered events in Http2MultiplexCodec (#8021)
Motivation:

There is an inconsistency between the order of events in the
StreamChannel implementation in Http2MultiplexCodec and other Channel
implementations that extend AbstractChannel where channelInactive and
channelUnregistered events are not performed 'later'. This can cause an
unexected order of events for ChannelHandler implementations that call
Channel.close() in response to some event.

Modification:

The Http2MultiplexCodec.DefaultHttp2StreamChannel.Http2ChannelUnsafe was
modified to bounce the deregistration and channelInactive events through
the parent channels EventLoop.

Result:

Stream events are now in the proper order.

Fixes #8018.
2018-06-15 08:03:37 +02:00
Bryce Anderson
400ca87334 Provide an API for controlling and h2c upgrade response stream in Http2MultiplexCodec (#7968)
Motivation:

Http2MultiplexCodec doesn't currently have an API for using the response
of a h2c upgrade request.

Modifications:

Add a new API to the Http2MultiplexCodecBuilder which allows for setting
an upgrade handler and wire it into the Http2MultiplexCodec
implementation.

Result:

When using the Http2MultiplexCodec with h2c upgrades the upgrade handler
will get added to the Http2StreamChannel which represents the
half-closed (local) response of stream 1. It is then up to the user to
manage the transition from the IO channel pipeline configuration
necessary for making the h2c upgrade request to a form where it can read
the response from the new stream channel.

Fixes #7947.
2018-06-07 16:01:41 -07:00
Bryce Anderson
abe77511b9 Remove dead code in Http2CodecUtil (#8009)
Motivation:

The `ByteBuffer emptyPingBuf()` method of Http2CodecUtils is has been dead
code since DefaultHttp2PingFrame switched from using a ByteBuf to represent
the 8 octets to a long.

Modifications:

Remove the method and the unused static ByteBuf.

Result:

Less dead code.

Fixes #8002
2018-06-07 15:53:21 -07:00
Norman Maurer
f904c63a53
Correctly let Http2UnkownFrame extend HttpStreamFrame and so be usable with Http2MultiplexCodec. (#7976)
Motivation:

This is a followup for #7860. In the fix for #7860 we only partly fixed the problem as Http2UnknownFrame did not correctly extend HttpStreamFrame and so only worked when using the Http2FrameCodec. We need to have it extend HttpStreamFrame as otherwise Http2MultiplexCodec will reject to handle it correctly.

Modifications:

- Let Http2UnknownFrame extend HttpStreamFrame
- Add unit tests for writing and reading Http2UnkownFrame instances when the Http2MultiplexCodec is used.

Result:

Fixes https://github.com/netty/netty/issues/7969.
2018-05-29 07:27:40 +02:00
Eric Anderson
88f0586a7e Remove HpackDecoder.maxHeaderListSizeGoAway (#7911)
Motivation:

When a sender sends too large of headers it should not unnecessarily
kill the connection, as killing the connection is a heavy-handed
solution while SETTINGS_MAX_HEADER_LIST_SIZE is advisory and may be
ignored.

The maxHeaderListSizeGoAway limit in HpackDecoder is unnecessary because
any headers causing the list to exceeding the max size can simply be
thrown away. In addition, DefaultHttp2FrameReader.HeadersBlockBuilder
limits the entire block to maxHeaderListSizeGoAway. Thus individual
literals are limited to maxHeaderListSizeGoAway.

(Technically, literals are limited to 1.6x maxHeaderListSizeGoAway,
since the canonical Huffman code has a maximum compression ratio of
.625. However, the "unnecessary" limit in HpackDecoder was also being
applied to compressed sizes.)

Modifications:

Remove maxHeaderListSizeGoAway checking in HpackDecoder and instead
eagerly throw away any headers causing the list to exceed
maxHeaderListSize.

Result:

Fewer large header cases will trigger connection-killing.
DefaultHttp2FrameReader.HeadersBlockBuilder will still kill the
connection when maxHeaderListSizeGoAway is exceeded, however.

Fixes #7887
2018-05-19 08:31:59 +02:00
Nick Hill
30a02b441a Avoid implicit allocations in Http2FrameLogger when logging is disabled (#7937)
Motivation:

Integer autoboxing in this class (and possibly also the varargs arrays)
showed non-negligible CPU and garbage contribution when profiling a gRPC
service. grpc-java currently hardcodes use of Http2FrameLogger, set at
DEBUG level.

Modifications:

Wrap offending log statements in conditional blocks.

Result:

Garbage won't be produced by Http2FrameLogger when set to a disabled
logging level.
2018-05-15 09:06:32 +02:00
Norman Maurer
64bb279f47 [maven-release-plugin] prepare for next development iteration 2018-05-14 11:11:45 +00:00
Norman Maurer
c67a3b0507 [maven-release-plugin] prepare release netty-4.1.25.Final 2018-05-14 11:11:24 +00:00
Daniel Schobel
bd800fa7e7 Add null-check to Htt2FrameCodec#consumeBytes. (#7899)
Motivation:

Streams can be deregistered so we can't assume their existence in the stream map.

Modifications:

Add a null-check in case a stream has been deregistered.

Result:

Fixes #7898.
2018-05-02 13:01:26 +02:00
Norman Maurer
dfeb4b15b5
Fix IllegalReferenceCountException when using Http2MultiplexCodec and a DefaultHttp2GoAwayFrame with a non empty ByteBuffer is received. (#7894)
Motivation:

We incorrectly called frame.release() in onHttp2GoAwayFrame which could lead to IllegalReferenceCountExceptions.  The call of release() is inappropriate because the fireChannelRead() in onHttp2Frame() will handle it.

Modifications:

- Not call frame.release()
- Add a unit test

Result:

Fxies https://github.com/netty/netty/issues/7892.
2018-04-28 22:03:49 +02:00
Bryce Anderson
f9604eeff5 Motivation: (#7848)
It is possible to create streams in the half-closed state where the
stream state doesn't reflect that the request headers have been sent by
the client or the server hasn't received the request headers. This
state isn't possible in the H2 spec as a half closed stream must have
either received a full request or have received the headers from a
pushed stream. In the current implementation, this can cause the stream
created as part of an h2c upgrade request to be in this invalid state
and result in the omission of RST frames as the client doesn't believe
it has sent the request to begin with.

Modification:

The `DefaultHttp2Connection.activate` method checks the state and
modifies the status of the request headers as appropriate.

Result:

Fixes #7847.
2018-04-21 08:23:15 +02:00
Norman Maurer
b75f44db9a [maven-release-plugin] prepare for next development iteration 2018-04-19 11:56:07 +00:00
Norman Maurer
04fac00c8c [maven-release-plugin] prepare release netty-4.1.24.Final 2018-04-19 11:55:47 +00:00
JLofgren
a4146b706c Do not enforce arbitrary max header list size in HpackEncoder (#7853)
Motivation:

When connecting to an HTTP/2 server that did not set any value for the
SETTINGS_MAX_HEADER_LIST_SIZE in the settings frame, the netty client was
imposing an arbitrary maximum header list size of 8kB. There should be no need
for the client to enforce such a limit if the server has not specified any
limit. This caused an issue for a grpc-java client that needed to send a large
header to a server via an Envoy proxy server. The error condition is
demonstrated here: https://github.com/JLofgren/demo-grpc-java-bug-4284

Fixes grpc-java issue #4284 - https://github.com/grpc/grpc-java/issues/4284
and netty issue #7825 - https://github.com/netty/netty/issues/7825

Modifications:

In HpackEncoder use MAX_HEADER_LIST_SIZE as default maxHeader list size.

Result:

HpackEncoder will only enforce a max header list size if the server has
specified a limit in its settings frame.
2018-04-16 14:27:36 -07:00
Norman Maurer
da2e91b33a
Allow to write Http2UnkownFrame when using Http2FrameCodec / Http2MultiplexCodec (#7867)
Motivation:

We should allow to write Http2UnkownFrame to allow custom extensions.

Modifications:

Allow to write Http2UnkownFrame
Add unit test

Result:

Fixes https://github.com/netty/netty/issues/7860.
2018-04-13 07:50:44 +02:00
root
0a61f055f5 [maven-release-plugin] prepare for next development iteration 2018-04-04 10:44:46 +00:00
root
8c549bad38 [maven-release-plugin] prepare release netty-4.1.23.Final 2018-04-04 10:44:15 +00:00
Bryce Anderson
b309271e49 HttpServerUpgradeHandler shouldn't wait for flush to reshape pipeline
Motivation:

There is a race between both flushing the upgrade response and receiving
more data before the flush ChannelPromise can fire and reshape the
pipeline. Since We have already committed to an upgrade by writing the
upgrade response, we need to be immediately prepared for handling the
next protocol.

Modifications:

The pipeline reshaping logic in HttpServerUpgradeHandler has been moved
out of the ChannelFutureListener attached to the write of the upgrade
response and happens immediately after the writeAndFlush call, but
before the method returns.

Result:

The pipeline is no longer subject to receiving more data before the
pipeline has been reformed.
2018-03-28 19:54:30 +02:00
Norman Maurer
2c90b6235d Correctly include the stream id when convert from Http2HeadersFrame to HttpMessage
Motivation:

We did not correctly set the stream id in the headers of HttpMessage when converting a Http2HeadersFrame. This is based on https://github.com/netty/netty/pull/7778 so thanks to @jprante.

Modifications:

- Correctly set the id when possible in the header.
- Add test case

Result:

Correctly include stream id.
2018-03-17 09:46:01 +01:00
Norman Maurer
69582c0b6c [maven-release-plugin] prepare for next development iteration 2018-02-21 12:52:33 +00:00
Norman Maurer
786f35c6c9 [maven-release-plugin] prepare release netty-4.1.22.Final 2018-02-21 12:52:19 +00:00
Shohei Kamimori
73f23c5faa Fix typos in docs.
Motivation:

There are same typos in the docs.

Modifications:

Fix typos. Docs only changing.

Result:

More correct docs.
2018-02-14 08:44:07 +01:00
shorea
650406c0a3 Http2MultiplexCodec now propagates SETTINGS and GOAWAY frames in pipeline.
Motivation:

Allow the observation of SETTINGS frame by other handlers in the pipeline. For my particular use case this allows me to observe the value of MAX_CONCURRENT_STREAMS for a ChannelPool abstraction that supports HTTP/2 multiplexing. Beside this also forward GOAWAY frames.

Modification:

Always forward SETTINGS and GOAWAY frames

Result:

Settings / Goaway can now be observed in the parent channel. Previously it was not possible (to my knowledge) to capture the settings when using Http2MultiplexCodec.
2018-02-14 08:39:01 +01:00
Norman Maurer
501662a77f Use long for http2 ping payload.
Motivation:

At the moment we use a ByteBuf as the payload for a http2 frame. This complicates life-time management a lot with no real gain and also may produce more objects then needed. We should just use a long as it is required to be 8 bytes anyway.

Modifications:

Use long for ping payloads.

Result:

Fixes [#7629].
2018-02-08 10:23:34 +01:00
Norman Maurer
e71fa1e7b6 [maven-release-plugin] prepare for next development iteration 2018-02-05 12:02:35 +00:00
Norman Maurer
41ebb5fcca [maven-release-plugin] prepare release netty-4.1.21.Final 2018-02-05 12:02:19 +00:00
Norman Maurer
e72c197aa3 Reflective setAccessible(true) will produce scary warnings on the console when using java9+, dont do it
Motivation:

Reflective setAccessible(true) will produce scary warnings on the console when using java9+, while netty still works. That said users may feel uncomfortable with these warnings, we should not try to do it by default when using java9+.

Modifications:

Add io.netty.tryReflectionSetAccessible  system property which controls if setAccessible(...) will be used. By default it will bet set to false when using java9+.

Result:

Fixes [#7254].
2018-01-30 12:18:34 +01:00
Julien Hoarau
3e6b54bb59 Fix failing h2spec tests 8.1.2.1 related to pseudo-headers validation
Motivation:

According to the spec:
All pseudo-header fields MUST appear in the header block before regular
header fields. Any request or response that contains a pseudo-header
field that appears in a header block after
a regular header field MUST be treated as malformed (Section 8.1.2.6).

Pseudo-header fields are only valid in the context in which they are defined.
Pseudo-header fields defined for requests MUST NOT appear in responses;
pseudo-header fields defined for responses MUST NOT appear in requests.
Pseudo-header fields MUST NOT appear in trailers.
Endpoints MUST treat a request or response that contains undefined or
invalid pseudo-header fields as malformed (Section 8.1.2.6).

Clients MUST NOT accept a malformed response. Note that these requirements
are intended to protect against several types of common attacks against HTTP;
they are deliberately strict because being permissive can expose
implementations to these vulnerabilities.

Modifications:

- Introduce validation in HPackDecoder

Result:

- Requests with unknown pseudo-field headers are rejected
- Requests with containing response specific pseudo-headers are rejected
- Requests where pseudo-header appear after regular header are rejected
- h2spec 8.1.2.1 pass
2018-01-29 19:42:56 -08:00
Norman Maurer
c795e8897b Convert Http2Error.STREAM_CLOSED to ClosedChannelException when using child channels
Motivation:

We should convert Http2Exceptions that are produced because of STREAM_CLOSED to ClosedChannelException when hand-over to the child channel to make it more consistent with other transports.

Modifications:

- Check if STREAM_CLOSED is used and if so create a new ClosedChannelException (while preserve the original exception as cause) and use it in the child channel
- Ensure STREAM_CLOSED is used in DefaultHttp2RemoteFlowController when writes are failed because of a closed stream.
- Add testcase

Result:

More consistent and correct exception usage.
2018-01-29 17:50:29 -08:00
Norman Maurer
6e6edb59e7 Remove unused variable in DefaultHttp2StreamChannel
Motivation:

We should remove unused variable (was never read).

Modifications:

Remove unused variable (was never read).

Result:

Cleanup.
2018-01-29 11:32:22 +01:00
Thomas Segismont
7f23c34b55 ReadOnlyHttp2Headers.contains always ignores case for values
Motivation:

When checking if a value is present, ReadOnlyHttp2Headers always ignores
case for values.

RFC 7540 says: https://tools.ietf.org/html/rfc7540#section-8.1.2
"header field names are strings of ASCII characters that are compared in a case-insensitive fashion"

But there is no such constraint on header values

Modifications:

Updated ReadOnlyHttp2Headers.contains to compare header value in a
case-sensitive way.

Result:

ReadOnlyHttp2Headers compares header names in a case-insensitive way,
values in a case-sensitive way.
2018-01-27 20:29:40 +01:00
Thomas Segismont
0db652277f DefaultHttp2Headers#contains(CharSequence, CharSequence) does not work with String
Motivation:

If you test a header value providing a String, contains() returns false.
This is due to the implementation inherited from DefaultHeaders using
the JAVA_HASHER.

JAVA_HASHER.equals returns false because a is a String and b an
AsciiString.

Modifications:

DefaultHttp2Headers overrides contains and uses CASE_SENSITIVE_HASHER.

Result:

You can test a header value with any CharSequence implementation.
2018-01-27 20:27:50 +01:00
Scott Mitchell
614b9e0f25 Add tests for Http2MultiplexChannel close promise completion consistency with AbstractChannel
Motivation:
The completion order of promises in Http2MultiplexChannel#close should be consistent with that of AbstractChannel. Otherwise this may result in Future listeners seeing incorrect channel state.

Modifications:
Add tests cases.

Result:
Ensure consistent behavior between Http2MultiplexChannel and AbstractChannel.
2018-01-27 08:57:17 +01:00
Norman Maurer
b423a35783 Correctly handle multiple calls to DefaultHttp2StreamChannel.Unsafe.close(...)
Motivation:

Calling DefaultHttp2StreamChannel.Unsafe.close(...) multiple times should not fail.

Modification:

- Correctly handle multiple calls to DefaultHttp2StreamChannel.Unsafe.close(...)
- Complete closePromise and promise that is given to close(...) in the correct order.
- Add unit test

Result:

Fixes [#7628] and [#7641]
2018-01-27 08:55:35 +01:00
Norman Maurer
b1695fe17d Ensure async failures are correctly propagated to Http2LifecycleManager.onError(...) in all cases.
Motivation:

If DefaultHttp2ConnectionEncoder process outbound operation it sometimes missed to call Http2LifecycleManager.onError(...) when the operation was executed asynchronously.

Modifications:

Make best effort to update flags but still ensure failures are propageted to Http2LifecycleManager.onError(...) in all cases.

Result:

More consistent handling of errors.
2018-01-27 08:46:23 +01:00
Norman Maurer
c43dc3364b Cleanup Http2MultiplexCodec by removing out-dated TODO
Motivation:

Http2MultiplexCodec contains some TODO that is outdated.

Modifications:

Remove TODO which is outdated

Result:

Cleaner code.
2018-01-27 08:42:17 +01:00
Carl Mastrangelo
9ba942e59b Swap header check in ReadOnlyHttp2Headers
Motivation:
Pseudo headers are checked less frequently than normal headers, so
it is more efficient to check the latter first.

Modifications:
Swap the order of the check, and fix minor formatting

Result:
Possibly more efficient header checks
2018-01-26 19:01:02 -08:00
Norman Maurer
e975c5f6fe Reduce objects by directly implement interface in internal implementations of DefaultHttp2RemoteFlowController
Motivation:

We can just implement the interfaces directly and so reduce object creation in DefaultHttp2RemoteFlowController.

Modifications:

Directly implement the interfaces.

Result:

Less object creation.
2018-01-26 18:33:58 -08:00
Norman Maurer
49d1db4113 Http2MultiplexCodec.DefaultHttpStreamChannel.isOpen() / isActive() shoule be false when fireChannelActive() is called
Motivation:

When part of a HTTP/2 StreamChannel the Http2StreamChannel.isOpen() / isActive() should report false within a call to a ChannelInboundHandlers channelInactive() method.

Modifications:

Fullfill promise before call fireChannelInactive()

Result:

Correctly update state / promise before notify handlers. Fixes [#7638]
2018-01-26 17:34:37 -08:00
Thomas Segismont
bed74d8380 Method to check if a Http2 header is present and has a given value
Motivation:

With HTTP1, it's very easy to check if a header is present and has a
given value: you can simply invoke
io.netty.handler.codec.http.HttpHeaders#contains(java.lang.CharSequence, java.lang.CharSequence, boolean)

It is not possible to do the same with HTTP2. You have to get the list
of all headers (returned as String) and then iterate over it invoking
String#equals or String#equalsIgnoreCase

Modifications:

I've added io.netty.handler.codec.http2.Http2Headers#contains and
implemented it in DefaultHttp2Headers, EmptyHttp2Headers and ReadOnlyHttp2Headers.

Result:

You can use AsciiString constants to check if a header is present in a
consice and efficient manner.
2018-01-26 08:33:49 -08:00
Norman Maurer
1df5b02fd9 Do not fire outbound exception throught the pipeline when using Http2FrameCodec / Http2MultiplexCodec
Motivation:

Usually when using netty exceptions which happen for outbound operations should not be fired through the pipeline but only the ChannelPromise should be failed.

Modifications:

- Change Http2LifecycleManager.onError(...) to take also an boolean that indicate if the error was caused by an outbound operation
- Channel Http2ConnectionHandler.on*Error(...) methods to also take this boolean
- Change Http2FrameCodec to only fire exceptions through the pipeline if these are not outbound operations related
- Add unit test.

Result:

More consistent error handling when using Http2FrameCodec and Http2MultiplexCodec.
2018-01-25 13:42:28 -08:00
Bryce Anderson
8a095d0244 Http2MultiplexCodec should propagate unhandled Http2Frames down the pipeline
Motivation:

Http2MultiplexCodec swallows Http2PingFrames without releasing the payload, resulting in a memory leak.

Modification:

Send unhandled frames down the pipeline for consumption/disposal by another InboundChannelHandler.

Result:

Fixes #7607.
2018-01-25 13:26:45 -08:00
Scott Mitchell
4921f62c8a
HttpResponseStatus object allocation reduction
Motivation:
Usages of HttpResponseStatus may result in more object allocation then necessary due to not looking for cached objects and the AsciiString parsing method not being used due to CharSequence method being used instead.

Modifications:
- HttpResponseDecoder should attempt to get the HttpResponseStatus from cache instead of allocating a new object
- HttpResponseStatus#parseLine(CharSequence) should check if the type is AsciiString and redirect to the AsciiString parsing method which may not require an additional toString call
- HttpResponseStatus#parseLine(AsciiString) can be optimized and doesn't require and may not require object allocation

Result:
Less allocations when dealing with HttpResponseStatus.
2018-01-24 22:01:52 -08:00
Norman Maurer
b769ec0934 Reduce overhead of cancel flowcontrolled writes.
Motivation:

When we cancel the flowcontrolled writes we did create a new StreamException for each write that was enqueued. Creating Exceptions is very expensive due of filling the stacktrace.

Modifications:

Only create the StreamException once and reuse the same for all the flowcontrolled writes (per stream).

Result:

Less expensive to cancel flowcontrolled writes.
2018-01-24 19:42:13 +01:00
Scott Mitchell
46dac128f7
Http2FrameCodec WindowUpdate bug
Motivation:
Http2FrameCodec increases the initialWindowSize when the user attempts to increase the connection flow control window. The initialWindowSize should only be touched as a result of a SETTINGS frame, and otherwise may result in flow control getting out of sync with our peer.

Modifications:
- Http2FrameCodec shouldn't update the initialWindowSize when a WindowUpdateFrame is written on the connection channel

Result:
More correct WindowUpdate processing.
2018-01-22 11:17:56 -08:00
Norman Maurer
ea58dc7ac7 [maven-release-plugin] prepare for next development iteration 2018-01-21 12:53:51 +00:00
Norman Maurer
96c7132dee [maven-release-plugin] prepare release netty-4.1.20.Final 2018-01-21 12:53:34 +00:00
Scott Mitchell
322a062185 HpackDecoderTest cleanup
Motivation:
public ExpectedException fields should be final.
setUp method signature has a throws, but it isn't necessary
2018-01-19 17:26:06 +01:00