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.

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

Fix possible leak in SslHandler if wrap(...) throws. #7338

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

Projects
None yet
4 participants
@normanmaurer
Member

normanmaurer commented Oct 24, 2017

Motivation:

We can end up with a buffer leak if SSLEngine.wrap(...) throws.

Modifications:

Correctly release the ByteBuf if SSLEngine.wrap(...) throws.

Result:

Fixes [#7337].

@eun-ice

This comment has been minimized.

Show comment
Hide comment
@eun-ice

eun-ice Oct 24, 2017

Contributor

I have seen SSLEngine.wrap throw with OpenSSL in production and some weird half-closed SSL states.
So thank you for this fix!

Contributor

eun-ice commented Oct 24, 2017

I have seen SSLEngine.wrap throw with OpenSSL in production and some weird half-closed SSL states.
So thank you for this fix!

Fix possible leak in SslHandler if wrap(...) throws.
Motivation:

We can end up with a buffer leak if SSLEngine.wrap(...) throws.

Modifications:

Correctly release the ByteBuf if SSLEngine.wrap(...) throws.

Result:

Fixes [#7337].
@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 24, 2017

Member

Cherry-picked into 4.1 (92b786e). 4.0 is not affected

Member

normanmaurer commented Oct 24, 2017

Cherry-picked into 4.1 (92b786e). 4.0 is not affected

@normanmaurer normanmaurer deleted the fix_possible_leak branch Oct 24, 2017

@jchambers

This comment has been minimized.

Show comment
Hide comment
@jchambers

jchambers Nov 12, 2017

Contributor

I'm trying to figure out if this might have been the cause of some leaks I've been chasing down. My understanding is that:

  1. This problem has been around for at least six months, and possibly longer.
  2. In practical terms, this problem is most likely to manifest on a lossy connection, and generally should not be an issue on high-quality, low-loss connections.

Is that accurate? If it is, then I think this solves our problem. Thanks kindly!

Contributor

jchambers commented Nov 12, 2017

I'm trying to figure out if this might have been the cause of some leaks I've been chasing down. My understanding is that:

  1. This problem has been around for at least six months, and possibly longer.
  2. In practical terms, this problem is most likely to manifest on a lossy connection, and generally should not be an issue on high-quality, low-loss connections.

Is that accurate? If it is, then I think this solves our problem. Thanks kindly!

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Nov 12, 2017

Member
Member

normanmaurer commented Nov 12, 2017

@jchambers

This comment has been minimized.

Show comment
Hide comment
@jchambers

jchambers Nov 12, 2017

Contributor

Thank you!

Contributor

jchambers commented Nov 12, 2017

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment