Fixes #7348 - Slow CONNECT request causes NPE (#7349) (#7352)

* Fixes #7348 - Slow CONNECT request causes NPE (#7349) Added NPE guard in `HttpReceiverOverHTTP.onUpgradeFrom()`. Expanded logic in `HttpReceiverOverHTTP.parse()` to return true in case of CONNECT + 200. Fixed `ProxyConnection.toConnectionString()` to avoid NPEs. Fixed `HttpClientTest.testCONNECTWithHTTP10()` logic after changes to fix this issue. Now a tunneled connection is not put back into the connection pool, and if applications explicitly want to use it, they must re-enable fill interest, similarly to what should be done after upgrade+101. Signed-off-by: Simone Bordet <[email protected]> (cherry picked from commit 5eb7b70) Signed-off-by: Simone Bordet <[email protected]>
jetty · Jan 6, 2022 · 3042f2b · 3042f2b
1 parent 13956b2
commit 3042f2b
Show file tree

Hide file tree

Showing 10 changed files with 110 additions and 17 deletions.
diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpChannelOverHTTP.java b/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpChannelOverHTTP.java
@@ -25,6 +25,7 @@
 import org.eclipse.jetty.http.HttpMethod;
 import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.http.HttpVersion;
+import org.eclipse.jetty.http.MetaData;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -95,6 +96,7 @@ public void exchangeTerminated(HttpExchange exchange, Result result)
     {
         super.exchangeTerminated(exchange, result);
 
+        String method = exchange.getRequest().getMethod();
         Response response = result.getResponse();
         HttpFields responseHeaders = response.getHeaders();
 
@@ -113,7 +115,7 @@ else if (sender.isShutdown() && response.getStatus() != HttpStatus.SWITCHING_PRO
                 // HTTP 1.0 must close the connection unless it has
                 // an explicit keep alive or it's a CONNECT method.
                 boolean keepAlive = responseHeaders.contains(HttpHeader.CONNECTION, HttpHeaderValue.KEEP_ALIVE.asString());
-                boolean connect = HttpMethod.CONNECT.is(exchange.getRequest().getMethod());
+                boolean connect = HttpMethod.CONNECT.is(method);
                 if (!keepAlive && !connect)
                     closeReason = "http/1.0";
             }
@@ -133,7 +135,8 @@ else if (sender.isShutdown() && response.getStatus() != HttpStatus.SWITCHING_PRO
         }
         else
         {
-            if (response.getStatus() == HttpStatus.SWITCHING_PROTOCOLS_101)
+            int status = response.getStatus();
+            if (status == HttpStatus.SWITCHING_PROTOCOLS_101 || isTunnel(method, status))
                 connection.remove();
             else
                 release();
@@ -150,6 +153,11 @@ protected long getMessagesOut()
         return outMessages.longValue();
     }
 
+    boolean isTunnel(String method, int status)
+    {
+        return MetaData.isTunnel(method, status);
+    }
+
     @Override
     public String toString()
     {

diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpReceiverOverHTTP.java b/jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpReceiverOverHTTP.java
@@ -48,6 +48,7 @@ public class HttpReceiverOverHTTP extends HttpReceiver implements HttpParser.Res
     private boolean shutdown;
     private boolean complete;
     private boolean unsolicited;
+    private String method;
     private int status;
 
     public HttpReceiverOverHTTP(HttpChannelOverHTTP channel)
@@ -130,6 +131,10 @@ private void releaseNetworkBuffer()
 
     protected ByteBuffer onUpgradeFrom()
     {
+        RetainableByteBuffer networkBuffer = this.networkBuffer;
+        if (networkBuffer == null)
+            return null;
+
         ByteBuffer upgradeBuffer = null;
         if (networkBuffer.hasRemaining())
         {
@@ -226,14 +231,20 @@ private boolean parse()
             boolean complete = this.complete;
             this.complete = false;
             if (LOG.isDebugEnabled())
-                LOG.debug("Parse complete={}, remaining {} {}", complete, networkBuffer.remaining(), parser);
+                LOG.debug("Parse complete={}, {} {}", complete, networkBuffer, parser);
 
             if (complete)
             {
                 int status = this.status;
                 this.status = 0;
+                // Connection upgrade due to 101, bail out.
                 if (status == HttpStatus.SWITCHING_PROTOCOLS_101)
                     return true;
+                // Connection upgrade due to CONNECT + 200, bail out.
+                String method = this.method;
+                this.method = null;
+                if (getHttpChannel().isTunnel(method, status))
+                    return true;
             }
 
             if (networkBuffer.isEmpty())
@@ -283,10 +294,9 @@ public void startResponse(HttpVersion version, int status, String reason)
         if (exchange == null)
             return;
 
+        this.method = exchange.getRequest().getMethod();
         this.status = status;
-        String method = exchange.getRequest().getMethod();
-        parser.setHeadResponse(HttpMethod.HEAD.is(method) ||
-            (HttpMethod.CONNECT.is(method) && status == HttpStatus.OK_200));
+        parser.setHeadResponse(HttpMethod.HEAD.is(method) || getHttpChannel().isTunnel(method, status));
         exchange.getResponse().version(version).status(status).reason(reason);
 
         responseBegin(exchange);

diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTest.java
@@ -1535,8 +1535,18 @@ public void testCONNECTWithHTTP10(Scenario scenario) throws Exception
 
                 ContentResponse response = listener.get(5, TimeUnit.SECONDS);
                 assertEquals(200, response.getStatus());
+                assertThat(connection, Matchers.instanceOf(HttpConnectionOverHTTP.class));
+                HttpConnectionOverHTTP httpConnection = (HttpConnectionOverHTTP)connection;
+                EndPoint endPoint = httpConnection.getEndPoint();
+                assertTrue(endPoint.isOpen());
+
+                // After a CONNECT+200, this connection is in "tunnel mode",
+                // and applications that want to deal with tunnel bytes must
+                // likely access the underlying EndPoint.
+                // For the purpose of this test, we just re-enable fill interest
+                // so that we can send another clear-text HTTP request.
+                httpConnection.fillInterested();
 
-                // Test that I can send another request on the same connection.
                 request = client.newRequest(host, port);
                 listener = new FutureResponseListener(request);
                 connection.send(request, listener);

diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/MetaData.java b/jetty-http/src/main/java/org/eclipse/jetty/http/MetaData.java
@@ -19,6 +19,19 @@
 
 public class MetaData implements Iterable<HttpField>
 {
+    /**
+     * <p>Returns whether the given HTTP request method and HTTP response status code
+     * identify a successful HTTP CONNECT tunnel.</p>
+     *
+     * @param method the HTTP request method
+     * @param status the HTTP response status code
+     * @return whether method and status identify a successful HTTP CONNECT tunnel
+     */
+    public static boolean isTunnel(String method, int status)
+    {
+        return HttpMethod.CONNECT.is(method) && HttpStatus.isSuccess(status);
+    }
+
     private final HttpVersion _httpVersion;
     private final HttpFields _fields;
     private final long _contentLength;

diff --git a/...nt-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpReceiverOverHTTP2.java b/...nt-transport/src/main/java/org/eclipse/jetty/http2/client/http/HttpReceiverOverHTTP2.java
@@ -31,7 +31,6 @@
 import org.eclipse.jetty.client.api.Response;
 import org.eclipse.jetty.http.HttpField;
 import org.eclipse.jetty.http.HttpFields;
-import org.eclipse.jetty.http.HttpMethod;
 import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.http.MetaData;
 import org.eclipse.jetty.http2.ErrorCode;
@@ -102,7 +101,7 @@ void onHeaders(Stream stream, HeadersFrame frame)
                 }
 
                 HttpRequest httpRequest = exchange.getRequest();
-                if (HttpMethod.CONNECT.is(httpRequest.getMethod()) && httpResponse.getStatus() == HttpStatus.OK_200)
+                if (MetaData.isTunnel(httpRequest.getMethod(), httpResponse.getStatus()))
                 {
                     ClientHTTP2StreamEndPoint endPoint = new ClientHTTP2StreamEndPoint((IStream)stream);
                     long idleTimeout = httpRequest.getIdleTimeout();

diff --git a/...ttp2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerConnection.java b/...ttp2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HTTP2ServerConnection.java
@@ -28,7 +28,6 @@
 import org.eclipse.jetty.http.HttpFields;
 import org.eclipse.jetty.http.HttpHeader;
 import org.eclipse.jetty.http.HttpMethod;
-import org.eclipse.jetty.http.HttpStatus;
 import org.eclipse.jetty.http.MetaData;
 import org.eclipse.jetty.http.MetaData.Request;
 import org.eclipse.jetty.http2.ErrorCode;
@@ -355,7 +354,7 @@ public void onCompleted()
 
         private boolean isTunnel()
         {
-            return HttpMethod.CONNECT.is(getRequest().getMethod()) && getResponse().getStatus() == HttpStatus.OK_200;
+            return MetaData.isTunnel(getRequest().getMethod(), getResponse().getStatus());
         }
 
         @Override

diff --git a/...tp2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java b/...tp2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpTransportOverHTTP2.java
@@ -261,7 +261,7 @@ private HttpFields retrieveTrailers()
 
     private boolean isTunnel(MetaData.Request request, MetaData.Response response)
     {
-        return HttpMethod.CONNECT.is(request.getMethod()) && response.getStatus() == HttpStatus.OK_200;
+        return MetaData.isTunnel(request.getMethod(), response.getStatus());
     }
 
     @Override

diff --git a/...-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpTransportOverHTTP3.java b/...-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpTransportOverHTTP3.java
@@ -239,7 +239,7 @@ private HttpFields retrieveTrailers()
 
     private boolean isTunnel(MetaData.Request request, MetaData.Response response)
     {
-        return HttpMethod.CONNECT.is(request.getMethod()) && response.getStatus() == HttpStatus.OK_200;
+        return MetaData.isTunnel(request.getMethod(), response.getStatus());
     }
 
     @Override

diff --git a/jetty-proxy/src/main/java/org/eclipse/jetty/proxy/ProxyConnection.java b/jetty-proxy/src/main/java/org/eclipse/jetty/proxy/ProxyConnection.java
@@ -79,11 +79,12 @@ protected void close(Throwable failure)
     @Override
     public String toConnectionString()
     {
+        EndPoint endPoint = getEndPoint();
         return String.format("%s@%x[l:%s<=>r:%s]",
             getClass().getSimpleName(),
             hashCode(),
-            getEndPoint().getLocalSocketAddress(),
-            getEndPoint().getRemoteSocketAddress());
+            endPoint.getLocalSocketAddress(),
+            endPoint.getRemoteSocketAddress());
     }
 
     private class ProxyIteratingCallback extends IteratingCallback

diff --git a/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ForwardProxyTLSServerTest.java b/jetty-proxy/src/test/java/org/eclipse/jetty/proxy/ForwardProxyTLSServerTest.java
@@ -351,10 +351,10 @@ protected void handleConnect(Request baseRequest, HttpServletRequest request, Ht
                 try
                 {
                     // Make sure the proxy remains idle enough.
-                    Thread.sleep(2 * idleTimeout);
+                    sleep(2 * idleTimeout);
                     super.handleConnect(baseRequest, request, response, serverAddress);
                 }
-                catch (InterruptedException x)
+                catch (Throwable x)
                 {
                     onConnectFailure(request, response, null, x);
                 }
@@ -788,6 +788,47 @@ public SSLEngine newSSLEngine(String host, int port)
         }
     }
 
+    @ParameterizedTest
+    @MethodSource("proxyTLS")
+    public void testRequestCompletionDelayed(SslContextFactory.Server proxyTLS) throws Exception
+    {
+        startTLSServer(new ServerHandler());
+        startProxy(proxyTLS);
+
+        HttpClient httpClient = newHttpClient();
+        httpClient.getProxyConfiguration().getProxies().add(newHttpProxy());
+        httpClient.start();
+
+        try
+        {
+            httpClient.getRequestListeners().add(new org.eclipse.jetty.client.api.Request.Listener()
+            {
+                @Override
+                public void onSuccess(org.eclipse.jetty.client.api.Request request)
+                {
+                    if (HttpMethod.CONNECT.is(request.getMethod()))
+                        sleep(250);
+                }
+            });
+
+            String body = "BODY";
+            ContentResponse response = httpClient.newRequest("localhost", serverConnector.getLocalPort())
+                .scheme(HttpScheme.HTTPS.asString())
+                .method(HttpMethod.GET)
+                .path("/echo?body=" + URLEncoder.encode(body, StandardCharsets.UTF_8))
+                .timeout(5, TimeUnit.SECONDS)
+                .send();
+
+            assertEquals(HttpStatus.OK_200, response.getStatus());
+            String content = response.getContentAsString();
+            assertEquals(body, content);
+        }
+        finally
+        {
+            httpClient.stop();
+        }
+    }
+
     @Test
     @Tag("external")
     @Disabled
@@ -823,6 +864,18 @@ public void testExternalProxy() throws Exception
         }
     }
 
+    private static void sleep(long ms)
+    {
+        try
+        {
+            Thread.sleep(ms);
+        }
+        catch (InterruptedException x)
+        {
+            throw new RuntimeException(x);
+        }
+    }
+
     private static class ServerHandler extends AbstractHandler
     {
         @Override