Repository: netty/netty
Issue: NullPointerException in HttpPostRequestEncoder
Labels: defect

Hello,

There is a bug in the `nextChunk()` algorithm of the `HttpPostRequestEncoder`. When chunking a big request or multipart data, if the size of the resulting query is a multiple of the chunk size, `isLastChunkSent` is not set to `true` at the end, resulting in a NullPointerException.

This bug is present in version 3.10 as well as 4.x

Here is a unitest that shows this problem:

``` java
    @Test
    public void testNpe() throws Exception {
        DefaultFullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1,
                HttpMethod.POST, "http://localhost");
        HttpPostRequestEncoder encoder = new HttpPostRequestEncoder(request, true);
        int length = 7943;
        char[] array = new char[length];
        Arrays.fill(array, 'a');
        String longText = new String(array);
        encoder.addBodyAttribute("foo", longText);
        encoder.finalizeRequest();

        while (! encoder.isEndOfInput()) {
            encoder.readChunk((ByteBufAllocator) null);
        }
    }
```


Issue comments:
Can you provide a PR with the test and a fix?

> Am 30.06.2016 um 16:49 schrieb Benjamin Fiorini notifications@github.com:
> 
> Hello,
> 
> There is a bug in the nextChunk() algorithm of the HttpPostRequestEncoder. When chunking a big request or multipart data, if the size of the resulting query is a multiple of the chunk size, isLastChunkSent is not set to true at the end, resulting in a NullPointerException.
> 
> This bug is present in version 3.10 as well as 4.x
> 
> Here is a unitest that shows this problem:
> 
> ```
> @Test
> public void testNpe() throws Exception {
>     DefaultFullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1,
>             HttpMethod.POST, "http://localhost");
>     HttpPostRequestEncoder encoder = new HttpPostRequestEncoder(request, true);
>     int length = 7943;
>     char[] array = new char[length];
>     Arrays.fill(array, 'a');
>     String longText = new String(array);
>     encoder.addBodyAttribute("foo", longText);
>     encoder.finalizeRequest();
> 
>     while (! encoder.isEndOfInput()) {
>         encoder.readChunk((ByteBufAllocator) null);
>     }
> }
> ```
> 
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub, or mute the thread.

@bfiorini any interest in provide a fix as PR with a unit test ?

On my side, not able to reproduce it. As far as I read the code, if the data is a multiple of chunk size, the next call will set to true the isLastChunk and returns LastHttpContent.EMPTY_LAST_CONTENT (as defined in the API).

Maybe I'm missing something?
For those interested here is a test case that reproduces on `4.1.8.Final`

```java
@Test
public void test() throws Exception {
    DefaultHttpDataFactory factory = new DefaultHttpDataFactory(DefaultHttpDataFactory.MINSIZE);
    DefaultFullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "http://localhost");
    HttpPostRequestEncoder encoder = new HttpPostRequestEncoder(factory, request, true, HttpConstants.DEFAULT_CHARSET, HttpPostRequestEncoder.EncoderMode.RFC1738);

    MemoryFileUpload first = new MemoryFileUpload("resources", "", "application/json", null, Charset.forName("UTF-8"), -1);
    first.setMaxSize(-1);
    first.setContent(new ByteArrayInputStream(new byte[7955]));
    encoder.addBodyHttpData(first);

    MemoryFileUpload second = new MemoryFileUpload("resources2", "", "application/json", null, Charset.forName("UTF-8"), -1);
    second.setMaxSize(-1);
    second.setContent(new ByteArrayInputStream(new byte[7955]));
    encoder.addBodyHttpData(second);

    encoder.finalizeRequest();
    assertEquals("First chunk is not 8096 bytes", 8096, encoder.readChunk((ByteBufAllocator) null).content().readableBytes());
    assertFalse("Already at end of input", encoder.isEndOfInput());
    encoder.readChunk((ByteBufAllocator) null);
}
```
Fixed by https://github.com/netty/netty/pull/6991

