From 41e0ef2e9ac5b19744e75392543632b21da498c6 Mon Sep 17 00:00:00 2001
From: Trustin Lee <trustin@gmail.com>
Date: Fri, 28 Sep 2012 15:16:29 +0900
Subject: [PATCH] [#441] Provide a better way to handle decoder failures

* Update toString() of all HttpObject implementations
* HttpMessageDecoder does not raise an exception but sets decoderResult property of the decoded message.
* HttpMessageDecoder discards inbound traffic once decoding fails, by adding a new state called BAD_MESSAGE.
* Add a test case that tests this behavior.
---
 .../handler/codec/http/DefaultHttpChunk.java  |  20 +++
 .../codec/http/DefaultHttpChunkTrailer.java   |  34 +++++
 .../codec/http/DefaultHttpRequest.java        |   2 +
 .../codec/http/DefaultHttpResponse.java       |   2 +
 .../codec/http/HttpMessageDecoder.java        |  51 ++++++--
 .../codec/http/HttpRequestDecoder.java        |   5 +
 .../codec/http/HttpResponseDecoder.java       |   7 ++
 .../codec/rtsp/RtspRequestDecoder.java        |   5 +
 .../codec/rtsp/RtspResponseDecoder.java       |   7 ++
 .../codec/http/HttpInvalidMessageTest.java    | 117 ++++++++++++++++++
 10 files changed, 243 insertions(+), 7 deletions(-)
 create mode 100644 codec-http/src/test/java/io/netty/handler/codec/http/HttpInvalidMessageTest.java

diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpChunk.java b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpChunk.java
index f78c018d4d..9cf64e8711 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpChunk.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpChunk.java
@@ -51,4 +51,24 @@ public class DefaultHttpChunk extends DefaultHttpObject implements HttpChunk {
     public boolean isLast() {
         return last;
     }
+
+    @Override
+    public String toString() {
+        StringBuilder buf = new StringBuilder();
+        buf.append(getClass().getSimpleName());
+
+        final boolean last = isLast();
+        buf.append("(last: ");
+        buf.append(last);
+        if (!last) {
+            buf.append(", size: ");
+            buf.append(getContent().readableBytes());
+        }
+
+        buf.append(", decodeResult: ");
+        buf.append(getDecodeResult());
+        buf.append(')');
+
+        return buf.toString();
+    }
 }
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpChunkTrailer.java b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpChunkTrailer.java
index 65235d4b86..2f3c6bc591 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpChunkTrailer.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpChunkTrailer.java
@@ -17,6 +17,7 @@ package io.netty.handler.codec.http;
 
 import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
+import io.netty.util.internal.StringUtil;
 
 import java.util.List;
 import java.util.Map;
@@ -104,4 +105,37 @@ public class DefaultHttpChunkTrailer extends DefaultHttpObject implements HttpCh
     public void setContent(ByteBuf content) {
         throw new IllegalStateException("read-only");
     }
+
+    @Override
+    public String toString() {
+        StringBuilder buf = new StringBuilder();
+        buf.append(getClass().getSimpleName());
+
+        final boolean last = isLast();
+        buf.append("(last: ");
+        buf.append(last);
+        if (!last) {
+            buf.append(", size: ");
+            buf.append(getContent().readableBytes());
+        }
+
+        buf.append(", decodeResult: ");
+        buf.append(getDecodeResult());
+        buf.append(')');
+        buf.append(StringUtil.NEWLINE);
+        appendHeaders(buf);
+
+        // Remove the last newline.
+        buf.setLength(buf.length() - StringUtil.NEWLINE.length());
+        return buf.toString();
+    }
+
+    private void appendHeaders(StringBuilder buf) {
+        for (Map.Entry<String, String> e: getHeaders()) {
+            buf.append(e.getKey());
+            buf.append(": ");
+            buf.append(e.getValue());
+            buf.append(StringUtil.NEWLINE);
+        }
+    }
 }
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpRequest.java b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpRequest.java
index afadec21bd..2729b9e9d4 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpRequest.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpRequest.java
@@ -70,6 +70,8 @@ public class DefaultHttpRequest extends DefaultHttpMessage implements HttpReques
         buf.append(getClass().getSimpleName());
         buf.append("(transferEncoding: ");
         buf.append(getTransferEncoding());
