From 08748344d852a07011be6e74bb5ddc6dcf221c45 Mon Sep 17 00:00:00 2001 From: Violeta Georgieva Date: Wed, 19 Jul 2017 10:46:27 +0300 Subject: [PATCH] Fix NPEs in HttpPostRequestEncoder#nextChunk Motivation: HttpPostRequestEncoder maintains an internal buffer that holds the current encoded data. There are use cases when this internal buffer becomes null, the next chunk processing implementation should take this into consideration. Modifications: - When preparing the last chunk if currentBuffer is null, mark isLastChunkSent as true and send LastHttpContent.EMPTY_LAST_CONTENT - When calculating the remaining size take into consideration that the currentBuffer might be null - Tests are based on those provided in the issue by @nebhale and @bfiorini Result: Fixes #5478 --- .../multipart/HttpPostRequestEncoder.java | 50 +++++++------- .../multipart/HttpPostRequestEncoderTest.java | 65 +++++++++++++++++++ 2 files changed, 91 insertions(+), 24 deletions(-) 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 57f81f5b66..a56431e222 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 @@ public class HttpPostRequestEncoder implements ChunkedInput { 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); } - size = HttpPostBodyUtil.chunkSize - currentBuffer.readableBytes(); + if (chunk != null) { + // NextChunk from data + return chunk; + } + 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 @@ public class HttpPostRequestEncoder implements ChunkedInput { } 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); } 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 3a206f44ba..2d821273fa 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.ByteBuf; 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_LENGTH; 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 @@ public class HttpPostRequestEncoderTest { 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(); + } }