Repository: netty/netty
Issue: HAProxyMessage memory leak due unreleased TLVs 

`HAProxyMessage` isn't a `ReferencedCounted` object. 
A component consuming an `HAProxyMessage` object and not passing it further might not release it, while it should be released as it contains a list of TLVs created in the static function `readNextTLV`:

https://github.com/netty/netty/blob/00afb19d7a37de21b35ce4f6cb3fa7f74809f2ab/codec-haproxy/src/main/java/io/netty/handler/codec/haproxy/HAProxyMessage.java#L235-L241

Notice that this function retains a slice of the original header ByteBuf increasing its refCnt and causing a possible memory leak if one forgets to release them explicitly:

https://github.com/netty/netty/blob/00afb19d7a37de21b35ce4f6cb3fa7f74809f2ab/codec-haproxy/src/main/java/io/netty/handler/codec/haproxy/HAProxyMessage.java#L296

I would expect `HAProxyMessage` to implement `ReferencedCounted` and release the TLVs in a similar fashion to: `haProxyMessage.tlvs().forEach(tlv -> tlv.release());` 

We have found this issue only after we had leaks.

Furthermore, it seems that slices are purposely retained as seen in Netty tests, am I missing something?

https://github.com/netty/netty/blob/00afb19d7a37de21b35ce4f6cb3fa7f74809f2ab/codec-haproxy/src/test/java/io/netty/handler/codec/haproxy/HAProxyMessageDecoderTest.java#L642-L650

### Netty version
4.1.36

Issue comments:
@shevah yes this sounds like a bug... Would you be willing to create a PR to fix it ?
Yes

Pull request review comments:
imho we should call `retain` as last method to ensure we can not leak 
`toString(...)` will never return null so this does not look correct. Maybe it should check if the buffer is empty ?
why not an `ArrayList` to reduce the GC ?
why not an `ArrayList` to reduce the GC ?
why not an `ArrayList` to reduce the GC ?
Should return `HAProxyMessage `.
Should return `HAProxyMessage `.
Done :-)
Done
Done
Done
Done
Done
Done
imho we should not extend `DefaultByteBufHolder` but `AbstractReferenceCounted`. There is no need to hold a reference to the "original" `ByteBuf` here.
I like this idea, I think if `HAProxyMessage` inherits `AbstractReferenceCounted`, then maybe we can make the implementation a bit simpler. Just overwrite the `AbstractReferenceCounted.deallocate()` method and release the TLV list in this method, like this:
```java
class HAProxyMessage extends AbstractReferenceCounted {
    @Override
    public HAProxyMessage retain() {
        return (HAProxyMessage) super.retain();
    }
    // overwrite other methods...

    @Override
    protected void deallocate() {
       for (HAProxyTLV tlv : this.tlvs){
           tlv.release();
       }
    }
}
```
In this case, the TLV list will be released together when `HAProxyMessage` is released, but when `HAProxyMessage refCnt` is increased, the TLV `refCnt` will not increase with `HAProxyMessage`.

@normanmaurer What do you think of this? Will it cause other additional problems?

By the way, I found that this is also the case in DNSMessage:
https://github.com/netty/netty/blob/00afb19d7a37de21b35ce4f6cb3fa7f74809f2ab/codec-dns/src/main/java/io/netty/handler/codec/dns/AbstractDnsMessage.java#L378-L388
@qeesung good question...Let me think about it but in general I would say it is fine.
@normanmaurer It's done in this way. Could you please review it again :-)?
I think you also want to call `leak.record`  in the other `touch`, `retain` and `release` methods.
put all of this in a finally block, just in case.
Done
Done
you can move this to a method and re-use....


```
private void tryRecord() {
    if (leak != null) {
        leak.record();
    }
}
```
Done!
for this you need to duplicate the code as you should pass down the `hint`.
I am sorry for my carelessness. Done!
nit... you can move this out of this block and so remove the same code above.
Done

Patch that resolved the issue:
diff --git a/codec-haproxy/src/main/java/io/netty/handler/codec/haproxy/HAProxyMessage.java b/codec-haproxy/src/main/java/io/netty/handler/codec/haproxy/HAProxyMessage.java
index b40bf42df26..bc623855915 100644
--- a/codec-haproxy/src/main/java/io/netty/handler/codec/haproxy/HAProxyMessage.java
+++ b/codec-haproxy/src/main/java/io/netty/handler/codec/haproxy/HAProxyMessage.java
@@ -17,9 +17,13 @@
 
 import io.netty.buffer.ByteBuf;
 import io.netty.handler.codec.haproxy.HAProxyProxiedProtocol.AddressFamily;