+        buf.append(", decodeResult: ");
+        buf.append(getDecodeResult());
         buf.append(')');
         buf.append(StringUtil.NEWLINE);
         buf.append(getMethod().toString());
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpResponse.java b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpResponse.java
index 440b34fed0..c872e940e4 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpResponse.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/DefaultHttpResponse.java
@@ -54,6 +54,8 @@ public class DefaultHttpResponse extends DefaultHttpMessage implements HttpRespo
         buf.append(getClass().getSimpleName());
         buf.append("(transferEncoding: ");
         buf.append(getTransferEncoding());
+        buf.append(", decodeResult: ");
+        buf.append(getDecodeResult());
         buf.append(')');
         buf.append(StringUtil.NEWLINE);
         buf.append(getProtocolVersion().getText());
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpMessageDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpMessageDecoder.java
index 13ff6ad8f8..82a49f6ea0 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpMessageDecoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpMessageDecoder.java
@@ -19,6 +19,7 @@ import io.netty.buffer.ByteBuf;
 import io.netty.buffer.Unpooled;
 import io.netty.channel.ChannelHandlerContext;
 import io.netty.channel.ChannelPipeline;
+import io.netty.handler.codec.DecodeResult;
 import io.netty.handler.codec.ReplayingDecoder;
 import io.netty.handler.codec.TooLongFrameException;
 
@@ -125,7 +126,8 @@ public abstract class HttpMessageDecoder extends ReplayingDecoder<Object, HttpMe
         READ_CHUNKED_CONTENT,
         READ_CHUNKED_CONTENT_AS_CHUNKS,
         READ_CHUNK_DELIMITER,
-        READ_CHUNK_FOOTER
+        READ_CHUNK_FOOTER,
+        BAD_MESSAGE
     }
 
     /**
@@ -176,7 +178,7 @@ public abstract class HttpMessageDecoder extends ReplayingDecoder<Object, HttpMe
                 checkpoint();
             }
         }
-        case READ_INITIAL: {
+        case READ_INITIAL: try {
             String[] initialLine = splitInitialLine(readLine(buffer, maxInitialLineLength));
             if (initialLine.length < 3) {
                 // Invalid initial line - ignore.
@@ -186,8 +188,10 @@ public abstract class HttpMessageDecoder extends ReplayingDecoder<Object, HttpMe
 
             message = createMessage(initialLine);
             checkpoint(State.READ_HEADER);
+        } catch (Exception e) {
+            return invalidMessage(e);
         }
-        case READ_HEADER: {
+        case READ_HEADER: try {
             State nextState = readHeaders(buffer);
             checkpoint(nextState);
             if (nextState == State.READ_CHUNK_SIZE) {
@@ -195,7 +199,7 @@ public abstract class HttpMessageDecoder extends ReplayingDecoder<Object, HttpMe
                 return message;
             } else if (nextState == State.SKIP_CONTROL_CHARS) {
                 // No content is expected.
-                return message;
+                return reset();
             } else {
                 long contentLength = HttpHeaders.getContentLength(message, -1);
                 if (contentLength == 0 || contentLength == -1 && isDecodingRequest()) {
@@ -229,6 +233,8 @@ public abstract class HttpMessageDecoder extends ReplayingDecoder<Object, HttpMe
             }
             // We return null here, this forces decode to be called again where we will decode the content
             return null;
+        } catch (Exception e) {
+            return invalidMessage(e);
         }
         case READ_VARIABLE_LENGTH_CONTENT: {
             int toRead = actualReadableBytes();
@@ -238,7 +244,7 @@ public abstract class HttpMessageDecoder extends ReplayingDecoder<Object, HttpMe
 
             if (message.getTransferEncoding() != HttpTransferEncoding.STREAMED) {
                 message.setTransferEncoding(HttpTransferEncoding.STREAMED);
-                return new Object[] {message, new DefaultHttpChunk(buffer.readBytes(toRead))};
+                return new Object[] { message, new DefaultHttpChunk(buffer.readBytes(toRead))};
             } else {
                 return new DefaultHttpChunk(buffer.readBytes(toRead));
             }
@@ -308,7 +314,7 @@ public abstract class HttpMessageDecoder extends ReplayingDecoder<Object, HttpMe
          * everything else after this point takes care of reading chunked content. basically, read chunk size,
          * read chunk, read and ignore the CRLF and repeat until 0
          */
