### Motivation:
I've now found two libraries that use Netty to be vulnerable to [CWE-113: Improper Neutralization of CRLF Sequences in HTTP Headers ('HTTP Response Splitting')](https://cwe.mitre.org/data/definitions/113.html) due to using `new DefaultHttpHeaders(false)`.
Some part of me hopes that this warning will help dissuade library authors from disabling this important security check.
### Modification:
Add documentation to `DefaultHttpHeaders(boolean)` to warn about the implications of `false`.
### Result:
This improves the documentation on `DefaultHttpHeaders`.
Motivation:
In the current implementation of Base64 decoder an invalid
character `\u00BD` treated as `=`.
Also character `\u007F` leads to ArrayIndexOutOfBoundsException.
Modification:
Explicitly checks that all input bytes are ASCII characters
(greater than zero). Fix `decodabet` tables.
Result:
Correctly validation input bytes in Base64 decoder.
Motivation:
031c2e2e88 introduced some change to reduce the risk of have the `ReferenceCountedOpenSslContext` be destroyed while the `ReferenceCountedSslEngine` is still in us. Unfortunaly it missed to adjust a few tests which make assumptions about the refCnt of the context.
Modifications:
Adjust tests to take new semenatics into acount.
Result:
No more tests failures
Motivation:
We should not commit vscode specific files, so at it to gitignore
Modifications:
Add files to .gitignore
Result:
Correctly ignore ide related files
Motivation:
We should use adaptjdk 13 and not oracle openjdk 13 when building with Java 13
Modifications:
Use adopt@1.13.0-0
Result:
More consistent java vendor usage
Motivation:
Http2MultiplexCodec extends Http2FrameCodec extends Http2ConnectionHandler. It appears Http2MultiplexCodec overrode the channelWritabilityChanged method, which prevented the flow controller from becoming active. In the case the parent channel becomes unwritable, and then later becomes writable, it needs to indicate that the child channels can still write data. This is slightly confusing, because the child channels may still themselves be unwritable, but should still drain their data to the parent channel.
Modification:
Still propagate writability changes to the HTTP/2 flow controller
Result:
Fixes https://github.com/netty/netty/issues/9636
Motivation
The current event loop shutdown logic is quite fragile and in the
epoll/NIO cases relies on the default 1 second wait/select timeout that
applies when there are no scheduled tasks. Without this default timeout
the shutdown would hang indefinitely.
The timeout only takes effect in this case because queued scheduled
tasks are first cancelled in
SingleThreadEventExecutor#confirmShutdown(), but I _think_ even this
isn't robust, since the main task queue is subsequently serviced which
could result in some new scheduled task being queued with much later
deadline.
It also means shutdowns are unnecessarily delayed by up to 1 second.
Modifications
- Add/extend unit tests to expose the issue
- Adjust SingleThreadEventExecutor shutdown and confirmShutdown methods
to explicitly add no-op tasks to the taskQueue so that the subsequent
event loop iteration doesn't enter blocking wait (as looks like was
originally intended)
Results
Faster and more robust shutdown of event loops, allows removal of the default wait timeout.
This is a port of https://github.com/netty/netty/pull/9616
Motivation:
There is a goto statement above the current position of initialize dynamicMethods, and dynamicMethods is used after the goto which might cause undefined behavior.
Modifications:
Initialize dynamicMehtods at the top.
Result:
No more undefined behavior.
Motivation:
Native image compilation is failing without extra flags:
```
Warning: Aborting stand-alone image build. No instances of io.netty.buffer.UnpooledHeapByteBuf are allowed in the image heap as this class should be initialized at image runtime. Object has been initialized by the io.netty.handler.codec.http2.Http2ConnectionHandler class initializer with a trace:
at io.netty.buffer.Unpooled.wrappedBuffer(Unpooled.java:157)
at io.netty.handler.codec.http2.Http2ConnectionHandler.<clinit>(Http2ConnectionHandler.java:74)
. To fix the issue mark io.netty.buffer.UnpooledHeapByteBuf for build-time initialization with --initialize-at-build-time=io.netty.buffer.UnpooledHeapByteBuf or use the the information from the trace to find the culprit and --initialize-at-run-time=<culprit> to prevent its instantiation.
Detailed message:
Trace: object io.netty.buffer.ReadOnlyByteBuf
object io.netty.buffer.UnreleasableByteBuf
method io.netty.handler.codec.http2.Http2ConnectionHandler.access$500()
Call path from entry point to io.netty.handler.codec.http2.Http2ConnectionHandler.access$500():
at io.netty.handler.codec.http2.Http2ConnectionHandler.access$500(Http2ConnectionHandler.java:66)
at io.netty.handler.codec.http2.Http2ConnectionHandler$PrefaceDecoder.readClientPrefaceString(Http2ConnectionHandler.java:299)
at io.netty.handler.codec.http2.Http2ConnectionHandler$PrefaceDecoder.decode(Http2ConnectionHandler.java:239)
at io.netty.handler.codec.http2.Http2ConnectionHandler.decode(Http2ConnectionHandler.java:438)
at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:505)
at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:444)
at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:283)
at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:374)
at io.netty.channel.AbstractChannelHandlerContext.access$600(AbstractChannelHandlerContext.java:56)
at io.netty.channel.AbstractChannelHandlerContext$7.run(AbstractChannelHandlerContext.java:365)
at com.oracle.svm.core.jdk.RuntimeSupport.executeHooks(RuntimeSupport.java:144)
at com.oracle.svm.core.jdk.RuntimeSupport.executeStartupHooks(RuntimeSupport.java:89)
at com.oracle.svm.core.JavaMainWrapper.runCore(JavaMainWrapper.java:143)
at com.oracle.svm.core.JavaMainWrapper.run(JavaMainWrapper.java:186)
at com.oracle.svm.core.code.IsolateEnterStub.JavaMainWrapper_run_5087f5482cc9a6abc971913ece43acb471d2631b(generated:0)
```
Modification:
Add `io.netty.handler.codec.http2.Http2ConnectionHandler` for runtime compilation, as the buffer library's `io.netty.buffer.UnpooledHeapByteBuf` is also marked for runtime.
Result:
Native image compilation works again.
Motivation:
`DefaultHttp2UnknownFrame#equals` may produce NPE due to
incorrect comparison of `stream` field.
Modification:
- Fix the `stream` field compare.
- Cleanup usage of class fields: use direct access instead of getters
(because the class is final).
Result:
No NPE in `equals` method.
Motivation:
Classes `AbstractHttp2StreamChannel.Http2StreamChannelConfig`
and `DnsNameResolver.AddressedEnvelopeAdapter` may be static:
it doesn't reference its enclosing instance.
Modification:
Add `static` modifier.
Result:
Prevents a possible memory leak and uses less memory per class instance.
Motivation:
With the Netty ref-counted OpenSSL implementation the parent SslContext
maintains state necessary for the SslEngine's it produces. However, it's
possible for the parent context to be closed and release those resources
before the child engines are finished which causes problems.
Modification:
Spawned ReferenceCountedOpenSslEngine's retain a reference to their
parent ReferenceCountedOpenSslContext.
Result:
The lifetime of the shared data is extended to include the lifetime of
the dependents.
Motivation:
On JDK > 9 Netty uses Unsafe to write two internal JDK fields: sun.nio.ch.SelectorImp.selectedKeys and sun.nio.ch.SelectorImpl.publicSelectedKeys. This is done in transport/src/main/java/io/netty/channel/nio/NioEventLoop.java:225, in openSelector() method. The GraalVM analysis cannot do the Unsafe registration automatically because the object field offset computation is hidden behind two layers of calls.
Modifications:
This PR updates the Netty GraalVM configuration by registering those fields for unsafe access.
Result:
Improved support for Netty on GraalVM with JDK > 9.
Motivation:
It is not possible to build Netty on an s390_64 platform.
Modifications:
Modify pom.xml so that s390_64 is acceptable as os.detected.arch.
Result:
Netty can be built on an s390_64 platform.
Signed-off-by: Tatsushi Inagaki <e29253@jp.ibm.com>
Motivation:
We would like to enable Netty also on a big endian platform such as
s390_64. We need to fix a function which assumes that the target
platform is little endian.
Modifications:
Modify netty_unix_socket_accept() to write an address length as jbyte
instead of jsize.
Result:
Netty can be enabled on a big endian platform.
Signed-off-by: Tatsushi Inagaki <e29253@jp.ibm.com>
Motivation:
Due a bug we did not always correctly calculate the next buffer size in AdaptiveRecvByteBufAllocator.
Modification:
Fix calculation and add unit test
Result:
Correct calculation is always used.
Motivation
This is another iteration of #9476.
Modifications
Instead of maintaining a count of all writes performed and then using
reads during shutdown to ensure all are accounted for, just set a flag
after each write and don't reset it until the corresponding event has
been returned from epoll_wait.
This requires that while a write is still pending we don't reset
wakenUp, i.e. continue to block writes from the wakeup() method.
Result
Race condition eliminated. Fixes#9362
Co-authored-by: Norman Maurer <norman_maurer@apple.com>
Motivation:
The build script for the module Netty Transport Native Epoll can cause
intermittent build break due to broken pipe by /usr/bin/ldd. This issue
likely to occur on a build environment with multiple processors.
Modifications:
The root cause is that the consumer head command finishes earlier the
producer ldd command. Buffering the outputs of the ldd command by an
intermediate tail command avoids the broken pipe.
Result:
A build on multiple processors can finish successfully.
Signed-off-by: Tatsushi Inagaki <e29253@jp.ibm.com>
Motivation:
At the moment we use Unpooled.buffer(...) in InboundHttp2ToHttpAdapter when we need to do a copy of the message. We should better use the configured ByteBufAllocator for the Channel
Modifications:
Change internal interface to also take the ByteBufAllocator as argument and use it when we need to allocate a ByteBuf.
Result:
Use the "correct" ByteBufAllocator in InboundHttp2ToHttpAdapter in all cases
Motivation:
At the moment we set release to false before we call writeData(...). This could let to the sitatuation that we will miss to release the message if writeData(...) throws. We should set release to false after we called writeData(...) to ensure the ownership of the buffer is correctly transferred.
Modifications:
- Set release to false after writeData(...) was successfully called only
Result:
No possibility for a buffer leak
Motivation:
We did miss to take Http2FrameCodecBuilder.isValidateHeaders() into account when a Http2FrameWriter was set on the builder and always assumed validation should be enabled.
Modifications:
Remove hardcode value and use configured value
Result:
Http2FrameCodecBuilder.isValidateHeaders() is respected in all cases
Motivation:
Recycler$Stack.pop will occurs `ArrayIndexOutOfBoundsException` in some race cases, we should double check `size` even after `scavenge` called.
Modifications:
Double check `size` after `scavenge`
Result:
avoid ArrayIndexOutOfBoundsException in `pop`
Motivation:
aebe206 added support for using a ChannelOption to set / get Buffer sizes but did not add the methods to the DomainSocketChannelConfig interface itself (due not be able to break the API)
Modifications:
Add methods to interface (as this is a next major release)
Result:
Easier access to configure these buffer sizes
Motiviation:
A lot of reentrancy bugs and cycles can happen because the DefaultPromise will notify the FutureListener directly when completely in the calling Thread if the Thread is the EventExecutor Thread. To reduce the risk of this we should always notify the listeners via the EventExecutor which basically means that we will put a task into the taskqueue of the EventExecutor and pick it up for execution after the setSuccess / setFailure methods complete the promise.
Modifications:
- Always notify via the EventExecutor
- Adjust test to ensure we correctly account for this
- Adjust tests that use the EmbeddedChannel to ensure we execute the scheduled work.
Result:
Reentrancy bugs related to the FutureListeners cant happen anymore.
Motivation:
At the moment we not consistently (and also not correctly) free allocated native memory in all cases during loading the JNI library. This can lead to native memory leaks in the unlikely case of failure while trying to load the library.
Beside this we also not always correctly handle the case when a new java object can not be created in native code because of out of memory.
Modification:
- Copy some macros from netty-tcnative to be able to handle errors in a more easy fashion
- Correctly account for New* functions to return NULL
- Share code
Result:
More robust and clean JNI code
Motivation:
Users can reuse the same FileChannel for different ChunkedNioFile
instances without being worried that FileChannel::position will be
changed concurrently by them.
In addition, FileChannel::read with absolute position allows to
use on *nix pread that is more efficient then fread.
Modifications:
Always use absolute FileChannel::read ops
Result:
Faster and more flexible uses of FileChannel for ChunkedNioFile
Motivation:
Error: Class that is marked for delaying initialization to run time got initialized during image building: io.netty.handler.codec.http2.Http2CodecUtil. Try marking this class for build-time initialization with --initialize-at-build-time=io.netty.handler.codec.http2.Http2CodecUtil
Error: Use -H:+ReportExceptionStackTraces to print stacktrace of underlying exception
Error: Image build request failed with exit status 1
Modification:
After debugging, it seems the culprit is io.netty.handler.codec.http2.Http2ClientUpgradeCodec, which also needs runtime initialisation.
Result:
Fixes #micronaut-projects/micronaut-grpc#8
Motivation:
It is not safe to cache a jclass without obtaining a global reference via NewGlobalRef.
Modifications:
Correctly use NewGlobalRef(...) before caching
Result:
Correctly cache jclass instance
Motivation:
We just released a new version of netty-tcnative.
Modifications:
Bump up to netty-tcnative 2.0.26.Final
Result:
Use latest netty-tcnative release
Motivation:
Due some bug we did endup with ClassCastExceptions in some cases. Beside this we also did not correctly handle the case when ReferenceCountedOpenSslEngineTest did produce tasks to run in on test.
Modifications:
- Correctly unwrap the engine before to fix ClassCastExceptions
- Run delegated tasks when needed.
Result:
All tests pass with different OpenSSL implementations (OpenSSL, BoringSSL etc)
Motivation:
Running tests with a `KQueueDomainSocketChannel` showed worse performance than an `NioSocketChannel`. It turns out that the default send buffer size for Nio sockets is 64k while for KQueue sockets it's 8k. I verified that manually setting the socket's send buffer size improved perf to expected levels.
Modification:
Plumb the `SO_SNDBUF` and `SO_RCVBUF` options into the `*DomainSocketChannelConfig`.
Result:
Can now configure send and receive buffer sizes for domain sockets.
Motivation:
Optimize the QueryStringEncoder for lower memory overhead and higher encode speed.
Modification:
Encode the space to + directly, and reuse the uriStringBuilder rather then create a new one.
Result:
Improved performance
Motivation:
When parsing HTTP headers special care needs to be taken when a whitespace is detected in the header name.
Modifications:
- Ignore whitespace when decoding response (just like before)
- Throw exception when whitespace is detected during parsing
- Add unit tests
Result:
Fixes https://github.com/netty/netty/issues/9571
Motivation:
Socks5InitialRequestDecoder does not correctly handle fragmentation
Modifications:
- Delete detection of not enough bytes as ReplyingDecoder already handles all of this correctly.
- Add unit test
Result:
Fixes#9574.
Motivation:
peek() is implemented in a similar way to poll() for the mpsc queue, thus it is more like a consumer call.
It is possible that we could have multiple thread call peek() and possibly one thread calls poll() at at the same time.
This lead to multiple consumer scenario, which violates the multiple producer single consumer condition and could lead to spin in an infinite loop in peek()
Modification:
Use isEmpty() instead of peek() to check if task queue is empty
Result:
Dont violate the mpsc semantics.
Motivation:
We should correctly reset the cached local and remote address when a Channel.disconnect() is called and the channel has a notion of disconnect vs close (for example DatagramChannel implementations).
Modifications:
- Correctly reset cached kicak abd remote address
- Update testcase to cover it and so ensure all transports work in a consistent way
Result:
Correctly handle disconnect()
Motivation:
SystemPropertyUtil already uses the AccessController internally so not need to wrap its usage with AccessController as well.
Modifications:
Remove explicit AccessController usage when SystemPropertyUtil is used.
Result:
Code cleanup
Motivation:
We did not correctly handle taskoffloading when using BoringSSL / OpenSSL. This could lead to the situation that we did not write the SSL alert out for the remote peer before closing the connection.
Modifications:
- Correctly handle exceptions when we resume processing on the EventLoop after the task was offloadded
- Ensure we call SSL.doHandshake(...) to flush the alert out to the outboundbuffer when an handshake exception was detected
- Correctly signal back the need to call WRAP again when a handshake exception is pending. This will ensure we flush out the alert in all cases.
Result:
No more failures when task offloading is used.
Motivation:
When using io.netty.handler.ssl.openssl.useTasks=true we may call ReferenceCountedOpenSslEngine.setKeyMaterial(...) from another thread and so need to synchronize and also check if the engine was destroyed in the meantime to eliminate of the possibility of a native crash.
The same is try when trying to access the authentication methods.
Modification:
- Add synchronized and isDestroyed() checks where missing
- Add null checks for the case when a callback is executed by another thread after the engine was destroyed already
- Move code for master key extraction to ReferenceCountedOpenSslEngine to ensure there can be no races.
Result:
No native crash possible anymore when using io.netty.handler.ssl.openssl.useTasks=true
Motivation:
calculateMaxBytesPerGatheringWrite() contains duplicated calculation: getSendBufferSize() << 1
Modifications:
Remove the duplicated calculation
Result:
The method will be clear and better
Motivation:
At the moment it is not possible to build netty on a power 8 systems.
Modifications:
- Improve detection of the possibility of using Conscrypt
- Skip testsuite-shading when not on x86_64 as this is the only platform for which we build tcnative atm
- Only include classifier if on x86_64 for tcnative as dependency as this is the only platform for which we build tcnative atm
- Better detect if UDT test can be run
Result:
Fixes https://github.com/netty/netty/issues/9479
Motivation:
291f80733a introduced a change to use a byte[] to construct the InetAddress when receiving datagram messages to reduce the overhead. Unfortunally it introduced a regression when handling IPv6-mapped-IPv4 addresses and so produced an IndexOutOfBoundsException when trying to fill the byte[] in native code.
Modifications:
- Correctly use the offset on the pointer of the address.
- Add testcase
- Make tests more robust and include more details when the test fails
Result:
No more IndexOutOfBoundsException
Motivation:
At the current moment HttpContentEncoder handle only first value of multiple accept-encoding headers.
Modification:
Join multiple accept-encoding headers to one separated by comma.
Result:
Fixes#9553
Motivation:
There appears to be a thread-safety issue in the way that `SocksAuthRequest` is using its `CharsetEncoder` instance. `CharsetUtil#encoder` returns a cached thread-local encoder instance, so it is not correct to store this instance in a static member variable and reuse it across multiple threads. The result is an occasional `IllegalStateException` as in the following example:
```
java.lang.IllegalStateException: Current state = RESET, new state = FLUSHED
at java.base/java.nio.charset.CharsetEncoder.throwIllegalStateException(CharsetEncoder.java:989)
at java.base/java.nio.charset.CharsetEncoder.flush(CharsetEncoder.java:672)
at java.base/java.nio.charset.CharsetEncoder.encode(CharsetEncoder.java:801)
at java.base/java.nio.charset.CharsetEncoder.canEncode(CharsetEncoder.java:907)
at java.base/java.nio.charset.CharsetEncoder.canEncode(CharsetEncoder.java:982)
at io.netty.handler.codec.socks.SocksAuthRequest.<init>(SocksAuthRequest.java:43)
```
Modification:
Instead of retrieving the thread-local encoder instance once and storing it as a static member instance, the encoder should be retrieved each time the constructor is invoked. This change prevents any potential concurrency issues where multiple threads may end up using the same encoder instance.
Result:
Fixes#9556.
Motivation:
The java doc doesn't match the real case: The exception only happen when a write operation
cannot finish in a certain period of time instead of write idle happen.
Modification:
Correct java doc
Result:
java doc matched the real case
Motivation
Currently every call to get() on a promise results in two reads of the
volatile result field when one would suffice. Maybe this is optimized
away but it seems sensible not to rely on that.
Modification
Reimplement get() and get(...) in DefaultPromise to reduce volatile access.
Result
Fewer volatile reads.
Motivation:
394a1b3485 added support for recvmmsg(...) for unconnected datagram channels, this change also allows to use recvmmsg(...) with connected datagram channels.
Modifications:
- Always try to use recvmmsg(...) if configured to do so
- Adjust unit test to cover it
Result:
Less syscalls when reading datagram packets
