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

Ensure calling ReferenceCountedOpenSslEngine.wrap(...) after closeOut… #6262

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

Projects
None yet
3 participants
@normanmaurer
Member

normanmaurer commented Jan 20, 2017

…bound() was called will not throw an SSLException

Motivation:

PR [#6238] added guards to be able to call wrap(...) / unwrap(...) after the engine was shutdown. Unfortunally one case was missed which is when closeOutbound() was called and produced some data while closeInbound() was not called yet.

Modifications:

Correctly guard against SSLException when closeOutbound() was called, produced data and someone calls wrap(...) after it.

Result:

No more SSLException. Fixes [#6260].

@Scottmitch

lgtm ... unit test would be nice if possible :)

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jan 20, 2017

Member

@Scottmitch @rkapsi added an unit-test FTW 🎉

Member

normanmaurer commented Jan 20, 2017

@Scottmitch @rkapsi added an unit-test FTW 🎉

Ensure calling ReferenceCountedOpenSslEngine.wrap(...) after closeOut…
…bound() was called will not throw an SSLException

Motivation:

PR [#6238] added guards to be able to call wrap(...) / unwrap(...) after the engine was shutdown. Unfortunally one case was missed which is when closeOutbound() was called and produced some data while closeInbound() was not called yet.

Modifications:

Correctly guard against SSLException when closeOutbound() was called, produced data and someone calls wrap(...) after it.

Result:

No more SSLException. Fixes [#6260].
@rkapsi

This comment has been minimized.

Show comment
Hide comment
@rkapsi

rkapsi Jan 20, 2017

Contributor

Deploying... Should get some results in a few minutes...

Contributor

rkapsi commented Jan 20, 2017

Deploying... Should get some results in a few minutes...

@Scottmitch

double approve

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jan 20, 2017

Member

@Scottmitch double is better then single approved 👍

Member

normanmaurer commented Jan 20, 2017

@Scottmitch double is better then single approved 👍

@rkapsi

This comment has been minimized.

Show comment
Hide comment
@rkapsi

rkapsi Jan 20, 2017

Contributor

Make it triple, exception is gone.

Contributor

rkapsi commented Jan 20, 2017

Make it triple, exception is gone.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jan 20, 2017

Member

@rkapsi 😍 thanks for the quick turn-around

Member

normanmaurer commented Jan 20, 2017

@rkapsi 😍 thanks for the quick turn-around

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Jan 20, 2017

Member

that was quick @rkapsi! I guess the exception reproduces quickly when the issue is present?

Member

Scottmitch commented Jan 20, 2017

that was quick @rkapsi! I guess the exception reproduces quickly when the issue is present?

@rkapsi

This comment has been minimized.

Show comment
Hide comment
@rkapsi

rkapsi Jan 20, 2017

Contributor

Yea, a couple of times per minute (before the first fix it was every other second). All I needed to do is to deploy one server with the new artifact and watch the logs. It's something we can thankfully do very quickly (and Netty is thanks to the Unit Tests quite low risk).

Contributor

rkapsi commented Jan 20, 2017

Yea, a couple of times per minute (before the first fix it was every other second). All I needed to do is to deploy one server with the new artifact and watch the logs. It's something we can thankfully do very quickly (and Netty is thanks to the Unit Tests quite low risk).

@Scottmitch

This comment has been minimized.

Show comment
Hide comment
@Scottmitch

Scottmitch Jan 20, 2017

Member

sgtm @rkapsi ... ship it @normanmaurer

Member

Scottmitch commented Jan 20, 2017

sgtm @rkapsi ... ship it @normanmaurer

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jan 21, 2017

Member

Cherry-picked into 4.1 (e9fa40d) and 4.0 (f1130e4)

Member

normanmaurer commented Jan 21, 2017

Cherry-picked into 4.1 (e9fa40d) and 4.0 (f1130e4)

@normanmaurer normanmaurer deleted the ssl_write_shutdown branch Jan 21, 2017

@normanmaurer normanmaurer added the defect label Jan 21, 2017

@normanmaurer normanmaurer self-assigned this Jan 21, 2017

@normanmaurer normanmaurer added this to the 4.0.44.Final milestone Jan 21, 2017

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