Skip to content

Commit

Permalink
Add locking around AbstractSessionContext JNI.
Browse files Browse the repository at this point in the history
We don't have a definitive root cause for google#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.
  • Loading branch information
prbprbprb committed Aug 4, 2023
1 parent 92a8a67 commit 89c47db
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 5 deletions.
46 changes: 44 additions & 2 deletions common/src/main/java/org/conscrypt/AbstractSessionContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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>() {
Expand Down Expand Up @@ -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();
}
Expand Down
3 changes: 1 addition & 2 deletions common/src/main/java/org/conscrypt/NativeSsl.java
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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[] { ' ' });
}

/**
Expand Down

0 comments on commit 89c47db

Please sign in to comment.