Commit Graph

1439 Commits

Author SHA1 Message Date
Aleksandar Gradinac
5dc238341a Delay initialization of io.netty.handler.codec.compression.BrotliDecoder to runtime (#11428)
Motivation:
Including codec-http in the project and building a native-image out of it using a GraalVM 21.2 nightly can result in a failure.

Modification:
By delaying the initialization of `io.netty.handler.codec.compression.BrotliDecoder` to runtime, native-image will not try to eagerly initialize the class during the image build, avoiding the build failure described in the issue.

Result:
Fixes #11427
2021-07-01 15:09:11 +02:00
Stephane Landelle
7aaf9c1b2e Fix HttpHeaderValue#XML_HTTP_REQUEST case (#11433)
Motivation:

HTTP header values are case sensitive. The expected value for `x-request-with` header is `XMLHttpRequest`, not `XmlHttpRequest`.

Modification:

Fix constant's case.

Result:

Correct `XMLHttpRequest` HTTP header value.
2021-07-01 08:13:22 +02:00
Nitesh Kant
df1d4dcd10 HttpUtil#normalizeAndGetContentLength() should handle empty value (#11409)
__Motivation__

`HttpUtil#normalizeAndGetContentLength()` throws `StringIndexOutOfBoundsException` for empty `content-length` values, it should instead throw `IllegalArgumentException` for all invalid values.

__Modification__

- Throw `IllegalArgumentException` if the `content-length` value is empty.
- Add tests

__Result__

Fixes https://github.com/netty/netty/issues/11408
2021-06-23 12:07:28 +02:00
ktqco
e59722037c Accept smaller server_max_window_bits than requested (#11394)
Motivation:

Netty will fail a handshake for the Per-Message Deflate WebSocket
extension if the server response contains a smaller
`server_max_window_bits` value than the client offered.

However, this is allowed by RFC 7692:

> A server accepts an extension negotiation offer with this parameter
> by including the “server_max_window_bits” extension parameter in the
> extension negotiation response to send back to the client with the
> same or smaller value as the offer.

Modifications:

- Allow the server to respond with a smaller value than offered.
- Change the unit tests to test for this.

Result:

The client will not fail when the server indicates it is using a
smaller window size than offered by the client.
2021-06-18 11:57:29 +02:00
Nitesh Kant
998a576f65 HttpUtil.getCharset() fails for charset in double-quotes (#11373)
__Motivation__

As described in https://github.com/netty/netty/issues/11370 we should support quoted charset values

__Modification__

Modify `HttpUtil.getCharset(CharSequence contentTypeValue, Charset defaultCharset)` to trim the double-quotes if present.

__Result__

`HttpUtil.getCharset()` now supports quoted charsets. Fixes https://github.com/netty/netty/issues/11370
2021-06-09 12:43:23 +02:00
Norman Maurer
07baabaac5
Remove Progressive*Promise / Progressive*Future (#11374)
Motivation:

This special case implementation of Promise / Future requires the implementations responsible for completing the promise to have knowledge of this class to provide value. It also requires that the implementations are able to provide intermediate status while the work is being done. Even throughout the core of Netty it is not really supported most of the times and so just brings more complexity without real gain.

Let's remove it completely which is better then only support it sometimes.

Modifications:

Remove Progressive* API

Result:

Code cleanup.... Fixes https://github.com/netty/netty/issues/8519
2021-06-09 08:32:38 +02:00
Norman Maurer
abdaa769de
Remove Void*Promise (#11348)
Motivation:

Sometime in the past we introduced the concept of Void*Promise. As it turned out this was not a good idea at all as basically each handler in the pipeline need to be very careful to correctly handle this. We should better just remove this "optimization".

Modifications:

- Remove Void*Promise and all the related APIs
- Remove tests which were related to Void*Promise

Result:

Less error-prone API
2021-06-08 14:22:16 +02:00
Violeta Georgieva
1a28c26b4b Fix ServerCookieDecoder javadoc (#11372)
Motivation:
When decoding the cookies on the server, the "Cookie" HTTP request header value should be considered.
The "Set-Cookie" HTTP response header is used to send cookies from the server to the user agent.

Modification:
- Specify in javadoc that the "Cookie" HTTP request header value should be considered and
not the "Set-Cookie" HTTP response header value.

Result:
Correct ServerCookieDecoder javadoc
2021-06-07 16:37:15 -07:00
Stuart Douglas
f1db0b9014 Fix issue if encoding is enabled but not used (#11358)
Motivation:

Fixes an IllegalReferenceCountException

Modification:

Retained the buffers so the encoder works correctly.

Result:

Fixes #11357
2021-06-07 08:52:57 +02:00
skyguard1
7c0662007d Add default block in HttpObjectDecoder (#11342)
Motivation:

There should always be a default in switch blocks.

Modification:

Add default

Result:

Code cleanup.

Signed-off-by: xingrufei <xingrufei@sogou-inc.com>
2021-06-02 08:42:09 +02:00
skyguard1
a975f42d6e Add default block in HttpClientCodec (#11352)
Motivation:

There should always be a default in switch blocks.

Modification:

Add default

Result:

Code cleanup.

Signed-off-by: xingrufei <xingrufei@sogou-inc.com>
2021-06-02 08:42:00 +02:00
Frédéric Brégier
cebf46af40 HttpPostMultipartRequestDecoder IndexOutOfBoundsException error (#11335)
Motivation:

When searching for the delimiter, the decoder part within HttpPostBodyUtil
was not checking the left space to check if it could be included or not,
while it should.

Modifications:

Add a check on toRead being greater or equal than delimiterLength before
going within the loop. If the check is wrong, the delimiter is obviously not found.

Add a Junit test to preserve regression.

Result:

No more IndexOutOfBoundsException

Fixes #11334
2021-05-31 08:43:59 +02:00
skyguard1
02c3712067 Add default block in DefaultHttpHeaders (#11329)
Motivation:

Every switch block should also have a default case.

Modification:

Add default block in DefaultHttpHeaders to ensure we not fall-through by mistake

Result:

Cleanup

Signed-off-by: xingrufei <xingrufei@sogou-inc.com>
2021-05-28 08:48:15 +02:00
Riley Park
65e1f0e27c
Migrate codec-http tests to JUnit 5 (#11316)
Motivation:

JUnit 5 is more expressive, extensible, and composable in many ways, and it's better able to run tests in parallel.

Modifications:

Use JUnit5 in tests

Result:

Related to https://github.com/netty/netty/issues/10757
2021-05-27 09:06:23 +02:00
Norman Maurer
c44a8d7e1d Add builds for windows (#11284)
Motivation:

Let's also build on windows during PR validation

Modifications:

Add build on windows during PR

Result:

Validate that all also pass on windows
2021-05-26 12:12:30 +02:00
Trustin Lee
b487f71821 Provide a way to pass through a certain HTTP upgrade request (#11267)
Motivation:

A user might want to handle a certain HTTP upgrade request differently
than what `HttpServerUpgradeHandler` does by default. For example, a
user could let `HttpServerUpgradeHandler` handle HTTP/2 upgrades but
not WebSocket upgrades.

Modifications:

- Added `HttpServerUpgradeHandler.isUpgrade(HttpRequest)` so a user can
  tell `HttpServerUpgradeHandler` to pass the request as it is to the
  next handler.

Result:

- A user can handle a certain upgrade request specially.
2021-05-18 11:47:58 +02:00
Stephane Landelle
92ff402f0f Introduce BrotliDecoder (#10960)
Motivation:

Netty lacks client side support for decompressing Brotli compressed response bodies.

Modification:

* Introduce optional dependency to brotli4j by @hyperxpro. It will be up to the user to provide the brotli4j libraries for the target platform in the classpath. brotli4j is currently available for Linux, OSX and Windows, all for x86 only.
* Introduce BrotliDecoder in codec module
* Plug it onto `HttpContentDecompressor` for HTTP/1 and `DelegatingDecompressorFrameListener` for HTTP/2
* Add test in `HttpContentDecoderTest`
* Add `BrotliDecoderTest` that doesn't extend `AbstractDecoderTest` that looks flaky

Result:

Netty now support decompressing Brotli compressed response bodies.
2021-05-10 15:39:01 +02:00
Norman Maurer
127644235e Move HttpPostMultiPartRequestDecoder specific tests to HttpPostMultiPartRequestDecoderTest
Motivation:

Some of the HttpPostMultiPartRequestDecoder specific tests were included in HttpPostRequestDecoderTest. We should better move these in the correct test class.

Modifications:

Move specific tests

Result:

Cleanup
2021-04-29 16:01:03 +02:00
Frédéric Brégier
5309422669 Fix Memory release not correctly in Multipart Decoder (#11188)
Motivation:
2 years ago a change remove the default clearing of all HttpData, whatever
they are disk based or memory based.

A lot of users were probably releasing HttpData directly, so there was no issue.
But now, it seems, and as the Javadoc said, that `decoder.destroy()` shall clean up
also Memory based HttpData, and not only Disk based HttpData as currently.

Change:
- Add in `destroy()` method the necessary code to release if necessary
the underlying Memory based HttpDatas.

- Change one Junit Test (using Mixed, Memory and Disk based factories)
in order to check the correctness of this behavior and to really act
as a handler (releasing buffers or requests).

- Modify one Junit core to check validity when a delimiter is present in the Chunk
but not CRLF/LF (false delimiter), to ensure correctness.

Result:
No more issue on memory leak

Note that still the List and the Map are not cleaned, since they were not
before. No change is done on this, since it could produce backward issue compatibility.

Fix issues #11175 and #11184
2021-04-29 12:27:27 +02:00
Norman Maurer
8636aad989 Destroy HttpPostMultipartRequestDecoder if contructor throws (#11207)
Motivation:

We need to call destroy() if the constructor of HttpPostMultipartRequestDecoder throws as otherwise we may leak memory.

Modifications:

- Call destroy() if we throw
- Add unit test

Result:

No more leaks when constructor throws

Co-authored-by: Frederic Bregier <frederic.bregier@waarp.fr>
2021-04-28 12:27:37 +02:00
Norman Maurer
4c11ce7241 Correctly throw ErrorDataDecoderException for errors while decoding (#11198)
Motivation:

We didn't correctly handle the case when no content-type header was found or if the charset was illegal and just did throw a NPE or ICE. We should in both cases throw an ErrorDataDecoderException to reflect what is documented in the javadocs.

Modifications:

- Throw correct exception
- Merge private method into the constructor as it is only used there
- Add unit tests

Result:

Throw expected exceptions on decoding errors
2021-04-27 16:37:51 +02:00
roy
636244c287 adjust validation logic when websocket server check starts with '/' (#11191)
Motivation:

When create a WebSocketServerProtocolConfig to check URI path starts from '/',
only '/' or '//subPath' can be passed by the checker,but '/subPath' should be
passed as well

Modifications:

in `WebSocketServerProtocolHandshakeHandler.isWebSocketPath()` treat '/' a special case

Result:
'/subPath' can be passed
2021-04-26 10:32:41 +02:00
Boris Unckel
10758eef73 Utilize i.n.u.internal.ObjectUtil to assert Preconditions (codec-http) (#11170) (#11187)
Motivation:

NullChecks resulting in a NullPointerException or IllegalArgumentException, numeric ranges (>0, >=0) checks, not empty strings/arrays checks must never be anonymous but with the parameter or variable name which is checked. They must be specific and should not be done with an "OR-Logic" (if a == null || b == null) throw new NullPointerEx.

Modifications:

* import static relevant checks
* Replace manual checks with ObjectUtil methods

Result:

All checks needed are done with ObjectUtil, some exception texts are improved.

Fixes #11170
2021-04-23 08:18:02 +02:00
Norman Maurer
a270a4b84c Add WebSocketClientHandshaker / WebSocketServerHandshaker close methods that take ChannelHandlerContext as parameter (#11171)
Motivation:

At the moment we only expose close(...) methods that take a Channel as paramater. This can be problematic as the write will start at the end of the pipeline which may contain ChannelOutboundHandler implementations that not expect WebSocketFrame objects. We should better also support to pass in a ChannelHandlerContext as starting point for the write which ensures that the WebSocketFrame objects will be handled correctly from this position of the pipeline.

Modifications:

- Add new close(...) methods that take a ChannelHandlerContext
- Add javadoc sentence to point users to the new methods.

Result:

Be able to "start" the close at the right position in the pipeline.
2021-04-21 15:22:34 +02:00
skyguard1
5cdfd57298 [Clean] Remove useless code (#11154)
Motivation:
There is an unused field

Modifications:
Remove useless code

Result:
Code cleanup

Co-authored-by: xingrufei <xingrufei@sogou-inc.com>
2021-04-19 10:10:07 +02:00
Frédéric Brégier
42dc696c6c Fix behavior of HttpPostMultipartRequestDecoder for Memory based Factory (#11145)
Motivation:
When Memory based Factory is used, if the first chunk starts with Line Break, the HttpData
is not filled with the current available buffer if the delimiter is not found yet, while it may
add some.

Fix JavaDoc to note potential wrong usage of content() or getByteBuf() if HttpDatais has
a huge content with the risk of Out Of Memory Exception.

Fix JavaDoc to explain how to release properly the Factory, whatever it is in Memory,
Disk or Mixed mode.

Fix issue #11143

Modifications:
First, when the delimiter is not found, instead of searching Line Break from readerIndex(), we should search
from readerIndex() + readableBytes() - delimiter size, since this is the only part where usefull
Line Break could be searched for, except if readableBytes is less than delimiter size (then we search from
readerIndex).

Second, when a Memory HttpData is created, it should be assigned an empty buffer to be
consistent with the other implementations (Disk or Mixed mode).
We cannot change the default behavior of the content() or getByteBuf() of the Memory based HttpData
since the ByteBuf is supposed to be null when released, but not empty.
When a new ByteBuf is added, one more check verifies if the current ByteBuf is empty, and if so, it
is released and replaced by the new one, without creating a new CompositeByteBuf.

Result:
In the tests testBIgFileUploadDelimiterInMiddleChunkDecoderMemoryFactory and related for other modes,
the buffers are starting with a CRLF.
When we offer only the prefix part of the multipart (no data at all), the current Partial HttpData has
an empty buffer.
The first time we offer the data starting with CRLF to the decoder, it now
has a correct current Partial HttpData with a buffer not empty.

The Benchmark was re-run against this new version.

Old Benchmark                                                                       Mode  Cnt  Score   Error   Units
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigAdvancedLevel   thrpt    6  4,037 ± 0,358  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigDisabledLevel   thrpt    6  4,226 ± 0,471  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigParanoidLevel   thrpt    6  0,875 ± 0,029  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigSimpleLevel     thrpt    6  4,346 ± 0,275  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighAdvancedLevel  thrpt    6  2,044 ± 0,020  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighDisabledLevel  thrpt    6  2,278 ± 0,159  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighParanoidLevel  thrpt    6  0,174 ± 0,004  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighSimpleLevel    thrpt    6  2,370 ± 0,065  ops/ms

New Benchmark                                                                       Mode  Cnt  Score   Error   Units
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigAdvancedLevel   thrpt    6  5,604 ± 0,415  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigDisabledLevel   thrpt    6  6,058 ± 0,111  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigParanoidLevel   thrpt    6  0,914 ± 0,031  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigSimpleLevel     thrpt    6  6,053 ± 0,051  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighAdvancedLevel  thrpt    6  2,636 ± 0,141  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighDisabledLevel  thrpt    6  3,033 ± 0,181  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighParanoidLevel  thrpt    6  0,178 ± 0,006  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighSimpleLevel    thrpt    6  2,859 ± 0,189  ops/ms

So +20 to +40% improvement due to not searching for CRLF/LF into the full buffer when no delimiter is found,
but only from the end and delimiter size + 2 (CRLF).
2021-04-16 11:05:09 +02:00
Dmitriy Dumanskiy
9403ceaeae remove unnecessary check in WebSocketFrameDecoder (#11113)
Motivation:

There are some redundant checks and so these can be removed

Modifications:

- First check frameOpcode != OPCODE_PING is removed because the code executed int the branch where frameOpcode  <= 7, while OPCODE_PING is 9.
- Second check frameOpcode != OPCODE_PING is removed because its checked before.

Result:

Code cleanup
2021-03-29 09:02:00 +02:00
Stuart Douglas
56703b93ba
Fix exception if Response has content (#11093)
Motivation:

If compression is enabled and the HttpResponse also
implements HttpContent (but not LastHttpContent) then
the buffer will be freed to eagerly.

Modification:

I retain the buffer the same way that is done for the LastHttpContent case.

Note that there is another suspicious looking call a few lines above (if beginEncode returns null). I am not sure if this should also be retained.

Result:

Fixes #11092
2021-03-21 14:50:05 +01:00
Norman Maurer
8da6ed3fc0 Merge pull request from GHSA-wm47-8v5p-wjpj
Motivation:

As stated by https://tools.ietf.org/html/rfc7540#section-8.1.2.6 we should report a stream error if the content-length does not match the sum of all data frames.

Modifications:

- Verify that the sum of data frames match if a content-length header was send.
- Handle multiple content-length headers and also handle negative values
- Add io.netty.http2.validateContentLength system property which allows to disable the more strict validation
- Add unit tests

Result:

Correctly handle the case when the content-length header was included but not match what is send and also when content-length header is invalid
2021-03-14 14:15:22 +01:00
Bennett Lynch
00eb5618df Introduce HttpMessageDecoderResult to expose decoded header size (#11068)
Motivation

The HttpObjectDecoder accepts input parameters for maxInitialLineLength
and maxHeaderSize. These are important variables since both message
components must be buffered in memory. As such, many decoders (like
Netty and others) introduce constraints. Due to their importance, many
users may wish to add instrumentation on the values of successful
decoder results, or otherwise be able to access these values to enforce
their own supplemental constraints.

While users can perhaps estimate the sizes today, they will not be
exact, due to the decoder being responsible for consuming optional
whitespace and the like.

Modifications

* Add HttpMessageDecoderResult class. This class extends DecoderResult
and is intended for HttpMessage objects successfully decoded by the
HttpObjectDecoder. It exposes attributes for the decoded
initialLineLength and headerSize.
* Modify HttpObjectDecoder to produce HttpMessageDecoderResults upon
successfully decoding the last HTTP header.
* Add corresponding tests to HttpRequestDecoderTest &
HttpResponseDecoderTest.

Co-authored-by: Bennett Lynch <Bennett-Lynch@users.noreply.github.com>
2021-03-12 13:50:51 +01:00
Bennett Lynch
172472c0c0 [HttpObjectDecoder] Include hex-value of illegal whitespace char (#11067)
Motivation:

HttpObjectDecoder may throw an IllegalArgumentException if it encounters
a character that Character.isWhitespace() returns true for, but is not
one of the two valid OWS (optional whitespace) values. Such values may
not have friendly or readable toString() values. We should include the
hex value so that the illegal character can always be determined.

Modifications:

Add hex value as well

Result:

Easier to debug 

Co-authored-by: Bennett Lynch <Bennett-Lynch@users.noreply.github.com>
2021-03-09 08:13:44 +01:00
Chris Vest
ec18aa8731
Introduce ByteBufConvertible interface (#11036)
Motivation:
To make it possible to experiment with alternative buffer implementations, we need a way to abstract away the concrete buffers used throughout most of the Netty pipelines, while still having a common currency for doing IO in the end.

Modification:
- Introduce an ByteBufConvertible interface, that allow arbitrary objects to convert themselves into ByteBuf objects.
- Every place in the code, where we did an instanceof check for ByteBuf, we now do an instanceof check for ByteBufConvertible.
- ByteBuf itself implements ByteBufConvertible, and returns itself from the asByteBuf method.

Result:
It is now possible to use Netty with alternative buffer implementations, as long as they can be converted to ByteBuf.
This has been verified elsewhere, with an alternative buffer implementation.
2021-02-26 15:03:58 +01:00
Frédéric Brégier
d421ae10d7 Minimize get byte multipart and fix buffer reuse (#11001)
Motivation:
- Underlying buffer usages might be erroneous when releasing them internaly
in HttpPostMultipartRequestDecoder.

2 bugs occurs:
1) Final File upload seems not to be of the right size.
2) Memory, even in Disk mode, is increasing continuously, while it shouldn't.

- Method `getByte(position)` is too often called within the current implementation
of the HttpPostMultipartRequestDecoder.
This implies too much activities which is visible when PARANOID mode is active.
This is also true in standard mode.

Apply the same fix on buffer from HttpPostMultipartRequestDecoder to HttpPostStandardRequestDecoder
made previously.

Finally in order to ensure we do not rewrite already decoded HttpData when decoding
next ones within multipart, we must ensure the buffers are copied and not a retained slice.

Modification:
- Add some tests to check consistency for HttpPostMultipartRequestDecoder.
Add a package protected method for testing purpose only.

- Use the `bytesBefore(...)` method instead of `getByte(pos)` in order to limit the external
access to the underlying buffer by retrieving iteratively the beginning of a correct start
position.
It is used to find both LF/CRLF and delimiter.
2 methods in HttpPostBodyUtil were created for that.

The undecodedChunk is copied when adding a chunk to a DataMultipart is loaded.
The same buffer is also rewritten in order to release the copied memory part.

Result:

Just for note, for both Memory or Disk or Mixed mode factories, the release has to be done as:

      for (InterfaceHttpData httpData: decoder.getBodyHttpDatas()) {
          httpData.release();
          factory.removeHttpDataFromClean(request, httpData);
      }
      factory.cleanAllHttpData();
      decoder.destroy();

The memory used is minimal in Disk or Mixed mode. In Memory mode, a big file is still
in memory but not more in the undecodedChunk but its own buffer (copied).

In terms of benchmarking, the results are:

Original code Benchmark                                                             Mode  Cnt  Score    Error   Units
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigAdvancedLevel   thrpt    6  0,152 ±  0,100  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigDisabledLevel   thrpt    6  0,543 ±  0,218  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigParanoidLevel   thrpt    6  0,001 ±  0,001  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigSimpleLevel     thrpt    6  0,615 ±  0,070  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighAdvancedLevel  thrpt    6  0,114 ±  0,063  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighDisabledLevel  thrpt    6  0,664 ±  0,034  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighParanoidLevel  thrpt    6  0,001 ±  0,001  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighSimpleLevel    thrpt    6  0,620 ±  0,140  ops/ms

New code Benchmark                                                                  Mode  Cnt  Score   Error   Units
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigAdvancedLevel   thrpt    6  4,037 ± 0,358  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigDisabledLevel   thrpt    6  4,226 ± 0,471  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigParanoidLevel   thrpt    6  0,875 ± 0,029  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigSimpleLevel     thrpt    6  4,346 ± 0,275  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighAdvancedLevel  thrpt    6  2,044 ± 0,020  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighDisabledLevel  thrpt    6  2,278 ± 0,159  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighParanoidLevel  thrpt    6  0,174 ± 0,004  ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighSimpleLevel    thrpt    6  2,370 ± 0,065  ops/ms

In short, using big file transfers, this is about 7 times faster with new code, while
using high number of HttpData, this is about 4 times faster with new code when using Simple Level.
When using Paranoid Level, using big file transfers, this is about 800 times faster with new code, while
using high number of HttpData, this is about 170 times faster with new code.
2021-02-26 14:26:24 +01:00
strugcoder
c1b3ffafcf Simplity some code (#11000)
Motivation:

There was some code that could be simplified.

Modification:

Simplify code.

Result:

Code cleanup
2021-02-11 09:06:16 +01:00
Norman Maurer
9c2de76add Use Files.createTempFile(...) to ensure the file is created with proper permissions
Motivation:

File.createTempFile(String, String)` will create a temporary file in the system temporary directory if the 'java.io.tmpdir'. The permissions on that file utilize the umask. In a majority of cases, this means that the file that java creates has the permissions: `-rw-r--r--`, thus, any other local user on that system can read the contents of that file.
This can be a security concern if any sensitive data is stored in this file.

This was reported by Jonathan Leitschuh <jonathan.leitschuh@gmail.com> as a security problem.

Modifications:

Use Files.createTempFile(...) which will use safe-defaults when running on java 7 and later. If running on java 6 there isnt much we can do, which is fair enough as java 6 shouldnt be considered "safe" anyway.

Result:

Create temporary files with sane permissions by default.
2021-02-08 18:17:31 +01:00
Norman Maurer
d7fb0f5c0b Revert HttpPostMultipartRequestDecoder and HttpPostStandardRequestDecoder to e5951d46fc (#10989)
Motivation:

The changes introduced in 1c230405fd did cause various issues while the fix itself is not considered critical. For now it is considered the best to just rollback and investigate more.

Modifications:

- Revert changes done in 1c230405fd (and later) for
the post decoders.
- Ensure we give memory back to the system as soon as possible in a safe manner

Result:

Fixes https://github.com/netty/netty/issues/10973
2021-02-03 20:41:28 +01:00
Stephane Landelle
6c041c3104 Merge WebSocket extensions, close #10792 (#10956)
Motivation:

We currently append extensions to the user defined "sec-websocket-extensions" headers. This can cause duplicated entries.

Modifications:

* Replace existing `WebSocketExtensionUtil#appendExtension` private helper with a new `computeMergeExtensionsHeaderValue`. User defined parameters have higher precedence.
* Add tests (existing method wasn't tested)
* Reuse code for both client and server side (code was duplicated).

Result:

No more duplicated entries when user defined extensions overlap with the ones Netty generated.
2021-01-22 08:50:25 +01:00
Aayush Atharva
ab340e1de0 Change switch to if (#10880)
Motivation:
switch is used when we have a good amount of cases because switch is faster than if-else. However, we're using only 1 case in switch which can affect performance.

Modification:
Changed switch to if.

Result:
Good code.
2020-12-22 19:25:52 +01:00
James Kleeh
88cb2113ba Fix infinite loop (#10855)
Motivation:

To fix the infinite loop parsing a multipart body.

Modifications:

Modified the loop to use the correct variable.

Result:

Multipart bodies will be parsed correctly again.
2020-12-11 14:15:57 +01:00
Violeta Georgieva
591b0f69bc Enforce status code validation in CloseWebSocketFrame (#10846)
Motivation:

According to specification 1006 status code must not be set as a status code in a
Close control frame by the endpoint. However 1006 status code can be
used in applications to indicate that the connection was closed abnormally.

Modifications:

- Enforce status code validation in CloseWebSocketFrame
- Add WebSocketCloseStatus construction with disabled validation
- Add test

Result:

Fixes #10838
2020-12-07 13:20:36 +01:00
Aayush Atharva
376bb0dcf4 Add state in exception message (#10842)
Motivation:
We should add `state` in the exception message of `HttpObjectEncoder` because it makes debugging a little easier.

Modification:
Added `state` in the exception message.

Result:
Better exception message for smooth debugging.
2020-12-07 10:49:29 +01:00
Andrey Mizurov
379d08615f Override Sec-WebSocket-Protocol websocket handshake response header after custom headers to avoid duplication (#10793)
Motivation:

According rfc (https://tools.ietf.org/html/rfc6455#section-11.3.4), `Sec-WebSocket-Protocol` header field MUST NOT appear
more than once in an HTTP response.
At the moment we can pass `Sec-WebSocket-Protocol`  via custom headers and it will be added to response.

Modification:

Change method add() to set() for avoid duplication. If we pass sub protocols in handshaker constructor it means that they are preferred over custom ones.

Result:

Less error prone behavior.
2020-11-19 09:50:06 +01:00
Frédéric Brégier
3a58063fe7 Fix for performance regression on HttpPost RequestDecoder (#10623)
Fix issue #10508 where PARANOID mode slow down about 1000 times compared to ADVANCED.
Also fix a rare issue when internal buffer was growing over a limit, it was partially discarded
using `discardReadBytes()` which causes bad changes within previously discovered HttpData.

Reasons were:

Too many `readByte()` method calls while other ways exist (such as keep in memory the last scan position when trying to find a delimiter or using `bytesBefore(firstByte)` instead of looping externally).

Changes done:
- major change on way buffer are parsed: instead of read byte per byte until found delimiter, try to find the delimiter using `bytesBefore()` and keep the last unfound position to skeep already parsed parts (algorithms are the same but implementation of scan are different)
- Change the condition to discard read bytes when refCnt is at most 1.

Observations using Async-Profiler:
==================================

1) Without optimizations, most of the time (more than 95%) is through `readByte()` method within `loadDataMultipartStandard` method.
2) With using `bytesBefore(byte)` instead of `readByte()` to find various delimiter, the `loadDataMultipartStandard` method is going down to 19 to 33% depending on the test used. the `readByte()` method or equivalent `getByte(pos)` method are going down to 15% (from 95%).

Times are confirming those profiling:
- With optimizations, in SIMPLE mode about 82% better, in ADVANCED mode about 79% better and in PARANOID mode about 99% better (most of the duplicate read accesses are removed or make internally through `bytesBefore(byte)` method)

A benchmark is added to show the behavior of the various cases (one big item, such as File upload, and many items) and various level of detection (Disabled, Simple, Advanced, Paranoid). This benchmark is intend to alert if new implementations make too many differences (such as the previous version where about PARANOID gives about 1000 times slower than other levels, while it is now about at most 10 times).

Extract of Benchmark run:
=========================

Run complete. Total time: 00:13:27

Benchmark                                                                           Mode  Cnt  Score   Error   Units
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigAdvancedLevel   thrpt    6  2,248 ± 0,198 ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigDisabledLevel   thrpt    6  2,067 ± 1,219 ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigParanoidLevel   thrpt    6  1,109 ± 0,038 ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderBigSimpleLevel     thrpt    6  2,326 ± 0,314 ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighAdvancedLevel  thrpt    6  1,444 ± 0,226 ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighDisabledLevel  thrpt    6  1,462 ± 0,642 ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighParanoidLevel  thrpt    6  0,159 ± 0,003 ops/ms
HttpPostMultipartRequestDecoderBenchmark.multipartRequestDecoderHighSimpleLevel    thrpt    6  1,522 ± 0,049 ops/ms
2020-11-19 08:01:05 +01:00
Norman Maurer
eeece4cfa5 Use http in xmlns URIs to make maven release plugin happy again (#10788)
Motivation:

https in xmlns URIs does not work and will let the maven release plugin fail:

```
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  1.779 s
[INFO] Finished at: 2020-11-10T07:45:21Z
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-release-plugin:2.5.3:prepare (default-cli) on project netty-parent: Execution default-cli of goal org.apache.maven.plugins:maven-release-plugin:2.5.3:prepare failed: The namespace xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" could not be added as a namespace to "project": The namespace prefix "xsi" collides with an additional namespace declared by the element -> [Help 1]
[ERROR]
```

See also https://issues.apache.org/jira/browse/HBASE-24014.

Modifications:

Use http for xmlns

Result:

Be able to use maven release plugin
2020-11-10 10:51:05 +01:00
Chris Vest
10af555f46
ByteProcessor shouldn't throw checked exception (#10767)
Motivation:
There is no need for ByteProcessor to throw a checked exception.
The declared checked exception causes unnecessary code complications just to propagate it.
This can be cleaned up.

Modification:
ByteProcessor.process no longer declares to throw a checked exception, and all the places that were trying to cope with the checked exception have been simplified.

Result:
Simpler code.
2020-11-03 18:54:16 +01:00
Chris Vest
ff2e790e89 Revert "ByteProcessor shouldn't throw checked exception"
This reverts commit b70d0fa6e3.
2020-11-03 16:12:54 +01:00
Chris Vest
b70d0fa6e3 ByteProcessor shouldn't throw checked exception
Motivation:
There is no need for ByteProcessor to throw a checked exception.
The declared checked exception causes unnecessary code complications just to propagate it.
This can be cleaned up.

Modification:
ByteProcessor.process no longer declares to throw a checked exception, and all the places that were trying to cope with the checked exception have been simplified.

Result:
Simpler code.
2020-11-03 16:12:13 +01:00
Artem Smotrakov
3e41a7f231 Enable header valication in HttpServerUpgradeHandler (#10643)
Motivation:

HttpServerUpgradeHandler takes a list of protocols from an incoming
request and uses them for building a response.
Although the class does some validation while parsing the list,
it then disables HTTP header validation when it builds a responst.
The disabled validation may potentially allow
HTTP response splitting attacks.

Modifications:

- Enabled HTTP header validation in HttpServerUpgradeHandler
  as a defense-in-depth measure to prevent possible
  HTTP response splitting attacks.
- Added a new constructor that allows disabling the validation.

Result:

HttpServerUpgradeHandler validates incoming protocols
before including them into a response.
That should prevent possible HTTP response splitting attacks.
2020-10-30 11:29:45 +01:00
Norman Maurer
930aec7f0a Fix checkstyle errors introduced by 33de96f448 2020-10-24 14:50:52 +02:00
Andrey Mizurov
634a6b70d7 Provide new client and server websocket handshake exceptions (#10646)
Motivation:

At the moment we have only one base `WebSocketHandshakeException` for handling WebSocket upgrade issues.
Unfortunately, this message contains only a string message about the cause of the failure, which is inconvenient in handling.

Modification:

Provide new `WebSocketClientHandshakeException` with `HttpResponse` field  and `WebSocketServerHandshakeException` with `HttpRequest` field both of them without content for avoid reference counting
problems.

Result:

More information for more flexible handling.

Fixes #10277 #4528 #10639.
2020-10-24 14:44:24 +02:00