Motivation:
Socks5InitialRequestDecoder does not correctly handle fragmentation
Modifications:
- Delete detection of not enough bytes as ReplyingDecoder already handles all of this correctly.
- Add unit test
Result:
Fixes#9574.
Motivation:
There appears to be a thread-safety issue in the way that `SocksAuthRequest` is using its `CharsetEncoder` instance. `CharsetUtil#encoder` returns a cached thread-local encoder instance, so it is not correct to store this instance in a static member variable and reuse it across multiple threads. The result is an occasional `IllegalStateException` as in the following example:
```
java.lang.IllegalStateException: Current state = RESET, new state = FLUSHED
at java.base/java.nio.charset.CharsetEncoder.throwIllegalStateException(CharsetEncoder.java:989)
at java.base/java.nio.charset.CharsetEncoder.flush(CharsetEncoder.java:672)
at java.base/java.nio.charset.CharsetEncoder.encode(CharsetEncoder.java:801)
at java.base/java.nio.charset.CharsetEncoder.canEncode(CharsetEncoder.java:907)
at java.base/java.nio.charset.CharsetEncoder.canEncode(CharsetEncoder.java:982)
at io.netty.handler.codec.socks.SocksAuthRequest.<init>(SocksAuthRequest.java:43)
```
Modification:
Instead of retrieving the thread-local encoder instance once and storing it as a static member instance, the encoder should be retrieved each time the constructor is invoked. This change prevents any potential concurrency issues where multiple threads may end up using the same encoder instance.
Result:
Fixes#9556.
* Optimize AbstractByteBuf.getCharSequence() in US_ASCII case
Motivation:
Inspired by https://github.com/netty/netty/pull/8388, I noticed this
simple optimization to avoid char[] allocation (also suggested in a TODO
here).
Modifications:
Return an AsciiString from AbstractByteBuf.getCharSequence() if
requested charset is US_ASCII or ISO_8859_1 (latter thanks to
@Scottmitch's suggestion). Also tweak unit tests not to require Strings
and include a new benchmark to demonstrate the speedup.
Result:
Speed-up of AbstractByteBuf.getCharSequence() in ascii and iso 8859/1
cases
Motivation:
If you encode a SOCKS5 message like new DefaultSocks5CommandResponse(FAILURE, DOMAIN, "", 0)
you correctly get a result of 05010003000000.
But if the bndAddr is null, for example like new DefaultSocks5CommandResponse(FAILURE, DOMAIN)
the encoded result is 0501000301000000 which means the domain name has a length of one and consists of a 0-byte.
Modification:
With this commit it is also correctly encoded as a string of 0 length.
Result:
Correctly encode empty SOCKS5 address
Motivation: Today when Netty encounters a general error while decoding
it treats this as a decoder exception. However, for fatal causes this
should not be treated as such, instead the fatal error should be carried
up the stack without the callee having to unwind causes. This was
probably done for byte to byte message decoder but is now done for all
decoders.
Modifications: Instead of translating any error to a decoder exception,
we let those unwind out the stack (note that finally blocks still
execute) except in places where an event needs to fire where we fire
with the error instead of wrapping in a decoder exception.
Result: Fatal errors will not be treated as innocent decoder exceptions.
Motivation:
According to SOCKS 5 spec, dstPort = 0 is a valid value in case of UDP ASSOCIATE.
Modifications:
- Allow 0 as port.
- Add unit tests.
Result:
Fixes [#7156].
Motivation:
In the SocksCmdRequest and SocksCmdResponse constructors a host param converts from IDN to ascii compatible form regardless address type.
Modifications:
Use `IDN#toASCII` only for `DOMAIN` address type.
Result:
More correct host handling in socks commands.
Motivation:
1. `ByteBuf` contains methods to writing `CharSequence` which optimized for UTF-8 and ASCII encodings. We can also apply optimization for ISO-8859-1.
2. In many places appropriate methods are not used.
Modifications:
1. Apply optimization for ISO-8859-1 encoding in the `ByteBuf#setCharSequence` realizations.
2. Apply appropriate methods for writing `CharSequences` into buffers.
Result:
Reduce overhead from string-to-bytes conversion.
Motivation:
1. Internal class `SocksCommonUtils` contains a method `intToIp` that also exists in the `NetUtil`.
2. A `SocksCommonUtils#ipv6toCompressedForm` is never used.
Modifications:
1. Replace `intToIp` method usage with `NetUtil#intToIpAddress`.
2. Remove unused methods from `SocksCommonUtils`.
Result:
Less code for supports.
Motivation:
Some classes have fields which can be local.
Modifications:
Convert fields to the local variable when possible.
Result:
Clean up. More chances for young generation or scalar replacement.
Motivation:
Currently Netty does not wrap socket connect, bind, or accept
operations in doPrivileged blocks. Nor does it wrap cases where a dns
lookup might happen.
This prevents an application utilizing the SecurityManager from
isolating SocketPermissions to Netty.
Modifications:
I have introduced a class (SocketUtils) that wraps operations
requiring SocketPermissions in doPrivileged blocks.
Result:
A user of Netty can grant SocketPermissions explicitly to the Netty
jar, without granting it to the rest of their application.
Motivation:
NetUtil.bytesToIpAddress does not correctly translate IPv4 address to String. Also IPv6 addresses may not follow minimization conventions when converting to a String (see rfc 5952).
Modifications:
- NetUtil.bytesToIpAddress should correctly handle negative byte values for IPv4
- NetUtil.bytesToIpAddress should leverage existing to string conversion code in NetUtil
Result:
Fixes https://github.com/netty/netty/issues/5821
Related: #4333#4421#5128
Motivation:
slice(), duplicate() and readSlice() currently create a non-recyclable
derived buffer instance. Under heavy load, an application that creates a
lot of derived buffers can put the garbage collector under pressure.
Modifications:
- Add the following methods which creates a non-recyclable derived buffer
- retainedSlice()
- retainedDuplicate()
- readRetainedSlice()
- Add the new recyclable derived buffer implementations, which has its
own reference count value
- Add ByteBufHolder.retainedDuplicate()
- Add ByteBufHolder.replace(ByteBuf) so that..
- a user can replace the content of the holder in a consistent way
- copy/duplicate/retainedDuplicate() can delegate the holder
construction to replace(ByteBuf)
- Use retainedDuplicate() and retainedSlice() wherever possible
- Miscellaneous:
- Rename DuplicateByteBufTest to DuplicatedByteBufTest (missing 'D')
- Make ReplayingDecoderByteBuf.reject() return an exception instead of
throwing it so that its callers don't need to add dummy return
statement
Result:
Derived buffers are now recycled when created via retainedSlice() and
retainedDuplicate() and derived from a pooled buffer
Motivation:
We use ByteBuf.readBytes(int) in various places where we could either remove it completely or use readSlice(int).retain().
Modifications:
- Remove ByteBuf.readBytes(int) when possible or replace by readSlice(int).retain().
Result:
Faster code.
Motivation:
See #3321
Modifications:
1. Add CharsetUtil.encoder/decoder() methods
2. Deprecate CharsetUtil.getEncoder/getDecoder() methods
Result:
Users can use new CharsetUtil.encoder/decoder() to specify error actions
Related: #3504
Motivation:
There are two places in the SocksAuthRequest constructor where an
IllegalArgumentException is thrown with a password as part of the
exception message.
This constitutes mishandling of confidential information, which can
compromise user privacy and is flagged as critical by security scanners.
Modifications:
Mask the password in the exception messages
Result:
No unexpected password leak
While implementing netty-handler-proxy, I realized various issues in our
current socksx package. Here's the list of the modifications and their
background:
- Split message types into interfaces and default implementations
- so that a user can implement an alternative message implementations
- Use classes instead of enums when a user might want to define a new
constant
- so that a user can extend SOCKS5 protocol, such as:
- defining a new error code
- defining a new address type
- Rename the message classes
- to avoid abbreviated class names. e.g:
- Cmd -> Command
- Init -> Initial
- so that the class names align better with the protocol
specifications. e.g:
- AuthRequest -> PasswordAuthRequest
- AuthScheme -> AuthMethod
- Rename the property names of the messages
- so that the property names align better when the field names in the
protocol specifications
- Improve the decoder implementations
- Give a user more control over when a decoder has to be removed
- Use DecoderResult and DecoderResultProvider to handle decode failure
gracefully. i.e. no more Unknown* message classes
- Add SocksPortUnifinicationServerHandler since it's useful to the users
who write a SOCKS server
- Cleaned up and moved from the socksproxy example
Related issue: #1133
Motivation:
There is no support for client socket connections via a proxy server in
Netty.
Modifications:
- Add a new module 'handler-proxy'
- Add ProxyHandler and its subclasses to support SOCKS 4a/5 and HTTP(S)
proxy connections
- Add a full parameterized test for most scenarios
- Clean up pom.xml
Result:
A user can make an outgoing connection via proxy servers with only
trivial effort.
Motivation:
Socks5CmdRequestDecoder uses ByteBuf.array() naively assuming that the
array's base offset is always 0, which is not the case.
Modification:
- Allocate a new byte array and copy the content there instead
Result:
Another bug fixed
Motivation:
The unit tests in codec-socks contained redundant casts and empty test classes.
Modifications:
- Remove redundant casts
- Delete empty test classes
Result:
Cleanup
- SocksV[45] -> Socks[45]
- Make encodeAsByteBuf package private with some hassle
- Split SocksMessageEncoder into Socks4MessageEncoder and
Socks5MessageEncoder, and remove the original
- Remove lazy singleton instantiation; we don't need it.
- Remove the deprecated methods
- Fix Javadoc errors
Motivation:
SOCKS 4 and 5 are very different protocols although they share the same
name. It is not possible to incorporate the two protocol versions into
a single package.
Modifications:
- Add a new package called 'socksx' to supercede 'socks' package.
- Add SOCKS 4/4a support to the 'socksx' package
Result:
codec-socks now supports all SOCKS versions
Motivation:
The examples have not been updated since long time ago, showing various
issues fixed in this commit.
Modifications:
- Overall simplification to reduce LoC
- Use system properties to get options instead of parsing args.
- Minimize option validation
- Just use System.out/err instead of Logger
- Do not pass config as parameters - just access it directly
- Move the main logic to main(String[]) instead of creating a new
instance meaninglessly
- Update netty-build-21 to make checkstyle not complain
- Remove 'throws Exception' clause if possible
- Line wrap at 120 (previously at 80)
- Add an option to enable SSL for most examples
- Use ChannelFuture.sync() instead of await()
- Use System.out for the actual result. Use System.err otherwise.
- Delete examples that are not very useful:
- applet
- websocket/html5
- websocketx/sslserver
- localecho/multithreaded
- Add run-example.sh which simplifies launching an example from command
line
- Rewrite FileServer example
Result:
Shorter and simpler examples. A user can focus more on what it actually
does than miscellaneous stuff. A user can launch an example very
easily.
Motivation:
Ports range check is not correct
Modification:
Allow port between 0 and 65535. 0 is wildcard / unknown port here
Result:
Correct validation
Motivation:
In the Internet Protocol, the valid port number range is from 1 to 65535
(inclusive on the both side.) However, SocksCmdRequest and SocksCmdResponse
refuses to construct itself when the port number 65535 is specified. Beside
this it excepts 0 as port number which should not allowed.
Modification:
* Not raise an exception when the specified port number is 65535.
* Raise an exception when the specified port number is 0
Result:
Fixes#2428
Motivation:
In the Internet Protocol, the valid port number range is from 1 to 65535
(inclusive on the both side.) However, SocksCmdRequest refuses to
construct itself when the port number 65535 is specified.
Modification:
Do not raise an exception when the specified port number is 65535.
Result:
Fixes#2428
I must admit MesageList was pain in the ass. Instead of forcing a
handler always loop over the list of messages, this commit splits
messageReceived(ctx, list) into two event handlers:
- messageReceived(ctx, msg)
- mmessageReceivedLast(ctx)
When Netty reads one or more messages, messageReceived(ctx, msg) event
is triggered for each message. Once the current read operation is
finished, messageReceivedLast() is triggered to tell the handler that
the last messageReceived() was the last message in the current batch.
Similarly, for outbound, write(ctx, list) has been split into two:
- write(ctx, msg)
- flush(ctx, promise)
Instead of writing a list of message with a promise, a user is now
supposed to call write(msg) multiple times and then call flush() to
actually flush the buffered messages.
Please note that write() doesn't have a promise with it. You must call
flush() to get notified on completion. (or you can use writeAndFlush())
Other changes:
- Because MessageList is completely hidden, codec framework uses
List<Object> instead of MessageList as an output parameter.
The API changes made so far turned out to increase the memory footprint
and consumption while our intention was actually decreasing them.
Memory consumption issue:
When there are many connections which does not exchange data frequently,
the old Netty 4 API spent a lot more memory than 3 because it always
allocates per-handler buffer for each connection unless otherwise
explicitly stated by a user. In a usual real world load, a client
doesn't always send requests without pausing, so the idea of having a
buffer whose life cycle if bound to the life cycle of a connection
didn't work as expected.
Memory footprint issue:
The old Netty 4 API decreased overall memory footprint by a great deal
in many cases. It was mainly because the old Netty 4 API did not
allocate a new buffer and event object for each read. Instead, it
created a new buffer for each handler in a pipeline. This works pretty
well as long as the number of handlers in a pipeline is only a few.
However, for a highly modular application with many handlers which
handles connections which lasts for relatively short period, it actually
makes the memory footprint issue much worse.
Changes:
All in all, this is about retaining all the good changes we made in 4 so
far such as better thread model and going back to the way how we dealt
with message events in 3.
To fix the memory consumption/footprint issue mentioned above, we made a
hard decision to break the backward compatibility again with the
following changes:
- Remove MessageBuf
- Merge Buf into ByteBuf
- Merge ChannelInboundByte/MessageHandler and ChannelStateHandler into ChannelInboundHandler
- Similar changes were made to the adapter classes
- Merge ChannelOutboundByte/MessageHandler and ChannelOperationHandler into ChannelOutboundHandler
- Similar changes were made to the adapter classes
- Introduce MessageList which is similar to `MessageEvent` in Netty 3
- Replace inboundBufferUpdated(ctx) with messageReceived(ctx, MessageList)
- Replace flush(ctx, promise) with write(ctx, MessageList, promise)
- Remove ByteToByteEncoder/Decoder/Codec
- Replaced by MessageToByteEncoder<ByteBuf>, ByteToMessageDecoder<ByteBuf>, and ByteMessageCodec<ByteBuf>
- Merge EmbeddedByteChannel and EmbeddedMessageChannel into EmbeddedChannel
- Add SimpleChannelInboundHandler which is sometimes more useful than
ChannelInboundHandlerAdapter
- Bring back Channel.isWritable() from Netty 3
- Add ChannelInboundHandler.channelWritabilityChanges() event
- Add RecvByteBufAllocator configuration property
- Similar to ReceiveBufferSizePredictor in Netty 3
- Some existing configuration properties such as
DatagramChannelConfig.receivePacketSize is gone now.
- Remove suspend/resumeIntermediaryDeallocation() in ByteBuf
This change would have been impossible without @normanmaurer's help. He
fixed, ported, and improved many parts of the changes.
- Fixes#1229
- Primarily written by @normanmaurer and revised by @trustin
This commit removes the notion of unfolding from the codec framework
completely. Unfolding was introduced in Netty 3.x to work around the
shortcoming of the codec framework where encode() and decode() did not
allow generating multiple messages.
Such a shortcoming can be fixed by changing the signature of encode()
and decode() instead of introducing an obscure workaround like
unfolding. Therefore, we changed the signature of them in 4.0.
The change is simple, but backward-incompatible. encode() and decode()
do not return anything. Instead, the codec framework will pass a
MessageBuf<Object> so encode() and decode() can add the generated
messages into the MessageBuf.
