Ensure networkBio is closed in ConscryptEngine #893

kruton · 2020-09-11T20:08:35Z

Since BioWrapper does not have a finalizer, we need to ensure that the
native resources are freed when it goes away. In all paths,
"networkBio.close()" needs to be called to free the native resources.
This change rearranges some items to make sure that the native resource
never gets leaked.

Possible fix for leak identified in #835

This change is

Since BioWrapper does not have a finalizer, we need to ensure that the native resources are freed when it goes away. In all paths, "networkBio.close()" needs to be called to free the native resources. This change rearranges some items to make sure that the native resource never gets leaked. Possible fix for leak identified in google#835

For direct BIO access, the NativeSsl was not checked to see if it was open resulting in an exception every once in a while. Exception in thread "Keep-Alive-SocketCleaner" java.lang.NullPointerException: ssl == null at org.conscrypt.NativeCrypto.ENGINE_SSL_write_BIO_direct(Native Method) at org.conscrypt.NativeSsl$BioWrapper.writeDirectByteBuffer(NativeSsl.java:666) at org.conscrypt.ConscryptEngine.writeEncryptedDataDirect(ConscryptEngine.java:1166) at org.conscrypt.ConscryptEngine.writeEncryptedDataHeap(ConscryptEngine.java:1193) at org.conscrypt.ConscryptEngine.writeEncryptedData(ConscryptEngine.java:1151) at org.conscrypt.ConscryptEngine.unwrap(ConscryptEngine.java:838)

kruton · 2020-09-11T21:24:44Z

common/src/main/java/org/conscrypt/NativeSsl.java

