Repository: netty/netty
Issue: SslHandler - DirectByteBuffer - OutOfMemory
Labels: defect

I tested the performance of my project with and without SSL (configuration: 25 threads that run 10 more threads (total 250) each of them refers to the server for download the file). 
Test without Ssl completed without error, but the test with Ssl threw </code>OutOfMemoryError</code> when SslHandler trying to create the new DirectByteBuffer in his methods <code>wrap()</code> (<code>Bits.reserveMemory</code>).

System Monitor shows memory usage in 10 times higher than the same test without using Ssl.
Perhaps you need to add a method call <code>Util.free(ByteBuffer)</code> for sended <code>DirectByteBuffer</code> and thus exempt are reserved memory? Because I haven't found such place in which someone performs this action and free direct memory.

PS (In my test I send and receive over 5GB of data for test)


Issue comments:
Hi @Lekanich and thanks for reaching out.  We will need a few more details to narrow in on the issue:
- What version of netty are you using?
- What is your system configuration (`mvn --version`)?
- Are you using openssl or JDK providers for SSL?
- Where is the location in the code that the buffer is not being released?
- Have you tried to release this buffer and see if the behavior improves?

1) netty 4.0.23 
2) Maven home: /usr/lib/apache-maven/apache-maven-3.2.1
Java version: 1.8.0, vendor: Oracle Corporation
Java home: /usr/java/jdk1.8.0/jre
Default locale: en_US, platform encoding: UTF-8
OS name: "linux", version: "3.11.0-26-generic", arch: "amd64", family: "unix"
3) JDK provider
4) line: 596 (method SslHandler.wrap(SslEngine, ByteBuf, ByteBuf)
in line 599 ByteBuffer.allocateDirect(in0.remaining())
and then it never dealloc
5) It's Internall netty SslHandler call, so haven't try to release it

PS I think it passed to SslEngine and then someone forget to release it (line 606)

@Lekanich - Would you mind verifying your assumptions?  If you could make the code change you suggested and test that your issues are resolved that would be very helpful.  Also you could then submit a PR and get credit for your contributions ;)

@Scottmitch I try...

