Correctly clear the error stack in all cases when using ReferenceCoun… #7941
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tedOpenSslEngine. Motivation: We missed to correctly clear the error stack in one case when using the ReferenceCountedOpenSslEngine. Because of this it was possible to pick up an error on an unrelated operation. Modifications: - Correctly clear the stack - Add verification of empty error stack to tests. Result: Not possible to observe unrelated errors.
normanmaurer
requested review from
trustin,
ejona86,
Scottmitch and
carl-mastrangelo
normanmaurer
commented
May 15, 2018
| @@ -1093,8 +1093,6 @@ public final SSLEngineResult unwrap( | |||
| } | |||
|
|
|||
| private SSLEngineResult sslReadErrorResult(int err, int bytesConsumed, int bytesProduced) throws SSLException { | |||
| String errStr = SSL.getErrorString(err); | |||
There was a problem hiding this comment.
This change was just done to eliminate the need to create it when its not needed.
ddossot
reviewed
May 15, 2018
| @Override | ||
| public void tearDown() throws InterruptedException { | ||
| super.tearDown(); | ||
| Assert.assertEquals("SSL error stack not correctly consumed", 0, SSL.getLastErrorNumber()); |
There was a problem hiding this comment.
nit: static import Assert.assertEquals
johnou
reviewed
May 15, 2018
| } | ||
| // We need to clear all errors so we not pick up anything that was left on the stack on the next | ||
| // operation. Note that shutdownWithError(...) will cleanup the stack as well so its only needed here. | ||
| SSL.clearError(); |
There was a problem hiding this comment.
@normanmaurer without this what behaviour did you observe?
There was a problem hiding this comment.
basically whatever test that ran after OpenSslEngineTest.testMultipleRecordsInOneBufferWithNonZeroPositionJDKCompatabilityModeOff failed when using BoringSSL. Thats also why I added asserts now to ensure the error stack is empty when we teardown.
ejona86
approved these changes
May 15, 2018
Scottmitch
approved these changes
May 15, 2018
| @@ -1093,8 +1093,6 @@ public final SSLEngineResult unwrap( | |||
| } | |||
|
|
|||
| private SSLEngineResult sslReadErrorResult(int err, int bytesConsumed, int bytesProduced) throws SSLException { | |||
There was a problem hiding this comment.
future improvement: we don't actually use err for anything besides getting the string. We could add a method to netty-tcnative that is SSL.getLastErrorString() to save a JNI call. Looks like this is the only place that getLastErrorNumber is used in non-unit test code too.
normanmaurer
added a commit
that referenced
this pull request
May 15, 2018
…CountedOpenSslEngine. Motivation: #7941 proved that its easy to not correctly clear the error stack sometimes. We should do carefully test this. Modifications: Add a new SSLEngine wrapper that is used during tests, which verifies that the error stack is empty after each method call. Result: Better testing.
normanmaurer
added a commit
that referenced
this pull request
May 16, 2018
…CountedOpenSslEngine. Motivation: #7941 proved that its easy to not correctly clear the error stack sometimes. We should do carefully test this. Modifications: Add a new SSLEngine wrapper that is used during tests, which verifies that the error stack is empty after each method call. Result: Better testing.
normanmaurer
added a commit
that referenced
this pull request
May 16, 2018
…CountedOpenSslEngine. (#7943) Motivation: #7941 proved that its easy to not correctly clear the error stack sometimes. We should do carefully test this. Modifications: Add a new SSLEngine wrapper that is used during tests, which verifies that the error stack is empty after each method call. Result: Better testing.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…tedOpenSslEngine.
Motivation:
We missed to correctly clear the error stack in one case when using the ReferenceCountedOpenSslEngine. Because of this it was possible to pick up an error on an unrelated operation.
Modifications:
Result:
Not possible to observe unrelated errors.