Fixes #7348 - Slow CONNECT request causes NPE

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. Signed-off-by: Simone Bordet <[email protected]>
jetty · Jan 3, 2022 · f1f1c3a · f1f1c3a
1 parent 783f06e
commit f1f1c3a
Show file tree

Hide file tree

Showing 3 changed files with 73 additions and 8 deletions.
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
@@ -47,6 +47,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)
@@ -119,6 +120,10 @@ private void releaseNetworkBuffer()
 
     protected ByteBuffer onUpgradeFrom()
     {
+        RetainableByteBuffer networkBuffer = this.networkBuffer;
+        if (networkBuffer == null)
+            return null;
+
         ByteBuffer upgradeBuffer = null;
         if (networkBuffer.hasRemaining())
         {
@@ -127,7 +132,6 @@ protected ByteBuffer onUpgradeFrom()
             BufferUtil.put(networkBuffer.getBuffer(), upgradeBuffer);
             BufferUtil.flipToFlush(upgradeBuffer, 0);
         }
-
         releaseNetworkBuffer();
         return upgradeBuffer;
     }
@@ -215,14 +219,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 (status == HttpStatus.OK_200 && HttpMethod.CONNECT.is(method))
+                    return true;
             }
 
             if (networkBuffer.isEmpty())
@@ -279,8 +289,8 @@ public boolean startResponse(HttpVersion version, int status, String reason)
         if (exchange == null)
             return false;
 
+        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));
         exchange.getResponse().version(version).status(status).reason(reason);

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 @@ public void onFillable()
     @Override
     public String toConnectionString()
     {
-        return String.format("%s@%x[l:%d<=>r:%d]",
+        EndPoint endPoint = getEndPoint();
+        return String.format("%s@%x[l:%s<=>r:%s]",
             getClass().getSimpleName(),
             hashCode(),
-            getEndPoint().getLocalAddress().getPort(),
-            getEndPoint().getRemoteAddress().getPort());
+            endPoint.getLocalAddress(),
+            endPoint.getRemoteAddress());
     }
 
     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
@@ -344,10 +344,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);
                 }
@@ -789,6 +789,48 @@ 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 = new HttpClient(newClientSslContextFactory());
+        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)
+                {
+                    System.err.println("SIMON: 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, "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
@@ -824,6 +866,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