Commit Graph

7921 Commits

Author SHA1 Message Date
chhsiao90
72916b9960 Add unit test on DefaultHttp2FrameReader
Motivation:

DefaultHttp2FrameReader contains the logic for how it parsed the network
traffic from http2 client,
it also validate the content is legal or not.
So keep high coverage rate on it will increase the stability of api.

Modifications:

Add unit test on DefaultHttp2FrameReader

Result:

Coverage rate increased
2017-02-06 23:57:03 -08:00
Norman Maurer
a7c0ff665c Only use Mockito for mocking.
Motivation:

We used various mocking frameworks. We should only use one...

Modifications:

Make usage of mocking framework consistent by only using Mockito.

Result:

Less dependencies and more consistent mocking usage.
2017-02-07 08:47:22 +01:00
Jason Tedor
42c0359820 Do not prefer empty MAC address
Motivation:

When comparing MAC addresses searching for the best MAC address, if
locally-administered address (e.g., from a Docker container) is compared
against an empty MAC address, the empty MAC address will be marked as
preferred. In cases this is the only available MAC address, this leaves
Netty using a random machine ID instead of using a perfectly valid
machine ID from the locally-adminstered address.

Modifications:

This commit modifies the MAC address logic so that the empty MAC address
is not preferred over a locally-administered address. This commit also
simplifies the comparison logic here.

Result:

Empty MAC addresses will not be preferred over locally-administered
addresses thus permitting the default machine ID to be the
locally-adminstered MAC address if it is the only available MAC address.
2017-02-06 12:14:27 -08:00
Norman Maurer
661ff2538e Implement correct handling of recursive DNS
Motivation:

DnsNameResolver does not handle recursive DNS and so fails if you query a DNS server (for example a ROOT dns server) which provides the correct redirect for a domain.

Modification:

Add support for redirects (a.k.a. handling of AUTHORITY section').

Result:

Its now possible to use a DNS server that redirects.
2017-02-06 20:33:52 +01:00
Norman Maurer
8a5e42ad2e Correct fail write with NotYetConnectedException when OioDatagramChannel is not connected yet.
Motivation:

NioDatagramChannel fails a write with NotYetConnectedException when the DatagramChannel was not yet connected and a ByteBuf is written. The same should be done for OioDatagramChannel as well.

Modifications:

Make OioDatagramChannel consistent with NioDatagramChannel

Result:

Correct and consistent implementations of DatagramChannel
2017-02-06 11:06:00 +01:00
Kiril Menshikov
66b9be3a46 Allow to allign allocated Buffers
Motivation:

64-byte alignment is recommended by the Intel performance guide (https://software.intel.com/en-us/articles/practical-intel-avx-optimization-on-2nd-generation-intel-core-processors) for data-structures over 64 bytes.
Requiring padding to a multiple of 64 bytes allows for using SIMD instructions consistently in loops without additional conditional checks. This should allow for simpler and more efficient code.

Modification:

At the moment cache alignment must be setup manually. But probably it might be taken from the system. The original code was introduced by @normanmaurer https://github.com/netty/netty/pull/4726/files

Result:

Buffer alignment works better than miss-align cache.
2017-02-06 07:58:29 +01:00
周岑
48f6541cb3 delete no useful intermediate variables
delete no useful intermediate variables
2017-02-06 07:55:29 +01:00
Norman Maurer
756b78b7df Add common tests for ByteBufAllocator / AbstractByteBufAllocator implementations.
Motivation:

We not had tests for ByteBufAllocator implementations in general.

Modifications:

Added ByteBufAllocatorTest, AbstractByteBufAllocatorTest and UnpooledByteBufAllocatorTest

Result:

More tests for allocator implementations.
2017-02-06 07:51:10 +01:00
Vladimir Kostyukov
0f9b739508 AggregatedFullHttpMessage.replace should also copy a decoder result
Motivation:

HttpObjectAggregator yields full HTTP messgaes (AggregatedFullHttpMessages) that don't respect decoder result when copied/replaced.

Modifications:

Copy the decoding result over to a new instance produced by AggregatedFullHttpRequest.replace or AggregatedFullHttpResponse.replace .

Result:

DecoderResult is now copied over when an original AggregatedFullHttpMessage is being replaced (i.e., AggregatedFullHttpRequest.replace or AggregatedFullHttpResponse.replace is being called).

New unit tests are passing on this branch but are failing on master.
2017-02-06 07:49:53 +01:00
Dmitriy Dumanskiy
b9abd3c9fc Cleanup : for loops for arrays to make code easier to read and removed unnecessary toLowerCase() 2017-02-06 07:47:59 +01:00
Norman Maurer
1a05463c56 More strict testing of handshake behaviour
Motiviation:

We should ensure we not need any extra wrap / unwrap calls during handshake once the handshake was signaled as finished

Modifications:

More strict testing

Result:

Better testing of handshake behaviour
2017-02-03 09:45:09 +01:00
Roger Kapsi
d688e35e70 Fixing argument names
Motivation

Misleading argument names

Modifications

Stripping xMillis suffix from arguments because there's a TimeUnit

Result

Less confusion
2017-02-03 08:39:25 +01:00
Norman Maurer
1d128c7a65 Switch to netty-tcnative 2.0.0 which uses different package names
Motivation:

Previous versions of netty-tcnative used the org.apache.tomcat namespace which could lead to problems when a user tried to use tomcat and netty in the same app.

Modifications:

Use netty-tcnative which now uses a different namespace and adjust code to some API changes.

Result:

Its now possible to use netty-tcnative even when running together with tomcat.
2017-02-02 10:44:38 +01:00
Scott Mitchell
3482651e0c HTTP/2 Non Active Stream RFC Corrections
Motivation:
codec-http2 couples the dependency tree state with the remainder of the stream state (Http2Stream). This makes implementing constraints where stream state and dependency tree state diverge in the RFC challenging. For example the RFC recommends retaining dependency tree state after a stream transitions to closed [1]. Dependency tree state can be exchanged on streams in IDLE. In practice clients may use stream IDs for the purpose of establishing QoS classes and therefore retaining this dependency tree state can be important to client perceived performance. It is difficult to limit the total amount of state we retain when stream state and dependency tree state is combined.

Modifications:
- Remove dependency tree, priority, and weight related items from public facing Http2Connection and Http2Stream APIs. This information is optional to track and depends on the flow controller implementation.
- Move all dependency tree, priority, and weight related code from DefaultHttp2Connection to WeightedFairQueueByteDistributor. This is currently the only place which cares about priority. We can pull out the dependency tree related code in the future if it is generally useful to expose for other implementations.
- DefaultHttp2Connection should explicitly limit the number of reserved streams now that IDLE streams are no longer created.

Result:
More compliant with the HTTP/2 RFC.
Fixes https://github.com/netty/netty/issues/6206.

[1] https://tools.ietf.org/html/rfc7540#section-5.3.4
2017-02-01 10:34:27 -08:00
Scott Mitchell
6e5b25733f HTTP/2 Connection Preface User Event
Motivation:
If an HTTP/2 client writes data before the connection preface the peer will shutdown the socket. Depending on what is in the pipeline (SslHandler) may require different evaluation criteria to infer when the codec-http2 has written the connection preface on behalf of the client. This can lead to unnecessarily complexity and error prone/racy application code.

Modifications:
- Introduce a user event that is fired up the pipeline when codec-http2 writes the connection preface

Result:
Reliable mechanism for applications to use to know when connection preface has been written (related to https://github.com/netty/netty/issues/6272).
2017-02-01 10:10:42 -08:00
Norman Maurer
66b1731041 PooledByteBuf.capacity(...) not enforces maxCapacity()
Motivation:

PooledByteBuf.capacity(...) miss to enforce maxCapacity() and so its possible to increase the capacity of the buffer even if it will be bigger then maxCapacity().

Modifications:

- Correctly enforce maxCapacity()
- Add unit tests for capacity(...) calls.

Result:

Correctly enforce maxCapacity().
2017-02-01 18:45:54 +01:00
Norman Maurer
0d5b665fba Automatically decode DNS domain name to unicode
Motivation:

DnsNameResolver will return the domain / host name as ascii code using punycode (https://tools.ietf.org/html/rfc3492). This is different to what the JDK does which always convert it to unicode. We should do the same by default but allow to also not do it.

Modifications:

- Add new builder method on DnsNameResolverBuilder which allow to disable / enable converting. Default is to convert just like the JDK does.
- Add unit tests for it.

Result:

DnsNameResolver and JDK impl behave the same way.
2017-01-31 09:28:57 +01:00
Carl Mastrangelo
ead9938980 Include Http 1 request in error message
Motivation:

When An HTTP server is listening in plaintext mode, it doesn't have
a chance to negotiate "h2" in the tls handshake.  HTTP 1 clients
that are not expecting an HTTP2 server will accidentally a request
that isn't an upgrade, which the HTTP/2 decoder will not
understand.  The decoder treats the bytes as hex and adds them to
the error message.

These error messages are hard to understand by humans, and result
in extra, manual work to decode.

Modification:

If the first bytes of the request are not the preface, the decoder
will now see if they are an HTTP/1 request first.  If so, the error
message will include the method and path of the original request in
the error message.

In case the path is long, the decoder will check up to the first
1024 bytes to see if it matches.  This could be a DoS vector if
tons of bad requests or other garbage come in.  A future optimization
would be to treat the first few bytes as an AsciiString and not do
any Charset decoding.  ByteBuf.toCharSequence alludes to such an
optimization.

The code has been left simple for the time being.

Result:

Faster identification of errant HTTP requests.
2017-01-30 09:46:38 -08:00
Norman Maurer
735d6dd636 [maven-release-plugin] prepare for next development iteration 2017-01-30 15:14:02 +01:00
Norman Maurer
76e22e63f3 [maven-release-plugin] prepare release netty-4.1.8.Final 2017-01-30 15:12:36 +01:00
Norman Maurer
7736534b34 Ensure tests added in 91f050d2ef work with different openssl installations
Motivation:

Tests were added in 91f050d2ef to run with different protocols / ciphers. These may fail currently when openssl was compiled without support for the protocol / ciphers.

Modifications:

- Refactor tests to easier understand for which protocol / cipher it failed
- Not fail the test if the protocol is not supported with the used openssl version.

Result:

More robust testing.
2017-01-30 13:21:56 +01:00
ming.ma
f10f8a3131 Calculate correct count for tiny/small/normal allocation
Motivation:

Disable ThreadLocal Cache, then allocate Pooled ByteBuf and release all these buffers, PoolArena's tiny/small/normal allocation count is incorrect.

Modifications:

- Calculate PoolArena's tiny/small/normal allocation one time
- Add testAllocationCounter TestCase

Result:

Fixes #6282 .
2017-01-30 10:32:58 +01:00
Jon Chambers
94cb389c04 Restore add(Promise) and addAll(Promise...) methods to PromiseCombiner.
Motivation:

A testing goof in 7c630fe introduced a binary incompatibility when the old Promise-specific `add` and `addAll` methods in PromiseCombiner were generalized to accept `Futures`.

Modification:

- Restore (but mark as `@Deprecated`) old PromiseCombiner methods.
- Fixed a couple minor documentation typos because sure why not.

Result:

`PromiseCombiner` is binary-compatible with previous versions of Netty.
2017-01-30 09:23:11 +01:00
Norman Maurer
7a39afd031 Correctly detect which protocols are supported when using OpenSSL
Motivation:

We failed to properly test if a protocol is supported on an OpenSSL installation and just always returned all protocols.

Modifications:

- Detect which protocols are supported on a platform.
- Skip protocols in tests when not supported. This fixes a build error on some platforms introduced by [#6276].

Result:

Correctly return only the supported protocols
2017-01-27 23:37:10 +01:00
Norman Maurer
91f050d2ef More precise calculate the maximum record size when using SslProvider.OPENSSL* and so decrease mem usage.
Motivation:

We used ca 2k as maximum overhead for encrypted packets which is a lot more then what is needed in reality by OpenSSL. This could lead to the need of more memory.

Modification:

- Use a lower overhead of 86 bytes as defined by the spec and openssl itself
- Fix unit test to use the correct session to calculate needed buffer size

Result:

Less memory usage.
2017-01-27 19:51:45 +01:00
Scott Mitchell
e13da218e9 HTTP/2 revert Http2FrameWriter throws API change
Motivation:
2fd42cfc6b fixed a bug related to encoding headers but it also introduced a throws statement onto the Http2FrameWriter methods which write headers. This throws statement makes the API more verbose and is not necessary because we can communicate the failure in the ChannelFuture that is returned by these methods.

Modifications:
- Remove throws from all Http2FrameWriter methods.

Result:
Http2FrameWriter APIs do not propagate checked exceptions.
2017-01-26 23:26:17 -08:00
Norman Maurer
81735c2535 Check if Epoll is avaible when construct EpollEventLoopGroup
Motivation:

We should call Epoll.ensureAvailability() when init EpollEventLoopGroup to fail fast and with a proper exception.

Modifications:

Call Epoll.ensureAvailability() during EpollEventLoopGroup init.

Result:

Fail fast if epoll is not availability (for whatever reason).
2017-01-27 08:19:45 +01:00
Norman Maurer
8dda984afe Null out references to tmpNioBuf and chunk to allow quicker collecting
Motivation:

In PooledByteBuf we missed to null out the chunk and tmpNioBuf fields before recycle it to the Recycler. This could lead to keep objects longer alive then necessary which may hold a lot of memory.

Modifications:

Null out tmpNioBuf and chunk before recycle.

Result:

Possible to earlier GC objects.
2017-01-26 22:06:47 +01:00
chhsiao90
f4c2c1926f Fixed rfc violation about sending extension frame in the middle of headers
Motivation:

At rfc7540 5.5, it said that it's not permitted to send extension
frame in the middle of header block and need be treated as
protocol error

Modifications:

When received a extension frame, in netty it's called unknown frame,
will verify that is there an headersContinuation exists

Result:

When received a extension frame in the middle of header block,
will throw connection error and closed the connection
2017-01-26 11:25:15 -08:00
Norman Maurer
5cd8133477 Add unit test that shows we correctly return BUFFER_UNDERFLOW
Motivation:

We should test that we correctly return BUFFER_UNDERFLOW if the src buffer not contains enough data to unwrap it.

Modification:

Add unit test to verify behaviour.

Result:

Better test coverrage of SSLEngine implementations.
2017-01-26 15:04:10 +01:00
Norman Maurer
b69c7723f2 Update jetty-alpn-agent to support latest JDK releases.
Motivation:

We need to update jetty-alpn-agent to support latest JDK releases.

Modifications:

Update jetty-alpn-agent to 2.0.6

Result:

Be able to run tests with latest JDK releases.
2017-01-26 12:18:46 +01:00
ming.ma
44add3c525 Log correct value for useCacheForAllThreads
Motivation:

Log about "-Dio.netty.allocator.useCacheForAllThreads" is missing log placeholder, and so can't output correct value.

Modification:

- Add placeholder

Result:

Fixes #6265 .
2017-01-25 08:01:23 +01:00
Chris Conroy
9bec25a6eb Set the Transfer-Encoding header instead of adding
Motivation:

HttpUtil.setTransferEncodingChunked could add a second Transfer-Encoding
header if one was already present. While this is technically valid, it
does not appear to be the intent of the method.

Result:

Only one Transfer-Encoding header is present after calling this method.
2017-01-25 07:53:53 +01:00
Christopher Exell
d509365974 Move location of where oversized headers that don't exceed the go away limit is done
so that the check occurs after all headers have been read
2017-01-24 15:19:46 -08:00
Norman Maurer
a416b79d86 DnsNameResolver.resolve*(...) never notifies the Future when empty hostname is used.
Motivation:

When an empty hostname is used in DnsNameResolver.resolve*(...) it will never notify the future / promise. The root cause is that we not correctly guard against errors of IDN.toASCII(...) which will throw an IllegalArgumentException when it can not parse its input. That said we should also handle an empty hostname the same way as the JDK does and just use "localhost" when this happens.

Modifications:

- If the try to resolve an empty hostname we use localhost
- Correctly guard against errors raised by IDN.toASCII(...) so we will always noify the future / promise
- Add unit test.

Result:

DnsNameResolver.resolve*(...) will always notify the future.
2017-01-24 21:21:49 +01:00
Norman Maurer
640ef615be Allow to configure SslHandler to wait for close_notify response before closing the Channel and fix racy flush close_notify timeout scheduling.
Motivation:

SslHandler closed the channel as soon as it was able to write out the close_notify message. This may not be what the user want as it may make sense to only close it after the actual response to the close_notify was received in order to guarantee a clean-shutdown of the connection in all cases.

Beside this closeNotifyFlushTimeoutMillis is volatile so may change between two reads. We need to cache it in a local variable to ensure it not change int between. Beside this we also need to check if the flush promise was complete the schedule timeout as this may happened but we were not able to cancel the timeout yet. Otherwise we will produce an missleading log message.

Modifications:

- Add new setter / getter to SslHandler which allows to specify the behavior (old behavior is preserved as default)
- Added unit test.
- Cache volatile closeNotifyTimeoutMillis.
- Correctly check if flush promise was complete before we try to forcibly close the Channel and log a warning.
- Add missing javadocs.

Result:

More clean shutdown of connection possible when using SSL and fix racy way of schedule close_notify flush timeouts and javadocs.
2017-01-24 10:51:16 +01:00
Christopher Exell
907726988d Update hpack Decoder CTOR to allow for overflow in maxHeaderList size, as we do when we apply our ack'ed settings
This prevents us from having the first request, that hasn't ack'ed the setting causing a GOAWAY when we'd would
be under the maxHeaderListSizeGoAway that would have been set after the settings ack.
2017-01-21 10:20:49 -08:00
Norman Maurer
e9fa40d770 Ensure calling ReferenceCountedOpenSslEngine.wrap(...) after closeOutbound() was called will not throw an SSLException
Motivation:

PR [#6238] added guards to be able to call wrap(...) / unwrap(...) after the engine was shutdown. Unfortunally one case was missed which is when closeOutbound() was called and produced some data while closeInbound() was not called yet.

Modifications:

Correctly guard against SSLException when closeOutbound() was called, produced data and someone calls wrap(...) after it.

Result:

No more SSLException. Fixes [#6260].
2017-01-21 07:36:05 +01:00
Norman Maurer
4e04b63746 Update netty-tcnative
Motivation:

We released a new netty-tcnative version as a memory leak was fixed.

Modifications:

Update netty-tcnative.

Result:

Fixes [#6249].
2017-01-20 20:18:03 +01:00
Norman Maurer
1b31397249 Deprecate methods on SslHandler that have other replacements
Motivation:

SslHandler has multiple methods which have better replacements now or are obsolete. We should mark these as `@Deprecated`.

Modifications:

Mark methods as deprecated.

Result:

API cleanup preparation.
2017-01-19 21:34:22 +01:00
Norman Maurer
ac5f701a5c Use less memory during writes when using SslHandler with SslProvider.OpenSsl
Motivation:

In commit fc3c9c9523 I changes the way how we calculate the capacity of the needed ByteBuf for wrap operations that happen during writes when the SslHandler is used. This had the effect that the same capacity for ByteBufs is needed for the JDK implementation of SSLEngine but also for our SSLEngine implementation that uses OpenSSL / BoringSSL / LibreSSL. Unfortunally this had the side-effect that applications that used our SSLEngine implementation now need a lot more memory as bascially the JDK implementation always needs a 16kb buffer for each wrap while we can do a lot better for our SSLEngine implementation.

Modification:

- Resurrect code that calculate a better ByteBuf capacity when using our SSLEngine implementation and so be able to safe a lot of memory
- Add test-case to ensure it works as expected and is not removed again later on.

Result:

Memory footprint of applications that uses our SSLEngine implementation based on OpenSSL / BoringSSL / LibreSSL is back to the same amount of before commit fc3c9c9523.
2017-01-19 21:24:39 +01:00
Tim Brooks
3344cd21ac Wrap operations requiring SocketPermission with doPrivileged blocks
Motivation:

Currently Netty does not wrap socket connect, bind, or accept
operations in doPrivileged blocks. Nor does it wrap cases where a dns
lookup might happen.

This prevents an application utilizing the SecurityManager from
isolating SocketPermissions to Netty.

Modifications:

I have introduced a class (SocketUtils) that wraps operations
requiring SocketPermissions in doPrivileged blocks.

Result:

A user of Netty can grant SocketPermissions explicitly to the Netty
jar, without granting it to the rest of their application.
2017-01-19 21:12:52 +01:00
Dmitriy Dumanskiy
2d11331591 Typo fix in post encoder and replaced static hashmap with array. 2017-01-19 10:44:27 -08:00
Scott Mitchell
2fd42cfc6b HTTP/2 Max Header List Size Bug
Motivation:
If the HPACK Decoder detects that SETTINGS_MAX_HEADER_LIST_SIZE has been violated it aborts immediately and sends a RST_STREAM frame for what ever stream caused the issue. Because HPACK is stateful this means that the HPACK state may become out of sync between peers, and the issue won't be detected until the next headers frame. We should make a best effort to keep processing to keep the HPACK state in sync with our peer, or completely close the connection.
If the HPACK Encoder is configured to verify SETTINGS_MAX_HEADER_LIST_SIZE it checks the limit and encodes at the same time. This may result in modifying the HPACK local state but not sending the headers to the peer if SETTINGS_MAX_HEADER_LIST_SIZE is violated. This will also lead to an inconsistency in HPACK state that will be flagged at some later time.

Modifications:
- HPACK Decoder now has 2 levels of limits related to SETTINGS_MAX_HEADER_LIST_SIZE. The first will attempt to keep processing data and send a RST_STREAM after all data is processed. The second will send a GO_AWAY and close the entire connection.
- When the HPACK Encoder enforces SETTINGS_MAX_HEADER_LIST_SIZE it should not modify the HPACK state until the size has been checked.
- https://tools.ietf.org/html/rfc7540#section-6.5.2 states that the initial value of SETTINGS_MAX_HEADER_LIST_SIZE is "unlimited". We currently use 8k as a limit. We should honor the specifications default value so we don't unintentionally close a connection before the remote peer is aware of the local settings.
- Remove unnecessary object allocation in DefaultHttp2HeadersDecoder and DefaultHttp2HeadersEncoder.

Result:
Fixes https://github.com/netty/netty/issues/6209.
2017-01-19 10:42:43 -08:00
Norman Maurer
ead87b7df8 Respect resolvedAddressTypes when follow CNAME records.
Motivation:

When we follow CNAME records we should respect resolvedAddressTypes and only query A / AAAA depending on which address types are expected.

Modifications:

Check if we should query A / AAAA when follow CNAMEs depending on resolvedAddressTypes.

Result:

Correct behaviour when follow CNAMEs.
2017-01-19 19:36:56 +01:00
Norman Maurer
71efb0b39e Do not replace System.err during Slf4JLoggerFactory construction
Motivation:

Replacing System.err during Slf4JLoggerFactory construction is problematic as another class may optain the System.err reference before we set it back to the original value.

Modifications:

Remove code that temporary replaced System.err.

Result:

Fixes [#6212].
2017-01-19 19:28:01 +01:00
Norman Maurer
cf8e07f62f Run all tests in SSLEngineTest with heap, direct and mixed buffers
Motivation:

As we use different execution path in our SSLEngine implementation depending on if heap, direct or mixed buffers are used we should run the tests with all of them.

Modification:

Ensure we run all tests with different buffer types.

Result:

Better test-coverage
2017-01-19 19:18:31 +01:00
Norman Maurer
d55c321306 Add SslCloseCompletionEvent that is fired once a close_notify was received
Motivation:

For the completion of a handshake we already fire a SslHandshakeCompletionEvent which the user can intercept. We should do the same for the receiving of close_notify.

Modifications:

Add SslCloseCompletionEvent and test-case.

Result:

More consistent API.
2017-01-19 19:15:24 +01:00
Norman Maurer
cd9008f95b Remove unnecessray for loop missed by fac0ca8319 2017-01-19 19:13:57 +01:00
Dmitriy Dumanskiy
0e654f77e2 Level initialization cleanup. 2017-01-19 10:11:38 -08:00