Repository: netty/netty
Issue: HttpPostRequestDecoder: copy-free implementation

`HttpPostRequestDecoder` decoder implementations (`HttpPostStandardRequestDecoder` and `HttpPostMultipartRequestDecoder`) both do a full copy of the `ByteBuf` before decoding.
 
```
    public HttpPostStandardRequestDecoder offer(HttpContent content) {
        checkDestroyed();

        // Maybe we should better not copy here for performance reasons but this will need
        // more care by the caller to release the content in a correct manner later
        // So maybe something to optimize on a later stage
        ByteBuf buf = content.content();
        if (undecodedChunk == null) {
            undecodedChunk = buf.copy();
        } else ...
```

https://github.com/netty/netty/blob/dcbfe17eeb588a8ba2acb95800776526d01cc5a2/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostStandardRequestDecoder.java#L283
https://github.com/netty/netty/blob/dcbfe17eeb588a8ba2acb95800776526d01cc5a2/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java#L324

They also uses unpooled buffers very broadly:
```
import static io.netty.buffer.Unpooled.*;
```

These classes should be re-implemented to not require copying the ByteBuf and use the default ByteBuf allocator instead of unpooled.

Issue comments:

Pull request review comments:
@fabienrenaud let's use `buf.retainedSlice()`
right...
done!
@fabienrenaud Can you add a comment that its only fine to not copy here as we will never call `undecodedChunk.writeBytes(...)` later on ?
same comment as above 
added
added
@fabienrenaud why this change ?
the previous sequence was failing decoding immediately (on the first character). 
the new sequence causes the decoder to create a few new/sliced ByteBufs before failing the decoding of the last entry `==`.
in other words, the check now increases the chance of catching a leak caused by failing to decode the Nth element instead of just the first one.
got it... thanks 

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java
index 4d59e4d47e0..95079f25fd6 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostMultipartRequestDecoder.java
@@ -321,18 +321,23 @@ public InterfaceHttpData getBodyHttpData(String name) {
     public HttpPostMultipartRequestDecoder offer(HttpContent content) {
         checkDestroyed();
 
-        // Maybe we should better not copy here for performance reasons but this will need
-        // more care by the caller to release the content in a correct manner later
-        // So maybe something to optimize on a later stage
+        if (content instanceof LastHttpContent) {
+            isLastChunk = true;
+        }
+
         ByteBuf buf = content.content();
         if (undecodedChunk == null) {
-            undecodedChunk = buf.copy();
+            undecodedChunk = isLastChunk
+                // Take a slice instead of copying when the first chunk is also the last
+                // as undecodedChunk.writeBytes will never be called.
+                ? buf.retainedSlice()
+                // Maybe we should better not copy here for performance reasons but this will need
+                // more care by the caller to release the content in a correct manner later
+                // So maybe something to optimize on a later stage
+                : buf.copy();
         } else {
             undecodedChunk.writeBytes(buf);
         }
-        if (content instanceof LastHttpContent) {
-            isLastChunk = true;
-        }
         parseBody();
         if (undecodedChunk != null && undecodedChunk.writerIndex() > discardThreshold) {
             undecodedChunk.discardReadBytes();
@@ -1315,7 +1320,7 @@ private static boolean loadDataMultipartStandard(ByteBuf undecodedChunk, String
         if (prevByte == HttpConstants.CR) {
             lastPosition--;
         }
-        ByteBuf content = undecodedChunk.copy(startReaderIndex, lastPosition - startReaderIndex);
+        ByteBuf content = undecodedChunk.retainedSlice(startReaderIndex, lastPosition - startReaderIndex);
         try {
             httpData.addContent(content, delimiterFound);
         } catch (IOException e) {
@@ -1364,7 +1369,7 @@ private static boolean loadDataMultipart(ByteBuf undecodedChunk, String delimite
             lastRealPos--;
         }
         final int lastPosition = sao.getReadPosition(lastRealPos);
-        final ByteBuf content = undecodedChunk.copy(startReaderIndex, lastPosition - startReaderIndex);
+        final ByteBuf content = undecodedChunk.retainedSlice(startReaderIndex, lastPosition - startReaderIndex);
         try {
             httpData.addContent(content, delimiterFound);
         } catch (IOException e) {
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostStandardRequestDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostStandardRequestDecoder.java
index 7b94a7c8a32..deb35a66f39 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostStandardRequestDecoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostStandardRequestDecoder.java
@@ -26,7 +26,10 @@
 import io.netty.handler.codec.http.multipart.HttpPostRequestDecoder.ErrorDataDecoderException;
 import io.netty.handler.codec.http.multipart.HttpPostRequestDecoder.MultiPartStatus;
 import io.netty.handler.codec.http.multipart.HttpPostRequestDecoder.NotEnoughDataDecoderException;
+import io.netty.util.ByteProcessor;
+import io.netty.util.CharsetUtil;
 import io.netty.util.internal.PlatformDependent;
+import io.netty.util.internal.StringUtil;
 
 import java.io.IOException;
 import java.nio.charset.Charset;
@@ -280,18 +283,23 @@ public InterfaceHttpData getBodyHttpData(String name) {
     public HttpPostStandardRequestDecoder offer(HttpContent content) {
         checkDestroyed();
 
-        // Maybe we should better not copy here for performance reasons but this will need
-        // more care by the caller to release the content in a correct manner later
-        // So maybe something to optimize on a later stage
+        if (content instanceof LastHttpContent) {
+            isLastChunk = true;
+        }
+
         ByteBuf buf = content.content();
         if (undecodedChunk == null) {
-            undecodedChunk = buf.copy();
+            undecodedChunk = isLastChunk
+                // Take a slice instead of copying when the first chunk is also the last
+                // as undecodedChunk.writeBytes will never be called.
+                ? buf.retainedSlice()
+                // Maybe we should better not copy here for performance reasons but this will need
+                // more care by the caller to release the content in a correct manner later
+                // So maybe something to optimize on a later stage
+                : buf.copy();
         } else {
             undecodedChunk.writeBytes(buf);
         }
-        if (content instanceof LastHttpContent) {
-            isLastChunk = true;
-        }
         parseBody();
         if (undecodedChunk != null && undecodedChunk.writerIndex() > discardThreshold) {
             undecodedChunk.discardReadBytes();
@@ -429,7 +437,7 @@ private void parseBodyAttributesStandard() {
                     if (read == '&') {
                         currentStatus = MultiPartStatus.DISPOSITION;
                         ampersandpos = currentpos - 1;
-                        setFinalBuffer(undecodedChunk.copy(firstpos, ampersandpos - firstpos));
+                        setFinalBuffer(undecodedChunk.retainedSlice(firstpos, ampersandpos - firstpos));
                         firstpos = currentpos;
                         contRead = true;
                     } else if (read == HttpConstants.CR) {
@@ -439,7 +447,7 @@ private void parseBodyAttributesStandard() {
                             if (read == HttpConstants.LF) {
                                 currentStatus = MultiPartStatus.PREEPILOGUE;
                                 ampersandpos = currentpos - 2;
-                                setFinalBuffer(undecodedChunk.copy(firstpos, ampersandpos - firstpos));
+                                setFinalBuffer(undecodedChunk.retainedSlice(firstpos, ampersandpos - firstpos));
                                 firstpos = currentpos;
                                 contRead = false;
                             } else {
@@ -452,7 +460,7 @@ private void parseBodyAttributesStandard() {
                     } else if (read == HttpConstants.LF) {
                         currentStatus = MultiPartStatus.PREEPILOGUE;
                         ampersandpos = currentpos - 1;
-                        setFinalBuffer(undecodedChunk.copy(firstpos, ampersandpos - firstpos));
+                        setFinalBuffer(undecodedChunk.retainedSlice(firstpos, ampersandpos - firstpos));
                         firstpos = currentpos;
                         contRead = false;
                     }
@@ -466,7 +474,7 @@ private void parseBodyAttributesStandard() {
                 // special case
                 ampersandpos = currentpos;
                 if (ampersandpos > firstpos) {
-                    setFinalBuffer(undecodedChunk.copy(firstpos, ampersandpos - firstpos));
+                    setFinalBuffer(undecodedChunk.retainedSlice(firstpos, ampersandpos - firstpos));
                 } else if (!currentAttribute.isCompleted()) {
                     setFinalBuffer(EMPTY_BUFFER);
                 }
@@ -474,7 +482,7 @@ private void parseBodyAttributesStandard() {
                 currentStatus = MultiPartStatus.EPILOGUE;
             } else if (contRead && currentAttribute != null && currentStatus == MultiPartStatus.FIELD) {
                 // reset index except if to continue in case of FIELD getStatus
-                currentAttribute.addContent(undecodedChunk.copy(firstpos, currentpos - firstpos),
+                currentAttribute.addContent(undecodedChunk.retainedSlice(firstpos, currentpos - firstpos),
                                             false);
                 firstpos = currentpos;
             }
@@ -546,7 +554,7 @@ private void parseBodyAttributes() {
                     if (read == '&') {
                         currentStatus = MultiPartStatus.DISPOSITION;
                         ampersandpos = currentpos - 1;
-                        setFinalBuffer(undecodedChunk.copy(firstpos, ampersandpos - firstpos));
+                        setFinalBuffer(undecodedChunk.retainedSlice(firstpos, ampersandpos - firstpos));
                         firstpos = currentpos;
                         contRead = true;
                     } else if (read == HttpConstants.CR) {
@@ -557,7 +565,7 @@ private void parseBodyAttributes() {
                                 currentStatus = MultiPartStatus.PREEPILOGUE;
                                 ampersandpos = currentpos - 2;
                                 sao.setReadPosition(0);
-                                setFinalBuffer(undecodedChunk.copy(firstpos, ampersandpos - firstpos));
+                                setFinalBuffer(undecodedChunk.retainedSlice(firstpos, ampersandpos - firstpos));
                                 firstpos = currentpos;
                                 contRead = false;
                                 break loop;
@@ -575,7 +583,7 @@ private void parseBodyAttributes() {
                         currentStatus = MultiPartStatus.PREEPILOGUE;
                         ampersandpos = currentpos - 1;
                         sao.setReadPosition(0);
-                        setFinalBuffer(undecodedChunk.copy(firstpos, ampersandpos - firstpos));
+                        setFinalBuffer(undecodedChunk.retainedSlice(firstpos, ampersandpos - firstpos));
                         firstpos = currentpos;
                         contRead = false;
                         break loop;
@@ -592,7 +600,7 @@ private void parseBodyAttributes() {
                 // special case
                 ampersandpos = currentpos;
                 if (ampersandpos > firstpos) {
-                    setFinalBuffer(undecodedChunk.copy(firstpos, ampersandpos - firstpos));
+                    setFinalBuffer(undecodedChunk.retainedSlice(firstpos, ampersandpos - firstpos));
                 } else if (!currentAttribute.isCompleted()) {
                     setFinalBuffer(EMPTY_BUFFER);
                 }
@@ -600,7 +608,7 @@ private void parseBodyAttributes() {
                 currentStatus = MultiPartStatus.EPILOGUE;
             } else if (contRead && currentAttribute != null && currentStatus == MultiPartStatus.FIELD) {
                 // reset index except if to continue in case of FIELD getStatus
-                currentAttribute.addContent(undecodedChunk.copy(firstpos, currentpos - firstpos),
+                currentAttribute.addContent(undecodedChunk.retainedSlice(firstpos, currentpos - firstpos),
                                             false);
                 firstpos = currentpos;
             }
@@ -622,8 +630,10 @@ private void parseBodyAttributes() {
 
     private void setFinalBuffer(ByteBuf buffer) throws IOException {
         currentAttribute.addContent(buffer, true);
-        String value = decodeAttribute(currentAttribute.getByteBuf().toString(charset), charset);
-        currentAttribute.setValue(value);
+        ByteBuf decodedBuf = decodeAttribute(currentAttribute.getByteBuf(), charset);
+        if (decodedBuf != null) { // override content only when ByteBuf needed decoding
+            currentAttribute.setContent(decodedBuf);
+        }
         addHttpData(currentAttribute);
         currentAttribute = null;
     }
@@ -641,6 +651,28 @@ private static String decodeAttribute(String s, Charset charset) {
         }
     }
 
+    private static ByteBuf decodeAttribute(ByteBuf b, Charset charset) {
+        int firstEscaped = b.forEachByte(new ByteProcessor.IndexOfProcessor((byte) '%'));
+        if (firstEscaped == -1) {
+            return null; // nothing to decode
+        }
+
+        ByteBuf buf = b.alloc().buffer(b.readableBytes());
+        UrlDecoder urlDecode = new UrlDecoder(buf);
+        int idx = b.forEachByte(urlDecode);
+        if (urlDecode.nextEscapedIdx != 0) { // incomplete hex byte
+            if (idx == -1) {
+                idx = b.readableBytes() - 1;
+            }
+            idx -= urlDecode.nextEscapedIdx - 1;
+            buf.release();
+            throw new ErrorDataDecoderException(
+                String.format("Invalid hex byte at index '%d' in string: '%s'", idx, b.toString(charset)));
+        }
+
+        return buf;
+    }
+
     /**
      * Destroy the {@link HttpPostStandardRequestDecoder} and release all it resources. After this method
      * was called it is not possible to operate on it anymore.
@@ -677,4 +709,39 @@ public void removeHttpDataFromClean(InterfaceHttpData data) {
 
         factory.removeHttpDataFromClean(request, data);
     }
+
+    private static final class UrlDecoder implements ByteProcessor {
+
+        private final ByteBuf output;
+        private int nextEscapedIdx;
+        private byte hiByte;
+
+        UrlDecoder(ByteBuf output) {
+            this.output = output;
+        }
+
+        @Override
+        public boolean process(byte value) {
+            if (nextEscapedIdx != 0) {
+                if (nextEscapedIdx == 1) {
+                    hiByte = value;
+                    ++nextEscapedIdx;
+                } else {
+                    int hi = StringUtil.decodeHexNibble((char) hiByte);
+                    int lo = StringUtil.decodeHexNibble((char) value);
+                    if (hi == -1 || lo == -1) {
+                        ++nextEscapedIdx;
+                        return false;
+                    }
+                    output.writeByte((hi << 4) + lo);
+                    nextEscapedIdx = 0;
+                }
+            } else if (value == '%') {
+                nextEscapedIdx = 1;
+            } else {
+                output.writeByte(value);
+            }
+            return true;
+        }
+    }
 }


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoderTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoderTest.java
index 40771e017ad..7efdc55849e 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoderTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoderTest.java
@@ -25,7 +25,6 @@
 import io.netty.handler.codec.http.DefaultHttpRequest;
 import io.netty.handler.codec.http.DefaultLastHttpContent;
 import io.netty.handler.codec.http.FullHttpRequest;
-import io.netty.handler.codec.http.HttpContent;
 import io.netty.handler.codec.http.HttpHeaderNames;
 import io.netty.handler.codec.http.HttpHeaderValues;
 import io.netty.handler.codec.http.HttpMethod;
@@ -39,13 +38,11 @@
 import java.nio.charset.UnsupportedCharsetException;
 import java.util.Arrays;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNotNull;
-import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
+import static org.junit.Assert.*;
 
-/** {@link HttpPostRequestDecoder} test case. */
+/**
+ * {@link HttpPostRequestDecoder} test case.
+ */
 public class HttpPostRequestDecoderTest {
 
     @Test
@@ -80,11 +77,11 @@ private static void testBinaryStreamUpload(boolean withSpace) throws Exception {
         for (String data : Arrays.asList("", "\r", "\r\r", "\r\r\r")) {
             final String body =
                     "--" + boundary + "\r\n" +
-                    "Content-Disposition: form-data; name=\"file\"; filename=\"tmp-0.txt\"\r\n" +
-                    "Content-Type: image/gif\r\n" +
-                    "\r\n" +
-                    data + "\r\n" +
-                    "--" + boundary + "--\r\n";
+                            "Content-Disposition: form-data; name=\"file\"; filename=\"tmp-0.txt\"\r\n" +
+                            "Content-Type: image/gif\r\n" +
+                            "\r\n" +
+                            data + "\r\n" +
+                            "--" + boundary + "--\r\n";
 
             // Create decoder instance to test.
             final HttpPostRequestDecoder decoder = new HttpPostRequestDecoder(inMemoryFactory, req);
@@ -100,7 +97,7 @@ private static void testBinaryStreamUpload(boolean withSpace) throws Exception {
 
             // Validate data has been parsed correctly as it was passed into request.
             assertEquals("Invalid decoded data [data=" + data.replaceAll("\r", "\\\\r") + ", upload=" + upload + ']',
-                data, upload.getString(CharsetUtil.UTF_8));
+                    data, upload.getString(CharsetUtil.UTF_8));
             upload.release();
             decoder.destroy();
         }
@@ -225,28 +222,28 @@ public void testNoZeroOut() throws Exception {
         final DefaultHttpDataFactory aMemFactory = new DefaultHttpDataFactory(false);
 
         DefaultHttpRequest aRequest = new DefaultHttpRequest(HttpVersion.HTTP_1_1,
-                                                             HttpMethod.POST,
-                                                             "http://localhost");
+                HttpMethod.POST,
+                "http://localhost");
         aRequest.headers().set(HttpHeaderNames.CONTENT_TYPE,
-                               "multipart/form-data; boundary=" + boundary);
+                "multipart/form-data; boundary=" + boundary);
         aRequest.headers().set(HttpHeaderNames.TRANSFER_ENCODING,
-                               HttpHeaderValues.CHUNKED);
+                HttpHeaderValues.CHUNKED);
 
         HttpPostRequestDecoder aDecoder = new HttpPostRequestDecoder(aMemFactory, aRequest);
 
         final String aData = "some data would be here. the data should be long enough that it " +
-                             "will be longer than the original buffer length of 256 bytes in " +
-                             "the HttpPostRequestDecoder in order to trigger the issue. Some more " +
-                             "data just to be on the safe side.";
+                "will be longer than the original buffer length of 256 bytes in " +
+                "the HttpPostRequestDecoder in order to trigger the issue. Some more " +
+                "data just to be on the safe side.";
 
         final String body =
                 "--" + boundary + "\r\n" +
-                "Content-Disposition: form-data; name=\"root\"\r\n" +
-                "Content-Type: text/plain\r\n" +
-                "\r\n" +
-                aData +
-                "\r\n" +
-                "--" + boundary + "--\r\n";
+                        "Content-Disposition: form-data; name=\"root\"\r\n" +
+                        "Content-Type: text/plain\r\n" +
+                        "\r\n" +
+                        aData +
+                        "\r\n" +
+                        "--" + boundary + "--\r\n";
 
         byte[] aBytes = body.getBytes();
 
@@ -279,7 +276,7 @@ public void testNoZeroOut() throws Exception {
     // See https://github.com/netty/netty/issues/2305
     @Test
     public void testChunkCorrect() throws Exception {
-       String payload = "town=794649819&town=784444184&town=794649672&town=794657800&town=" +
+        String payload = "town=794649819&town=784444184&town=794649672&town=794657800&town=" +
                 "794655734&town=794649377&town=794652136&town=789936338&town=789948986&town=" +
                 "789949643&town=786358677&town=794655880&town=786398977&town=789901165&town=" +
                 "789913325&town=789903418&town=789903579&town=794645251&town=794694126&town=" +
@@ -308,26 +305,44 @@ public void testChunkCorrect() throws Exception {
                 "789958999&town=789961555&town=794694050&town=794650241&town=794656286&town=" +
                 "794692081&town=794660090&town=794665227&town=794665136&town=794669931";
         DefaultHttpRequest defaultHttpRequest =
-                    new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/");
+                new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/");
 
         HttpPostRequestDecoder decoder = new HttpPostRequestDecoder(defaultHttpRequest);
 
         int firstChunk = 10;
         int middleChunk = 1024;
 
-        HttpContent part1 = new DefaultHttpContent(Unpooled.wrappedBuffer(
-                payload.substring(0, firstChunk).getBytes()));
-        HttpContent part2 = new DefaultHttpContent(Unpooled.wrappedBuffer(
-                payload.substring(firstChunk, firstChunk + middleChunk).getBytes()));
-        HttpContent part3 = new DefaultHttpContent(Unpooled.wrappedBuffer(
-                payload.substring(firstChunk + middleChunk, firstChunk + middleChunk * 2).getBytes()));
-        HttpContent part4 = new DefaultHttpContent(Unpooled.wrappedBuffer(
-                payload.substring(firstChunk + middleChunk * 2).getBytes()));
+        byte[] payload1 = payload.substring(0, firstChunk).getBytes();
+        byte[] payload2 = payload.substring(firstChunk, firstChunk + middleChunk).getBytes();
+        byte[] payload3 = payload.substring(firstChunk + middleChunk, firstChunk + middleChunk * 2).getBytes();
+        byte[] payload4 = payload.substring(firstChunk + middleChunk * 2).getBytes();
+
+        ByteBuf buf1 = Unpooled.directBuffer(payload1.length);
+        ByteBuf buf2 = Unpooled.directBuffer(payload2.length);
+        ByteBuf buf3 = Unpooled.directBuffer(payload3.length);
+        ByteBuf buf4 = Unpooled.directBuffer(payload4.length);
+
+        buf1.writeBytes(payload1);
+        buf2.writeBytes(payload2);
+        buf3.writeBytes(payload3);
+        buf4.writeBytes(payload4);
+
+        decoder.offer(new DefaultHttpContent(buf1));
+        decoder.offer(new DefaultHttpContent(buf2));
+        decoder.offer(new DefaultHttpContent(buf3));
+        decoder.offer(new DefaultLastHttpContent(buf4));
+
+        assertFalse(decoder.getBodyHttpDatas().isEmpty());
+        assertEquals(139, decoder.getBodyHttpDatas().size());
+
+        Attribute attr = (Attribute) decoder.getBodyHttpData("town");
+        assertEquals("794649819", attr.getValue());
 
-        decoder.offer(part1);
-        decoder.offer(part2);
-        decoder.offer(part3);
-        decoder.offer(part4);
+        decoder.destroy();
+        buf1.release();
+        buf2.release();
+        buf3.release();
+        buf4.release();
     }
 
     // See https://github.com/netty/netty/issues/3326
@@ -360,7 +375,7 @@ public void testFilenameContainingSemicolon() throws Exception {
     public void testFilenameContainingSemicolon2() throws Exception {
         final String boundary = "dLV9Wyq26L_-JQxk6ferf-RT153LhOO";
         final DefaultFullHttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST,
-                                                                      "http://localhost");
+                "http://localhost");
         req.headers().add(HttpHeaderNames.CONTENT_TYPE, "multipart/form-data; boundary=" + boundary);
         // Force to use memory-based data.
         final DefaultHttpDataFactory inMemoryFactory = new DefaultHttpDataFactory(false);
@@ -368,11 +383,11 @@ public void testFilenameContainingSemicolon2() throws Exception {
         final String filename = "tmp;0.txt";
         final String body =
                 "--" + boundary + "\r\n" +
-                "Content-Disposition: form-data; name=\"file\"; filename=\"" + filename + "\"\r\n" +
-                "Content-Type: image/gif\r\n" +
-                "\r\n" +
-                data + "\r\n" +
-                "--" + boundary + "--\r\n";
+                        "Content-Disposition: form-data; name=\"file\"; filename=\"" + filename + "\"\r\n" +
+                        "Content-Type: image/gif\r\n" +
+                        "\r\n" +
+                        data + "\r\n" +
+                        "--" + boundary + "--\r\n";
 
         req.content().writeBytes(body.getBytes(CharsetUtil.UTF_8.name()));
         // Create decoder instance to test.
@@ -421,19 +436,19 @@ public void testDecodeOtherMimeHeaderFields() throws Exception {
         String filecontent = "123456";
 
         final String body = "--" + boundary + "\r\n" +
-                            "Content-Disposition: form-data; name=\"file\"; filename=" + "\"" + "attached.txt" + "\"" +
-                            "\r\n" +
-                            "Content-Type: application/octet-stream" + "\r\n" +
-                            "Content-Encoding: gzip" + "\r\n" +
-                            "\r\n" +
-                            filecontent +
-                            "\r\n" +
-                            "--" + boundary + "--";
+                "Content-Disposition: form-data; name=\"file\"; filename=" + "\"" + "attached.txt" + "\"" +
+                "\r\n" +
+                "Content-Type: application/octet-stream" + "\r\n" +
+                "Content-Encoding: gzip" + "\r\n" +
+                "\r\n" +
+                filecontent +
+                "\r\n" +
+                "--" + boundary + "--";
 
         final DefaultFullHttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1,
-                                                                      HttpMethod.POST,
-                                                                      "http://localhost",
-                                                                      Unpooled.wrappedBuffer(body.getBytes()));
+                HttpMethod.POST,
+                "http://localhost",
+                Unpooled.wrappedBuffer(body.getBytes()));
         req.headers().add(HttpHeaderNames.CONTENT_TYPE, "multipart/form-data; boundary=" + boundary);
         req.headers().add(HttpHeaderNames.TRANSFER_ENCODING, HttpHeaderValues.CHUNKED);
         final DefaultHttpDataFactory inMemoryFactory = new DefaultHttpDataFactory(false);
@@ -452,19 +467,19 @@ public void testDecodeOtherMimeHeaderFields() throws Exception {
     public void testMultipartRequestWithFileInvalidCharset() throws Exception {
         final String boundary = "dLV9Wyq26L_-JQxk6ferf-RT153LhOO";
         final DefaultFullHttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST,
-            "http://localhost");
+                "http://localhost");
         req.headers().add(HttpHeaderNames.CONTENT_TYPE, "multipart/form-data; boundary=" + boundary);
         // Force to use memory-based data.
         final DefaultHttpDataFactory inMemoryFactory = new DefaultHttpDataFactory(false);
         final String data = "asdf";
         final String filename = "tmp;0.txt";
         final String body =
-            "--" + boundary + "\r\n" +
-                "Content-Disposition: form-data; name=\"file\"; filename=\"" + filename + "\"\r\n" +
-                "Content-Type: image/gif; charset=ABCD\r\n" +
-                "\r\n" +
-                data + "\r\n" +
-                "--" + boundary + "--\r\n";
+                "--" + boundary + "\r\n" +
+                        "Content-Disposition: form-data; name=\"file\"; filename=\"" + filename + "\"\r\n" +
+                        "Content-Type: image/gif; charset=ABCD\r\n" +
+                        "\r\n" +
+                        data + "\r\n" +
+                        "--" + boundary + "--\r\n";
 
         req.content().writeBytes(body.getBytes(CharsetUtil.UTF_8));
         // Create decoder instance to test.
@@ -482,22 +497,22 @@ public void testMultipartRequestWithFileInvalidCharset() throws Exception {
     public void testMultipartRequestWithFieldInvalidCharset() throws Exception {
         final String boundary = "dLV9Wyq26L_-JQxk6ferf-RT153LhOO";
         final DefaultFullHttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST,
-            "http://localhost");
+                "http://localhost");
         req.headers().add(HttpHeaderNames.CONTENT_TYPE, "multipart/form-data; boundary=" + boundary);
         // Force to use memory-based data.
         final DefaultHttpDataFactory inMemoryFactory = new DefaultHttpDataFactory(false);
         final String aData = "some data would be here. the data should be long enough that it " +
-            "will be longer than the original buffer length of 256 bytes in " +
-            "the HttpPostRequestDecoder in order to trigger the issue. Some more " +
-            "data just to be on the safe side.";
+                "will be longer than the original buffer length of 256 bytes in " +
+                "the HttpPostRequestDecoder in order to trigger the issue. Some more " +
+                "data just to be on the safe side.";
         final String body =
-            "--" + boundary + "\r\n" +
-                "Content-Disposition: form-data; name=\"root\"\r\n" +
-                "Content-Type: text/plain; charset=ABCD\r\n" +
-                "\r\n" +
-                aData +
-                "\r\n" +
-                "--" + boundary + "--\r\n";
+                "--" + boundary + "\r\n" +
+                        "Content-Disposition: form-data; name=\"root\"\r\n" +
+                        "Content-Type: text/plain; charset=ABCD\r\n" +
+                        "\r\n" +
+                        aData +
+                        "\r\n" +
+                        "--" + boundary + "--\r\n";
 
         req.content().writeBytes(body.getBytes(CharsetUtil.UTF_8));
         // Create decoder instance to test.
@@ -539,16 +554,16 @@ public void testDecodeContentDispositionFieldParameters() throws Exception {
         String filenameEncoded = URLEncoder.encode(filename, encoding);
 
         final String body = "--" + boundary + "\r\n" +
-          "Content-Disposition: form-data; name=\"file\"; filename*=" + encoding + "''" + filenameEncoded + "\r\n" +
-          "\r\n" +
-          "foo\r\n" +
-          "\r\n" +
-          "--" + boundary + "--";
+                "Content-Disposition: form-data; name=\"file\"; filename*=" + encoding + "''" + filenameEncoded +
+                "\r\n\r\n" +
+                "foo\r\n" +
+                "\r\n" +
+                "--" + boundary + "--";
 
         final DefaultFullHttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1,
-                                                                      HttpMethod.POST,
-                                                                      "http://localhost",
-                                                                      Unpooled.wrappedBuffer(body.getBytes()));
+                HttpMethod.POST,
+                "http://localhost",
+                Unpooled.wrappedBuffer(body.getBytes()));
 
         req.headers().add(HttpHeaderNames.CONTENT_TYPE, "multipart/form-data; boundary=" + boundary);
         final DefaultHttpDataFactory inMemoryFactory = new DefaultHttpDataFactory(false);
@@ -574,17 +589,17 @@ public void testDecodeWithLanguageContentDispositionFieldParameters() throws Exc
         String filenameEncoded = URLEncoder.encode(filename, encoding);
 
         final String body = "--" + boundary + "\r\n" +
-          "Content-Disposition: form-data; name=\"file\"; filename*=" +
-              encoding + "'" + language + "'" + filenameEncoded + "\r\n" +
-          "\r\n" +
-          "foo\r\n" +
-          "\r\n" +
-          "--" + boundary + "--";
+                "Content-Disposition: form-data; name=\"file\"; filename*=" +
+                encoding + "'" + language + "'" + filenameEncoded + "\r\n" +
+                "\r\n" +
+                "foo\r\n" +
+                "\r\n" +
+                "--" + boundary + "--";
 
         final DefaultFullHttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1,
-                                                                      HttpMethod.POST,
-                                                                      "http://localhost",
-                                                                      Unpooled.wrappedBuffer(body.getBytes()));
+                HttpMethod.POST,
+                "http://localhost",
+                Unpooled.wrappedBuffer(body.getBytes()));
 
         req.headers().add(HttpHeaderNames.CONTENT_TYPE, "multipart/form-data; boundary=" + boundary);
         final DefaultHttpDataFactory inMemoryFactory = new DefaultHttpDataFactory(false);
@@ -605,16 +620,16 @@ public void testDecodeMalformedNotEncodedContentDispositionFieldParameters() thr
         final String boundary = "74e78d11b0214bdcbc2f86491eeb4902";
 
         final String body = "--" + boundary + "\r\n" +
-          "Content-Disposition: form-data; name=\"file\"; filename*=not-encoded\r\n" +
-          "\r\n" +
-          "foo\r\n" +
-          "\r\n" +
-          "--" + boundary + "--";
+                "Content-Disposition: form-data; name=\"file\"; filename*=not-encoded\r\n" +
+                "\r\n" +
+                "foo\r\n" +
+                "\r\n" +
+                "--" + boundary + "--";
 
         final DefaultFullHttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1,
-                                                                      HttpMethod.POST,
-                                                                      "http://localhost",
-                                                                      Unpooled.wrappedBuffer(body.getBytes()));
+                HttpMethod.POST,
+                "http://localhost",
+                Unpooled.wrappedBuffer(body.getBytes()));
 
         req.headers().add(HttpHeaderNames.CONTENT_TYPE, "multipart/form-data; boundary=" + boundary);
 
@@ -637,16 +652,16 @@ public void testDecodeMalformedBadCharsetContentDispositionFieldParameters() thr
         final String boundary = "74e78d11b0214bdcbc2f86491eeb4902";
 
         final String body = "--" + boundary + "\r\n" +
-          "Content-Disposition: form-data; name=\"file\"; filename*=not-a-charset''filename\r\n" +
-          "\r\n" +
-          "foo\r\n" +
-          "\r\n" +
-          "--" + boundary + "--";
+                "Content-Disposition: form-data; name=\"file\"; filename*=not-a-charset''filename\r\n" +
+                "\r\n" +
+                "foo\r\n" +
+                "\r\n" +
+                "--" + boundary + "--";
 
         final DefaultFullHttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1,
-                                                                      HttpMethod.POST,
-                                                                      "http://localhost",
-                                                                      Unpooled.wrappedBuffer(body.getBytes()));
+                HttpMethod.POST,
+                "http://localhost",
+                Unpooled.wrappedBuffer(body.getBytes()));
 
         req.headers().add(HttpHeaderNames.CONTENT_TYPE, "multipart/form-data; boundary=" + boundary);
 
@@ -667,7 +682,7 @@ public void testDecodeMalformedBadCharsetContentDispositionFieldParameters() thr
     public void testDecodeMalformedEmptyContentTypeFieldParameters() throws Exception {
         final String boundary = "dLV9Wyq26L_-JQxk6ferf-RT153LhOO";
         final DefaultFullHttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST,
-                                                                      "http://localhost");
+                "http://localhost");
         req.headers().add(HttpHeaderNames.CONTENT_TYPE, "multipart/form-data; boundary=" + boundary);
         // Force to use memory-based data.
         final DefaultHttpDataFactory inMemoryFactory = new DefaultHttpDataFactory(false);
@@ -675,11 +690,11 @@ public void testDecodeMalformedEmptyContentTypeFieldParameters() throws Exceptio
         final String filename = "tmp-0.txt";
         final String body =
                 "--" + boundary + "\r\n" +
-                "Content-Disposition: form-data; name=\"file\"; filename=\"" + filename + "\"\r\n" +
-                "Content-Type: \r\n" +
-                "\r\n" +
-                data + "\r\n" +
-                "--" + boundary + "--\r\n";
+                        "Content-Disposition: form-data; name=\"file\"; filename=\"" + filename + "\"\r\n" +
+                        "Content-Type: \r\n" +
+                        "\r\n" +
+                        data + "\r\n" +
+                        "--" + boundary + "--\r\n";
 
         req.content().writeBytes(body.getBytes(CharsetUtil.UTF_8.name()));
         // Create decoder instance to test.
@@ -697,15 +712,17 @@ public void testDecodeMalformedEmptyContentTypeFieldParameters() throws Exceptio
     public void testMultipartRequest() throws Exception {
         String BOUNDARY = "01f136d9282f";
 
-        ByteBuf byteBuf = Unpooled.wrappedBuffer(("--" + BOUNDARY + "\n" +
-                "Content-Disposition: form-data; name=\"msg_id\"\n" +
-                "\n" +
-                "15200\n" +
-                "--" + BOUNDARY + "\n" +
-                "Content-Disposition: form-data; name=\"msg\"\n" +
-                "\n" +
-                "test message\n" +
-                "--" + BOUNDARY + "--").getBytes());
+        byte[] bodyBytes = ("--" + BOUNDARY + "\n" +
+            "Content-Disposition: form-data; name=\"msg_id\"\n" +
+            "\n" +
+            "15200\n" +
+            "--" + BOUNDARY + "\n" +
+            "Content-Disposition: form-data; name=\"msg\"\n" +
+            "\n" +
+            "test message\n" +
+            "--" + BOUNDARY + "--").getBytes();
+        ByteBuf byteBuf = Unpooled.directBuffer(bodyBytes.length);
+        byteBuf.writeBytes(bodyBytes);
 
         FullHttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_0, HttpMethod.POST, "/up", byteBuf);
         req.headers().add(HttpHeaderNames.CONTENT_TYPE, "multipart/form-data; boundary=" + BOUNDARY);
@@ -718,11 +735,16 @@ public void testMultipartRequest() throws Exception {
         assertTrue(decoder.isMultipart());
         assertFalse(decoder.getBodyHttpDatas().isEmpty());
         assertEquals(2, decoder.getBodyHttpDatas().size());
-        assertEquals("test message", ((Attribute) decoder.getBodyHttpData("msg")).getValue());
-        assertEquals("15200", ((Attribute) decoder.getBodyHttpData("msg_id")).getValue());
+
+        Attribute attrMsg = (Attribute) decoder.getBodyHttpData("msg");
+        assertTrue(attrMsg.getByteBuf().isDirect());
+        assertEquals("test message", attrMsg.getValue());
+        Attribute attrMsgId = (Attribute) decoder.getBodyHttpData("msg_id");
+        assertTrue(attrMsgId.getByteBuf().isDirect());
+        assertEquals("15200", attrMsgId.getValue());
 
         decoder.destroy();
-        assertEquals(1, req.refCnt());
+        assertTrue(req.release());
     }
 
     @Test(expected = HttpPostRequestDecoder.ErrorDataDecoderException.class)
@@ -747,7 +769,7 @@ public void testNotLeakHeapBufferWhenWrapIllegalArgumentException() {
     }
 
     private static void testNotLeakWhenWrapIllegalArgumentException(ByteBuf buf) {
-        buf.writeCharSequence("==", CharsetUtil.US_ASCII);
+        buf.writeCharSequence("a=b&foo=%22bar%22&==", CharsetUtil.US_ASCII);
         FullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/", buf);
         try {
             new HttpPostStandardRequestDecoder(request);
@@ -800,7 +822,109 @@ public void testDecodeWithLanguageContentDispositionFieldParametersForFix() thro
         assertTrue("the item should be a FileUpload", part1 instanceof FileUpload);
         FileUpload fileUpload = (FileUpload) part1;
         assertEquals("the filename should be decoded", filename, fileUpload.getFilename());
+
         decoder.destroy();
         req.release();
     }
+
+    @Test
+    public void testDecodeFullHttpRequestWithUrlEncodedBody() throws Exception {
+        byte[] bodyBytes = "foo=bar&a=b&empty=&city=%3c%22new%22%20york%20city%3e".getBytes();
+        ByteBuf content = Unpooled.directBuffer(bodyBytes.length);
+        content.writeBytes(bodyBytes);
+
+        FullHttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/", content);
+        HttpPostRequestDecoder decoder = new HttpPostRequestDecoder(req);
+        assertFalse(decoder.getBodyHttpDatas().isEmpty());
+
+        assertFalse(decoder.getBodyHttpDatas().isEmpty());
+        assertEquals(4, decoder.getBodyHttpDatas().size());
+
+        Attribute attr = (Attribute) decoder.getBodyHttpData("foo");
+        assertTrue(attr.getByteBuf().isDirect());
+        assertEquals("bar", attr.getValue());
+
+        attr = (Attribute) decoder.getBodyHttpData("a");
+        assertTrue(attr.getByteBuf().isDirect());
+        assertEquals("b", attr.getValue());
+
+        attr = (Attribute) decoder.getBodyHttpData("empty");
+        assertTrue(attr.getByteBuf().isDirect());
+        assertEquals("", attr.getValue());
+
+        attr = (Attribute) decoder.getBodyHttpData("city");
+        assertTrue(attr.getByteBuf().isDirect());
+        assertEquals("<\"new\" york city>", attr.getValue());
+
+        decoder.destroy();
+        req.release();
+    }
+
+    @Test
+    public void testDecodeFullHttpRequestWithUrlEncodedBodyWithBrokenHexByte0() {
+        byte[] bodyBytes = "foo=bar&a=b&empty=%&city=paris".getBytes();
+        ByteBuf content = Unpooled.directBuffer(bodyBytes.length);
+        content.writeBytes(bodyBytes);
+
+        FullHttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/", content);
+        try {
+            new HttpPostRequestDecoder(req);
+            fail("Was expecting an ErrorDataDecoderException");
+        } catch (HttpPostRequestDecoder.ErrorDataDecoderException e) {
+            assertEquals("Invalid hex byte at index '0' in string: '%'", e.getMessage());
+        } finally {
+            req.release();
+        }
+    }
+
+    @Test
+    public void testDecodeFullHttpRequestWithUrlEncodedBodyWithBrokenHexByte1() {
+        byte[] bodyBytes = "foo=bar&a=b&empty=%2&city=london".getBytes();
+        ByteBuf content = Unpooled.directBuffer(bodyBytes.length);
+        content.writeBytes(bodyBytes);
+
+        FullHttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/", content);
+        try {
+            new HttpPostRequestDecoder(req);
+            fail("Was expecting an ErrorDataDecoderException");
+        } catch (HttpPostRequestDecoder.ErrorDataDecoderException e) {
+            assertEquals("Invalid hex byte at index '0' in string: '%2'", e.getMessage());
+        } finally {
+            req.release();
+        }
+    }
+
+    @Test
+    public void testDecodeFullHttpRequestWithUrlEncodedBodyWithInvalidHexNibbleHi() {
+        byte[] bodyBytes = "foo=bar&a=b&empty=%Zc&city=london".getBytes();
+        ByteBuf content = Unpooled.directBuffer(bodyBytes.length);
+        content.writeBytes(bodyBytes);
+
+        FullHttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/", content);
+        try {
+            new HttpPostRequestDecoder(req);
+            fail("Was expecting an ErrorDataDecoderException");
+        } catch (HttpPostRequestDecoder.ErrorDataDecoderException e) {
+            assertEquals("Invalid hex byte at index '0' in string: '%Zc'", e.getMessage());
+        } finally {
+            req.release();
+        }
+    }
+
+    @Test
+    public void testDecodeFullHttpRequestWithUrlEncodedBodyWithInvalidHexNibbleLo() {
+        byte[] bodyBytes = "foo=bar&a=b&empty=%2g&city=london".getBytes();
+        ByteBuf content = Unpooled.directBuffer(bodyBytes.length);
+        content.writeBytes(bodyBytes);
+
+        FullHttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/", content);
+        try {
+            new HttpPostRequestDecoder(req);
+            fail("Was expecting an ErrorDataDecoderException");
+        } catch (HttpPostRequestDecoder.ErrorDataDecoderException e) {
+            assertEquals("Invalid hex byte at index '0' in string: '%2g'", e.getMessage());
+        } finally {
+            req.release();
+        }
+    }
 }