Remove synchronized (ReferenceCountedOpenSslContext.class) blocks

Motivation:

We had some useless synchronized (ReferenceCountedOpenSslContext.class) blocks in our code which could slow down concurrent collecting and creating of ReferenceCountedOpenSslContext instances. Beside this we missed a few guards.

Modifications:

Use ReadWriteLock to correctly guard. A ReadWriteLock was choosen as SSL.newSSL(...) will be called from multiple threads all the time so using synchronized would be worse and there would be no way for the JIT to optimize it away

Result:

Faster concurrent creating and collecting of ReferenceCountedOpenSslContext instances and correctly guard in all cases.

handler/src/main/java/io/netty/handler/ssl/OpenSslServerSessionContext.java

@@ -18,6 +18,8 @@ package io.netty.handler.ssl;
 import io.netty.internal.tcnative.SSL;
 import io.netty.internal.tcnative.SSLContext;
 
+import java.util.concurrent.locks.Lock;
+
 
 /**
  * {@link OpenSslSessionContext} implementation which offers extra methods which are only useful for the server-side.
@@ -32,12 +34,24 @@ public final class OpenSslServerSessionContext extends OpenSslSessionContext {
         if (seconds < 0) {
             throw new IllegalArgumentException();
         }
+        Lock writerLock = context.ctxLock.writeLock();
+        writerLock.lock();
+        try {
             SSLContext.setSessionCacheTimeout(context.ctx, seconds);
+        } finally {
+            writerLock.unlock();
+        }
     }
 
     @Override
     public int getSessionTimeout() {
+        Lock readerLock = context.ctxLock.readLock();
+        readerLock.lock();
+        try {
             return (int) SSLContext.getSessionCacheTimeout(context.ctx);
+        } finally {
+            readerLock.unlock();
+        }
     }
 
     @Override
@@ -45,23 +59,48 @@ public final class OpenSslServerSessionContext extends OpenSslSessionContext {
         if (size < 0) {
             throw new IllegalArgumentException();
         }
+        Lock writerLock = context.ctxLock.writeLock();
+        writerLock.lock();
+        try {
             SSLContext.setSessionCacheSize(context.ctx, size);
+        } finally {
+            writerLock.unlock();
+        }
     }
 
     @Override
     public int getSessionCacheSize() {
+        Lock readerLock = context.ctxLock.readLock();
+        readerLock.lock();
+        try {
             return (int) SSLContext.getSessionCacheSize(context.ctx);
+        } finally {
+            readerLock.unlock();
+        }
     }
 
     @Override
     public void setSessionCacheEnabled(boolean enabled) {
         long mode = enabled ? SSL.SSL_SESS_CACHE_SERVER : SSL.SSL_SESS_CACHE_OFF;
+
+        Lock writerLock = context.ctxLock.writeLock();
+        writerLock.lock();
+        try {
             SSLContext.setSessionCacheMode(context.ctx, mode);
+        } finally {
+            writerLock.unlock();
+        }
     }
 
     @Override
     public boolean isSessionCacheEnabled() {
+        Lock readerLock = context.ctxLock.readLock();
+        readerLock.lock();
+        try {
             return SSLContext.getSessionCacheMode(context.ctx) == SSL.SSL_SESS_CACHE_SERVER;
+        } finally {
+            readerLock.unlock();
+        }
     }
 
     /**
@@ -74,6 +113,12 @@ public final class OpenSslServerSessionContext extends OpenSslSessionContext {
      * @return {@code true} if success, {@code false} otherwise.
      */
     public boolean setSessionIdContext(byte[] sidCtx) {
+        Lock writerLock = context.ctxLock.writeLock();
+        writerLock.lock();
+        try {
             return SSLContext.setSessionIdContext(context.ctx, sidCtx);
+        } finally {
+            writerLock.unlock();
+        }
     }
 }

handler/src/main/java/io/netty/handler/ssl/OpenSslSessionContext.java