@Lekanich - I think your line numbers may be a bit off?  Is [line 580](https://github.com/netty/netty/blob/netty-4.0.23.Final/handler/src/main/java/io/netty/handler/ssl/SslHandler.java#L580) where the allocation in question is done?  If so can you describe why you feel this is an issue, and where/how you would suggest releasing the memory?

@Scottmitch  LIne is correct, because that DirectBuffer never clean?
I think rewrite as
From [line 587 ](https://github.com/netty/netty/blob/netty-4.0.23.Final/handler/src/main/java/io/netty/handler/ssl/SslHandler.java#L585)

``` java
        for (;;) {
            ByteBuffer out0 = out.nioBuffer(out.writerIndex(), out.writableBytes());
            SSLEngineResult result = engine.wrap(in0, out0);
            cleanDirectBuffer(in0);
            cleanDirectBuffer(out0);

```

Where <code>cleanDirectBuffer</code>

``` java
    private void cleanDirectBuffer(ByteBuffer buffer) {
        if (!buffer.isDirect()) {
            return;
        }
        Cleaner cleaner = ((DirectBuffer) buffer).cleaner();
        if (cleaner == null) {
            return;
        }

        cleaner.clean();
    }
```

PS Maybe need clean only <code>in0</code>

@Lekanich - I believe the allocations in the loop will be cleaned up by (and is the responsibility of) the `ByteBuf` after it has been released (for the internal buffers).  In your situation is the memory ever released (despite the OOM exception)?  Is the GC ever running when you observe the issue?   Is it possible you are holding on to references to `ByteBuf` parameters or not calling `release()` on them?

I'm investigating a potential for a leak on line 580 but I don't think the internal buffers to `ByteBuf` should be released here.

@Scottmitch I think <code>ByteBuffer</code> <code>in0</code> is in no way connected with the sending <code>ByteBuf</code>, so when will be called <code>release()</code> it's not release <code>ByteBuffer</code> <code>in0</code>.I think it release only  <code>ByteBuffer out0</code>

@Lekanich - Can you confirm that you are not using a direct buffer in this method (i.e. `!in0.isDirect()` returns true).  Also can you see if PR https://github.com/netty/netty/pull/3058 changes anything for you?

@Lekanich - When you say:

> I think ByteBuffer in0 is in no way connected with the sending ByteBuf

Do you still think this is the case if `in.nioBuffer().isDirect()` is `true`?

@Scottmitch During debug I was into this block

@Lekanich - Great news...not that there is a bug but that #3058 may fix your issue.  Please try this PR and let me know if this is the case.

@Scottmitch Yes it seems to help.

@Lekanich Thank you for testing this out and reporting this issue!

Just to clarify when you say "it seems to help" can you be a bit more specific? In the issue description you provide details as to how much more memory is used while SSL is enabled.
- Can you provide these metrics while using #3058?  I'm more just looking to see that it is not 10x more and that allocation/releasing pattern follows a similar pattern in the ssl/plaintext cases.
- Are you no longer seeing the OutOfMemory exception?
- Are there other areas you suspect to be leaky?

Fixed by 8db8aca1e7bac8a927bc82968291abbb5ade96be

@Scottmitch Perf Test with fix uses 3GB of memory, and without over 13GB (memory + swap)

@Lekanich - Sounds much better.  Thanks again for reporting!

@Scottmitch Thanks for the quick reply)


Pull request review comments:
use `newDirectIn.internalNioBuffer(0, newDirectIn.readableBytes()` . This way you also not need to reset the reader index.

Good call. Done.


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 40ef0ce5637..9ea071d5292 100644
--- a/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
+++ b/handler/src/main/java/io/netty/handler/ssl/SslHandler.java
@@ -436,6 +436,7 @@ public void flush(ChannelHandlerContext ctx) throws Exception {
     private void wrap(ChannelHandlerContext ctx, boolean inUnwrap) throws SSLException {
         ByteBuf out = null;
         ChannelPromise promise = null;
+        ByteBufAllocator alloc = ctx.alloc();
         try {
             for (;;) {
                 Object msg = pendingUnencryptedWrites.current();
@@ -453,7 +454,7 @@ private void wrap(ChannelHandlerContext ctx, boolean inUnwrap) throws SSLExcepti
                     out = allocateOutNetBuf(ctx, buf.readableBytes());
                 }
 
-                SSLEngineResult result = wrap(engine, buf, out);
+                SSLEngineResult result = wrap(alloc, engine, buf, out);
 
                 if (!buf.isReadable()) {
                     promise = pendingUnencryptedWrites.remove();
@@ -519,12 +520,13 @@ private void finishWrap(ChannelHandlerContext ctx, ByteBuf out, ChannelPromise p
 
     private void wrapNonAppData(ChannelHandlerContext ctx, boolean inUnwrap) throws SSLException {
         ByteBuf out = null;
+        ByteBufAllocator alloc = ctx.alloc();
         try {
             for (;;) {
                 if (out == null) {
                     out = allocateOutNetBuf(ctx, 0);
                 }
-                SSLEngineResult result = wrap(engine, Unpooled.EMPTY_BUFFER, out);
+                SSLEngineResult result = wrap(alloc, engine, Unpooled.EMPTY_BUFFER, out);
 
                 if (result.bytesProduced() > 0) {
                     ctx.write(out);
@@ -574,26 +576,37 @@ private void wrapNonAppData(ChannelHandlerContext ctx, boolean inUnwrap) throws
         }
     }
 
-    private SSLEngineResult wrap(SSLEngine engine, ByteBuf in, ByteBuf out) throws SSLException {
-        ByteBuffer in0 = in.nioBuffer();
-        if (!in0.isDirect()) {
-            ByteBuffer newIn0 = ByteBuffer.allocateDirect(in0.remaining());
-            newIn0.put(in0).flip();
-            in0 = newIn0;
-        }
+    private SSLEngineResult wrap(ByteBufAllocator alloc, SSLEngine engine, ByteBuf in, ByteBuf out)
+            throws SSLException {
+        ByteBuf newDirectIn = null;
+        try {
+            final ByteBuffer in0;
+            if (in.isDirect()) {
+                in0 = in.nioBuffer();
+            } else {
+                int readableBytes = in.readableBytes();
+                newDirectIn = alloc.directBuffer(readableBytes);
+                newDirectIn.writeBytes(in, in.readerIndex(), readableBytes);
+                in0 = newDirectIn.internalNioBuffer(0, readableBytes);
+            }
 
-        for (;;) {
-            ByteBuffer out0 = out.nioBuffer(out.writerIndex(), out.writableBytes());
-            SSLEngineResult result = engine.wrap(in0, out0);
-            in.skipBytes(result.bytesConsumed());
-            out.writerIndex(out.writerIndex() + result.bytesProduced());
+            for (;;) {
+                ByteBuffer out0 = out.nioBuffer(out.writerIndex(), out.writableBytes());
+                SSLEngineResult result = engine.wrap(in0, out0);
+                in.skipBytes(result.bytesConsumed());
+                out.writerIndex(out.writerIndex() + result.bytesProduced());
 
-            switch (result.getStatus()) {
+                switch (result.getStatus()) {
                 case BUFFER_OVERFLOW:
                     out.ensureWritable(maxPacketBufferSize);
                     break;
                 default:
                     return result;
+                }
+            }
+        } finally {
+            if (newDirectIn != null) {
+                newDirectIn.release();
             }
         }
     }


Test patch: