From 96f92929ab57ae9340400c60d92280e67cff2429 Mon Sep 17 00:00:00 2001
From: Norman Maurer <norman_maurer@apple.com>
Date: Fri, 16 Aug 2019 08:17:18 +0200
Subject: [PATCH] Fix possible NPE when using HttpClientCodec (#9465)

Motivation:

It was possible to produce a NPE when we for examples received more responses as requests as we did not check if the queue did not contain a method before trying to compare method names.

Modifications:

- Add extra null check
- Add unit tet

Result:

Fixes https://github.com/netty/netty/issues/9459
---
 .../handler/codec/http/HttpClientCodec.java   | 77 ++++++++++---------
 .../codec/http/HttpClientCodecTest.java       | 24 ++++++
 2 files changed, 64 insertions(+), 37 deletions(-)

diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java
index 8eff1d07d7..433f3dde55 100644
--- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java
+++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpClientCodec.java
@@ -160,7 +160,7 @@ public final class HttpClientCodec extends CombinedChannelDuplexHandler<HttpResp
                 return;
             }
 
-            if (msg instanceof HttpRequest && !done) {
+            if (msg instanceof HttpRequest) {
                 queue.offer(((HttpRequest) msg).method());
             }
 
@@ -233,46 +233,49 @@ public final class HttpClientCodec extends CombinedChannelDuplexHandler<HttpResp
             // current response.
             HttpMethod method = queue.poll();
 
-            char firstChar = method.name().charAt(0);
-            switch (firstChar) {
-            case 'H':
-                // According to 4.3, RFC2616:
-                // All responses to the HEAD request method MUST NOT include a
-                // message-body, even though the presence of entity-header fields
-                // might lead one to believe they do.
-                if (HttpMethod.HEAD.equals(method)) {
-                    return true;
+            // If the remote peer did for example send multiple responses for one request (which is not allowed per
+            // spec but may still be possible) method will be null so guard against it.
+            if (method != null) {
+                char firstChar = method.name().charAt(0);
+                switch (firstChar) {
+                    case 'H':
+                        // According to 4.3, RFC2616:
+                        // All responses to the HEAD request method MUST NOT include a
+                        // message-body, even though the presence of entity-header fields
+                        // might lead one to believe they do.
+                        if (HttpMethod.HEAD.equals(method)) {
+                            return true;
 
-                    // The following code was inserted to work around the servers
-                    // that behave incorrectly.  It has been commented out
-                    // because it does not work with well behaving servers.
-                    // Please note, even if the 'Transfer-Encoding: chunked'
-                    // header exists in the HEAD response, the response should
-                    // have absolutely no content.
-                    //
-                    //// Interesting edge case:
-                    //// Some poorly implemented servers will send a zero-byte
-                    //// chunk if Transfer-Encoding of the response is 'chunked'.
-                    ////
-                    //// return !msg.isChunked();
-                }
-                break;
-            case 'C':
-                // Successful CONNECT request results in a response with empty body.
-                if (statusCode == 200) {
-                    if (HttpMethod.CONNECT.equals(method)) {
-                        // Proxy connection established - Parse HTTP only if configured by parseHttpAfterConnectRequest,
-                        // else pass through.
-                        if (!parseHttpAfterConnectRequest) {
-                            done = true;
-                            queue.clear();
+                            // The following code was inserted to work around the servers
+                            // that behave incorrectly.  It has been commented out
+                            // because it does not work with well behaving servers.
+                            // Please note, even if the 'Transfer-Encoding: chunked'
+                            // header exists in the HEAD response, the response should
+                            // have absolutely no content.
+                            //
+                            //// Interesting edge case:
+                            //// Some poorly implemented servers will send a zero-byte
+                            //// chunk if Transfer-Encoding of the response is 'chunked'.
+                            ////
+                            //// return !msg.isChunked();
                         }
-                        return true;
-                    }
+                        break;
+                    case 'C':
+                        // Successful CONNECT request results in a response with empty body.
+                        if (statusCode == 200) {
+                            if (HttpMethod.CONNECT.equals(method)) {
+                                // Proxy connection established - Parse HTTP only if configured by
+                                // parseHttpAfterConnectRequest, else pass through.
+                                if (!parseHttpAfterConnectRequest) {
+                                    done = true;
+                                    queue.clear();
+                                }
+                                return true;
+                            }
+                        }
+                        break;
                 }
-                break;
             }
-
             return super.isContentAlwaysEmpty(msg);
         }
 
diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpClientCodecTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpClientCodecTest.java
index 184df7bf61..cb16a05bfe 100644
--- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpClientCodecTest.java
+++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpClientCodecTest.java
@@ -326,4 +326,28 @@ public class HttpClientCodecTest {
 
         assertThat(ch.readInbound(), is(nullValue()));
     }
+
+    @Test
+    public void testMultipleResponses() {
+        String response = "HTTP/1.1 200 OK\r\n" +
+                "Content-Length: 0\r\n\r\n";
+
+        HttpClientCodec codec = new HttpClientCodec(4096, 8192, 8192, true);
+        EmbeddedChannel ch = new EmbeddedChannel(codec, new HttpObjectAggregator(1024));
+
+        HttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "http://localhost/");
+        assertTrue(ch.writeOutbound(request));
+
+        assertTrue(ch.writeInbound(Unpooled.copiedBuffer(response, CharsetUtil.UTF_8)));
+        assertTrue(ch.writeInbound(Unpooled.copiedBuffer(response, CharsetUtil.UTF_8)));
+        FullHttpResponse resp = ch.readInbound();
+        assertTrue(resp.decoderResult().isSuccess());
+        resp.release();
+
+        resp = ch.readInbound();
+        assertTrue(resp.decoderResult().isSuccess());
+        resp.release();
+        assertTrue(ch.finishAndReleaseAll());
+    }
+
 }