+import io.netty.util.AbstractReferenceCounted;
 import io.netty.util.ByteProcessor;
 import io.netty.util.CharsetUtil;
 import io.netty.util.NetUtil;
+import io.netty.util.ResourceLeakDetector;
+import io.netty.util.ResourceLeakDetectorFactory;
+import io.netty.util.ResourceLeakTracker;
 
 import java.util.ArrayList;
 import java.util.Collections;
@@ -28,29 +32,11 @@
 /**
  * Message container for decoded HAProxy proxy protocol parameters
  */
-public final class HAProxyMessage {
-
-    /**
-     * Version 1 proxy protocol message for 'UNKNOWN' proxied protocols. Per spec, when the proxied protocol is
-     * 'UNKNOWN' we must discard all other header values.
-     */
-    private static final HAProxyMessage V1_UNKNOWN_MSG = new HAProxyMessage(
-            HAProxyProtocolVersion.V1, HAProxyCommand.PROXY, HAProxyProxiedProtocol.UNKNOWN, null, null, 0, 0);
-
-    /**
-     * Version 2 proxy protocol message for 'UNKNOWN' proxied protocols. Per spec, when the proxied protocol is
-     * 'UNKNOWN' we must discard all other header values.
-     */
-    private static final HAProxyMessage V2_UNKNOWN_MSG = new HAProxyMessage(
-            HAProxyProtocolVersion.V2, HAProxyCommand.PROXY, HAProxyProxiedProtocol.UNKNOWN, null, null, 0, 0);
-
-    /**
-     * Version 2 proxy protocol message for local requests. Per spec, we should use an unspecified protocol and family
-     * for 'LOCAL' commands. Per spec, when the proxied protocol is 'UNKNOWN' we must discard all other header values.
-     */
-    private static final HAProxyMessage V2_LOCAL_MSG = new HAProxyMessage(
-            HAProxyProtocolVersion.V2, HAProxyCommand.LOCAL, HAProxyProxiedProtocol.UNKNOWN, null, null, 0, 0);
+public final class HAProxyMessage extends AbstractReferenceCounted {
+    private static final ResourceLeakDetector<HAProxyMessage> leakDetector =
+            ResourceLeakDetectorFactory.instance().newResourceLeakDetector(HAProxyMessage.class);
 
+    private final ResourceLeakTracker<HAProxyMessage> leak;
     private final HAProxyProtocolVersion protocolVersion;
     private final HAProxyCommand command;
     private final HAProxyProxiedProtocol proxiedProtocol;
@@ -108,6 +94,8 @@ private HAProxyMessage(
         this.sourcePort = sourcePort;
         this.destinationPort = destinationPort;
         this.tlvs = Collections.unmodifiableList(tlvs);
+
+        leak = leakDetector.track(this);
     }
 
     /**
@@ -150,7 +138,7 @@ static HAProxyMessage decodeHeader(ByteBuf header) {
         }
 
         if (cmd == HAProxyCommand.LOCAL) {
-            return V2_LOCAL_MSG;
+            return unknownMsg(HAProxyProtocolVersion.V2, HAProxyCommand.LOCAL);
         }
 
         // Per spec, the 14th byte is the protocol and address family byte
@@ -162,7 +150,7 @@ static HAProxyMessage decodeHeader(ByteBuf header) {
         }
 
         if (protAndFam == HAProxyProxiedProtocol.UNKNOWN) {
-            return V2_UNKNOWN_MSG;
+            return unknownMsg(HAProxyProtocolVersion.V2, HAProxyCommand.PROXY);
         }
 
         int addressInfoLen = header.readUnsignedShort();
@@ -337,7 +325,7 @@ static HAProxyMessage decodeHeader(String header) {
         }
 
         if (protAndFam == HAProxyProxiedProtocol.UNKNOWN) {
-            return V1_UNKNOWN_MSG;
+            return unknownMsg(HAProxyProtocolVersion.V1, HAProxyCommand.PROXY);
         }
 
         if (numParts != 6) {
@@ -349,6 +337,14 @@ static HAProxyMessage decodeHeader(String header) {
                 protAndFam, parts[2], parts[3], parts[4], parts[5]);
     }
 
+    /**
+     * Proxy protocol message for 'UNKNOWN' proxied protocols. Per spec, when the proxied protocol is
+     * 'UNKNOWN' we must discard all other header values.
+     */
+    private static HAProxyMessage unknownMsg(HAProxyProtocolVersion version, HAProxyCommand command) {
+        return new HAProxyMessage(version, command, HAProxyProxiedProtocol.UNKNOWN, null, null, 0, 0);
+    }
+
     /**
      * Convert ip address bytes to string representation
      *
@@ -358,31 +354,20 @@ static HAProxyMessage decodeHeader(String header) {
      */
     private static String ipBytesToString(ByteBuf header, int addressLen) {
         StringBuilder sb = new StringBuilder();
-        if (addressLen == 4) {
-            sb.append(header.readByte() & 0xff);
-            sb.append('.');
-            sb.append(header.readByte() & 0xff);
-            sb.append('.');
-            sb.append(header.readByte() & 0xff);
-            sb.append('.');
-            sb.append(header.readByte() & 0xff);
+        final int ipv4Len = 4;
+        final int ipv6Len = 8;
+        if (addressLen == ipv4Len) {
+            for (int i = 0; i < ipv4Len; i++) {
+                sb.append(header.readByte() & 0xff);
+                sb.append('.');
+            }
         } else {
-            sb.append(Integer.toHexString(header.readUnsignedShort()));
-            sb.append(':');
-            sb.append(Integer.toHexString(header.readUnsignedShort()));
-            sb.append(':');
-            sb.append(Integer.toHexString(header.readUnsignedShort()));
-            sb.append(':');
-            sb.append(Integer.toHexString(header.readUnsignedShort()));
-            sb.append(':');
-            sb.append(Integer.toHexString(header.readUnsignedShort()));
-            sb.append(':');
-            sb.append(Integer.toHexString(header.readUnsignedShort()));
-            sb.append(':');
-            sb.append(Integer.toHexString(header.readUnsignedShort()));
-            sb.append(':');
-            sb.append(Integer.toHexString(header.readUnsignedShort()));
+            for (int i = 0; i < ipv6Len; i++) {
+                sb.append(Integer.toHexString(header.readUnsignedShort()));
+                sb.append(':');
+            }
         }
+        sb.setLength(sb.length() - 1);
         return sb.toString();
     }
 
@@ -519,4 +504,63 @@ public int destinationPort() {
     public List<HAProxyTLV> tlvs() {
         return tlvs;
     }
+
+    @Override
+    public HAProxyMessage touch() {
+        tryRecord();
+        return (HAProxyMessage) super.touch();
+    }
+
+    @Override
+    public HAProxyMessage touch(Object hint) {
+        if (leak != null) {
+            leak.record(hint);
+        }
+        return this;
+    }
+
+    @Override
+    public HAProxyMessage retain() {
+        tryRecord();
+        return (HAProxyMessage) super.retain();
+    }
+
+    @Override
+    public HAProxyMessage retain(int increment) {
+        tryRecord();
+        return (HAProxyMessage) super.retain(increment);
+    }
+
+    @Override
+    public boolean release() {
+        tryRecord();
+        return super.release();
+    }
+
+    @Override
+    public boolean release(int decrement) {
+        tryRecord();
+        return super.release(decrement);
+    }
+
+    private void tryRecord() {
+        if (leak != null) {
+            leak.record();
+        }
+    }
+
+    @Override
+    protected void deallocate() {
+        try {
+            for (HAProxyTLV tlv : tlvs) {
+                tlv.release();
+            }
+        } finally {
+            final ResourceLeakTracker<HAProxyMessage> leak = this.leak;
+            if (leak != null) {
+                boolean closed = leak.close(this);
+                assert closed;
+            }
+        }
+    }
 }


Test patch:
diff --git a/codec-haproxy/src/test/java/io/netty/handler/codec/haproxy/HAProxyMessageDecoderTest.java b/codec-haproxy/src/test/java/io/netty/handler/codec/haproxy/HAProxyMessageDecoderTest.java
index 2d4039de3d0..02ff285565c 100644
--- a/codec-haproxy/src/test/java/io/netty/handler/codec/haproxy/HAProxyMessageDecoderTest.java
+++ b/codec-haproxy/src/test/java/io/netty/handler/codec/haproxy/HAProxyMessageDecoderTest.java
@@ -58,6 +58,7 @@ public void testIPV4Decode() {
         assertEquals(443, msg.destinationPort());
         assertNull(ch.readInbound());
         assertFalse(ch.finish());
+        assertTrue(msg.release());
     }
 
     @Test
@@ -78,6 +79,7 @@ public void testIPV6Decode() {
         assertEquals(443, msg.destinationPort());
         assertNull(ch.readInbound());
         assertFalse(ch.finish());
+        assertTrue(msg.release());
     }
 
     @Test
@@ -98,6 +100,7 @@ public void testUnknownProtocolDecode() {
         assertEquals(0, msg.destinationPort());
         assertNull(ch.readInbound());
         assertFalse(ch.finish());
+        assertTrue(msg.release());
     }
 
     @Test(expected = HAProxyProtocolException.class)
@@ -264,6 +267,7 @@ public void testV2IPV4Decode() {
         assertEquals(443, msg.destinationPort());
         assertNull(ch.readInbound());
         assertFalse(ch.finish());
+        assertTrue(msg.release());
     }
 
     @Test
@@ -319,6 +323,7 @@ public void testV2UDPDecode() {
         assertEquals(443, msg.destinationPort());
         assertNull(ch.readInbound());
         assertFalse(ch.finish());
+        assertTrue(msg.release());
     }
 
     @Test
@@ -398,6 +403,7 @@ public void testv2IPV6Decode() {
         assertEquals(443, msg.destinationPort());
         assertNull(ch.readInbound());
         assertFalse(ch.finish());
+        assertTrue(msg.release());
     }
 
     @Test
@@ -476,6 +482,7 @@ public void testv2UnixDecode() {
         assertEquals(0, msg.destinationPort());
         assertNull(ch.readInbound());
         assertFalse(ch.finish());
+        assertTrue(msg.release());
     }
 
     @Test
@@ -531,6 +538,7 @@ public void testV2LocalProtocolDecode() {
         assertEquals(0, msg.destinationPort());
         assertNull(ch.readInbound());
         assertFalse(ch.finish());
+        assertTrue(msg.release());
     }
 
     @Test
@@ -586,6 +594,7 @@ public void testV2UnknownProtocolDecode() {
         assertEquals(0, msg.destinationPort());
         assertNull(ch.readInbound());
         assertFalse(ch.finish());
+        assertTrue(msg.release());
     }
 
     @Test
@@ -642,9 +651,7 @@ public void testV2WithSslTLVs() throws Exception {
         assertTrue(0 < firstTlv.refCnt());
         assertTrue(0 < secondTlv.refCnt());
         assertTrue(0 < thirdTLV.refCnt());
-        assertFalse(thirdTLV.release());
-        assertFalse(secondTlv.release());
-        assertTrue(firstTlv.release());
+        assertTrue(msg.release());
         assertEquals(0, firstTlv.refCnt());
         assertEquals(0, secondTlv.refCnt());
         assertEquals(0, thirdTLV.refCnt());
@@ -653,6 +660,51 @@ public void testV2WithSslTLVs() throws Exception {
         assertFalse(ch.finish());
     }
 
+    @Test
+    public void testReleaseHAProxyMessage() {
+        ch = new EmbeddedChannel(new HAProxyMessageDecoder());
+
+        final byte[] bytes = {
+                13, 10, 13, 10, 0, 13, 10, 81, 85, 73, 84, 10, 33, 17, 0, 35, 127, 0, 0, 1, 127, 0, 0, 1,
+                -55, -90, 7, 89, 32, 0, 20, 5, 0, 0, 0, 0, 33, 0, 5, 84, 76, 83, 118, 49, 34, 0, 4, 76, 69, 65, 70
+        };
+
+        int startChannels = ch.pipeline().names().size();
+        assertTrue(ch.writeInbound(copiedBuffer(bytes)));
+        Object msgObj = ch.readInbound();
+        assertEquals(startChannels - 1, ch.pipeline().names().size());
+        HAProxyMessage msg = (HAProxyMessage) msgObj;
+
+        final List<HAProxyTLV> tlvs = msg.tlvs();
+        assertEquals(3, tlvs.size());
+
+        assertEquals(1, msg.refCnt());
+        for (HAProxyTLV tlv : tlvs) {
+            assertEquals(3, tlv.refCnt());
+        }
+
+        // Retain the haproxy message
+        msg.retain();
+        assertEquals(2, msg.refCnt());
+        for (HAProxyTLV tlv : tlvs) {
+            assertEquals(3, tlv.refCnt());
+        }
+
+        // Decrease the haproxy message refCnt
+        msg.release();
+        assertEquals(1, msg.refCnt());
+        for (HAProxyTLV tlv : tlvs) {
+            assertEquals(3, tlv.refCnt());
+        }
+
+        // Release haproxy message, TLVs will be released with it
+        msg.release();
+        assertEquals(0, msg.refCnt());
+        for (HAProxyTLV tlv : tlvs) {
+            assertEquals(0, tlv.refCnt());
+        }
+    }
+
     @Test
     public void testV2WithTLV() {
         ch = new EmbeddedChannel(new HAProxyMessageDecoder(4));
@@ -738,6 +790,7 @@ public void testV2WithTLV() {
         assertEquals(0, msg.destinationPort());
         assertNull(ch.readInbound());
         assertFalse(ch.finish());
+        assertTrue(msg.release());
     }
 
     @Test(expected = HAProxyProtocolException.class)