@@ -25,6 +25,7 @@ import javax.net.ssl.SSLSessionContext;
 import java.util.Arrays;
 import java.util.Enumeration;
 import java.util.NoSuchElementException;
+import java.util.concurrent.locks.Lock;
 
 /**
  * OpenSSL specific {@link SSLSessionContext} implementation.
@@ -76,8 +77,14 @@ public abstract class OpenSslSessionContext implements SSLSessionContext {
             a += SessionTicketKey.AES_KEY_SIZE;
             tickets[i] = new SessionTicketKey(name, hmacKey, aesKey);
         }
+        Lock writerLock = context.ctxLock.writeLock();
+        writerLock.lock();
+        try {
             SSLContext.clearOptions(context.ctx, SSL.SSL_OP_NO_TICKET);
             SSLContext.setSessionTicketKeys(context.ctx, tickets);
+        } finally {
+            writerLock.unlock();
+        }
     }
 
     /**
@@ -85,12 +92,18 @@ public abstract class OpenSslSessionContext implements SSLSessionContext {
      */
     public void setTicketKeys(OpenSslSessionTicketKey... keys) {
         ObjectUtil.checkNotNull(keys, "keys");
-        SSLContext.clearOptions(context.ctx, SSL.SSL_OP_NO_TICKET);
         SessionTicketKey[] ticketKeys = new SessionTicketKey[keys.length];
         for (int i = 0; i < ticketKeys.length; i++) {
             ticketKeys[i] = keys[i].key;
         }
+        Lock writerLock = context.ctxLock.writeLock();
+        writerLock.lock();
+        try {
+            SSLContext.clearOptions(context.ctx, SSL.SSL_OP_NO_TICKET);
             SSLContext.setSessionTicketKeys(context.ctx, ticketKeys);
+        } finally {
+            writerLock.unlock();
+        }
     }
 
     /**

handler/src/main/java/io/netty/handler/ssl/OpenSslSessionStats.java

@@ -18,6 +18,8 @@ package io.netty.handler.ssl;
 
 import io.netty.internal.tcnative.SSLContext;
 
+import java.util.concurrent.locks.Lock;
+
 /**
  * Stats exposed by an OpenSSL session context.
  *
@@ -39,49 +41,91 @@ public final class OpenSslSessionStats {
      * Returns the current number of sessions in the internal session cache.
      */
     public long number() {
+        Lock readerLock = context.ctxLock.readLock();
+        readerLock.lock();
+        try {
             return SSLContext.sessionNumber(context.ctx);
+        } finally {
+            readerLock.unlock();
+        }
     }
 
     /**
      * Returns the number of started SSL/TLS handshakes in client mode.
      */
     public long connect() {
+        Lock readerLock = context.ctxLock.readLock();
+        readerLock.lock();
+        try {
             return SSLContext.sessionConnect(context.ctx);
+        } finally {
+            readerLock.unlock();
+        }
     }
 
     /**
      * Returns the number of successfully established SSL/TLS sessions in client mode.
      */
     public long connectGood() {
+        Lock readerLock = context.ctxLock.readLock();
+        readerLock.lock();
+        try {
             return SSLContext.sessionConnectGood(context.ctx);
+        } finally {
+            readerLock.unlock();
+        }
     }
 
     /**
      * Returns the number of start renegotiations in client mode.
      */
     public long connectRenegotiate() {
+        Lock readerLock = context.ctxLock.readLock();
+        readerLock.lock();
+        try {
             return SSLContext.sessionConnectRenegotiate(context.ctx);
+        } finally {
+            readerLock.unlock();
+        }
     }
 
     /**
      * Returns the number of started SSL/TLS handshakes in server mode.
      */
     public long accept() {
+        Lock readerLock = context.ctxLock.readLock();
+        readerLock.lock();
+        try {
             return SSLContext.sessionAccept(context.ctx);
+        } finally {
+            readerLock.unlock();
+        }
     }
 
     /**
      * Returns the number of successfully established SSL/TLS sessions in server mode.
      */
     public long acceptGood() {
+        Lock readerLock = context.ctxLock.readLock();
+        readerLock.lock();
+        try {
             return SSLContext.sessionAcceptGood(context.ctx);
+        } finally {
+            readerLock.unlock();
+        }
     }
 
     /**
      * Returns the number of start renegotiations in server mode.
      */
     public long acceptRenegotiate() {
+        Lock readerLock = context.ctxLock.readLock();
+        readerLock.lock();
+        try {
             return SSLContext.sessionAcceptRenegotiate(context.ctx);
+        } finally {
+            readerLock.unlock();
+        }
     }
 
     /**
@@ -90,14 +134,26 @@ public final class OpenSslSessionStats {
      * external cache is counted as a hit.
      */
     public long hits() {
+        Lock readerLock = context.ctxLock.readLock();
+        readerLock.lock();
+        try {
             return SSLContext.sessionHits(context.ctx);
+        } finally {
+            readerLock.unlock();
+        }
     }
 
     /**
      * Returns the number of successfully retrieved sessions from the external session cache in server mode.
      */
     public long cbHits() {
+        Lock readerLock = context.ctxLock.readLock();
+        readerLock.lock();
+        try {
             return SSLContext.sessionCbHits(context.ctx);
+        } finally {
+            readerLock.unlock();
+        }
     }
 
     /**
@@ -105,7 +161,13 @@ public final class OpenSslSessionStats {
      * in server mode.
      */
     public long misses() {
+        Lock readerLock = context.ctxLock.readLock();
+        readerLock.lock();
+        try {
             return SSLContext.sessionMisses(context.ctx);
+        } finally {
+            readerLock.unlock();
+        }
     }
 
     /**
@@ -114,28 +176,52 @@ public final class OpenSslSessionStats {
      * count.
      */
     public long timeouts() {
+        Lock readerLock = context.ctxLock.readLock();
+        readerLock.lock();
+        try {
             return SSLContext.sessionTimeouts(context.ctx);
+        } finally {
+            readerLock.unlock();
+        }
     }
 
     /**
      * Returns the number of sessions that were removed because the maximum session cache size was exceeded.
      */
     public long cacheFull() {
+        Lock readerLock = context.ctxLock.readLock();
+        readerLock.lock();
+        try {
             return SSLContext.sessionCacheFull(context.ctx);
+        } finally {
+            readerLock.unlock();
+        }
     }
 
     /**
      * Returns the number of times a client presented a ticket that did not match any key in the list.
      */
     public long ticketKeyFail() {
+        Lock readerLock = context.ctxLock.readLock();
+        readerLock.lock();
+        try {
             return SSLContext.sessionTicketKeyFail(context.ctx);
+        } finally {
+            readerLock.unlock();
+        }
     }
 
     /**
      * Returns the number of times a client did not present a ticket and we issued a new one
      */
     public long ticketKeyNew() {
+        Lock readerLock = context.ctxLock.readLock();
+        readerLock.lock();
+        try {
             return SSLContext.sessionTicketKeyNew(context.ctx);
+        } finally {
+            readerLock.unlock();
+        }
     }
 
     /**
@@ -143,13 +229,25 @@ public final class OpenSslSessionStats {
      * and we upgraded to the primary key.
      */
     public long ticketKeyRenew() {
+        Lock readerLock = context.ctxLock.readLock();
+        readerLock.lock();
+        try {
             return SSLContext.sessionTicketKeyRenew(context.ctx);
+        } finally {
+            readerLock.unlock();
+        }
     }
 
     /**
      * Returns the number of times a client presented a ticket derived from the primary key.
      */
     public long ticketKeyResume() {
+        Lock readerLock = context.ctxLock.readLock();
+        readerLock.lock();
+        try {
             return SSLContext.sessionTicketKeyResume(context.ctx);
+        } finally {
+            readerLock.unlock();
+        }
     }
 }

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslClientContext.java