+                        ssl, NativeSsl.this, bio, destAddress, destLength, handshakeCallbacks);
+            } finally {
+                lock.readLock().unlock();
+            }
        }

        void close() {


It seems like this should hold the lock.writeLock()

Definitely needs synchronisation as the finaliser will run on a GC thread, and the write lock seems more appropriate.

kruton · 2020-09-11T21:25:16Z

common/src/main/java/org/conscrypt/NativeSsl.java

-                    ssl, NativeSsl.this, bio, destAddress, destLength, handshakeCallbacks);
+            lock.readLock().lock();
+            try {
+                if (isClosed()) {


Should this check for bio == 0L ?

kruton · 2020-09-11T21:25:22Z

common/src/main/java/org/conscrypt/NativeSsl.java

-                    ssl, NativeSsl.this, bio, address, length, handshakeCallbacks);
+            lock.readLock().lock();
+            try {
+                if (isClosed()) {


Should this check for bio == 0L ?

Either that or make the native code throw on bio == 0 rather than return -1 as it does at present, which doesn't seem to be handled consistently.

kruton · 2020-09-11T21:25:53Z

common/src/main/java/org/conscrypt/NativeSsl.java

@@ -663,13 +663,29 @@ int getPendingWrittenBytes() {
        }


Should we use lock.readLock() in the check in getPendingWrittenBytes ?

Yes, please.

prbprbprb

Thanks for digging into this!

prbprbprb · 2020-09-14T14:56:26Z

common/src/main/java/org/conscrypt/ConscryptEngine.java

@@ -1683,6 +1683,7 @@ private void closeAndFreeResources() {
    protected void finalize() throws Throwable {
        try {
            transitionTo(STATE_CLOSED);
+            networkBio.close();


Maybe call closeAndFreeResources()? If we get to this point and still have a live BIO, then odds are we haven't closed the SSL either.

However this should only happen if an engine gets GCed without close() ever getting called (or at least, never completing), is that enough to account for the memory leakage?

It's possible that #844 made this worse in some cases... For the happy case, it ensures the native resources get freed on close() rather than waiting until GC, but if the socket blocks on write then close() can also hang. However that would manifest itself as a thread leak... Are you or @knaccc seeing that?

prbprbprb · 2020-09-14T14:58:37Z

common/src/main/java/org/conscrypt/NativeSsl.java

@@ -655,27 +655,45 @@ private BioWrapper() throws SSLException {
        }

        int getPendingWrittenBytes() {
-            if (bio != 0) {
+            if (bio != 0L) {


Note that NativeCrypto.SSL_pending_written_bytes_in_BIO() returns 0 anyway if bio is 0.

prbprbprb · 2020-09-14T15:07:51Z

common/src/main/java/org/conscrypt/NativeSsl.java

@@ -663,13 +663,29 @@ int getPendingWrittenBytes() {
        }


Yes, please.

prbprbprb · 2020-09-14T15:14:59Z

common/src/main/java/org/conscrypt/NativeSsl.java

-                    ssl, NativeSsl.this, bio, address, length, handshakeCallbacks);
+            lock.readLock().lock();
+            try {
+                if (isClosed()) {


Either that or make the native code throw on bio == 0 rather than return -1 as it does at present, which doesn't seem to be handled consistently.

prbprbprb · 2020-09-14T15:15:18Z

common/src/main/java/org/conscrypt/NativeSsl.java

-                    ssl, NativeSsl.this, bio, destAddress, destLength, handshakeCallbacks);
+            lock.readLock().lock();
+            try {
+                if (isClosed()) {


prbprbprb · 2020-09-14T15:17:11Z

common/src/main/java/org/conscrypt/NativeSsl.java

+            lock.readLock().lock();
+            try {
+                if (isClosed()) {
+                    throw new SocketException("Socket is closed");


That error message won't make sense for apps using SSLEngine directly.

I think these checks (here and in readDirect...) should be at the ConscryptEngine level, both methods are only called from a single place.

prbprbprb · 2020-09-14T15:18:29Z

common/src/main/java/org/conscrypt/NativeSsl.java

+                        ssl, NativeSsl.this, bio, destAddress, destLength, handshakeCallbacks);
+            } finally {
+                lock.readLock().unlock();
+            }
        }

        void close() {


Definitely needs synchronisation as the finaliser will run on a GC thread, and the write lock seems more appropriate.

kruton · 2020-09-28T01:22:54Z

@prbprbprb could you finish this PR? It doesn't appear I'll have time to respond this week.

prbprbprb · 2020-09-28T13:11:10Z

Sure! Are you mostly happy with my suggestions?

kruton · 2020-09-28T15:06:17Z

Yes, they all looked reasonable.

…

@kruton

Contains @kruton's PR google#893 plus my comments. Differences: Doesn't throw from `writeDirectByteBuffer()` (or read) if `isClosed()`. This code is only ever called from blocks that are synchronized on `ssl` so `ssl` cannot be null here, which is what `isClosed()` checks. 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.

@kruton

Contains @kruton's PR google#893 plus my comments. Differences: Throw `SslEnginei` not `SocketException` on read and write to closed `ssl`. 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.

@kruton

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 `SSLEnngine.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.

@kruton

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.

pparth · 2021-01-21T07:58:12Z

Any updates here?

prbprbprb · 2021-01-22T14:48:21Z

Any updates here?

Superceded by #942 which I just merged.

pparth · 2021-01-25T17:09:22Z

ok, we will test and see. thanks!

kruton requested a review from prbprbprb September 11, 2020 20:08

kruton mentioned this pull request Sep 11, 2020

Severe memory leak when setUseEngineSocketByDefault(true) #835

Open

kruton added 2 commits September 11, 2020 13:49

kruton force-pushed the fix-bio-leak branch from e4a4c9b to 30f9218 Compare September 11, 2020 20:56

kruton commented Sep 11, 2020

View reviewed changes

prbprbprb reviewed Sep 14, 2020

View reviewed changes

Merge branch 'master' into fix-bio-leak

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

e78e045

Merge branch 'master' into fix-bio-leak

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

0bc87cf

Merge branch 'master' into fix-bio-leak

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

1e46f96

prbprbprb mentioned this pull request Dec 21, 2020

Ensure networkBio is closed in ConscryptEngine. #942

Merged

prbprbprb closed this Jan 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure networkBio is closed in ConscryptEngine #893

Ensure networkBio is closed in ConscryptEngine #893

kruton commented Sep 11, 2020 •

edited by prbprbprb

Loading

kruton Sep 11, 2020

prbprbprb Sep 14, 2020

kruton Sep 11, 2020

prbprbprb Sep 14, 2020

kruton Sep 11, 2020

prbprbprb Sep 14, 2020

kruton Sep 11, 2020

prbprbprb Sep 14, 2020

prbprbprb left a comment

prbprbprb Sep 14, 2020 •

edited

Loading

sumiaktersompa Jan 13, 2023

prbprbprb Sep 14, 2020

prbprbprb Sep 14, 2020

prbprbprb Sep 14, 2020

prbprbprb Sep 14, 2020

prbprbprb Sep 14, 2020 •

edited

Loading

prbprbprb Sep 14, 2020

kruton commented Sep 28, 2020

prbprbprb commented Sep 28, 2020

kruton commented Sep 28, 2020 via email

pparth commented Jan 21, 2021

prbprbprb commented Jan 22, 2021

pparth commented Jan 25, 2021

Ensure networkBio is closed in ConscryptEngine #893

Ensure networkBio is closed in ConscryptEngine #893

Conversation

kruton commented Sep 11, 2020 • edited by prbprbprb Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prbprbprb left a comment

Choose a reason for hiding this comment

prbprbprb Sep 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prbprbprb Sep 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kruton commented Sep 28, 2020

prbprbprb commented Sep 28, 2020

kruton commented Sep 28, 2020 via email

pparth commented Jan 21, 2021

prbprbprb commented Jan 22, 2021

pparth commented Jan 25, 2021

kruton commented Sep 11, 2020 •

edited by prbprbprb

Loading

prbprbprb Sep 14, 2020 •

edited

Loading

prbprbprb Sep 14, 2020 •

edited

Loading