Repository: netty/netty
Issue: Possible buffer leak in SslHandler?
Labels: defect

Excuse me if I'm missing something obvious, but it seems to me that in SslHandler.java, "buf" removed from "pendingUnencryptedWrites" on line 751 may leak if "wrap(alloc, engine, buf, out)" called on line 762 throws. The wrap function on line 912 is marked as "throws SSLException", which I assume is something that could possibly happen and not be a fatal error for the entire server. So it seems like the aforementioned buffer had ought to be released if such an exception occurs, perhaps in the finally block on line 803.

I only noticed this randomly while helping a coworker to track down an unrelated issue, so it's not like I've actually seen this leak happen.

Issue comments:
You are correct... let me fix it.
Cool, thanks
@Anon43590 please check https://github.com/netty/netty/pull/7338
@normanmaurer looks correct to me, thanks

Pull request review comments:
`transfered ` -> `transferred`

Patch that resolved the issue:
diff --git a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
index c3920e68c4d..444ba2f4b4e 100644
--- a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
+++ b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
@@ -742,13 +742,14 @@ private void wrap(ChannelHandlerContext ctx, boolean inUnwrap) throws SSLExcepti
         ChannelPromise promise = null;
         ByteBufAllocator alloc = ctx.alloc();
         boolean needUnwrap = false;
+        ByteBuf buf = null;
         try {
             final int wrapDataSize = this.wrapDataSize;
             // Only continue to loop if the handler was not removed in the meantime.
             // See https://github.com/netty/netty/issues/5860
             while (!ctx.isRemoved()) {
                 promise = ctx.newPromise();
-                ByteBuf buf = wrapDataSize > 0 ?
+                buf = wrapDataSize > 0 ?
                         pendingUnencryptedWrites.remove(alloc, wrapDataSize, promise) :
                         pendingUnencryptedWrites.removeFirst(promise);
                 if (buf == null) {
@@ -763,6 +764,7 @@ private void wrap(ChannelHandlerContext ctx, boolean inUnwrap) throws SSLExcepti
 
                 if (result.getStatus() == Status.CLOSED) {
                     buf.release();
+                    buf = null;
                     promise.tryFailure(SSLENGINE_CLOSED);
                     promise = null;
                     // SSLEngine has been closed already.
@@ -775,6 +777,7 @@ private void wrap(ChannelHandlerContext ctx, boolean inUnwrap) throws SSLExcepti
                     } else {
                         buf.release();
                     }
+                    buf = null;
 
                     switch (result.getHandshakeStatus()) {
                         case NEED_TASK:
@@ -801,6 +804,10 @@ private void wrap(ChannelHandlerContext ctx, boolean inUnwrap) throws SSLExcepti
                 }
             }
         } finally {
+            // Ownership of buffer was not transferred, release it.
+            if (buf != null) {
+                buf.release();
+            }
             finishWrap(ctx, out, promise, inUnwrap, needUnwrap);
         }
     }


Test patch: