From 65d1fb474d8456fd5dcc6e664c500698f1b566bb Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Mon, 13 Jun 2016 11:34:14 +0200 Subject: [PATCH] Guard against possible segfault when OpenSslContext is gc'ed and user still hold reference to OpenSslSessionContext / OpenSslSessionStats Motivation: When the OpenSslContext is gc'ed and the user still hold a reference to OpenSslSessionContext / OpenSslSessionStats it is possible to produce a segfault when calling a method on any of these that tries to pass down the ctx pointer to the native methods. This is because the OpenSslContext finalizer will free the native pointer. Modifications: Change OpenSslSessionContext / OpenSslSessionContext to store a reference to OpenSslContext and so prevent the GC to collect it as long as the user has a reference to OpenSslSessionContext / OpenSslSessionContext. Result: No more sefault possible. --- .../handler/ssl/OpenSslClientContext.java | 4 +-- .../io/netty/handler/ssl/OpenSslContext.java | 5 ++- .../handler/ssl/OpenSslServerContext.java | 2 +- .../ssl/OpenSslServerSessionContext.java | 16 +++++----- .../handler/ssl/OpenSslSessionContext.java | 12 ++++--- .../handler/ssl/OpenSslSessionStats.java | 32 +++++++++++-------- 6 files changed, 41 insertions(+), 30 deletions(-) diff --git a/handler/src/main/java/io/netty/handler/ssl/OpenSslClientContext.java b/handler/src/main/java/io/netty/handler/ssl/OpenSslClientContext.java index 7c1a0b2c6e..e4da8786a7 100644 --- a/handler/src/main/java/io/netty/handler/ssl/OpenSslClientContext.java +++ b/handler/src/main/java/io/netty/handler/ssl/OpenSslClientContext.java @@ -264,7 +264,7 @@ public final class OpenSslClientContext extends OpenSslContext { throw new SSLException("unable to setup trustmanager", e); } } - sessionContext = new OpenSslClientSessionContext(ctx); + sessionContext = new OpenSslClientSessionContext(this); success = true; } finally { if (!success) { @@ -280,7 +280,7 @@ public final class OpenSslClientContext extends OpenSslContext { // No cache is currently supported for client side mode. private static final class OpenSslClientSessionContext extends OpenSslSessionContext { - private OpenSslClientSessionContext(long context) { + private OpenSslClientSessionContext(OpenSslContext context) { super(context); } diff --git a/handler/src/main/java/io/netty/handler/ssl/OpenSslContext.java b/handler/src/main/java/io/netty/handler/ssl/OpenSslContext.java index 3758e4f9c5..dff05b67d7 100644 --- a/handler/src/main/java/io/netty/handler/ssl/OpenSslContext.java +++ b/handler/src/main/java/io/netty/handler/ssl/OpenSslContext.java @@ -365,7 +365,10 @@ public abstract class OpenSslContext extends SslContext { return ctx; } - protected final void destroy() { + // 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() { synchronized (OpenSslContext.class) { if (ctx != 0) { SSLContext.free(ctx); diff --git a/handler/src/main/java/io/netty/handler/ssl/OpenSslServerContext.java b/handler/src/main/java/io/netty/handler/ssl/OpenSslServerContext.java index 76231f53f5..1d45c77079 100644 --- a/handler/src/main/java/io/netty/handler/ssl/OpenSslServerContext.java +++ b/handler/src/main/java/io/netty/handler/ssl/OpenSslServerContext.java @@ -433,7 +433,7 @@ public final class OpenSslServerContext extends OpenSslContext { throw new SSLException("unable to setup trustmanager", e); } } - sessionContext = new OpenSslServerSessionContext(ctx); + sessionContext = new OpenSslServerSessionContext(this); sessionContext.setSessionIdContext(ID); success = true; } finally { diff --git a/handler/src/main/java/io/netty/handler/ssl/OpenSslServerSessionContext.java b/handler/src/main/java/io/netty/handler/ssl/OpenSslServerSessionContext.java index 693801ff8a..7634897d87 100644 --- a/handler/src/main/java/io/netty/handler/ssl/OpenSslServerSessionContext.java +++ b/handler/src/main/java/io/netty/handler/ssl/OpenSslServerSessionContext.java @@ -23,7 +23,7 @@ import org.apache.tomcat.jni.SSLContext; * {@link OpenSslSessionContext} implementation which offers extra methods which are only useful for the server-side. */ public final class OpenSslServerSessionContext extends OpenSslSessionContext { - OpenSslServerSessionContext(long context) { + OpenSslServerSessionContext(OpenSslContext context) { super(context); } @@ -32,12 +32,12 @@ public final class OpenSslServerSessionContext extends OpenSslSessionContext { if (seconds < 0) { throw new IllegalArgumentException(); } - SSLContext.setSessionCacheTimeout(context, seconds); + SSLContext.setSessionCacheTimeout(context.ctx, seconds); } @Override public int getSessionTimeout() { - return (int) SSLContext.getSessionCacheTimeout(context); + return (int) SSLContext.getSessionCacheTimeout(context.ctx); } @Override @@ -45,23 +45,23 @@ public final class OpenSslServerSessionContext extends OpenSslSessionContext { if (size < 0) { throw new IllegalArgumentException(); } - SSLContext.setSessionCacheSize(context, size); + SSLContext.setSessionCacheSize(context.ctx, size); } @Override public int getSessionCacheSize() { - return (int) SSLContext.getSessionCacheSize(context); + return (int) SSLContext.getSessionCacheSize(context.ctx); } @Override public void setSessionCacheEnabled(boolean enabled) { long mode = enabled ? SSL.SSL_SESS_CACHE_SERVER : SSL.SSL_SESS_CACHE_OFF; - SSLContext.setSessionCacheMode(context, mode); + SSLContext.setSessionCacheMode(context.ctx, mode); } @Override public boolean isSessionCacheEnabled() { - return SSLContext.getSessionCacheMode(context) == SSL.SSL_SESS_CACHE_SERVER; + return SSLContext.getSessionCacheMode(context.ctx) == SSL.SSL_SESS_CACHE_SERVER; } /** @@ -74,6 +74,6 @@ public final class OpenSslServerSessionContext extends OpenSslSessionContext { * @return {@code true} if success, {@code false} otherwise. */ public boolean setSessionIdContext(byte[] sidCtx) { - return SSLContext.setSessionIdContext(context, sidCtx); + return SSLContext.setSessionIdContext(context.ctx, sidCtx); } } diff --git a/handler/src/main/java/io/netty/handler/ssl/OpenSslSessionContext.java b/handler/src/main/java/io/netty/handler/ssl/OpenSslSessionContext.java index 5b0f2a4299..45b646a701 100644 --- a/handler/src/main/java/io/netty/handler/ssl/OpenSslSessionContext.java +++ b/handler/src/main/java/io/netty/handler/ssl/OpenSslSessionContext.java @@ -30,9 +30,13 @@ public abstract class OpenSslSessionContext implements SSLSessionContext { private static final Enumeration EMPTY = new EmptyEnumeration(); private final OpenSslSessionStats stats; - final long context; + final OpenSslContext context; - OpenSslSessionContext(long context) { + // IMPORTANT: We take the OpenSslContext and not just the long (which points the native instance) to prevent + // the GC to collect OpenSslContext as this would also free the pointer and so could result in a + // segfault when the user calls any of the methods here that try to pass the pointer down to the native + // level. + OpenSslSessionContext(OpenSslContext context) { this.context = context; stats = new OpenSslSessionStats(context); } @@ -59,7 +63,7 @@ public abstract class OpenSslSessionContext implements SSLSessionContext { if (keys == null) { throw new NullPointerException("keys"); } - SSLContext.setSessionTicketKeys(context, keys); + SSLContext.setSessionTicketKeys(context.ctx, keys); } /** @@ -73,7 +77,7 @@ public abstract class OpenSslSessionContext implements SSLSessionContext { for (int i = 0; i < ticketKeys.length; i++) { ticketKeys[i] = keys[i].key; } - SSLContext.setSessionTicketKeys(context, ticketKeys); + SSLContext.setSessionTicketKeys(context.ctx, ticketKeys); } /** diff --git a/handler/src/main/java/io/netty/handler/ssl/OpenSslSessionStats.java b/handler/src/main/java/io/netty/handler/ssl/OpenSslSessionStats.java index 2ec514681d..7cc39758dd 100644 --- a/handler/src/main/java/io/netty/handler/ssl/OpenSslSessionStats.java +++ b/handler/src/main/java/io/netty/handler/ssl/OpenSslSessionStats.java @@ -25,9 +25,13 @@ import org.apache.tomcat.jni.SSLContext; */ public final class OpenSslSessionStats { - private final long context; + private final OpenSslContext context; - OpenSslSessionStats(long context) { + // IMPORTANT: We take the OpenSslContext and not just the long (which points the native instance) to prevent + // the GC to collect OpenSslContext as this would also free the pointer and so could result in a + // segfault when the user calls any of the methods here that try to pass the pointer down to the native + // level. + OpenSslSessionStats(OpenSslContext context) { this.context = context; } @@ -35,49 +39,49 @@ public final class OpenSslSessionStats { * Returns the current number of sessions in the internal session cache. */ public long number() { - return SSLContext.sessionNumber(context); + return SSLContext.sessionNumber(context.ctx); } /** * Returns the number of started SSL/TLS handshakes in client mode. */ public long connect() { - return SSLContext.sessionConnect(context); + return SSLContext.sessionConnect(context.ctx); } /** * Returns the number of successfully established SSL/TLS sessions in client mode. */ public long connectGood() { - return SSLContext.sessionConnectGood(context); + return SSLContext.sessionConnectGood(context.ctx); } /** * Returns the number of start renegotiations in client mode. */ public long connectRenegotiate() { - return SSLContext.sessionConnectRenegotiate(context); + return SSLContext.sessionConnectRenegotiate(context.ctx); } /** * Returns the number of started SSL/TLS handshakes in server mode. */ public long accept() { - return SSLContext.sessionAccept(context); + return SSLContext.sessionAccept(context.ctx); } /** * Returns the number of successfully established SSL/TLS sessions in server mode. */ public long acceptGood() { - return SSLContext.sessionAcceptGood(context); + return SSLContext.sessionAcceptGood(context.ctx); } /** * Returns the number of start renegotiations in server mode. */ public long acceptRenegotiate() { - return SSLContext.sessionAcceptRenegotiate(context); + return SSLContext.sessionAcceptRenegotiate(context.ctx); } /** @@ -86,14 +90,14 @@ public final class OpenSslSessionStats { * external cache is counted as a hit. */ public long hits() { - return SSLContext.sessionHits(context); + return SSLContext.sessionHits(context.ctx); } /** * Returns the number of successfully retrieved sessions from the external session cache in server mode. */ public long cbHits() { - return SSLContext.sessionCbHits(context); + return SSLContext.sessionCbHits(context.ctx); } /** @@ -101,7 +105,7 @@ public final class OpenSslSessionStats { * in server mode. */ public long misses() { - return SSLContext.sessionMisses(context); + return SSLContext.sessionMisses(context.ctx); } /** @@ -110,13 +114,13 @@ public final class OpenSslSessionStats { * count. */ public long timeouts() { - return SSLContext.sessionTimeouts(context); + return SSLContext.sessionTimeouts(context.ctx); } /** * Returns the number of sessions that were removed because the maximum session cache size was exceeded. */ public long cacheFull() { - return SSLContext.sessionCacheFull(context); + return SSLContext.sessionCacheFull(context.ctx); } }