Commit Graph

1200 Commits

Author SHA1 Message Date
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
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
Artem Smotrakov
b8ae2a2af4 Enable nohttp check during the build (#10708)
Motivation:

HTTP is a plaintext protocol which means that someone may be able
to eavesdrop the data. To prevent this, HTTPS should be used whenever
possible. However, maintaining using https:// in all URLs may be
difficult. The nohttp tool can help here. The tool scans all the files
in a repository and reports where http:// is used.

Modifications:

- Added nohttp (via checkstyle) into the build process.
- Suppressed findings for the websites
  that don't support HTTPS or that are not reachable

Result:

- Prevent using HTTP in the future.
- Encourage users to use HTTPS when they follow the links they found in
  the code.
2020-10-23 15:26:25 +02:00
Stuart Douglas
7d971a78a0 Minor performance improvement in websocket upgrade (#10710)
Motivation:

I noticed WebSocketServerExtensionHandler taking up a non-trivial
amount of CPU time for a non-websocket based menchmark. This attempts
to speed it up.

Modifications:

- It is faster to check for a 101 response than to look at headers,
so an initial response code check is done
- Move all the actual upgrade code into its own method to increase
chance of this method being inlined
- Add an extra contains() check for the upgrade header, to avoid
allocating an iterator if there is no upgrade header

Result:

A small but noticable performance increase.

Signed-off-by: Stuart Douglas <stuart.w.douglas@gmail.com>
2020-10-21 13:00:08 +02:00
Norman Maurer
3f2c5ccd46 Replace deprecated Assert.assertThat(...) with MatcherAssert.assertThat(...) (#10699)
Motivation:

junit deprecated Assert.assertThat(...)

Modifications:

Use MatcherAssert.assertThat(...) as replacement for deprecated method

Result:

Less deprecation warnings
2020-10-18 14:55:21 +02:00
Artem Smotrakov
f0448d6a8a Fix or suppress LGTM findings (#10689)
Motivation:

LGTM reports multiple issues. They need to be triaged,
and real ones should be fixed.

Modifications:
- Fixed multiple issues reported by LGTM, such as redundant conditions,
  resource leaks, typos, possible integer overflows.
- Suppressed false-positives.
- Added a few testcases.

Result:

Fixed several possible issues, get rid of false alarms in the LGTM report.
2020-10-17 09:57:52 +02:00
Aayush Atharva
da8412a054 Update OWASP Links in Cookie class (#10677)
Motivation:
Fix Broken Link of OWASP HttpOnly Cookie in Cookie class.

Modification:
Updated the broken link.

Result:
Broken Link Fix for better Documentation.
2020-10-15 14:08:56 +02:00
Aayush Atharva
473471bac6 Use ObjectUtil for multiple operations (#10679)
Motivation:
We should use ObjectUtil for checking if Compression parameters are in range. This will reduce LOC and make code more readable.

Modification:
Used ObjectUtil

Result:
More readable code
2020-10-14 12:13:10 +02:00
Artem Smotrakov
97c7c00dd5 Avoid casting numbers to narrower types (#10645)
Motivation:

Avoid implicit conversions to narrower types in
AbstractMemoryHttpData and Bzip2HuffmanStageEncoder classes
reported by LGTM.

Modifications:

Updated the classes to avoid implicit casting to narrower types.
It doesn't look like that an integer overflow is possible there,
therefore no checks for overflows were added.

Result:

No warnings about implicit conversions to narrower types.
2020-10-12 09:34:06 +02:00
Artem Smotrakov
327071c7b3 Suppress warnings about weak hash algorithms (#10647)
Motivation:

LGTM reported that WebSocketUtil uses MD5 and SHA-1
that are considered weak. Although those algorithms
are insecure, they are required by draft-ietf-hybi-thewebsocketprotocol-00
specification that is implemented in the corresponding WebSocket
handshakers. Once the handshakers are removed, WebSocketUtil can be
updated to stop using those weak hash functions.

Modifications:

Added SuppressWarnings annotations.

Result:

Suppressed warnings.
2020-10-12 09:24:29 +02:00
Doyun Geum
2233d1ca17 Add validation check about websocket path (#10583)
Add validation check about websocket path

Motivation:

I add websocket handler in custom server with netty.
I first add WebSocketServerProtocolHandler in my channel pipeline.
It does work! but I found that it can pass "/websocketabc". (websocketPath is "/websocket")

Modification:
`isWebSocketPath()` method of `WebSocketServerProtocolHandshakeHandler` now checks that "startsWith" applies to the first URL path component, rather than the URL as a string.

Result:
Requests to "/websocketabc" are no longer passed to handlers for requests that starts-with "/websocket".
2020-10-08 12:07:10 +02:00
Chris Vest
86c8f24d9a
Replace UNSAFE.throwException with alternatives supported on Java 8 (#10629)
Motivation:
 We wish to use Unsafe as little as possible, and Java 8 allows us
 to take some short-cuts or play some tricks with generics,
 for the purpose of working around having to declare all checked
 exceptions. Ideally all checked exceptions would be declared, but
 the code base is not ready for that yet.

Modification:
 The call to UNSAFE.throwException has been removed, so when we need
 that feature, we instead use the generic exception trick.
 In may cases, Java 8 allows us to throw Throwable directly. This
 happens in cases where no exception is declared to be thrown in a
 scope.
 Finally, some warnings have also been fixed, and some imports have
 been reorganised and cleaned up while I was modifying the files
 anyway.

Result:
 We no longer use Unsafe for throwing any exceptions.
2020-10-02 08:29:07 +02:00
Divij Vaidya
58911c1482 Clear scheduled timeout if channel is closed with incomplete WebSocket handshake (#10510)
Motivation:

Consider a scenario when the client iniitiates a WebSocket handshake but before the handshake is complete,
the channel is closed due to some reason. In such scenario, the handshake timeout scheduled on the executor
is not cleared. The reason it is not cleared is because in such cases the handshakePromise is not completed.

Modifications:

This change completes the handshakePromise exceptinoally on channelInactive callback, if it has not been
completed so far. This triggers the callback on completion of the promise which clears the timeout scheduled
on the executor.

This PR also adds a test case which reproduces the scenario described above. The test case fails before the
fix is added and succeeds when the fix is applied.

Result:

After this change, the timeout scheduled on the executor will be cleared, thus freeing up thread resources.
2020-09-16 10:10:50 +02:00
Kevin Wu
eef67e2f0e Fix DeleteOnExitHook cause memory leak (#10560)
Motivation:

If DeleteOnExitHook is in the open state and the program runs for a long time, the DeleteOnExitHook file keeps increasing.
This results in a memory leak

Modification:

I re-customized a DeleteOnExitHook hook. If DeleteOnExitHook is turned on, this hook will be added when creating a temporary file. After the request ends and the corresponding resources are released, the current file will be removed from this hook, so that it will not increase all the time.

Result:

Fixes https://github.com/netty/netty/issues/10351
2020-09-15 16:45:03 +02:00
Nick Hill
d7c1407d4c Use ByteBuf#isAccessible() in more places (#10506)
Motivation

ByteBuf has an isAccessible method which was introduced as part of ref
counting optimizations but there are some places still doing
accessibility checks by accessing the volatile refCnt() directly.

Modifications

- Have PooledNonRetained(Duplicate|Sliced)ByteBuf#isAccessible() use
their refcount delegate's isAccessible() method
- Add static isAccessible(buf) and ensureAccessible(buf) methods to
ByteBufUtil
(since ByteBuf#isAccessible() is package-private)
- Adjust DefaultByteBufHolder and similar classes to use these methods
rather than access refCnt() directly

Result

- More efficient accessibility checks in more places
2020-08-28 09:22:34 +02:00
Andrey Mizurov
ce132b15d5 Small fix that takes into account the remainder when assigning the size (see #10453) (#10491)
Motivation:

This is small fixes for #10453 PR according @njhill and @normanmaurer conversation.

Modification:

Simple refactor and takes into account remainder when calculate size.

Result:

Behavior is correct
2020-08-21 11:47:42 +02:00
Violeta Georgieva
4ce6774336 AbstractDiskHttpData#getChunk closes fileChannel only if everything w… (#10481)
Motivation:
AbstractDiskHttpData#getChunk opens and closes fileChannel every time when it is invoked,
as a result the uploaded file is corrupted. This is a regression caused by #10270.

Modifications:

- Close the fileChannel only if everything was read or an exception is thrown
- Add unit test

Result:
AbstractDiskHttpData#getChunk closes fileChannel only if everything was read or an exception is thrown
2020-08-14 11:14:50 +02:00
Andrey Mizurov
2b73c93581 Fix #10449, buffer.getBytes(...) not change a file channel position (#10453)
Motivation:

Regression appeared after making changes in fix #10360 .
The main problem here that `buffer.getBytes(buffer.readerIndex(), fileChannel, fileChannel.position(), localsize)`
doesn't change channel position after writes.

Modification:

Manually set position according to the written bytes.

Result:

Fixes #10449 .
2020-08-07 14:02:07 +02:00
Bennett Lynch
f7fa9fce72 Add option to HttpObjectDecoder to allow duplicate Content-Lengths (#10349)
Motivation:

Since https://github.com/netty/netty/pull/9865 (Netty 4.1.44) the
default behavior of the HttpObjectDecoder has been to reject any HTTP
message that is found to have multiple Content-Length headers when
decoding. This behavior is well-justified as per the risks outlined in
https://github.com/netty/netty/issues/9861, however, we can see from the
cited RFC section that there are multiple possible options offered for
responding to this scenario:

> If a message is received that has multiple Content-Length header
> fields with field-values consisting of the same decimal value, or a
> single Content-Length header field with a field value containing a
> list of identical decimal values (e.g., "Content-Length: 42, 42"),
> indicating that duplicate Content-Length header fields have been
> generated or combined by an upstream message processor, then the
> recipient MUST either reject the message as invalid or replace the
> duplicated field-values with a single valid Content-Length field
> containing that decimal value prior to determining the message body
> length or forwarding the message.

https://tools.ietf.org/html/rfc7230#section-3.3.2

Netty opted for the first option (rejecting as invalid), which seems
like the safest, but the second option (replacing duplicate values with
a single value) is also valid behavior.

Modifications:

* Introduce "allowDuplicateContentLengths" parameter to
HttpObjectDecoder (defaulting to false).
* When set to true, will allow multiple Content-Length headers only if
they are all the same value. The duplicated field-values will be
replaced with a single valid Content-Length field.
* Add new parameterized test class for testing different variations of
multiple Content-Length headers.

Result:

This is a backwards-compatible change with no functional change to the
existing behavior.

Note that the existing logic would result in NumberFormatExceptions
for header values like "Content-Length: 42, 42". The new logic correctly
reports these as IllegalArgumentException with the proper error message.

Additionally note that this behavior is only applied to HTTP/1.1, but I
suspect that we may want to expand that to include HTTP/1.0 as well...
That behavior is not modified here to minimize the scope of this change.
2020-07-06 14:50:15 +02:00
Norman Maurer
f8b3a388ff
Fix compile errors introduced by bad cherry-picks (#10391) 2020-07-06 13:22:31 +02:00
feijermu
0185ccc157 Fix a javadoc mistake. (#10364)
Motivation:

There exists a `javadoc` mistake in `HttpHeaderValues.java`.

Modification:

Just correct this `javadoc` mistake...
2020-06-23 09:24:48 +02:00
Norman Maurer
4dd6f82624 Fix memory leak in AbstractDiskHttpData when CompositeByteBuf is used (#10360)
Motivation:

AbstractDiskHttpData may cause a memory leak when a CompositeByteBuf is used. This happened because we may call copy() but actually never release the newly created ByteBuf.

Modifications:

- Remove copy() call and just use ByteBuf.getBytes(...) which will internally handle the writing to the FileChannel without any extra copies that need to be released later on.
- Add unit test

Result:

Fixes https://github.com/netty/netty/issues/10354
2020-06-22 13:55:02 +02:00
Bennett Lynch
f945cfbd66 Consolidate HttpObjectDecoder default values into constants (#10344)
Motivation

HttpObjectDecoder and its associated classes make frequent use of
default values for maxInitialLineLength, maxHeaderSize, maxChunkSize,
etc. Today, these defaults are defined in-line in constructors and
duplicated across many classes. This repetition is more prone to error
and inconsistencies.

Furthermore, due to the current lack of builder support, if a user wants
to change just one of these values (e.g., maxHeaderSize), they are also
required to know and repeat the other default values (e.g.,
maxInitialLineLength and maxChunkSize).

The primary motivation for this change is as we are considering adding
another constructor parameter (for multiple content length behavior),
appending this parameter may require some users to have prior knowledge
of the default initialBufferSize, and it would be cleaner to allow them
to reference the default constant.

Modifications

* Consolidate the HttpObjectDecoder default values into public constants
* Reference these constants where possible

Result

No functional change. Additional telescoping constructors will be easier
and safer to write. Users may have an easier experience changing single
parameters.
2020-06-12 08:44:39 +02:00
Lin Gao
4b6ceb2ca0 More values other than chunked defined in Transfer-Encoding header leads to decode failure (#10321)
Motivation:

`containsValue()` will check if there are multiple values defined in the specific header name, we need to use this method instead of `contains()` for the `Transfer-Encoding` header to cover the case that multiple values defined, like: `Transfer-Encoding: gzip, chunked`

Modification:

Change from `contains()` to `containsValue()` in `HttpUtil.isTransferEncodingChunked()` method.

Result:

Fixes #10320
2020-06-02 14:38:11 +02:00
Andrey Mizurov
4c7e017199 Set (and override) reserved websocket handshake response headers after custom to avoid duplication (#10319)
Motivation:
Currently we passing custom websocket handshaker response headers to a `WebSocketServerHandshaker` but they can contain a reserved headers (e.g. Connection, Upgrade, Sec-Websocket-Accept) what lead to duplication because we use response.headers().add(..) instead of response.headers().set(..).

Modification:
In each `WebSocketServerHandshaker00`, ... `WebSocketServerHandshaker13` implementation replace the method add(..) to set(..) for reserved response headers.

Result:

Less error-prone
2020-06-02 11:56:47 +02:00
prgitpr
e7c5773162 Fix a potential fd leak in AbstractDiskHttpData.getChunk (#10270)
Motivation:

`FileChannel.read()` may throw an IOException. We must deal with this in case of the occurrence of `I/O` error.

Modification:

Place the `FileChannel.read()` method call in the `try-finally` block.

Result:

Advoid fd leak.


Co-authored-by: Norman Maurer <norman_maurer@apple.com>
2020-05-14 10:16:33 +02:00
Fabien Renaud
3e8cbb3136 Fix regression in HttpPostStandardRequestDecoder to always decode + to whitespace (#10285)
Motivations
-----------
HttpPostStandardRequestDecoder was changed in 4.1.50 to provide its own
ByteBuf UrlDecoder. Prior to this change, it was using the decodeComponent
method from QueryStringDecoder which decoded + characters to
whitespaces. This behavior needs to be preserved to maintain backward
compatibility.

Modifications
-------------
Changed HttpPostStandardRequestDecoder to detect + bytes and decode them
toe whitespaces. Added a test.

Results
-------
Addresses issues#10284
2020-05-14 09:29:13 +02:00
Norman Maurer
7f54c6d90d Don't reuse ChannelPromise in WebSocketProtocolHandler (#10248)
Motivation:

We cant reuse the ChannelPromise as it will cause an error when trying to ful-fill it multiple times.

Modifications:

- Use a new promise and chain it with the old one
- Add unit test

Result:

Fixes https://github.com/netty/netty/issues/10240
2020-05-07 09:14:02 +02:00
Dmitriy Dumanskiy
a81858c520 …in order to minimize pipeline
Motivation:

Handling of `WebSocketCloseFrame` is part of websocket protocol, so it's logical to put it within the `WebSocketProtocolHandler`. Also, removal of `WebSocketCloseFrameHandler` will decrease the channel pipeline.

Modification:

- `WebSocketCloseFrameHandler` code merged into `WebSocketProtocolHandler`. `WebSocketCloseFrameHandler` not added to the pipeline anymore
- Added additional constructor to `WebSocketProtocolHandler`
- `WebSocketProtocolHandler` now implements `ChannelOutboundHandler` and implements basic methods from it

Result:

`WebSocketCloseFrameHandler` is no longer used.

Fixes https://github.com/netty/netty/issues/9944
2020-05-07 09:13:50 +02:00
Norman Maurer
a62fcd9d50 Reuse the same allocator as used by the ByteBuf that is used during… (#10226)
Motivation:

We should not use Unpooled to allocate buffers if possible to ensure we can make use of pooling etc.

Modifications:

- Only allocate a buffer if really needed
- Use the ByteBufAllocator of the offered ByteBuf
- Ensure we not use buffer.copy() but explicitly allocate a buffer and then copy into it to not hit the limit of maxCapacity()

Result:

Improve allocations
2020-04-29 15:37:51 +02:00
Norman Maurer
23e0b878af Remove some debugging cruft (#10229)
Motivation:

RtspDecoderTest did include a println(...) call which was a left over from debugging.

Modifications:

Remove println(...)

Result:

Cleanup
2020-04-29 11:35:53 +02:00
Norman Maurer
65a967c772 Fix memory leak in HttpPostMultipartRequestDecoder (#10227)
Motivation:

We need to release all ByteBufs that we allocate to prevent leaks. We missed to release the ByteBufs that are used to aggregate in two cases

Modifications:

Add release() calls

Result:

No more memory leak in HttpPostMultipartRequestDecoder
2020-04-29 08:23:55 +02:00
Fabien Renaud
8fe9c013dd HttpPostRequestDecoder: retain instead of copy when first buf is last (#10209)
Motivations
-----------
There is no need to copy the "offered" ByteBuf in HttpPostRequestDecoder
when the first HttpContent ByteBuf is also the last (LastHttpContent) as
the full content can immediately be decoded. No extra bookeeping needed.

Modifications
-------------
HttpPostMultipartRequestDecoder
 - Retain the first ByteBuf when it is both the first HttpContent offered
to the decoder and is also LastHttpContent.
 - Retain slices of the final buffers values

Results
-------
ByteBufs of FullHttpMessage decoded by HttpPostRequestDecoder are no longer
unnecessarily copied. Attributes are extracted as retained slices when
the content is multi-part. Non-multi-part content continues to return
Unpooled buffers.

Partially addresses issue #10200
2020-04-28 09:43:19 +02:00
feijermu
076f388d72 Move up the size check in AbstractDiskHttpData.setContent. (#10222)
Motivation:

`AbstractHttpData.checkSize` may throw an IOException if we set the max size limit via `AbstractHttpData.setMaxSize`. However, if this exception happens, the `AbstractDiskHttpData.file` and the `AbstractHttpData.size` are still be modified. In other words, it may break the failure atomicity here.

Modification:

Just move up the size check.

Result:

Keep the failure atomicity even if `AbstractHttpData.checkSize` fails.
2020-04-28 09:42:30 +02:00
feijermu
a1f23dbdd3 Fix a potential fd leak in AbstractDiskHttpData.delete (#10212)
Motivation:

An unexpected IOException may be thrown from `FileChannel.force`. If it happens, the `FileChannel.close` may not be invoked.

Modification:

Place the `FileChannel.close` in a finally block.

Result:

Avoid fd leak.
2020-04-27 07:04:10 +02:00
feijermu
586eca2809 Fix a potential fd leak in AbstractDiskHttpData.setContent (#10198)
Motivation:

`RandomAccessFile.setLength` may throw an IOException. We must deal with this in case of the occurrence of `I/O` error.

Modification:

Place the `RandomAccessFile.setLength` method call in the `try-finally` block.

Result:

Avoid fd leak.
2020-04-21 11:08:50 +02:00
feijermu
a4ad6d15cd Close the FileChannel in case of an IOException in AbstractDiskHttpData.addContent. (#10188)
Motivation:

`FileChannel.force` may throw an IOException. A fd leak may happen here.

Modification:

Close the fileChannel in a finally block.

Result:

Avoid fd leak.
2020-04-15 09:25:07 +02:00
feijermu
3808777c32 Close the file in case of an IOException in AbstractMemoryHttpData.renameTo. (#10163)
Motivation:

An `IOException` may be thrown from `FileChannel.write` or `FileChannel.force`, and cause the fd leak.

Modification:

Close the file in a finally block.

Result:

Avoid fd leak.
2020-04-06 14:16:30 +02:00