-        case READ_CHUNK_SIZE: {
+        case READ_CHUNK_SIZE: try {
             String line = readLine(buffer, maxInitialLineLength);
             int chunkSize = getChunkSize(line);
             this.chunkSize = chunkSize;
@@ -321,6 +327,8 @@ public abstract class HttpMessageDecoder extends ReplayingDecoder<Object, HttpMe
             } else {
                 checkpoint(State.READ_CHUNKED_CONTENT);
             }
+        } catch (Exception e) {
+            return invalidChunk(e);
         }
         case READ_CHUNKED_CONTENT: {
             assert chunkSize <= Integer.MAX_VALUE;
@@ -378,10 +386,12 @@ public abstract class HttpMessageDecoder extends ReplayingDecoder<Object, HttpMe
                 } else if (next == HttpConstants.LF) {
                     checkpoint(State.READ_CHUNK_SIZE);
                     return null;
+                } else {
+                    checkpoint();
                 }
             }
         }
-        case READ_CHUNK_FOOTER: {
+        case READ_CHUNK_FOOTER: try {
             HttpChunkTrailer trailer = readTrailingHeaders(buffer);
             if (maxChunkSize == 0) {
                 // Chunked encoding disabled.
@@ -391,6 +401,13 @@ public abstract class HttpMessageDecoder extends ReplayingDecoder<Object, HttpMe
                 // The last chunk, which is empty
                 return trailer;
             }
+        } catch (Exception e) {
+            return invalidChunk(e);
+        }
+        case BAD_MESSAGE: {
+            // Keep discarding until disconnection.
+            buffer.skipBytes(actualReadableBytes());
+            return null;
         }
         default: {
             throw new Error("Shouldn't reach here.");
@@ -439,6 +456,24 @@ public abstract class HttpMessageDecoder extends ReplayingDecoder<Object, HttpMe
         return message;
     }
 
+    private HttpMessage invalidMessage(Exception cause) {
+        checkpoint(State.BAD_MESSAGE);
+        if (message != null) {
+            message.setDecodeResult(DecodeResult.partialFailure(cause));
+        } else {
+            message = createInvalidMessage();
+            message.setDecodeResult(DecodeResult.failure(cause));
+        }
+        return message;
+    }
+
+    private HttpChunk invalidChunk(Exception cause) {
+        checkpoint(State.BAD_MESSAGE);
+        HttpChunk chunk = new DefaultHttpChunk(Unpooled.EMPTY_BUFFER);
+        chunk.setDecodeResult(DecodeResult.failure(cause));
+        return chunk;
+    }
+
     private static void skipControlCharacters(ByteBuf buffer) {
         for (;;) {
             char c = (char) buffer.readUnsignedByte();
@@ -621,6 +656,8 @@ public abstract class HttpMessageDecoder extends ReplayingDecoder<Object, HttpMe
 
     protected abstract boolean isDecodingRequest();
     protected abstract HttpMessage createMessage(String[] initialLine) throws Exception;
+    protected abstract HttpMessage createInvalidMessage();
+
 
     private static int getChunkSize(String hex) {
         hex = hex.trim();
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpRequestDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpRequestDecoder.java
index 650e34dfcc..8f8c5962ab 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpRequestDecoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpRequestDecoder.java
@@ -76,6 +76,11 @@ public class HttpRequestDecoder extends HttpMessageDecoder {
                 HttpVersion.valueOf(initialLine[2]), HttpMethod.valueOf(initialLine[0]), initialLine[1]);
     }
 
+    @Override
+    protected HttpMessage createInvalidMessage() {
+        return new DefaultHttpRequest(HttpVersion.HTTP_1_0, HttpMethod.GET, "/bad-request");
+    }
+
     @Override
     protected boolean isDecodingRequest() {
         return true;
diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpResponseDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpResponseDecoder.java
index 9393217119..bf78b0e1bb 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpResponseDecoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpResponseDecoder.java
@@ -83,6 +83,8 @@ import io.netty.handler.codec.TooLongFrameException;
  */
 public class HttpResponseDecoder extends HttpMessageDecoder {
 
+    private static final HttpResponseStatus UNKNOWN_STATUS = new HttpResponseStatus(999, "Unknown");
+
     /**
      * Creates a new instance with the default
      * {@code maxInitialLineLength (4096}}, {@code maxHeaderSize (8192)}, and
@@ -106,6 +108,11 @@ public class HttpResponseDecoder extends HttpMessageDecoder {
                 new HttpResponseStatus(Integer.valueOf(initialLine[1]), initialLine[2]));
     }
 
+    @Override
+    protected HttpMessage createInvalidMessage() {
+        return new DefaultHttpResponse(HttpVersion.HTTP_1_0, UNKNOWN_STATUS);
+    }
+
     @Override
     protected boolean isDecodingRequest() {
         return false;
diff --git a/codec-http/src/main/java/io/netty/handler/codec/rtsp/RtspRequestDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/rtsp/RtspRequestDecoder.java
index 1c1b2d723c..aa36d73e20 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/rtsp/RtspRequestDecoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/rtsp/RtspRequestDecoder.java
@@ -71,6 +71,11 @@ public class RtspRequestDecoder extends RtspMessageDecoder {
                 RtspMethods.valueOf(initialLine[0]), initialLine[1]);
     }
 
+    @Override
+    protected HttpMessage createInvalidMessage() {
+        return new DefaultHttpRequest(RtspVersions.RTSP_1_0, RtspMethods.OPTIONS, "/bad-request");
+    }
+
     @Override
     protected boolean isDecodingRequest() {
         return true;
diff --git a/codec-http/src/main/java/io/netty/handler/codec/rtsp/RtspResponseDecoder.java b/codec-http/src/main/java/io/netty/handler/codec/rtsp/RtspResponseDecoder.java
index 8369557be6..ab5ead22b4 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/rtsp/RtspResponseDecoder.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/rtsp/RtspResponseDecoder.java
@@ -51,6 +51,8 @@ import io.netty.handler.codec.http.HttpResponseStatus;
  */
 public class RtspResponseDecoder extends RtspMessageDecoder {
 
+    private static final HttpResponseStatus UNKNOWN_STATUS = new HttpResponseStatus(999, "Unknown");
+
     /**
      * Creates a new instance with the default
      * {@code maxInitialLineLength (4096}}, {@code maxHeaderSize (8192)}, and
@@ -74,6 +76,11 @@ public class RtspResponseDecoder extends RtspMessageDecoder {
                 new HttpResponseStatus(Integer.valueOf(initialLine[1]), initialLine[2]));
     }
 
+    @Override
+    protected HttpMessage createInvalidMessage() {
+        return new DefaultHttpResponse(RtspVersions.RTSP_1_0, UNKNOWN_STATUS);
+    }
+
     @Override
     protected boolean isDecodingRequest() {
         return false;
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpInvalidMessageTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpInvalidMessageTest.java
new file mode 100644
index 0000000000..33b207ef7e
--- /dev/null
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpInvalidMessageTest.java
@@ -0,0 +1,117 @@
+/*
+ * Copyright 2012 The Netty Project
+ *
+ * The Netty Project licenses this file to you under the Apache License,
+ * version 2.0 (the "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at:
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+ * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+ * License for the specific language governing permissions and limitations
+ * under the License.
+ */
+package io.netty.handler.codec.http;
+
+import io.netty.buffer.ByteBuf;
+import io.netty.buffer.Unpooled;
+import io.netty.channel.embedded.EmbeddedByteChannel;
+import io.netty.handler.codec.DecodeResult;
+import io.netty.util.CharsetUtil;
+
+import java.util.Random;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+public class HttpInvalidMessageTest {
+
+    private final Random rnd = new Random();
+
+    @Test
+    public void testRequestWithBadInitialLine() throws Exception {
+        EmbeddedByteChannel ch = new EmbeddedByteChannel(new HttpRequestDecoder());
+        ch.writeInbound(Unpooled.copiedBuffer("GET / HTTP/1.0 with extra\r\n", CharsetUtil.UTF_8));
+        HttpRequest req = (HttpRequest) ch.readInbound();
+        DecodeResult dr = req.getDecodeResult();
+        Assert.assertFalse(dr.isSuccess());
+        Assert.assertFalse(dr.isPartial());
+        ensureInboundTrafficDiscarded(ch);
+    }
+
+    @Test
+    public void testRequestWithBadHeader() throws Exception {
+        EmbeddedByteChannel ch = new EmbeddedByteChannel(new HttpRequestDecoder());
+        ch.writeInbound(Unpooled.copiedBuffer("GET /maybe-something HTTP/1.0\r\n", CharsetUtil.UTF_8));
+        ch.writeInbound(Unpooled.copiedBuffer("Good_Name: Good Value\r\n", CharsetUtil.UTF_8));
+        ch.writeInbound(Unpooled.copiedBuffer("Bad=Name: Bad Value\r\n", CharsetUtil.UTF_8));
+        ch.writeInbound(Unpooled.copiedBuffer("\r\n", CharsetUtil.UTF_8));
+        HttpRequest req = (HttpRequest) ch.readInbound();
+        DecodeResult dr = req.getDecodeResult();
+        Assert.assertFalse(dr.isSuccess());
+        Assert.assertTrue(dr.isPartial());
+        Assert.assertEquals("Good Value", req.getHeader("Good_Name"));
+        Assert.assertEquals("/maybe-something", req.getUri());
+        ensureInboundTrafficDiscarded(ch);
+    }
+
+    @Test
+    public void testResponseWithBadInitialLine() throws Exception {
+        EmbeddedByteChannel ch = new EmbeddedByteChannel(new HttpResponseDecoder());
+        ch.writeInbound(Unpooled.copiedBuffer("HTTP/1.0 BAD_CODE Bad Server\r\n", CharsetUtil.UTF_8));
+        HttpResponse res = (HttpResponse) ch.readInbound();
+        DecodeResult dr = res.getDecodeResult();
+        Assert.assertFalse(dr.isSuccess());
+        Assert.assertFalse(dr.isPartial());
+        ensureInboundTrafficDiscarded(ch);
+    }
+
+    @Test
+    public void testResponseWithBadHeader() throws Exception {
+        EmbeddedByteChannel ch = new EmbeddedByteChannel(new HttpResponseDecoder());
+        ch.writeInbound(Unpooled.copiedBuffer("HTTP/1.0 200 Maybe OK\r\n", CharsetUtil.UTF_8));
+        ch.writeInbound(Unpooled.copiedBuffer("Good_Name: Good Value\r\n", CharsetUtil.UTF_8));
+        ch.writeInbound(Unpooled.copiedBuffer("Bad=Name: Bad Value\r\n", CharsetUtil.UTF_8));
+        ch.writeInbound(Unpooled.copiedBuffer("\r\n", CharsetUtil.UTF_8));
+        HttpResponse res = (HttpResponse) ch.readInbound();
+        DecodeResult dr = res.getDecodeResult();
+        Assert.assertFalse(dr.isSuccess());
+        Assert.assertTrue(dr.isPartial());
+        Assert.assertEquals("Maybe OK", res.getStatus().getReasonPhrase());
+        Assert.assertEquals("Good Value", res.getHeader("Good_Name"));
+        ensureInboundTrafficDiscarded(ch);
+    }
+
+    @Test
+    public void testBadChunk() throws Exception {
+        EmbeddedByteChannel ch = new EmbeddedByteChannel(new HttpRequestDecoder());
+        ch.writeInbound(Unpooled.copiedBuffer("GET / HTTP/1.0\r\n", CharsetUtil.UTF_8));
+        ch.writeInbound(Unpooled.copiedBuffer("Transfer-Encoding: chunked\r\n\r\n", CharsetUtil.UTF_8));
+        ch.writeInbound(Unpooled.copiedBuffer("BAD_LENGTH\r\n", CharsetUtil.UTF_8));
+
+        HttpRequest req = (HttpRequest) ch.readInbound();
+        Assert.assertTrue(req.getDecodeResult().isSuccess());
+
+        HttpChunk chunk = (HttpChunk) ch.readInbound();
+        DecodeResult dr = chunk.getDecodeResult();
+        Assert.assertFalse(dr.isSuccess());
+        Assert.assertFalse(dr.isPartial());
+        ensureInboundTrafficDiscarded(ch);
+    }
+
+    private void ensureInboundTrafficDiscarded(EmbeddedByteChannel ch) {
+        // Generate a lot of random traffic to ensure that it's discarded silently.
+        byte[] data = new byte[1048576];
+        rnd.nextBytes(data);
+
+        ByteBuf buf = Unpooled.wrappedBuffer(data);
+        for (int i = 0; i < 4096; i ++) {
+            buf.setIndex(0, data.length);
+            ch.writeInbound(buf);
+            ch.checkException();
+            Assert.assertNull(ch.readInbound());
+        }
+    }
+}