Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Excessively large cumulation ByteBuf in SslHandler #3567

Closed
dkartaschew opened this issue Apr 2, 2015 · 15 comments
Closed

Excessively large cumulation ByteBuf in SslHandler #3567

dkartaschew opened this issue Apr 2, 2015 · 15 comments
Labels
defect
Milestone
4.0.27.Final

Comments

@dkartaschew
Copy link

Hi,

Upon upgrading from netty 4.0.23 to netty 4.0.26, we've found that during large/fast data transfers via TLS 1.2, the "cumulation" ByteBuf in the SslHandler grows to over 1GB in direct buffer usage, leading to OOM errors. The growth of the "cumulation" ByteBuf does appear to correspond with the amount of data received.

This growth occurs only the receiving side, irrespective of which side initiated the connection.

Testing with netty 4.0.23 and 4.0.24 shows the buffer never/rarely grows, but starting with 4.0.25 the growth is seen. We've tested with Oracle JRE 7u67 and 8u31 with the same result.

The channel pipeline setup is:

ChannelPipeline p = ch.pipeline();
if(engine != null){
  p.addLast("ssl", new SslHandler(engine));
}
p.addLast("frameDecoder", new ProtobufVarint32FrameDecoder());
p.addLast("protobufDecoder", new ProtobufDecoder(ApplicationProtos.getDefaultInstance()));
p.addLast("frameEncoder", new ProtobufVarint32LengthFieldPrepender());
p.addLast("protobufEncoder", new ProtobufEncoder());
p.addLast("handler", handler);

This may be related to bugs #3559 and #3447 , but ultimately unsure. Removing the SslHandler does improve things in our instance, but suspect that is due to the pipeline setup.

If there is any more information required, please let me know.
@trustin
Copy link
Member

trustin commented Apr 2, 2015

Thanks for reporting. I guess there might be some edge case we didn't handle correctly. Would you mind if I ask to share the heap dump with us?

@trustin
Copy link
Member

trustin commented Apr 2, 2015

Alternatively, you could just share the readerIndex and writerIndex of the cumulative buffer.

@trustin trustin added this to the 4.0.27.Final milestone Apr 2, 2015
@trustin trustin added the defect label Apr 2, 2015
@dkartaschew
Copy link
Author

Hi Trustin,

I've attached a screen shot from a heap dump using netty 4.0.25 and 4.0.26 showing the effect, a screenshot of the direct buffer usage from jvisualvm.

netty_4 0 25
netty_4 0 26
jvisualvm

@trustin
Copy link
Member

trustin commented Apr 2, 2015

@dkartaschew Thanks! It seems like the reader/writerIndex of the cumulative buffer is: 556527170/556581723, which seems way too large.

@trustin
Copy link
Member

trustin commented Apr 2, 2015

@dkartaschew Could you tell me which SSLEngine implementation you are using? (OpenSslEngine vs the one from JDK)

@trustin
Copy link
Member

trustin commented Apr 2, 2015

@dkartaschew I'm also interested in the vaule of the SslHandler.first (boolean)

trustin added a commit that referenced this issue Apr 2, 2015
@trustin
Fix unbounded expansion of cumulative buffer in SslHandler
2e509f7 
Related: #3567

Motivation:

SslHandler.channelReadComplete() forgets to call
super.channelReadComplete(), which discards read bytes from the
cumulative buffer.  As a result, the cumulative buffer can expand its
capacity unboundedly.

Modifications:

Call super.channelReadComplete() instead of calling
ctx.fireChannelReadComplete()

Result:

Fixes #3567
trustin added a commit that referenced this issue Apr 2, 2015
@trustin
Fix unbounded expansion of cumulative buffer in SslHandler
4d8f824 
Related: #3567

Motivation:

SslHandler.channelReadComplete() forgets to call
super.channelReadComplete(), which discards read bytes from the
cumulative buffer.  As a result, the cumulative buffer can expand its
capacity unboundedly.

Modifications:

Call super.channelReadComplete() instead of calling
ctx.fireChannelReadComplete()

Result:

Fixes #3567
@trustin trustin closed this as completed in cdd2e1c Apr 2, 2015
@trustin
Copy link
Member

trustin commented Apr 2, 2015

Should be fixed now: 4d8f824 (4.0), 2e509f7 (4.1), cdd2e1c (master)

@trustin
Copy link
Member

trustin commented Apr 2, 2015

Perhaps we should make the handler method implementations in ByteToMessageDecoder final and provide protected hook methods. What do you think, @normanmaurer?

@trustin
Copy link
Member

trustin commented Apr 2, 2015

@dkartaschew Please feel free to reopen this issue if you still observe the problem with the SNAPSHOT build.

@normanmaurer
Copy link
Member

@trustin ouch.... We should aim for release 4.0.27.Final very soon.

About your question: I agree we should probably do this but this is a breaking change so we will need to do it in master only.

@fredericBregier
Copy link
Member

@normanmaurer Do you mind to release "exceptionnaly" 4.1 too ?

@dkartaschew
Copy link
Author

@trustin, we're using the JRE's SSLEngine as the OpenSSL implementation isn't feature complete for our needs (#3005 and #3220).

And SslHandler.first is false in all cases we observed this issue.

@normanmaurer normanmaurer mentioned this issue Apr 9, 2015
Closed
@svnp10
Copy link

svnp10 commented Oct 19, 2016

Hi, I find this issue 4.1.4.Final and 4.1.6.Final as well...

@normanmaurer
Copy link
Member

@svnp10 please open a new issue and provide more infos

@svnp10
Copy link

svnp10 commented Oct 31, 2016

@normanmaurer issue here #5928

pulllock pushed a commit to pulllock/netty that referenced this issue Oct 19, 2023
@trustin
Fix unbounded expansion of cumulative buffer in SslHandler
a7ad924 
Related: netty#3567

Motivation:

SslHandler.channelReadComplete() forgets to call
super.channelReadComplete(), which discards read bytes from the
cumulative buffer.  As a result, the cumulative buffer can expand its
capacity unboundedly.

Modifications:

Call super.channelReadComplete() instead of calling
ctx.fireChannelReadComplete()

Result:

Fixes netty#3567
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
defect
Projects
None yet
Milestone
4.0.27.Final
Development

No branches or pull requests
5 participants
@trustin @normanmaurer @fredericBregier @dkartaschew @svnp10