From 89c47dbcc9fff2309ecaf5f84dc04340f1634351 Mon Sep 17 00:00:00 2001
From: Pete Bentley <prb@google.com>
Date: Fri, 4 Aug 2023 11:45:30 +0100
Subject: [PATCH] Add locking around AbstractSessionContext JNI.

We don't have a definitive root cause for #1131 but it seems like either use-after-free (e.g. finalizer ordering) or concurrency issue, so:
1. Make the native pointer private and move all accesses into AbstractSessionContext
2. Zero it out on finalisation
3. Add locking. Note we only need a read lock for the sslNew() path as this is thread safe and doesn't modify the native SSL_CTX.
---
 .../org/conscrypt/AbstractSessionContext.java | 46 ++++++++++++++++++-
 .../main/java/org/conscrypt/NativeSsl.java    |  3 +-
 .../org/conscrypt/ServerSessionContext.java   |  2 +-
 3 files changed, 46 insertions(+), 5 deletions(-)

diff --git a/common/src/main/java/org/conscrypt/AbstractSessionContext.java b/common/src/main/java/org/conscrypt/AbstractSessionContext.java
index c527b5883..fb50063b9 100644
--- a/common/src/main/java/org/conscrypt/AbstractSessionContext.java
+++ b/common/src/main/java/org/conscrypt/AbstractSessionContext.java
@@ -22,6 +22,10 @@
 import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.NoSuchElementException;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+import javax.net.ssl.SSLException;
 import javax.net.ssl.SSLSession;
 import javax.net.ssl.SSLSessionContext;
 
@@ -39,7 +43,10 @@ abstract class AbstractSessionContext implements SSLSessionContext {
     private volatile int maximumSize;
     private volatile int timeout = DEFAULT_SESSION_TIMEOUT_SECONDS;
 
-    final long sslCtxNativePointer = NativeCrypto.SSL_CTX_new();
+    private volatile long sslCtxNativePointer = NativeCrypto.SSL_CTX_new();
+
+    private final ReadWriteLock lock = new ReentrantReadWriteLock();
+
 
     private final Map<ByteArray, NativeSslSession> sessions =
             new LinkedHashMap<ByteArray, NativeSslSession>() {
@@ -186,11 +193,46 @@ public final void setSessionCacheSize(int size) throws IllegalArgumentException
         }
     }
 
+    long newSsl() throws SSLException {
+        lock.readLock().lock();
+        try {
+            if (sslCtxNativePointer != 0) {
+                return NativeCrypto.SSL_new(sslCtxNativePointer, this);
+            } else {
+                throw new SSLException("Closed.");
+            }
+        } finally {
+            lock.readLock().unlock();
+        }
+    }
+
+    protected void setSesssionIdContext(byte[] bytes) {
+        lock.writeLock().lock();
+        try {
+            NativeCrypto.SSL_CTX_set_session_id_context(sslCtxNativePointer, this, bytes);
+        } finally {
+            lock.readLock().unlock();
+        }
+    }
+
+    private void freeNative() {
+        lock.writeLock().lock();
+        try {
+            if (sslCtxNativePointer != 0) {
+                long toFree = sslCtxNativePointer;
+                sslCtxNativePointer = 0;
+                NativeCrypto.SSL_CTX_free(toFree, this);
+            }
+        } finally {
+            lock.writeLock().unlock();
+        }
+    }
+
     @Override
     @SuppressWarnings("deprecation")
     protected void finalize() throws Throwable {
         try {
-            NativeCrypto.SSL_CTX_free(sslCtxNativePointer, this);
+            freeNative();
         } finally {
             super.finalize();
         }
diff --git a/common/src/main/java/org/conscrypt/NativeSsl.java b/common/src/main/java/org/conscrypt/NativeSsl.java
index b7b02635f..79369caa8 100644
--- a/common/src/main/java/org/conscrypt/NativeSsl.java
+++ b/common/src/main/java/org/conscrypt/NativeSsl.java
@@ -76,8 +76,7 @@ private NativeSsl(long ssl, SSLParametersImpl parameters,
     static NativeSsl newInstance(SSLParametersImpl parameters,
             SSLHandshakeCallbacks handshakeCallbacks, AliasChooser chooser,
             PSKCallbacks pskCallbacks) throws SSLException {
-        AbstractSessionContext ctx = parameters.getSessionContext();
-        long ssl = NativeCrypto.SSL_new(ctx.sslCtxNativePointer, ctx);
+        long ssl = parameters.getSessionContext().newSsl();
         return new NativeSsl(ssl, parameters, handshakeCallbacks, chooser, pskCallbacks);
     }
 
diff --git a/common/src/main/java/org/conscrypt/ServerSessionContext.java b/common/src/main/java/org/conscrypt/ServerSessionContext.java
index ce7010d01..64fca9a79 100644
--- a/common/src/main/java/org/conscrypt/ServerSessionContext.java
+++ b/common/src/main/java/org/conscrypt/ServerSessionContext.java
@@ -42,7 +42,7 @@ public final class ServerSessionContext extends AbstractSessionContext {
         // sure you don't reuse sessions externalized with i2d_SSL_SESSION
         // between apps. However our sessions are either in memory or
         // exported to a app's SSLServerSessionCache.
-        NativeCrypto.SSL_CTX_set_session_id_context(sslCtxNativePointer, this, new byte[] { ' ' });
+        setSesssionIdContext(new byte[] { ' ' });
     }
 
     /**