Motivation:
The code for initiating a TLS handshake or renegotiation process is
currently difficult to reason about.
Modifications:
This commit introduces to clear paths for starting a handshake. The
first path is a normal handshake. The handshake is started and a timeout
is scheduled.
The second path is renegotiation. If the first handshake is incomplete,
the renegotiation promise is added as a listener to the handshake
promise. Otherwise, the renegotiation promise replaces the original
promsie. At that point the handshake is started again and a timeout is
scheduled.
Result:
Cleaner and easier to understand code.
Motivation:
When the DefaultHttp2ConnectionEncoder writes the initial headers for a new
locally created stream we create the stream in the half-closed state if the
end-stream flag is set which signals to the life cycle manager that the headers
have been sent. However, if we synchronously fail to write the headers the
life cycle manager then sends a RST_STREAM on our behalf which is a connection
level PROTOCOL_ERROR because the peer sees the stream in an IDLE state.
Modification:
Don't open the stream in the half-closed state if the end-stream flag is
set and let the life cycle manager take care of it.
Result:
Cleaner state management in the DefaultHttp2ConnectionEncoder.
Fixes#8434.
Motivation:
ByteBuf.retainedSlice() and similar methods produce sliced buffers with
an independent refcount to the buffer that they wrap.
One of the optimizations in 10539f4dc7 was
to use the ref to the unwrapped buffer object for added slices, but this
did not take into account the above special case when later releasing.
Thanks to @rkapsi for discovering this via #8495.
Modifications:
Since a reference to the slice is still kept in the Component class,
just changed Component.freeIfNecessary() to release the slice in
preference to the unwrapped buf.
Also added a unit test which reproduces the bug.
Result:
Fixes#8495
Motivation:
We did not correctly schedule the handshake timeout if the handshake was either started by a flush(...) or if starttls was used.
Modifications:
- Correctly setup timeout in all cases
- Add unit tests.
Result:
Fixes https://github.com/netty/netty/issues/8493.
Motivation:
We missed to include a profile for windows which means that we did not have the correct dependencies setup.
Modifications:
- Add missing profile
- Add assumeFalse(...) to ensure we do only test the native transpot shading on non windows platforms.
- Explicit specify dependency on netty-common
Result:
Fixes https://github.com/netty/netty/issues/8489.
Motivation:
If you attempt to write to a channel with an SslHandler prior to channelActive being called you can hit an assertion. In particular - if you write to a channel it forces some handshaking (through flush calls) to occur.
The AssertionError only happens on Java11+.
Modifications:
- Replace assert by an "early return" in case of the handshake be done already.
- Add unit test that verifies we do not hit the AssertionError anymore and that the future is correctly failed.
Result:
Fixes https://github.com/netty/netty/issues/8479.
Motivation:
We disabled the test at some point but it should work now without any problems.
Modifications:
Remove @Ignore from test.
Result:
Verify shading of netty-tcnative on CI.
Motivation:
Two similar bugs were introduced by myself in separate recent PRs #8393
and #8464, while optimizing the assignment/handling of temporary arrays
in ByteBufUtil and UnsafeByteBufUtil.
The temp arrays allocated for buffering data written to an OutputStream
are incorrectly sized to the full length of the data to copy rather than
being capped at WRITE_CHUNK_SIZE.
Unfortunately one of these is in the 4.1.31.Final release, I'm really
sorry and will be more careful in future.
This kind of thing is tricky to cover in unit tests.
Modifications:
Revert the temp array allocations back to their original sizes.
Avoid making duplicate calls to ByteBuf.capacity() in a couple of places
in ByteBufUtil (unrelated thing I noticed, can remove it from this PR if
desired!)
Result:
Temporary byte arrays will be reverted to their originally intended
sizes.
Motivation:
The `Http2StreamFrameToHttpObjectCodec` is marked `@Sharable` but mutates
an internal `HttpScheme` field every time it is added to a pipeline.
Modifications:
Instead of storing the `HttpScheme` in the handler we store it as an
attribute on the parent channel.
Result:
Fixes#8480.
Motivation:
We should call wrapEngine(...) in our SSLEngineTest to correctly detect all errors in case of the OpenSSLEngine.
Modifications:
Add missing wrapEngine(...) calls.
Result:
More correct tests
Motivation:
We did not override all methods in OpenSslX509Certificate and delegate to the internal 509Certificate.
Modifications:
Add missing overrides.
Result:
More correct implementation
Motivation:
allLeaks is to store the DefaultResourceLeak. When we actually use it, the key is DefaultResourceLeak, and the value is actually a meaningless value.
We only care about the keys of allLeaks and don't care about the values. So Set is more in line with this scenario.
Using Set as a container is more consistent with the definition of a container than Map.
Modification:
Replace allLeaks with set. Create a thread-safe set using 'Collections.newSetFromMap(new ConcurrentHashMap<DefaultResourceLeak<?>, Boolean>()).'
Motivation:
#8388 introduced a reusable ThreadLocal<byte[]> for use in
decodeString(...). It can be used in more places in the buffer package
to avoid temporary allocations of small arrays.
Modifications:
Encapsulate use of the ThreadLocal in a static package-private
ByteBufUtil.threadLocalTempArray(int) method, and make use of it from a
handful of new places including ByteBufUtil.readBytes(...).
Result:
Fewer short-lived small byte array allocations.
Motivation:
CompositeByteBuf is a powerful and versatile abstraction, allowing for
manipulation of large data without copying bytes. There is still a
non-negligible cost to reading/writing however relative to "singular"
ByteBufs, and this can be mostly eliminated with some rework of the
internals.
My use case is message modification/transformation while zero-copy
proxying. For example replacing a string within a large message with one
of a different length
Modifications:
- No longer slice added buffers and unwrap added slices
- Components store target buf offset relative to position in
composite buf
- Less allocations, object footprint, pointer indirection, offset
arithmetic
- Use Component[] rather than ArrayList<Component>
- Avoid pointer indirection and duplicate bounds check, more
efficient backing array growth
- Facilitates optimization when doing bulk-inserts - inserting n
ByteBufs behind m is now O(m + n) instead of O(mn)
- Avoid unnecessary casting and method call indirection via superclass
- Eliminate some duplicate range/ref checks via non-checking versions of
toComponentIndex and findComponent
- Add simple fast-path for toComponentIndex(0); add racy cache of
last-accessed Component to findComponent(int)
- Override forEachByte0(...) and forEachByteDesc0(...) methods
- Make use of RecyclableArrayList in nioBuffers(int, int) (in line with
FasterCompositeByteBuf impl)
- Modify addComponents0(boolean,int,Iterable) to use the Iterable
directly rather than copy to an array first (and possibly to an
ArrayList before that)
- Optimize addComponents0(boolean,int,ByteBuf[],int) to not perform
repeated array insertions and avoid second loop for offset updates
- Simplify other logic in various places, in particular the general
pattern used where a sub-range is iterated over
- Add benchmarks to demonstrate some improvements
While refactoring I also came across a couple of clear bugs. They are
fixed in these changes but I will open another PR with unit tests and
fixes to the current version.
Result:
Much faster creation, manipulation, and access; many fewer allocations
and smaller footprint. Benchmark results to follow.
Motivation:
If the encoder needs to flush more than one outbound message it will
create a new ChannelPromise for all but the last write which will
swallow failures.
Modification:
Use a PromiseCombiner in the case of multiple messages and the parent
promise isn't the `VoidPromise`.
Result:
Intermediate failures are propagated to the original ChannelPromise.
Motivation:
Due a bug in our implementation we tried to release the same ByteBuf two times when we failed to parse the X509Certificate as closing the ByteBufInputStream already closed it.
Modifications:
- Don't close the ByteBuf when closing the ByteBufInputStream
- Explicit release all ByteBufs after we are done parsing in a finally block.
- Add testcase.
Result:
Do not produce an IllegalReferenceCountException and throw the correct CertificateException.
Motivation:
After #7527 fix there is no need to manually release chunks (HttpData) during file upload as they will be released on HttpPostRequestDecoder.destroy().
Modification:
HttpUploadServer example doesn't release chunks manually (doesn't call data.release()).
Result:
Fixes#7695 and #7689
Motivation:
HWT does not support anything smaller then 1ms so we should make it clear that this is the case.
Modifications:
Log a warning if < 1ms is used.
Result:
Less suprising behaviour.
Motivation:
SHA1 is a broken hash function and shouldn't be used anymore (see: https://shattered.io/).
Security scanning tools will raise this as an issue and it will reflect badly on netty and I, therefore, recommend to use a SHA2 hash function which is secure and won't be flagged by such tools.
Modifications:
Replaced insecure SHA1 based signing scheme with SHA2.
Result:
Modern and thus secure cryptographic primitives will be in use and won't be flagged by security scanning tools.
Motivation:
In netty we use our own max direct memory limit that can be adjusted by io.netty.maxDirectMemory but we do not take this in acount when maxDirectMemory() is used. That will lead to non optimal configuration of PooledByteBufAllocator in some cases.
This came up on stackoverflow:
https://stackoverflow.com/questions/53097133/why-is-default-num-direct-arena-derived-from-platformdependent-maxdirectmemory
Modifications:
Correctly respect io.netty.maxDirectMemory and so configure PooledByteBufAllocator correctly by default.
Result:
Correct value for max direct memory.
Motivation:
https://github.com/netty/netty/issues/8444 reports that there is some issue with negative values passed to timerfd_settime. This test verifies that everything is working as expected.
Modifications:
Add testcase.
Result:
Test to verify expected behaviour.
Motivation:
Unpooled.wrap(byte[]...) and Unpooled.wrap(ByteBuffer...) currently
allocate/copy an intermediate ByteBuf ArrayList and array, which can be
avoided.
Modifications:
- Define new internal ByteWrapper interface and add a CompositeByteBuf
constructor which takes a ByteWrapper with an array of the type that it
wraps, and modify the appropriate Unpooled.wrap(...) methods to take
advantage of it
- Tidy up other constructors in CompositeByteBuf to remove duplication
and misleading len arg (which is really an end offset into provided
array)
Result:
Less allocation/copying when wrapping byte[] and ByteBuffer arrays,
tidier code.
Motivation:
https://github.com/netty/netty/issues/8442 reported that we fail to build a SslContext when an invalid cipher is used with netty-tcnative-boringssl-static, while it worked before. This test verifies that this is now consistent with all other SSLEngine implementations.
Modifications:
Add test-case to verify consistent behaviour
Result:
More tests to assert consistent behaviour across SSLEngine implementations
Motivation:
It should be possible to build a DnsNameResolver with a null resolvedAddressTypes, defaulting then to DEFAULT_RESOLVE_ADDRESS_TYPES (see line 309).
Sadly, `preferredAddressType` is then called on line 377 with the original parameter instead of the instance attribute, causing an NPE when it's null.
Modification:
Call preferredAddressType with instance attribuet instead of constructor parameter.
Result:
No more NPE
Motivation:
There are log messages emitted from Http2ConnectionDecoder of the form
```
INF i.n.h.c.h.DefaultHttp2ConnectionDecoder ignoring HEADERS frame for stream RST_STREAM sent. {}
```
Modifications:
Remove the trailing `{}` in the log message that doesn't have a value.
Result:
Log messages no longer have a trailing `{}`.
Motivation:
201e984cb3 added support to use native TLSv1.3 support even with Java versions prior to 11. For this we try to detect if we need to wrap the used KeyManager or not. This testing code did create an X509Certificate[1] but does not correctly also set the certficiate on index 0. While this should be harmless we should better do the right thing and set it.
Modifications:
Correctly init the array.
Result:
Cleaner and more correct code.
Motivation:
I came across two bugs:
- Components removed due to capacity reduction aren't released
- Offsets aren't set correctly on empty components that are added
between existing components
Modifications:
Add unit tests which expose these bugs, fix them.
Result:
Bugs are fixed
Motivation:
We should provide an easy way to run our testsuite against netty-tcnative-boringssl-static
Modifications:
- Add boringssl profile which can be used to enable usage of netty-tcnative-boringssl-static
- Make use of the profile in docker-compose
Result:
Cleaner and easier way of running testsuite against netty-tcnative-boringssl-static
Motivation:
There are currently many more places where this could be used which were
possibly not considered when the method was added.
If https://github.com/netty/netty/pull/8388 is included in its current
form, a number of these places could additionally make use of the same
BYTE_ARRAYS threadlocal.
There's also a couple of adjacent places where an optimistically-pooled
heap buffer is used for temp byte storage which could use the
threadlocal too in preference to allocating a temp heap bytebuf wrapper.
For example
https://github.com/netty/netty/blob/4.1/buffer/src/main/java/io/netty/buffer/ByteBufUtil.java#L1417.
Modifications:
Replace new byte[] with PlatformDependent.allocateUninitializedArray()
where appropriate; make use of ByteBufUtil.getBytes() in some places
which currently perform the equivalent logic, including avoiding copy of
backing array if possible (although would be rare).
Result:
Further potential speed-up with java9+ and appropriate compile flags.
Many of these places could be on latency-sensitive code paths.
* Optimize AbstractByteBuf.getCharSequence() in US_ASCII case
Motivation:
Inspired by https://github.com/netty/netty/pull/8388, I noticed this
simple optimization to avoid char[] allocation (also suggested in a TODO
here).
Modifications:
Return an AsciiString from AbstractByteBuf.getCharSequence() if
requested charset is US_ASCII or ISO_8859_1 (latter thanks to
@Scottmitch's suggestion). Also tweak unit tests not to require Strings
and include a new benchmark to demonstrate the speedup.
Result:
Speed-up of AbstractByteBuf.getCharSequence() in ascii and iso 8859/1
cases
Motivation:
0ddc62cec0 added support for TLSv1.3 when using openssl 1.1.1. Now that BoringSSL chromium-stable branch supports it as well we can also support it with netty-tcnative-boringssl-static.
During this some unit tests failed with BoringSSL which was caused by not correctly handling flush() while the handshake is still in progress.
Modification:
- Upgrade netty-tcnative version which also supports TLSv1.3 when using BoringSSL
- Correctly handle flush() when done while the handshake is still in progress in all cases.
Result:
Easier for people to enable TLSv1.3 when using native SSL impl.
Ensure flush() while handshake is in progress will always be honored.
Motivation:
trackedObject != null gives no guarantee that trackedObject remains reachable. This may cause problems related to premature finalization: false leak detector warnings.
Modifications:
Add private method reachabilityFence0 that works on JDK 8 and can be factored out into PlatformDependent. Later, it can be swapped for the real Reference.reachabilityFence.
Result:
No false leak detector warnings in future versions of JDK.
Motivation:
A new EA release was done, we should always run against the latest.
Modifications:
Update to EA 16.
Result:
CI runs with latest EA release for JDK12.
Motivation:
OpenSsl used SelfSignedCertificate in its static init block to detect if KeyManagerFactory is supported. Unfortunally this only works when either sun.security.x509.* can be accessed or bouncycastle is on the classpath.
We should not depend on either of it.
This came up in https://github.com/netty/netty-tcnative/issues/404#issuecomment-431551890.
Modifications:
Just directly use the bytes to generate the X509Certificate and so not depend on sun.security.x509.* / bouncycastle.
Result:
Correctly be able to detect if KeyManagerFactory can be supported in all cases.
Motivation:
DefaultResourceLeak.toString() did include the wrong value for duplicated records.
Modifications:
Include the correct value.
Result:
Correct toString() implementation.
Motivation:
Commons-compress < 2.0.18 has a security flaw so we should upgrade (even if we only use it in tests anyway).
Modifications:
Update to 2.0.18
Result:
Use latest version.
Motivation:
We had put some workaround in our tests due a bug in the Java11 implementation of TLSv1.3. This was now fixes as part of 11.0.1.
See https://bugs.openjdk.java.net/browse/JDK-8211067.
Modifications:
Remove workaround in SSL tests.
Result:
Run all tests with supported TLS version.
Motivation:
CompositeByteBuf.decompose(...) did not correctly slice the content and so produced an incorrect representation of the data.
Modifications:
- Rewrote implementation to fix bug and also improved it to reduce GC
- Add unit tests.
Result:
Fixes https://github.com/netty/netty/issues/8400.
Motivation:
2109f14c24 corrected how we run the testsuite with boringssl-static but missed to also adjust the testsuite-shading configuration which lead to test failures.
Modifications:
Correctly compose the native lib name when no classifier is used.
Result:
Testsuite passes again.
Motivation:
As mentioned in RFC 7692 :
The "server_no_context_takeover" Extension Parameter should be used on server side for compression and on client side for decompression.
The "client_no_context_takeover" Extension Parameter should be used on client side for compression and on server side for decompression.
Right now, in PerMessageDeflateClientExtensionHandshaker, the decoder uses clientNoContext instead of serverNoContext and the encoder uses serverNoContext instead of clientNoContext.
The same inversion is present in PerMessageDeflateServerExtensionHandshaker: the decoder uses
serverNoContext instead of clientNoContext, while the encoder uses serverNoContext instead of clientNoContext. Besides the context inversion, the sliding window sizes seem to be inversed as well.
Modification:
Inverse clientNoContext with serverNoContext and clientWindowSize with serverWindowSize for both the Decoder and Encoder in PerMessageDeflateServerExtensionHandshaker and PerMessageDeflateClientExtensionHandshaker.
Result:
This fixes the decompression fail in the case that one of the contexts is set and the other one is not.
Motivation:
At the moment it's only possible to use TLSv1.3 with netty-tcnative if Java 11 is used. It should be possible to do so even with Java 8, 9 and 10.
Modification:
Add a workaround to be able to use TLSv1.3 also when using Java version prior to Java 11 and the default X509ExtendedTrustManager is used.
Result:
Be able to use TLSv1.3 also with past versions of Java.
Motivation:
We should explicit specify the java version to use to ensure docker will rebuild the image once a new java version was released and we specify it. Also we should use openjdk for testing when possible.
Modifications:
- Explicit specify the java versions to use
- Use openjdk when possible.
Result:
Ensure latest java versions are used during testing
Motivation:
We need to update jetty-alpn-agent to be able to run tests with OpenJDK 8u191
Modifications:
Update to 2.0.8
Result:
Be able to run tests with latest JDK 8 release.
