Commit Graph

1358 Commits

Author SHA1 Message Date
feijermu
32e32fe98f
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 10:54:09 +02:00
feijermu
17f205fab3
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:24:26 +02:00
feijermu
3ebf3d7705
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 10:12:46 +02:00
feijermu
bdaa935756
Release the temporary ByteBuf in case of unexpected exception in WebSocketUtil.base64. (#10160)
Motivation:

An exception may occur between ByteBuf's allocation and release. So I think it's a good idea to place the release operation in a finally block.

Modification:

Release the temporary ByteBuf in finally blocks.

Result:

Avoid ByteBuf leak.
2020-04-03 14:49:40 +02:00
feijermu
d4c268ae3e
Close the file when IOException occurs in AbstractMemoryHttpData. (#10157)
Motivation:

An IOException may be thrown from FileChannel.read, and cause the fd leak.

Modification:

Close the file when IOException occurs.

Result:
Avoid fd leak.
2020-04-02 14:16:01 +02:00
Romain Manni-Bucau
84ebf47515
making DefaultHttpDataFactory able to configure basedir and deleteonexit (#10146)
Motivation: 

currently (http) disk based attributes or uploads are globally configured in a single directory and can also only globally be deleted on exit or not. it does not fit well multiple cases, in particular the case you have multiple servers in the same JVM.

Modification: 
make it configurable per attribute/fileupload.

Result:

This PR duplicates Disk* constructor to add basedir and deleteonexit parameters and wires it in default http daa factory.
2020-03-30 13:11:46 +02:00
feijermu
aed5a74e09
Release the ByteBuf when IOException occurs in AbstractMemoryHttpData. (#10133)
Motivation:

An IOException may be thrown from InputStream.read or checkSize method, and cause the ByteBuf leak.

Modification:

Release the ByteBuf when IOException occurs.

Result:
Avoid ByteBuf leak.
2020-03-30 11:26:03 +02:00
Stephane Landelle
fcf7144f7c
Add some HTTP header constants (#10127)
Motivation:

Add some missing HTTP header names and values constants.

Modification:

* names:
  * dnt (Do Not Track)
  * upgrade-insecure-requests
  * x-requested-with
* values:
  * application/xhtml+xml
  * application/xml
  * text/css
  * text/html
  * text/event-stream
  * XmlHttpRequest

Result:

More constants available
2020-03-23 13:05:58 +01:00
root
14e4afeba2 [maven-release-plugin] prepare for next development iteration 2020-03-17 09:20:54 +00:00
root
c10c697e5b [maven-release-plugin] prepare release netty-4.1.48.Final 2020-03-17 09:18:28 +00:00
Norman Maurer
0fb58d3c54
HttpObjectDecoder should limit the number of control chars (#10112)
Motivation:

At the moment HttpObjectDecoder does not limit the number of controls chars. These should be counted with the initial line and so a limit should be exposed

Modifications:

- Change LineParser to also be used when skipping control chars and so enforce a limit
- Add various tests to ensure that limit is enforced

Result:

Fixes https://github.com/netty/netty/issues/10111
2020-03-17 10:16:45 +01:00
Norman Maurer
4b22d8ab35
Use WebSocketVersion.toAsciiString() as header value when possible (#10105)
Motivation:

In our WebSocketClientHandshaker* implementations we "hardcode" the version number to use. This is error-prone, we should better use the WebSocketVersion so we dont need to maintain the value multiple times. Beside this we can also use an AsciiString to improve performance

Modifications:

- Use WebSocketVersion.toAsciiString

Result:

Less stuff to maintain and small performance win
2020-03-13 07:48:58 +01:00
Norman Maurer
260540b25a
Don't override HOST header if provided by user already in WebSocketClientHandshaker (#10104)
Motivation:

The user may need to provide a specific HOST header. We should not override it when specified during handshake.

Modifications:

Check if a custom HOST header is already provided by the user and if so dont override it

Result:

Fixes https://github.com/netty/netty/issues/10101
2020-03-12 20:25:53 +01:00
David Latorre
e4af5c3631
Added support for the SameSite attribute in Cookies (#10050)
Motivation:

Netty currently does not support the SameSite attribute for response cookies (see issue #8161 for discussion).

Modifications:

The attribute has been added to the DefaultCookie class as a quick fix since adding new methods to the Cookie interface would be backwards-incompatible.
ServerCookieEncoder and ClientCookieDecoder have been updated accordingly to process this value. No validation for allowed values (Lax, None, Strict) has been implemented.

Result:

Response cookies with the SameSite attribute set can be read or written by Netty.

Co-authored-by: David Latorre <a-dlatorre@hotels.com>
2020-03-12 09:48:30 +01:00
Stephane Landelle
2576a2dd74
Fix WebSocketClientHandshaker not generating correct handshake request when path is empty (#10095)
Motivation:

WebSocketClientHandshaker#upgradeUrl doesn't comperly compute relative url when path is empty and produces url such as `?access_token=foo` instead of `/?access_token=foo`.

Modifications:

* fix WebSocketClientHandshaker#upgradeUrl
* add tests for urls without path, with and without query

Result:

WebSocketClientHandshaker properly connects to url without path.
2020-03-10 15:17:22 +01:00
root
c623a50d19 [maven-release-plugin] prepare for next development iteration 2020-03-09 12:13:56 +00:00
root
a401b2ac92 [maven-release-plugin] prepare release netty-4.1.47.Final 2020-03-09 12:13:26 +00:00
zlm0125
161c237fb9
http multipart decode with chinese chars should work (#10089)
Motivation:

I am receiving a mutlipart/form_data upload from postman. The filename contains Chinese, and so some invalid chars. We should ensure all of these are removed before trying to decode.

Modification:

Ensure all invalid characters are removed

Result:

Fixes #10087

Co-authored-by: liming.zhang <liming.zhang@luckincoffee.com>
2020-03-06 10:33:58 +01:00
Antony T Curtis
7fce06a0d1
Preserve order when using alternate event loops (#10069)
Motivation:

When the HttpContentCompressor is put on an alternate EventExecutor, the order of events should be
preserved so that the compressed content is correctly created.

Modifications:
- checking that the executor in the ChannelHandlerContext is the same executor as the current executor when evaluating if the handler should be skipped.
- Add unit test

Result:

Fixes https://github.com/netty/netty/issues/10067

Co-authored-by: Norman Maurer <norman_maurer@apple.com>
2020-03-03 10:42:41 +01:00
root
e0d73bca4d [maven-release-plugin] prepare for next development iteration 2020-02-28 06:37:33 +00:00
root
ebe7af5102 [maven-release-plugin] prepare release netty-4.1.46.Final 2020-02-28 06:36:45 +00:00
Norman Maurer
9ae782d632
More strict parsing of initial line / http headers (#10058)
Motivation:

Our parsing of the initial line / http headers did treat some characters as separators which should better trigger an exception during parsing.

Modifications:

- Tighten up parsing of the inital line by follow recommentation of RFC7230
- Restrict separators to OWS for http headers
- Add unit test

Result:

Stricter parsing of HTTP1
2020-02-26 09:49:39 +01:00
Norman Maurer
f88a343455
Remove System.out.println(...) in test (#10024)
Motivation:

We did had some System.out.println(...) call in a test which seems to be some left-over from debugging.

Modifications:

Remove System.out.println(...)

Result:

Code cleanup
2020-02-13 08:42:49 +01:00
Bennett Lynch
6290346246 Remove "Content-Length" when decoding HTTP/1.1 message with both "Tra… (#10003)
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.
2020-02-10 10:41:57 +01:00
Artem Smotrakov
5f68897880
Added tests for Transfer-Encoding header with whitespace (#9997)
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
2020-02-05 14:33:28 +01:00
Dmitriy Dumanskiy
b49bd5644f
Use compile time constants instead of status field in WebSocketServer/ClientProtocolConfig (#9976)
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.
2020-01-29 15:01:23 +01:00
Dmitriy Dumanskiy
fb3ced28cf #9944 Merge WebSocketCloseFrameHandler into WebSocketProtocolHandler … (#9967)
…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-01-28 14:57:32 +01:00
Norman Maurer
2023a4f607
Remove usage of forbiddenHttpRequestResponder (#9941)
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
2020-01-28 06:04:20 +01:00
Andrey Mizurov
c9d8152e88 Fix remove 'WebSocketServerExtensionHandler' from pipeline after upgrade (#9940)
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.
2020-01-15 10:49:51 +01:00
root
9b1ea10a12 [maven-release-plugin] prepare for next development iteration 2020-01-13 09:13:53 +00:00
root
136db8680a [maven-release-plugin] prepare release netty-4.1.45.Final 2020-01-13 09:13:30 +00:00
Norman Maurer
06a5173e8d
Utf8FrameValidator must release buffer when validation fails (#9909)
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
2019-12-27 09:14:50 +01:00
Anuraag Agrawal
687308b4de Separate out query string encoding for non-encoded strings. (#9887)
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
```
2019-12-20 08:51:18 +01:00
Norman Maurer
b615495762
Add testcase for internal used Comparator in ClientCookieEncoder (#9897)
Motivation:

https://github.com/netty/netty/pull/9883 added a bug-fix for the Comparator in ClientCookieEncoder but did not add a testcase.

Modifications:

- Add testcase
- Simplify code

Result:

Include a test to ensure we not regress.
2019-12-20 08:43:20 +01:00
Gerd Riesselmann
f995426a59 Avoid possible comparison contract violation (#9883)
Motivation:

The current implementation causes IllegalArgumetExceptions to be thrown on Java 11.

The current implementation would violate comparison contract for two cookies C1 and C2 with same path length, since C1 < C2 and C2 < C1. Returning 0 (equality) does not since C1 == C2 and C2 == C1. See #9881

Modification:

Return equality instead of less than on same path length.

Result:

Fixes #9881.
2019-12-19 12:16:27 +01:00
Anuraag Agrawal
95b8db0633 Use array to buffer decoded query instead of ByteBuffer. (#9886)
Motivation:

In Java, it is almost always at least slower to use `ByteBuffer` than `byte[]` without pooling or I/O. `QueryStringDecoder` can use `byte[]` with arguably simpler code.

Modification:

Replace `ByteBuffer` / `CharsetDecoder` with `byte[]` and `new String`

Result:

After
```
Benchmark                                   Mode  Cnt  Score   Error   Units
QueryStringDecoderBenchmark.noDecoding     thrpt    6  5.612 ± 2.639  ops/us
QueryStringDecoderBenchmark.onlyDecoding   thrpt    6  1.393 ± 0.067  ops/us
QueryStringDecoderBenchmark.mixedDecoding  thrpt    6  1.223 ± 0.048  ops/us
```

Before
```
Benchmark                                   Mode  Cnt  Score   Error   Units
QueryStringDecoderBenchmark.noDecoding     thrpt    6  6.123 ± 0.250  ops/us
QueryStringDecoderBenchmark.onlyDecoding   thrpt    6  0.922 ± 0.159  ops/us
QueryStringDecoderBenchmark.mixedDecoding  thrpt    6  1.032 ± 0.178  ops/us
```

I notice #6781 switched from an array to `ByteBuffer` but I can't find any motivation for that in the PR. Unit tests pass fine with an array and we get a reasonable speed bump.
2019-12-18 21:11:28 +01:00
root
79d4e74019 [maven-release-plugin] prepare for next development iteration 2019-12-18 08:32:54 +00:00
root
5ddf45a2d5 [maven-release-plugin] prepare release netty-4.1.44.Final 2019-12-18 08:31:43 +00:00
Norman Maurer
d24dfb1051 Revert "Bugfix #9673: Origin header is always sent from WebSocket client (#9692)"
This reverts commit f48d9fa8d0 as this needs more thoughts.
2019-12-18 09:23:38 +01:00
Norman Maurer
8494b046ec
Verify we do not receive multiple content-length headers or a content-length and transfer-encoding: chunked header when using HTTP/1.1 (#9865)
Motivation:

RFC7230 states that we should not accept multiple content-length headers and also should not accept a content-length header in combination with transfer-encoding: chunked

Modifications:

- Check for multiple content-length headers and if found mark message as invalid
- Check if we found a content-length header and also a transfer-encoding: chunked and if so mark the message as invalid
- Add unit test

Result:

Fixes https://github.com/netty/netty/issues/9861
2019-12-13 08:53:19 +01:00
Dmitriy Dumanskiy
0992718f87 #9867 fix confusing method parameter name (#9874)
Motivation:

Parameter name is confusing and not match the actual type.

Modification:

Rename parameter.

Result:

Code cleanup
2019-12-12 14:41:41 +01:00
Norman Maurer
a7c18d44b4
Detect missing colon when parsing http headers with no value (#9871)
Motivation:

Technical speaking its valid to have http headers with no values so we should support it. That said we need to detect if these are "generated" because of an "invalid" fold.

Modifications:

- Detect if a colon is missing when parsing headers.
- Add unit test

Result:

Fixes https://github.com/netty/netty/issues/9866
2019-12-11 15:49:07 +01:00
Andrey Mizurov
cf63bc1005 Fix #9770, last frame may contain extra data that doesn't affect decompression (#9832)
Motivation:
Client can split data into different numbers of fragments and sometimes the last frame may contain trash data that doesn't affect decompression process.

Modification:
Added check if last frame is `ContinuationWebSocketFrame` and decompression data is empty
then don't throw an exception.

Result:
Fixes #9770
2019-12-11 15:00:52 +01:00
Carl Mastrangelo
4138cba861 Add Server Cookie Parser overload for multiple cookies. (#9856)
Motivation:

Multiple cookie values can be present in a single header.

Modification:

Add `decodeAll` overload which returns all cookies

Result:

Fixes #7210

Note:

This change is not as perscriptive as the ideas brought up in the linked issue.  Changing the Set implementation or the equals/compareTo definition is likely a breaking change, so they are practical.
2019-12-10 11:32:45 +01:00
时无两丶
0cde4d9cb4 Uniform null pointer check. (#9840)
Motivation:
Uniform null pointer check.

Modifications:

Use ObjectUtil.checkNonNull(...)

Result:
Less code, same result.
2019-12-09 09:47:35 +01:00
Norman Maurer
dcbfe17eeb
Prevent any leaks when HttpPostStandardRequestDecoder constructor throws (#9837)
Motivation:

HttpPostStandardRequestDecoder may throw multiple different exceptions in the constructor which could lead to memory leaks. We need to guard against this by explicit catch all of them and rethrow after we released any allocated memory.

Modifications:

- Catch, destroy and rethrow in any case
- Ensure we correctly wrap IllegalArgumentExceptions
- Add unit tests

Result:

Fixes https://github.com/netty/netty/issues/9829
2019-12-04 13:58:03 +01:00
ursa
19a4633859 Bugfix #9673: Origin header is always sent from WebSocket client (#9692)
### Motivation:

Those who need 'Origin' or 'Sec-WebSocket-Origin' headers should provide them explicitly, like it is stated in WebSocket specs.

E.g. through custom headers:

    HttpHeaders customHeaders = new DefaultHttpHeaders()
        .add(HttpHeaderNames.ORIGIN, "http://localhost:8080");
    new WebSocketClientProtocolHandler(
        new URI("ws://localhost:1234/test"), WebSocketVersion.V13, subprotocol, 
        allowExtensions, customHeaders, maxFramePayloadLength, handshakeTimeoutMillis)


### Modification:

* Remove enforced origin headers.
* Update tests

### Result:

Fixes #9673: Origin header is always sent from WebSocket client
2019-11-27 08:36:30 +01:00
Norman Maurer
2886bd6677
Simplify Deflate* implementations by using EmbeddedChannel.finishAndReleaseAll() (#9808)
Motivation:

We can simplify the code by just using finishAndReleaseAll()

Modifications:

Remove some code and simplify

Result:

Cleaner code
2019-11-27 06:54:55 +01:00
ursa
b5230c7b9c Send close frame on channel close, when this frame was not send manually (#9745)
Motivation:
By default CloseWebSocketFrames are handled automatically.
However I need manually manage their sending both on client- and on server-sides.

Modification:
Send close frame on channel close automatically, when it was not send before explicitly.

Result:
No more messages like "Connection closed by remote peer" for normal close flows.
2019-11-18 20:32:21 +01:00
ursa
8d99aa1235 Simplify WebSocket handlers constructor arguments hell #9698 (#9699)
### Motivation:

Introduction of `WebSocketDecoderConfig` made our server-side code more elegant and simpler for support.

However there is still some problem with maintenance and new features development for WebSocket codecs (`WebSocketServerProtocolHandler`, `WebSocketServerProtocolHandler`).

Particularly, it makes me ~~crying with blood~~ extremely sad to add new parameter and yet another one constructor into these handlers, when I want to contribute new feature.

### Modification:

I've extracted all parameters for client and server WebSocket handlers into config/builder structures, like it was made for decoders in PR #9116.

### Result:

* Fixes #9698: Simplify WebSocket handlers constructor arguments hell
* Unblock further development in this module (configurable close frame handling on server-side; automatic close-frame sending, when missed; memory leaks on protocol violations; etc...)

Bonuses:

* All defaults are gathered in one place and could be easily found/reused.
* New API greatly simplifies usage, but does NOT allow inheritance or modification.
* New API would simplify long-term maintenance of WebSockets module.

### Example

    WebSocketClientProtocolConfig config = WebSocketClientProtocolConfig.newBuilder()
        .webSocketUri("wss://localhost:8443/fx-spot")
        .subprotocol("trading")
        .handshakeTimeoutMillis(15000L)
        .build();
    ctx.pipeline().addLast(new WebSocketClientProtocolHandler(config));
2019-10-29 20:48:18 +01:00