SslHandler: Ensure buffers are never leaked when wrap(...) produce SS… (#14647)

normanmaurer · web-flow · commit b8e25e043a32 · 2025-01-13T07:26:12.000+01:00
…LException Motivation: After we removed the ByteBuf from the pendingUnencryptedWrites queue we need to ensure we will always release ByteBuf even in the case of wrapMultiple(...) or wrap(...) throwing a SSLException. Modifications: - Ensure we release the ByteBuf when a SSLException is thrown before rethrowing it. Result: Fixes #14644
diff --git a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
@@ -852,27 +852,39 @@ private void wrap(ChannelHandlerContext ctx, boolean inUnwrap) throws SSLExcepti
 
                 SSLEngineResult result;
 
-                if (buf.readableBytes() > MAX_PLAINTEXT_LENGTH) {
-                    // If we pulled a buffer larger than the supported packet size, we can slice it up and iteratively,
-                    // encrypting multiple packets into a single larger buffer. This substantially saves on allocations
-                    // for large responses. Here we estimate how large of a buffer we need. If we overestimate a bit,
-                    // that's fine. If we underestimate, we'll simply re-enqueue the remaining buffer and get it on the
-                    // next outer loop.
-                    int readableBytes = buf.readableBytes();
-                    int numPackets = readableBytes / MAX_PLAINTEXT_LENGTH;
-                    if (readableBytes % MAX_PLAINTEXT_LENGTH != 0) {
-                        numPackets += 1;
-                    }
+                try {
+                    if (buf.readableBytes() > MAX_PLAINTEXT_LENGTH) {
+                        // If we pulled a buffer larger than the supported packet size, we can slice it up and
+                        // iteratively, encrypting multiple packets into a single larger buffer. This substantially
+                        // saves on allocations for large responses. Here we estimate how large of a buffer we need.
+                        // If we overestimate a bit, that's fine. If we underestimate, we'll simply re-enqueue the
+                        // remaining buffer and get it on the next outer loop.
+                        int readableBytes = buf.readableBytes();
+                        int numPackets = readableBytes / MAX_PLAINTEXT_LENGTH;
+                        if (readableBytes % MAX_PLAINTEXT_LENGTH != 0) {
+                            numPackets += 1;
+                        }
 
-                    if (out == null) {
-                        out = allocateOutNetBuf(ctx, readableBytes, buf.nioBufferCount() + numPackets);
-                    }
-                    result = wrapMultiple(alloc, engine, buf, out);
-                } else {
-                    if (out == null) {
-                        out = allocateOutNetBuf(ctx, buf.readableBytes(), buf.nioBufferCount());
+                        if (out == null) {
+                            out = allocateOutNetBuf(ctx, readableBytes, buf.nioBufferCount() + numPackets);
+                        }
+                        result = wrapMultiple(alloc, engine, buf, out);
+                    } else {
+                        if (out == null) {
+                            out = allocateOutNetBuf(ctx, buf.readableBytes(), buf.nioBufferCount());
+                        }
+                        result = wrap(alloc, engine, buf, out);
                     }
-                    result = wrap(alloc, engine, buf, out);
+                } catch (SSLException e) {
+                    // Either wrapMultiple(...) or wrap(...) did throw. In this case we need to release the buffer
+                    // that we removed from pendingUnencryptedWrites before failing the promise and rethrowing it.
+                    // Failing to do so would result in a buffer leak.
+                    // See https://github.com/netty/netty/issues/14644
+                    //
+                    // We don't need to release out here as this is done in a finally block already.
+                    buf.release();
+                    promise.setFailure(e);
+                    throw e;
                 }
 
                 if (buf.isReadable()) {