Pull request review comments:
call `httpContent.release()`
call httpContent.release()
static
done
done
done

Patch that resolved the issue:
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostRequestEncoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostRequestEncoder.java
index 57f81f5b66b..a56431e2226 100755
--- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostRequestEncoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostRequestEncoder.java
@@ -1062,40 +1062,30 @@ private HttpContent nextChunk() throws ErrorDataEncoderException {
             isLastChunkSent = true;
             return LastHttpContent.EMPTY_LAST_CONTENT;
         }
-        ByteBuf buffer;
-        int size = HttpPostBodyUtil.chunkSize;
         // first test if previous buffer is not empty
-        if (currentBuffer != null) {
-            size -= currentBuffer.readableBytes();
-        }
+        int size = calculateRemainingSize();
         if (size <= 0) {
             // NextChunk from buffer
-            buffer = fillByteBuf();
+            ByteBuf buffer = fillByteBuf();
             return new DefaultHttpContent(buffer);
         }
         // size > 0
         if (currentData != null) {
             // continue to read data
+            HttpContent chunk;
             if (isMultipart) {
-                HttpContent chunk = encodeNextChunkMultipart(size);
-                if (chunk != null) {
-                    return chunk;
-                }
+                chunk = encodeNextChunkMultipart(size);
             } else {
-                HttpContent chunk = encodeNextChunkUrlEncoded(size);
-                if (chunk != null) {
-                    // NextChunk Url from currentData
-                    return chunk;
-                }
+                chunk = encodeNextChunkUrlEncoded(size);
+            }
+            if (chunk != null) {
+                // NextChunk from data
+                return chunk;
             }
-            size = HttpPostBodyUtil.chunkSize - currentBuffer.readableBytes();
+            size = calculateRemainingSize();
         }
         if (!iterator.hasNext()) {
-            isLastChunk = true;
-            // NextChunk as last non empty from buffer
-            buffer = currentBuffer;
-            currentBuffer = null;
-            return new DefaultHttpContent(buffer);
+            return lastChunk();
         }
         while (size > 0 && iterator.hasNext()) {
             currentData = iterator.next();
@@ -1107,21 +1097,33 @@ private HttpContent nextChunk() throws ErrorDataEncoderException {
             }
             if (chunk == null) {
                 // not enough
-                size = HttpPostBodyUtil.chunkSize - currentBuffer.readableBytes();
+                size = calculateRemainingSize();
                 continue;
             }
             // NextChunk from data
             return chunk;
         }
         // end since no more data
+        return lastChunk();
+    }
+
+    private int calculateRemainingSize() {
+        int size = HttpPostBodyUtil.chunkSize;
+        if (currentBuffer != null) {
+            size -= currentBuffer.readableBytes();
+        }
+        return size;
+    }
+
+    private HttpContent lastChunk() {
         isLastChunk = true;
         if (currentBuffer == null) {
             isLastChunkSent = true;
             // LastChunk with no more data
             return LastHttpContent.EMPTY_LAST_CONTENT;
         }
-        // Previous LastChunk with no more data
-        buffer = currentBuffer;
+        // NextChunk as last non empty from buffer
+        ByteBuf buffer = currentBuffer;
         currentBuffer = null;
         return new DefaultHttpContent(buffer);
     }


Test patch:
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestEncoderTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestEncoderTest.java
index 3a206f44ba7..2d821273fa8 100755
--- a/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestEncoderTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/multipart/HttpPostRequestEncoderTest.java
@@ -19,16 +19,20 @@
 import io.netty.buffer.ByteBufAllocator;
 import io.netty.buffer.Unpooled;
 import io.netty.handler.codec.http.DefaultFullHttpRequest;
+import io.netty.handler.codec.http.HttpConstants;
 import io.netty.handler.codec.http.HttpContent;
 import io.netty.handler.codec.http.HttpMethod;
 import io.netty.handler.codec.http.HttpVersion;
