From 91d3920aa298ea536be7b196f16b32b6ddd27f8d Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Thu, 31 Jan 2019 20:27:47 +0100 Subject: [PATCH] =?UTF-8?q?HttpObjectDecoder=20ignores=20HTTP=20trailer=20?= =?UTF-8?q?header=20when=20empty=20line=20is=20rece=E2=80=A6=20(#8799)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * HttpObjectDecoder ignores HTTP trailer header when empty line is received in seperate ByteBuf Motivation: When the empty line that termines the trailers was sent in a seperate ByteBuf we did ignore the previous parsed trailers and just returned none. Modifications: - Correct respect previous parsed trailers. - Add unit test. Result: Fixes https://github.com/netty/netty/issues/8736 --- .../handler/codec/http/HttpObjectDecoder.java | 83 ++++++++++--------- .../codec/http/HttpResponseDecoderTest.java | 29 +++++++ 2 files changed, 71 insertions(+), 41 deletions(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java index af1d642a03..f8220cdb04 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectDecoder.java @@ -640,49 +640,50 @@ public abstract class HttpObjectDecoder extends ByteToMessageDecoder { if (line == null) { return null; } - CharSequence lastHeader = null; - if (line.length() > 0) { - LastHttpContent trailer = this.trailer; - if (trailer == null) { - trailer = this.trailer = new DefaultLastHttpContent(Unpooled.EMPTY_BUFFER, validateHeaders); - } - do { - char firstChar = line.charAt(0); - if (lastHeader != null && (firstChar == ' ' || firstChar == '\t')) { - List current = trailer.trailingHeaders().getAll(lastHeader); - if (!current.isEmpty()) { - int lastPos = current.size() - 1; - //please do not make one line from below code - //as it breaks +XX:OptimizeStringConcat optimization - String lineTrimmed = line.toString().trim(); - String currentLastPos = current.get(lastPos); - current.set(lastPos, currentLastPos + lineTrimmed); - } - } else { - splitHeader(line); - CharSequence headerName = name; - if (!HttpHeaderNames.CONTENT_LENGTH.contentEqualsIgnoreCase(headerName) && - !HttpHeaderNames.TRANSFER_ENCODING.contentEqualsIgnoreCase(headerName) && - !HttpHeaderNames.TRAILER.contentEqualsIgnoreCase(headerName)) { - trailer.trailingHeaders().add(headerName, value); - } - lastHeader = name; - // reset name and value fields - name = null; - value = null; - } - - line = headerParser.parse(buffer); - if (line == null) { - return null; - } - } while (line.length() > 0); - - this.trailer = null; - return trailer; + LastHttpContent trailer = this.trailer; + if (line.length() == 0 && trailer == null) { + // We have received the empty line which signals the trailer is complete and did not parse any trailers + // before. Just return an empty last content to reduce allocations. + return LastHttpContent.EMPTY_LAST_CONTENT; } - return LastHttpContent.EMPTY_LAST_CONTENT; + CharSequence lastHeader = null; + if (trailer == null) { + trailer = this.trailer = new DefaultLastHttpContent(Unpooled.EMPTY_BUFFER, validateHeaders); + } + while (line.length() > 0) { + char firstChar = line.charAt(0); + if (lastHeader != null && (firstChar == ' ' || firstChar == '\t')) { + List current = trailer.trailingHeaders().getAll(lastHeader); + if (!current.isEmpty()) { + int lastPos = current.size() - 1; + //please do not make one line from below code + //as it breaks +XX:OptimizeStringConcat optimization + String lineTrimmed = line.toString().trim(); + String currentLastPos = current.get(lastPos); + current.set(lastPos, currentLastPos + lineTrimmed); + } + } else { + splitHeader(line); + CharSequence headerName = name; + if (!HttpHeaderNames.CONTENT_LENGTH.contentEqualsIgnoreCase(headerName) && + !HttpHeaderNames.TRANSFER_ENCODING.contentEqualsIgnoreCase(headerName) && + !HttpHeaderNames.TRAILER.contentEqualsIgnoreCase(headerName)) { + trailer.trailingHeaders().add(headerName, value); + } + lastHeader = name; + // reset name and value fields + name = null; + value = null; + } + line = headerParser.parse(buffer); + if (line == null) { + return null; + } + } + + this.trailer = null; + return trailer; } protected abstract boolean isDecodingRequest(); diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpResponseDecoderTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpResponseDecoderTest.java index 017dbd5ff9..f062da2bf1 100644 --- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpResponseDecoderTest.java +++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpResponseDecoderTest.java @@ -683,4 +683,33 @@ public class HttpResponseDecoderTest { assertThat(message.decoderResult().cause(), instanceOf(PrematureChannelClosureException.class)); assertNull(channel.readInbound()); } + + @Test + public void testTrailerWithEmptyLineInSeparateBuffer() { + HttpResponseDecoder decoder = new HttpResponseDecoder(); + EmbeddedChannel channel = new EmbeddedChannel(decoder); + + String headers = "HTTP/1.1 200 OK\r\n" + + "Transfer-Encoding: chunked\r\n" + + "Trailer: My-Trailer\r\n"; + assertFalse(channel.writeInbound(Unpooled.copiedBuffer(headers.getBytes(CharsetUtil.US_ASCII)))); + assertTrue(channel.writeInbound(Unpooled.copiedBuffer("\r\n".getBytes(CharsetUtil.US_ASCII)))); + + assertTrue(channel.writeInbound(Unpooled.copiedBuffer("0\r\n", CharsetUtil.US_ASCII))); + assertTrue(channel.writeInbound(Unpooled.copiedBuffer("My-Trailer: 42\r\n", CharsetUtil.US_ASCII))); + assertTrue(channel.writeInbound(Unpooled.copiedBuffer("\r\n", CharsetUtil.US_ASCII))); + + HttpResponse response = channel.readInbound(); + assertEquals(2, response.headers().size()); + assertEquals("chunked", response.headers().get(HttpHeaderNames.TRANSFER_ENCODING)); + assertEquals("My-Trailer", response.headers().get(HttpHeaderNames.TRAILER)); + + LastHttpContent lastContent = channel.readInbound(); + assertEquals(1, lastContent.trailingHeaders().size()); + assertEquals("42", lastContent.trailingHeaders().get("My-Trailer")); + assertEquals(0, lastContent.content().readableBytes()); + lastContent.release(); + + assertFalse(channel.finish()); + } }