@@ -90,7 +90,6 @@ public final class ReferenceCountedOpenSslClientContext extends ReferenceCounted
             throw new IllegalArgumentException(
                     "Either both keyCertChain and key needs to be null or none of them");
         }
-        synchronized (ReferenceCountedOpenSslContext.class) {
         try {
             if (!OpenSsl.useKeyManagerFactory()) {
                 if (keyManagerFactory != null) {
@@ -149,7 +148,6 @@ public final class ReferenceCountedOpenSslClientContext extends ReferenceCounted
         } catch (Exception e) {
             throw new SSLException("unable to setup trustmanager", e);
         }
-        }
         return new OpenSslClientSessionContext(thiz);
     }
 

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslContext.java

@@ -46,6 +46,9 @@ import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 import javax.net.ssl.KeyManager;
 import javax.net.ssl.SSLEngine;
 import javax.net.ssl.SSLException;
@@ -106,11 +109,11 @@ public abstract class ReferenceCountedOpenSslContext extends SslContext implemen
     protected static final int VERIFY_DEPTH = 10;
 
     /**
-     * The OpenSSL SSL_CTX object
+     * The OpenSSL SSL_CTX object.
+     *
+     * <strong>{@link #ctxLock} must be hold while using ctx!</strong>
      */
-    protected volatile long ctx;
+    protected long ctx;
-    @SuppressWarnings({ "unused", "FieldMayBeFinal" })
-    private volatile int aprPoolDestroyed;
     private final List<String> unmodifiableCiphers;
     private final long sessionCacheSize;
     private final long sessionTimeout;
@@ -144,6 +147,8 @@ public abstract class ReferenceCountedOpenSslContext extends SslContext implemen
     final String[] protocols;
     final boolean enableOcsp;
     final OpenSslEngineMap engineMap = new DefaultOpenSslEngineMap();
+    final ReadWriteLock ctxLock = new ReentrantReadWriteLock();
+
     private volatile boolean rejectRemoteInitiatedRenegotiation;
     private volatile int bioNonApplicationBufferSize = DEFAULT_BIO_NON_APPLICATION_BUFFER_SIZE;
 
@@ -273,7 +278,6 @@ public abstract class ReferenceCountedOpenSslContext extends SslContext implemen
         // Create a new SSL_CTX and configure it.
         boolean success = false;
         try {
-            synchronized (ReferenceCountedOpenSslContext.class) {
             try {
                 ctx = SSLContext.make(SSL.SSL_PROTOCOL_ALL, mode);
             } catch (Exception e) {
@@ -289,9 +293,9 @@ public abstract class ReferenceCountedOpenSslContext extends SslContext implemen
                     SSL.SSL_OP_NO_COMPRESSION |
 
                     // Disable ticket support by default to be more inline with SSLEngineImpl of the JDK.
-                // This also let SSLSession.getId() work the same way for the JDK implementation and the OpenSSLEngine.
+                    // This also let SSLSession.getId() work the same way for the JDK implementation and the
-                // If tickets are supported SSLSession.getId() will only return an ID on the server-side if it could
+                    // OpenSSLEngine. If tickets are supported SSLSession.getId() will only return an ID on the
-                // make use of tickets.
+                    // server-side if it could make use of tickets.
                     SSL.SSL_OP_NO_TICKET);
 
             // We need to enable SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER as the memory address may change between
@@ -359,7 +363,6 @@ public abstract class ReferenceCountedOpenSslContext extends SslContext implemen
             if (enableOcsp) {
                 SSLContext.enableOcsp(ctx, isClient());
             }
-            }
             success = true;
         } finally {
             if (!success) {
@@ -428,11 +431,17 @@ public abstract class ReferenceCountedOpenSslContext extends SslContext implemen
      * Be aware that it is freed as soon as the {@link #finalize()}  method is called.
      * At this point {@code 0} will be returned.
      *
-     * @deprecated use {@link #sslCtxPointer()}
+     * @deprecated this method is considered unsafe as the returned pointer may be released later. Dont use it!
      */
     @Deprecated
     public final long context() {
+        Lock readerLock = ctxLock.readLock();
+        readerLock.lock();
+        try {
             return ctx;
+        } finally {
+            readerLock.unlock();
+        }
     }
 
     /**
@@ -493,16 +502,27 @@ public abstract class ReferenceCountedOpenSslContext extends SslContext implemen
      * Returns the pointer to the {@code SSL_CTX} object for this {@link ReferenceCountedOpenSslContext}.
      * Be aware that it is freed as soon as the {@link #release()} method is called.
      * At this point {@code 0} will be returned.
+     *
+     * @deprecated this method is considered unsafe as the returned pointer may be released later. Dont use it!
      */
+    @Deprecated
     public final long sslCtxPointer() {
+        Lock readerLock = ctxLock.readLock();
+        readerLock.lock();
+        try {
             return ctx;
+        } finally {
+            readerLock.unlock();
+        }
     }
 
     // IMPORTANT: This method must only be called from either the constructor or the finalizer as a user MUST never
     //            get access to an OpenSslSessionContext after this method was called to prevent the user from
     //            producing a segfault.
-    final void destroy() {
+    private void destroy() {
-        synchronized (ReferenceCountedOpenSslContext.class) {
+        Lock writerLock = ctxLock.writeLock();
+        writerLock.lock();
+        try {
             if (ctx != 0) {
                 if (enableOcsp) {
                     SSLContext.disableOcsp(ctx);
@@ -511,6 +531,8 @@ public abstract class ReferenceCountedOpenSslContext extends SslContext implemen
                 SSLContext.free(ctx);
                 ctx = 0;
             }
+        } finally {
+            writerLock.unlock();
         }
     }
 

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslEngine.java

@@ -43,6 +43,8 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;
+
+import java.util.concurrent.locks.Lock;
 import javax.net.ssl.SSLEngine;
 import javax.net.ssl.SSLEngineResult;
 import javax.net.ssl.SSLException;
@@ -234,7 +236,14 @@ public class ReferenceCountedOpenSslEngine extends SSLEngine implements Referenc
         localCerts = context.keyCertChain;
         keyMaterialManager = context.keyMaterialManager();
         enableOcsp = context.enableOcsp;
+
+        Lock readerLock = context.ctxLock.readLock();
+        readerLock.lock();
+        try {
             ssl = SSL.newSSL(context.ctx, !context.isClient());
+        } finally {
+            readerLock.unlock();
+        }
         try {
             networkBIO = SSL.bioNewByteBuffer(ssl, context.getBioNonApplicationBufferSize());
 

handler/src/main/java/io/netty/handler/ssl/ReferenceCountedOpenSslServerContext.java

@@ -106,7 +106,6 @@ public final class ReferenceCountedOpenSslServerContext extends ReferenceCounted
                                            String keyPassword, KeyManagerFactory keyManagerFactory)
             throws SSLException {
         ServerContext result = new ServerContext();
-        synchronized (ReferenceCountedOpenSslContext.class) {
         try {
             SSLContext.setVerify(ctx, SSL.SSL_CVERIFY_NONE, VERIFY_DEPTH);
             if (!OpenSsl.useKeyManagerFactory()) {
@@ -171,11 +170,6 @@ public final class ReferenceCountedOpenSslServerContext extends ReferenceCounted
                     freeBio(bio);
                 }
             }
-            } catch (SSLException e) {
-                throw e;
-            } catch (Exception e) {
-                throw new SSLException("unable to setup trustmanager", e);
-            }
 
             if (PlatformDependent.javaVersion() >= 8) {
                 // Only do on Java8+ as SNIMatcher is not supported in earlier releases.
@@ -184,6 +178,10 @@ public final class ReferenceCountedOpenSslServerContext extends ReferenceCounted
                 //            a global reference to the matcher.
                 SSLContext.setSniHostnameMatcher(ctx, new OpenSslSniHostnameMatcher(engineMap));
             }
+        } catch (SSLException e) {
+            throw e;
+        } catch (Exception e) {
+            throw new SSLException("unable to setup trustmanager", e);
         }
 
         result.sessionContext = new OpenSslServerSessionContext(thiz);