+import io.netty.handler.codec.http.LastHttpContent;
 import io.netty.handler.codec.http.multipart.HttpPostRequestEncoder.EncoderMode;
 import io.netty.handler.codec.http.multipart.HttpPostRequestEncoder.ErrorDataEncoderException;
 import io.netty.util.CharsetUtil;
 import io.netty.util.internal.StringUtil;
 import org.junit.Test;
 
+import java.io.ByteArrayInputStream;
 import java.io.File;
+import java.nio.charset.Charset;
 import java.util.Arrays;
 import java.util.List;
 
@@ -37,6 +41,7 @@
 import static io.netty.handler.codec.http.HttpHeaderNames.CONTENT_TRANSFER_ENCODING;
 import static io.netty.handler.codec.http.HttpHeaderNames.CONTENT_TYPE;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
@@ -358,4 +363,64 @@ private static String getRequestBody(HttpPostRequestEncoder encoder) throws Exce
         content.release();
         return contentStr;
     }
+
+    @Test
+    public void testDataIsMultipleOfChunkSize1() throws Exception {
+        DefaultHttpDataFactory factory = new DefaultHttpDataFactory(DefaultHttpDataFactory.MINSIZE);
+        DefaultFullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1,
+                HttpMethod.POST, "http://localhost");
+        HttpPostRequestEncoder encoder = new HttpPostRequestEncoder(factory, request, true,
+                HttpConstants.DEFAULT_CHARSET, HttpPostRequestEncoder.EncoderMode.RFC1738);
+
+        MemoryFileUpload first = new MemoryFileUpload("resources", "", "application/json", null,
+                Charset.forName("UTF-8"), -1);
+        first.setMaxSize(-1);
+        first.setContent(new ByteArrayInputStream(new byte[7955]));
+        encoder.addBodyHttpData(first);
+
+        MemoryFileUpload second = new MemoryFileUpload("resources2", "", "application/json", null,
+                Charset.forName("UTF-8"), -1);
+        second.setMaxSize(-1);
+        second.setContent(new ByteArrayInputStream(new byte[7928]));
+        encoder.addBodyHttpData(second);
+
+        assertNotNull(encoder.finalizeRequest());
+
+        checkNextChunkSize(encoder, 8096);
+        checkNextChunkSize(encoder, 8096);
+
+        HttpContent httpContent = encoder.readChunk((ByteBufAllocator) null);
+        assertTrue("Expected LastHttpContent is not received", httpContent instanceof LastHttpContent);
+        httpContent.release();
+
+        assertTrue("Expected end of input is not receive", encoder.isEndOfInput());
+    }
+
+    @Test
+    public void testDataIsMultipleOfChunkSize2() throws Exception {
+        DefaultFullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1,
+                HttpMethod.POST, "http://localhost");
+        HttpPostRequestEncoder encoder = new HttpPostRequestEncoder(request, true);
+        int length = 7943;
+        char[] array = new char[length];
+        Arrays.fill(array, 'a');
+        String longText = new String(array);
+        encoder.addBodyAttribute("foo", longText);
+
+        assertNotNull(encoder.finalizeRequest());
+
+        checkNextChunkSize(encoder, 8096);
+
+        HttpContent httpContent = encoder.readChunk((ByteBufAllocator) null);
+        assertTrue("Expected LastHttpContent is not received", httpContent instanceof LastHttpContent);
+        httpContent.release();
+
+        assertTrue("Expected end of input is not receive", encoder.isEndOfInput());
+    }
+
+    private static void checkNextChunkSize(HttpPostRequestEncoder encoder, int expectedSize) throws Exception {
+        HttpContent httpContent = encoder.readChunk((ByteBufAllocator) null);
+        assertEquals("Chunk is not " + expectedSize + " bytes", expectedSize, httpContent.content().readableBytes());
+        httpContent.release();
+    }
 }