Ensure networkBio is closed in ConscryptEngine. #942
Merged
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
Contains @kruton's PR google#893 plus my comments. Differences: Throw `SslException` not `SocketException` on read and write to closed `ssl` as these will propogate out from `SSLEngine.wrap()` (and unwrap). Calls `closeAndFreeResources()` from `ConscryptEngine.finalizer()` which should ensure resources are consistently freed even if engines aren't properly closed. `closeAndFreeResources()` handles null `ssl` or `networkBio` - because it is called from the finalizer it must handle incompletely constructed instances. Also closes `networkBio` regardless of whether `ssl` is closed in case the engine ever gets into this state (e.g. if `close()` throws). Both close methods are idempotent. Contrary to my suggestion, no check for `bio == 0L` on read and write because native code will already throw `NullPointerException` for that which seems correct.
daulet
approved these changes
Jan 12, 2021
prbprbprb
added a commit
that referenced
this pull request
Feb 25, 2021
Ensure networkBio is closed in ConscryptEngine.
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.
Contains @kruton's PR #893 plus my comments.
Differences:
Throw
SslExceptionnotSocketExceptionon read and write toclosed
sslas these will propogate out fromSSLEngine.wrap()(and unwrap).
Calls
closeAndFreeResources()fromConscryptEngine.finalizer()which should ensure resources are consistently freed even if engines
aren't properly closed.
closeAndFreeResources()handles nullsslornetworkBio- becauseit is called from the finalizer it must handle incompletely constructed
instances. Also closes
networkBioregardless of whethersslisclosed in case the engine ever gets into this state (e.g. if
close()throws). Both close methods are idempotent.
Contrary to my suggestion, no check for
bio == 0Lon read and writebecause native code will already throw
NullPointerExceptionfor thatwhich seems correct.