Motivation:
We should keep the scope of the synchronized block as small as possible.
Modifications:
Reduce scope by copy to an array first before iterate through it
Result:
Smaller scope of synchronized
Motivation
As part of a recent commit for issue
https://github.com/netty/netty/issues/9861 the HttpObjectDecoder was
changed to throw an IllegalArgumentException (and produce a failed
decoder result) when decoding a message with both "Transfer-Encoding:
chunked" and "Content-Length".
While it seems correct for Netty to try to sanitize these types of
messages, the spec explicitly mentions that the Content-Length header
should be *removed* in this scenario.
Both Nginx 1.15.9 and Tomcat 9.0.31 also opt to remove the header:
b693d7c198/java/org/apache/coyote/http11/Http11Processor.java (L747-L755)0ad4393e30/src/http/ngx_http_request.c (L1946-L1953)
Modifications
* Change the default behavior from throwing an IllegalArgumentException
to removing the "Content-Length" header
* Extract the behavior to a new protected method,
handleChunkedEncodingWithContentLength(), that can be overridden to
change this behavior (or capture metrics)
Result
Messages of this nature will now be successfully decoded and have their
"Content-Length" header removed, rather than creating invalid messages
(decoder result failures). Users will be allowed to override and
configure this behavior.
Motivation:
Users of the DefaultHttp2ConnectionDecodcer are notified of inbound GoAwayFrames
after the connection has already closed any ignored streams, potentially
losing the signal that some streams may have been ignored by the peer and
are thus retryable.
Modifications:
Reorder the notifications of the frame and connection listeners to
propagate the frame first, giving the frame listeners the opportunity to
clean up ignored streams in their own way.
Result:
Fixes#9986
Motivation:
When an `AddressResolverGroup` closes, it can leave unwanted listeners attached to its `EventExecutor`'s termination future.
Modifications:
- Keep track of listeners attached to termination futures
- Clear listeners if the `AddressResolverGroup` closes before its associated executor(s)
Result:
Unwanted listeners no longer remain in memory after an `AddressResolverGroup` closes before its associated executor(s).
Motivation:
FixedCompositeByteBuf.isDirect() should return the same value as EMPTY_BUFFER.isDirect() when constructed via an empty array
Modifications:
- Return correct value when constructed via empty array.
- Add unit test
Result:
FixedCompositeByteBuf.isDirect() returns correct value
Motivation:
ClearTextHttp2ServerUpgradeHandler is currently more complex then needed. We can simplify it by directly implement the prior-knowledge logic as part of the handler.
Modifications:
Merge inner PriorKnowledgeHandler logic into ClearTextHttp2ServerUpgradeHandler by extending ByteToMessageDecoder directly
Result:
Cleaner code and less pipeline operations
Motivation:
AUTH command is used to login to a SMTP server.
EMPTY command is for request with only parameter.
Modifications:
Add AUTH & EMPTY to SmtpCommand & SmtpRequests
Update SmtpRequestEncoder#writeParameters, handle SP according to
command
Add unit test
Result:
fix#9995
Motivation:
https://github.com/netty/netty/pull/9848 changed how we handled ChannelOptions internally to use a ConcurrentHashMap. This unfortunally had the side-effect that the ordering may be affected and not stable anymore. Here the problem is that sometimes we do validation based on two different ChannelOptions (for example we validate high and low watermarks against each other). Thus even if the user specified the options in the same order we may fail to configure them.
Modifications:
- Use again a LinkedHashMap to preserve order
Result:
Apply ChannelOptions in correct and expected order
Motivation:
https://github.com/netty/netty/pull/9458 changed how we handled ChannelOptions internally to use a ConcurrentHashMap. This unfortunally had the side-effect that the ordering may be affected and not stable anymore. Here the problem is that sometimes we do validation based on two different ChannelOptions (for example we validate high and low watermarks against each other). Thus even if the user specified the options in the same order we may fail to configure them.
Modifications:
- Use again a LinkedHashMap to preserve order
- Add unit test
Result:
Apply ChannelOptions in correct and expected order
Motivation:
8dc6ad5 introduced IPV6-mapped-IPV4 address support but
copied the addresses incorrectly. It copied the first
4 bytes of the ipv6 address to the address byte array
at offset 12, instead of the other way around.
7a547aa implemented this correctly in netty_unix_socket.c
but it seems the change should've been applied to
netty_epoll_native.c as well.
The current behaviour will always set the address to
`0.0.0.0`.
Modifications:
Copy the correct bytes from the ipv6 mapped ipv4 address.
I.e. copy 4 bytes at offset 12 from the native address
to the byte array `addr` at offset 0.
Result:
When using recvmmsg with IPV6-mapped-IPV4 addresses,
the address will be correctly copied to the byte array
`addr` in the NativeDatagramPacket instance.
Motivation:
Sometimes it is useful to do something depending on the Ssl ClientHello (like for example select a SslContext to use). At the moment we only allow to hook into the SNI extension but this is not enough.
Modifications:
Add SslClientHelloHandler which allows to hook into ClientHello messages. This class is now also the super class of AbstractSniHandler
Result:
More flexible processing of SSL handshakes
Motivation:
We hat a typo in the property name that is used in PooledByteBufAllocator.
Modifications:
Change from "...allocation.." to "...allocator.."
Result:
More consistent property naming
Motivation:
Need tests to ensure that CVE-2020-7238 is fixed.
Modifications:
Added two test cases into HttpRequestDecoderTest which check that
no whitespace is allowed before the Transfer-Encoding header.
Result:
Improved test coverage for #9861
Motivation:
42aa7f0c58 did update the checkstyle version but missed that we declared it explicitly in the all artifact as well.
Modifications:
Remove explicit definition in the all artifact.
Result:
Use latest checkstyle version everywhere.
Motivation:
A new checkstyle version was released which fixes a security vulnerability.
Modifications:
- Update to latest checkstyle version
- Update netty-build to latest version to be compatible with latest checkstyle version
Result:
No more security vulnerability caused by checkstyle during build
Motivation:
Due a bug in DefaultChannelPipeline we did not call handlerRemoved(...) if the handlers was removed via removeLast() or removeFirst()
Modifications:
- Correctly implement removeLast() / removeFirst()
- Add unit tests
Result:
handlerRemoved(...) always called on removal
Motivation:
It is impossible to know in advance how much memory will be needed to
decompress a stream of bytes that was compressed using the DEFLATE
algorithm. In theory, up to 1032 times the compressed size could be
needed. For untrusted input, an attacker could exploit this to exhaust
the memory pool.
Modifications:
ZlibDecoder and its subclasses now support an optional limit on the size
of the decompressed buffer. By default, if the limit is reached,
decompression stops and a DecompressionException is thrown. Behavior
upon reaching the limit is modifiable by subclasses in case they desire
something else.
Result:
The decompressed buffer can now be limited to a configurable size, thus
mitigating the possibility of memory pool exhaustion.
Motivation:
If there was always a task in the taskQueue of GlobalEvenExecutor, scheduled tasks in the
scheduledTaskQueue will never be executed.
Related to #1614
Modifications:
fix bug in GlobalEventExecutor#takeTask
Result:
fix bug
Motivation:
JDK is the default SSL provider and internally uses blocking IO operations.
Modifications:
Add allowBlockingCallsInside configuration for SslHandler runAllDelegate function.
Result:
When BlockHound is installed, SSL works out of the box with the default SSL provider.
Co-authored-by: violetagg <milesg78@gmail.com>
Motivation:
Avoid allocation of default static `WebSocketServerProtocolConfig` and `WebSocketClientProtocolConfig` configs. Prefer compile time constants instead.
Modification:
Static field with config object replaced with constructor with default fields.
Result:
No more default config allocation and static field for it. Compile time variables used instead.
Motivation:
When SslHandler.finishWrap throws an exception, ensure that the promise and buf is not reused to avoid throwing IllegalArgumentException or IllegalReferenceCountException which causes the original exception to be lost.
Modification:
The change ensures that the values for the promise and bytebuf are nulled before calling finishWrap so that it will not be called again with the same arguments.
Result:
Fixes#9971 .
Co-authored-by: Norman Maurer <norman_maurer@apple.com>
Co-authored-by: Antony T Curtis <atcurtis@gmail.com>
Motivation:
At the moment we add a handler which will respond with 403 forbidden if a websocket handshake is in progress (and after). This makes not much sense as it is unexpected to have a remote peer to send another http request when the handshake was started. In this case it is much better to let the websocket decoder bail out.
Modifications:
Remove usage of forbiddenHttpRequestResponder
Result:
Fixes https://github.com/netty/netty/issues/9913
Motivation:
We should update the used java11 version when building via docker to the latest release
Modifications:
Update to 1.11.0-6
Result:
Use latest java11 version
Motivation:
Because we now tie and EventLoop to a Channel from the beginning its possible that we schedule something on it before the registration with the Selector is done. This can lead to AssertionErrors in the current form of the code like:
java.lang.AssertionError
at io.netty.channel.nio.AbstractNioChannel.selectionKey(AbstractNioChannel.java:109)
at io.netty.channel.nio.AbstractNioChannel$AbstractNioUnsafe.isFlushPending(AbstractNioChannel.java:344)
at io.netty.channel.nio.AbstractNioChannel$AbstractNioUnsafe.flush0(AbstractNioChannel.java:332)
at io.netty.channel.AbstractChannel$AbstractUnsafe.flush(AbstractChannel.java:855)
at io.netty.channel.DefaultChannelPipeline$HeadHandler.flush(DefaultChannelPipeline.java:1253)
at io.netty.channel.DefaultChannelHandlerContext.invokeFlush(DefaultChannelHandlerContext.java:747)
at io.netty.channel.DefaultChannelHandlerContext.access$1300(DefaultChannelHandlerContext.java:38)
at io.netty.channel.DefaultChannelHandlerContext$WriteAndFlushTask.write(DefaultChannelHandlerContext.java:1144)
at io.netty.channel.DefaultChannelHandlerContext$AbstractWriteTask.run(DefaultChannelHandlerContext.java:1055)
at io.netty.util.concurrent.SingleThreadEventExecutor.runAllTasks(SingleThreadEventExecutor.java:360)
at io.netty.channel.SingleThreadEventLoop.run(SingleThreadEventLoop.java:200)
at io.netty.util.concurrent.SingleThreadEventExecutor.lambda$doStartThread$4(SingleThreadEventExecutor.java:855)
at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:75)
at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
at java.lang.Thread.run(Thread.java:748)
Modifications:
Remove assert and add a few null checks.
Result:
No more java.lang.AssertionError.
Motivation:
Deploying a Micronaut application as GraalVM native image to AWS Lambda with custom runtime fails when using Micronaut Http Client.
This PR initializes at runtime some classes needed to fix the issue. There is more information in our original issue in Micronaut https://github.com/micronaut-projects/micronaut-core/issues/2335#issuecomment-570151944
At this moment I've added those classes into Micronaut (b383d3ab14) as a workaround but this should be included in Netty so it's available for everyone.
Modification:
Mark 3 classes to be initialized at runtime for GraalVM.
Result:
Mark 3 classes to be initialized at runtime for GraalVM.
Motivation
LoggingHandler is a very useful tool for debugging and for tracking the
sequence of events in a pipeline. LoggingHandler also includes the
functionality to log a hex dump of all written and received ByteBufs.
This can be useful for small messages, but for large messages, this can
potentially result in extremely large logs. E.g., a 1 MB payload will
result in over a 1 MB log message being recorded. While LoggingHandler
may only be intended for debugging, this can still be too excessive in
some debugging scenarios.
Modifications
* Create a new ByteBufFormat enum that allows users to specify "SIMPLE"
or "HEX_DUMP" logging for ByteBufs.
* For all constructors that currently accept a LogLevel parameter,
create new overloaded constructors that also accept this enum as a
parameter.
* Continue to record hex dumps by default.
Result
Users will be able to opt out of full hex dump recording, if they wish
to.
Motivation:
At the moment we create a new ChannelFutureListener per chunk when trying to write these to the underlying transport. This can be optimized by replacing the seperate write and flush call with writeAndFlush and only allocate the listener if the future is not complete yet.
Modifications:
- Replace seperate write and flush calls with writeAndFlush
- Only create listener if needed, otherwise execute directly
Result:
Less allocations
Motivation:
At the moment we use an extra field in ChunedWriteHandler to hold the current write. This is not needed and makes sense even more error-prone. We can just peek in the queue.
Modifications:
Use Queue.peek() to keep track of current write
Result:
Less error-prone code
Motivation:
In next major version of netty users should use ChannelHandler everywhere. We should ensure we do the same
Modifications:
Replace usage of deprecated classes / interfaces with ChannelHandler
Result:
Use non-deprecated code
Motivation:
At the moment resolving addresses during connect is done via setting an AddressResolverGroup on the Bootstrap. While this works most of the times as expected sometimes the user want to trigger the connect() from the Channel itself and not via the Bootstrap. For this cases we should provide a ChannelHandler that the user can use that will do the resolution.
Modifications:
Add ResolveAddressHandler and tests
Result:
Be able to resolve addresses without Bootstrap
Motivation:
I'm performing some security research against various Java-based projects as a part of the new [GitHub Security Lab](https://securitylab.github.com/) Bug Bounty program.
Currently, LGTM.com can't create a CodeQL database for this project because of missing dependencies. This fixes the build so it works correctly.
The maintainers may also want to consider enabling the free [LGTM Integration](https://github.com/marketplace/lgtm) once this PR is merged.
Modification:
Adds a `.lgtm.yml` with a tested configuration:
A successful build can be found here:
https://lgtm.com/logs/6695df28f6b2b1d3fd4d03e968b600a3e9c9aecf/lang:java
Result:
LGTM.com will be able to process this repository to create a CodeQL database.
Motivation:
Use latest JCTools, bug fixes etc.
Modification:
Change pom to depend on 3.0.0 version, and ignore shaded jar API changes
Result:
Using newer JCTools
Motivation:
We should remove WebSocketServerExtensionHandler from pipeline after successful WebSocket upgrade even if the client has not selected any extensions.
Modification:
Remove handler once upgrade is complete and no extensions are used.
Result:
Fixes#9939.
Motivation:
The resolver API and implementations should be considered stable by now so we should not mark these with @UnstableApi
Modifications:
Remove @UnstableApi annotation from API and implementation of resolver
Result:
Make it explicit that the API is considered stable
Motivation:
We should close encoder when `LzfEncoder` was removed from pipeline.
Modification:
call `encoder.close` when `handlerRemoved` triggered.
Result:
Close encoder to release internal buffer.
Motivation
Recent optimization #9765 introduced a bug where the native indices of
the internal reused duplicate nio buffer are not properly reset prior to
using it to copy data during a reallocation operation. This can result
in BufferOverflowExceptions thrown during ByteBuf capacity changes.
The code path in question applies only to pooled direct buffers when
Unsafe is disabled or not available.
Modification
Ensure ByteBuffer#clear() is always called on the reused internal nio
buffer prior to returning it from PooledByteBuf#internalNioBuffer()
(protected method); add unit test that exposes the bug.
Result
Fixes#9911
Motivation:
Since "Http2ClientInitializer" creates a new SSLContext Handler without specifying Host, Netty does not add SNI Extension in TLS Client Hello request and the request fails if the server uses SNI to establish TLS Connection.
Modification:
Specified Host while creating a new SSLContext Handler in "Http2ClientInitializer".
Result:
Netty adds SNI Extension of the Host Specified in new SSLContext Handler and sends it with TLS Client Hello request.
Fixes#9815.
Motivation:
41c47b4 introduced a change in an existing testcase which let the build fail when jdkCompatibilityMode is false.
Modifications:
Fix unit tests
Result:
Build passes when jdkCompatibilityMode is false as well
Motivation
A bug was introduced in #9806 which looks likely to be the cause of
#9919. SniHandler will enter an infinite loop if an SSL record is
received with SSL major version byte != 3 (i.e. something other than TLS
or SSL3.0)
Modifications
- Follow default path as intended for majorVersion != 3 in
AbstractSniHandler#decode(...)
- Add unit test to reproduce the hang
Result
Fixes#9919
Motivation:
We can extend `ResourceLeakDetector` through `ResourceLeakDetectorFactory`, and then report the leaked information by covering `reportTracedLeak` and `reportUntracedLeak`. However, the behavior of `reportTracedLeak` and `reportUntracedLeak` is controlled by `logger.isErrorEnabled()`, which is not reasonable. In the case of extending `ResourceLeakDetector`, we sometimes need `needReport` to always return true instead of relying on `logger.isErrorEnabled ()`.
Modification:
introduce `needReport` method and let it be `protected`
Result:
We can control the report leak behavior.
Motivation:
"io.netty.leakDetection.maxSampledRecords" was removed in 16b1dbdf92
Modification:
Replaced it with targetRecords
Result:
Use correct flag during test execution
Motivation:
When `consolidatedWhenNoReadInProgress` is true, `channel.writeAndFlush (data) .addListener (f-> channel.writeAndFlush (data2))` Will cause data2 to never be flushed.
Because the flush operation will synchronously execute the `channel.writeAndFlush (data2))` in the `listener`, and at this time, since the current execution thread is still an `eventloop`(`executor.inEventLoop()` was true), all handlers will be executed synchronously. At this time, since `nextScheduledFlush` is still not null, the `flush` operation of `data2` will be ignored in `FlushConsolidationHandler#scheduleFlush`.
Modification:
- reset `nextScheduledFlush` before `ctx.flush`
- use `ObjectUtil` to polish code
Result:
Fixes https://github.com/netty/netty/issues/9923
Motivation:
Utf8FrameValidator must release the input buffer if the validation fails to ensure no memory leak happens
Modifications:
- Catch exception, release frame and rethrow
- Adjust unit test
Result:
Fixes https://github.com/netty/netty/issues/9906
Motivation:
decodeHexNibble can be a lot faster using a lookup table
Modifications:
decodeHexNibble is made faster by using a lookup table
Result:
decodeHexNibble is faster
Motivation
This PR is a reduced-scope replacement for #8931. It doesn't include the
changes related to how/when discarding read bytes is done, which we plan
to address in subsequent updates.
Modifications
- Avoid copying bytes in COMPOSITE_CUMULATOR in all cases, performing a
shallow copy where necessary; also guard against (unusual) case where
input buffer is composite with writer index != capacity
- Ensure we don't pass a non-contiguous buffer when MERGE_CUMULATOR is
used
- Manually inline some calls to ByteBuf#writeBytes(...) to eliminate
redundant checks and reduce stack depth
Also includes prior minor review comments from @trustin
Result
More correct handling of merge/composite cases and
more efficient handling of composite case.
Motivation:
Currently, characters are appended to the encoded string char-by-char even when no encoding is needed. We can instead separate out codepath that appends the entire string in one go for better `StringBuilder` allocation performance.
Modification:
Only go into char-by-char loop when finding a character that requires encoding.
Result:
The results aren't so clear with noise on my hot laptop - the biggest impact is on long strings, both to reduce resizes of the buffer and also to reduce complexity of the loop. I don't think there's a significant downside though for the cases that hit the slow path.
After
```
Benchmark Mode Cnt Score Error Units
QueryStringEncoderBenchmark.longAscii thrpt 6 1.406 ± 0.069 ops/us
QueryStringEncoderBenchmark.longAsciiFirst thrpt 6 0.046 ± 0.001 ops/us
QueryStringEncoderBenchmark.longUtf8 thrpt 6 0.046 ± 0.001 ops/us
QueryStringEncoderBenchmark.shortAscii thrpt 6 15.781 ± 0.949 ops/us
QueryStringEncoderBenchmark.shortAsciiFirst thrpt 6 3.171 ± 0.232 ops/us
QueryStringEncoderBenchmark.shortUtf8 thrpt 6 3.900 ± 0.667 ops/us
```
Before
```
Benchmark Mode Cnt Score Error Units
QueryStringEncoderBenchmark.longAscii thrpt 6 0.444 ± 0.072 ops/us
QueryStringEncoderBenchmark.longAsciiFirst thrpt 6 0.043 ± 0.002 ops/us
QueryStringEncoderBenchmark.longUtf8 thrpt 6 0.047 ± 0.001 ops/us
QueryStringEncoderBenchmark.shortAscii thrpt 6 16.503 ± 1.015 ops/us
QueryStringEncoderBenchmark.shortAsciiFirst thrpt 6 3.316 ± 0.154 ops/us
QueryStringEncoderBenchmark.shortUtf8 thrpt 6 3.776 ± 0.956 ops/us
```
