From f402350d76e764ee0667b1d02563abe53df7eb6a Mon Sep 17 00:00:00 2001 From: Frederic Bregier Date: Sat, 7 Jun 2014 12:13:32 +0200 Subject: [PATCH] [#2544] Correctly parse Multipart-mixed POST HTTP request in case of entity ends with odd number of 0x0D Motivation: When an attribute is ending with an odd number of CR (0x0D), the decoder add an extra CR in the decoded attribute and should not. Modifications: Each time a CR is detected, the next byte was tested to be LF or not. If not, in a number of places, the CR byte was lost while it should not be. When a CR is detected, if the next byte is not LF, the CR byte should be saved as the position point to the next byte (not LF). When a CR is detected, if there is not yet other available bytes, the position is reset to the position of CR (since a LF could follow). A new Junit test case is added, using DECODER and variable number of CR in the final attribute (testMultipartCodecWithCRasEndOfAttribute). Result: The attribute is now correctly decoded with the right number of CR ending bytes. --- .../HttpPostMultipartRequestDecoder.java | 42 ++++++++++++++++- .../multipart/HttpPostRequestDecoderTest.java | 47 +++++++++++++++++++ 2 files changed, 88 insertions(+), 1 deletion(-) 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 4c5df58d28..28f41b5887 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 @@ -955,9 +955,15 @@ public class HttpPostMultipartRequestDecoder implements InterfaceHttpPostRequest while (undecodedChunk.isReadable()) { byte nextByte = undecodedChunk.readByte(); if (nextByte == HttpConstants.CR) { - nextByte = undecodedChunk.readByte(); + // check but do not changed readerIndex + nextByte = undecodedChunk.getByte(undecodedChunk.readerIndex()); if (nextByte == HttpConstants.LF) { + // force read + undecodedChunk.readByte(); return line.toString(charset); + } else { + // Write CR (not followed by LF) + line.writeByte(HttpConstants.CR); } } else if (nextByte == HttpConstants.LF) { return line.toString(charset); @@ -1000,6 +1006,10 @@ public class HttpPostMultipartRequestDecoder implements InterfaceHttpPostRequest if (nextByte == HttpConstants.LF) { sao.setReadPosition(0); return line.toString(charset); + } else { + // Write CR (not followed by LF) + sao.pos--; + line.writeByte(HttpConstants.CR); } } else { line.writeByte(nextByte); @@ -1162,6 +1172,11 @@ public class HttpPostMultipartRequestDecoder implements InterfaceHttpPostRequest if (nextByte == HttpConstants.LF) { sao.setReadPosition(0); return sb.toString(); + } else { + // error CR without LF + // delimiter not found so break here ! + undecodedChunk.readerIndex(readerIndex); + throw new NotEnoughDataDecoderException(); } } else { // error since CR must be followed by LF @@ -1190,6 +1205,11 @@ public class HttpPostMultipartRequestDecoder implements InterfaceHttpPostRequest if (nextByte == HttpConstants.LF) { sao.setReadPosition(0); return sb.toString(); + } else { + // error CR without LF + // delimiter not found so break here ! + undecodedChunk.readerIndex(readerIndex); + throw new NotEnoughDataDecoderException(); } } else { // error CR without LF @@ -1491,7 +1511,13 @@ public class HttpPostMultipartRequestDecoder implements InterfaceHttpPostRequest newLine = true; index = 0; lastPosition = undecodedChunk.readerIndex() - 2; + } else { + // Unread second nextByte + lastPosition = undecodedChunk.readerIndex() - 1; + undecodedChunk.readerIndex(lastPosition); } + } else { + lastPosition = undecodedChunk.readerIndex() - 1; } } else if (nextByte == HttpConstants.LF) { newLine = true; @@ -1510,7 +1536,13 @@ public class HttpPostMultipartRequestDecoder implements InterfaceHttpPostRequest newLine = true; index = 0; lastPosition = undecodedChunk.readerIndex() - 2; + } else { + // Unread second nextByte + lastPosition = undecodedChunk.readerIndex() - 1; + undecodedChunk.readerIndex(lastPosition); } + } else { + lastPosition = undecodedChunk.readerIndex() - 1; } } else if (nextByte == HttpConstants.LF) { newLine = true; @@ -1595,6 +1627,10 @@ public class HttpPostMultipartRequestDecoder implements InterfaceHttpPostRequest newLine = true; index = 0; lastrealpos = sao.pos - 2; + } else { + // Unread last nextByte + sao.pos--; + lastrealpos = sao.pos; } } } else if (nextByte == HttpConstants.LF) { @@ -1614,6 +1650,10 @@ public class HttpPostMultipartRequestDecoder implements InterfaceHttpPostRequest newLine = true; index = 0; lastrealpos = sao.pos - 2; + } else { + // Unread last nextByte + sao.pos--; + lastrealpos = sao.pos; } } } else if (nextByte == HttpConstants.LF) { 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 ea9dad54ec..f5fe26d907 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 @@ -129,6 +129,53 @@ public class HttpPostRequestDecoderTest { decoder.destroy(); } + // See https://github.com/netty/netty/issues/2544 + @Test + public void testMultipartCodecWithCRasEndOfAttribute() throws Exception { + final String boundary = "dLV9Wyq26L_-JQxk6ferf-RT153LhOO"; + + // Force to use memory-based data. + final DefaultHttpDataFactory inMemoryFactory = new DefaultHttpDataFactory(false); + // Build test case + String extradata = "aaaa"; + String [] datas = new String[5]; + for (int i = 0; i < 4; i++) { + datas[i] = extradata; + for (int j = 0; j < i ; j++) { + datas[i] += '\r'; + } + } + + for (int i = 0; i < 4; i++) { + final DefaultFullHttpRequest req = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, + "http://localhost"); + req.setDecoderResult(DecoderResult.SUCCESS); + req.headers().add(HttpHeaders.Names.CONTENT_TYPE, "multipart/form-data; boundary=" + boundary); + req.headers().add(HttpHeaders.Names.TRANSFER_ENCODING, HttpHeaders.Values.CHUNKED); + final String body = + "--" + boundary + "\r\n" + + "Content-Disposition: form-data; name=\"file" + i + "\"\r\n" + + "Content-Type: image/gif\r\n" + + "\r\n" + + datas[i] + "\r\n" + + "--" + boundary + "--\r\n"; + + req.content().writeBytes(body.getBytes(CharsetUtil.UTF_8)); + // Create decoder instance to test. + final HttpPostRequestDecoder decoder = new HttpPostRequestDecoder(inMemoryFactory, req); + assertFalse(decoder.getBodyHttpDatas().isEmpty()); + // Check correctness: data size + InterfaceHttpData httpdata = decoder.getBodyHttpData("file" + i); + assertNotNull(httpdata); + Attribute attribute = (Attribute) httpdata; + byte []datar = attribute.get(); + assertNotNull(datar); + assertEquals(datas[i].getBytes(CharsetUtil.UTF_8).length, datar.length); + + decoder.destroy(); + } + } + // See https://github.com/netty/netty/issues/1848 @Test public void testNoZeroOut() throws Exception {