From a0a8f1032b55d4ba5487a22e55a08638cefbdef9 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Sun, 8 Jun 2014 11:50:58 +0200 Subject: [PATCH] [#2544] Correctly parse Multipart-mixed POST HTTP request in case of entity ends with odd number of 0x0D. Port of @fredericBregier 's work. 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. --- .../multipart/HttpPostRequestDecoder.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/HttpPostRequestDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoder.java index 3820be7bc1..cbd2ac573b 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoder.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/multipart/HttpPostRequestDecoder.java @@ -1287,9 +1287,15 @@ public class HttpPostRequestDecoder { 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) { + // Skip + undecodedChunk.skipBytes(1); return line.toString(charset); + } else { + // Write CR (not followed by LF) + line.writeByte(HttpConstants.CR); } } else if (nextByte == HttpConstants.LF) { return line.toString(charset); @@ -1332,6 +1338,10 @@ public class HttpPostRequestDecoder { 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); @@ -1494,6 +1504,11 @@ public class HttpPostRequestDecoder { 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 @@ -1522,6 +1537,11 @@ public class HttpPostRequestDecoder { 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 @@ -1826,7 +1846,13 @@ public class HttpPostRequestDecoder { 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; @@ -1845,7 +1871,13 @@ public class HttpPostRequestDecoder { 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; @@ -1930,6 +1962,10 @@ public class HttpPostRequestDecoder { newLine = true; index = 0; lastrealpos = sao.pos - 2; + } else { + // Unread last nextByte + sao.pos--; + lastrealpos = sao.pos; } } } else if (nextByte == HttpConstants.LF) { @@ -1949,6 +1985,10 @@ public class HttpPostRequestDecoder { 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 {