Skip to content

Commit fd6011d

Browse files
lorbanolamy
lorban
and
olamy
authored
Fix loop caused by NegotiatingClientConnection talking to broken server (#14089)
#13964 rework when connections are upgraded or failed Signed-off-by: Ludovic Orban <[email protected]> Signed-off-by: Olivier Lamy <[email protected]> Co-authored-by: Olivier Lamy <[email protected]>
1 parent c1e300d commit fd6011d

File tree

2 files changed

+152
-41
lines changed

2 files changed

+152
-41
lines changed

‎jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/BrokenServerNegotiationTest.java‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,112 @@
1+
//
2+
// ========================================================================
3+
// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others.
4+
//
5+
// This program and the accompanying materials are made available under the
6+
// terms of the Eclipse Public License v. 2.0 which is available at
7+
// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
8+
// which is available at https://www.apache.org/licenses/LICENSE-2.0.
9+
//
10+
// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
11+
// ========================================================================
12+
//
13+
14+
package org.eclipse.jetty.http2.tests;
15+
16+
import java.net.ServerSocket;
17+
import java.net.Socket;
18+
import java.util.concurrent.ExecutionException;
19+
import java.util.concurrent.TimeUnit;
20+
import javax.net.ssl.SSLHandshakeException;
21+
22+
import org.eclipse.jetty.client.HttpClient;
23+
import org.eclipse.jetty.client.transport.HttpClientConnectionFactory;
24+
import org.eclipse.jetty.client.transport.HttpClientTransportDynamic;
25+
import org.eclipse.jetty.http2.client.HTTP2Client;
26+
import org.eclipse.jetty.http2.client.transport.ClientConnectionFactoryOverHTTP2;
27+
import org.eclipse.jetty.http2.client.transport.HttpClientTransportOverHTTP2;
28+
import org.eclipse.jetty.io.ClientConnectionFactory;
29+
import org.eclipse.jetty.io.ClientConnector;
30+
import org.eclipse.jetty.util.component.LifeCycle;
31+
import org.junit.jupiter.api.AfterEach;
32+
import org.junit.jupiter.params.ParameterizedTest;
33+
import org.junit.jupiter.params.provider.ValueSource;
34+
35+
import static org.awaitility.Awaitility.await;
36+
import static org.hamcrest.Matchers.notNullValue;
37+
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
38+
import static org.junit.jupiter.api.Assertions.assertThrows;
39+
40+
public class BrokenServerNegotiationTest
41+
{
42+
private volatile ServerSocket serverSocket;
43+
44+
public int startServer() throws Exception
45+
{
46+
if (serverSocket != null)
47+
return serverSocket.getLocalPort();
48+
Thread server = new Thread(() ->
49+
{
50+
try
51+
{
52+
serverSocket = new ServerSocket(0);
53+
while (true)
54+
{
55+
Socket accept = serverSocket.accept();
56+
accept.close();
57+
}
58+
}
59+
catch (Exception e)
60+
{
61+
// ignore, time to shut down
62+
}
63+
});
64+
server.start();
65+
await().atMost(5, TimeUnit.SECONDS).until(() -> serverSocket, notNullValue());
66+
return serverSocket.getLocalPort();
67+
}
68+
69+
@AfterEach
70+
public void stopServer() throws Exception
71+
{
72+
if (serverSocket != null)
73+
serverSocket.close();
74+
}
75+
76+
@ParameterizedTest
77+
@ValueSource(booleans = {false, true})
78+
public void testAlpnNegotiation(boolean useAlpn) throws Exception
79+
{
80+
int port = startServer();
81+
ClientConnector clientConnector = new ClientConnector();
82+
HTTP2Client h2Client = new HTTP2Client();
83+
h2Client.setUseALPN(useAlpn);
84+
ClientConnectionFactoryOverHTTP2.HTTP2 http2 = new ClientConnectionFactoryOverHTTP2.HTTP2(h2Client);
85+
ClientConnectionFactory.Info http1 = HttpClientConnectionFactory.HTTP11;
86+
HttpClientTransportDynamic transportDynamic = new HttpClientTransportDynamic(clientConnector, http1, http2);
87+
HttpClient client = new HttpClient(transportDynamic);
88+
client.start();
89+
90+
ExecutionException ee = assertThrows(ExecutionException.class, () -> client.GET("https://localhost:" + port + "/hello"));
91+
assertInstanceOf(SSLHandshakeException.class, ee.getCause());
92+
93+
LifeCycle.stop(client);
94+
}
95+
96+
@ParameterizedTest
97+
@ValueSource(booleans = {false, true})
98+
public void testH2Only(boolean useAlpn) throws Exception
99+
{
100+
int port = startServer();
101+
HTTP2Client h2Client = new HTTP2Client();
102+
h2Client.setUseALPN(useAlpn);
103+
HttpClientTransportOverHTTP2 httpClientTransportOverHTTP2 = new HttpClientTransportOverHTTP2(h2Client);
104+
HttpClient client = new HttpClient(httpClientTransportOverHTTP2);
105+
client.start();
106+
107+
ExecutionException ee = assertThrows(ExecutionException.class, () -> client.GET("https://localhost:" + port + "/hello"));
108+
assertInstanceOf(SSLHandshakeException.class, ee.getCause());
109+
110+
LifeCycle.stop(client);
111+
}
112+
}

‎jetty-core/jetty-io/src/main/java/org/eclipse/jetty/io/NegotiatingClientConnection.java‎

Lines changed: 40 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -18,9 +18,9 @@
1818
import java.util.concurrent.Executor;
1919
import java.util.concurrent.TimeoutException;
2020
import javax.net.ssl.SSLEngine;
21+
import javax.net.ssl.SSLHandshakeException;
2122

2223
import org.eclipse.jetty.util.BufferUtil;
23-
import org.eclipse.jetty.util.ExceptionUtil;
2424
import org.eclipse.jetty.util.Promise;
2525
import org.slf4j.Logger;
2626
import org.slf4j.LoggerFactory;
@@ -61,7 +61,7 @@ public String getProtocol()
6161
protected void completed(String protocol)
6262
{
6363
this.protocol = protocol;
64-
completed = true;
64+
this.completed = true;
6565
}
6666

6767
@Override
@@ -79,67 +79,53 @@ public void onOpen()
7979
catch (Throwable x)
8080
{
8181
close();
82-
// TODO: should we not fail the promise in the context here?
83-
throw ExceptionUtil.asRuntimeException(x);
82+
failConnectionPromise(x);
8483
}
8584
}
8685

8786
@Override
8887
public void onFillable()
8988
{
90-
while (true)
89+
Throwable failure = null;
90+
try
9191
{
92-
int filled = fill();
93-
if (completed || filled < 0)
92+
while (true)
9493
{
95-
replaceConnection();
96-
break;
97-
}
98-
if (filled == 0)
99-
{
100-
fillInterested();
101-
break;
94+
int filled = getEndPoint().fill(BufferUtil.EMPTY_BUFFER);
95+
if (completed)
96+
{
97+
replaceConnection();
98+
break;
99+
}
100+
else if (filled < 0)
101+
{
102+
throw new SSLHandshakeException("Abruptly closed by peer");
103+
}
104+
else if (filled == 0)
105+
{
106+
fillInterested();
107+
break;
108+
}
102109
}
103110
}
104-
}
105-
106-
private int fill()
107-
{
108-
try
109-
{
110-
return getEndPoint().fill(BufferUtil.EMPTY_BUFFER);
111-
}
112-
catch (IOException x)
111+
catch (Throwable x)
113112
{
114-
LOG.atDebug().setCause(x).log("Unable to fill from endpoint");
115-
close();
116-
return -1;
113+
failure = x;
117114
}
118-
}
119115

120-
private void replaceConnection()
121-
{
122-
EndPoint endPoint = getEndPoint();
123-
try
116+
if (failure != null)
124117
{
125-
endPoint.upgrade(connectionFactory.newConnection(endPoint, context));
126-
}
127-
catch (Throwable x)
128-
{
129-
LOG.atDebug().setCause(x).log("Unable to replace connection");
118+
LOG.atDebug().setCause(failure).log("Unable to fill from endpoint");
130119
close();
120+
failConnectionPromise(failure);
131121
}
132122
}
133123

134124
@Override
135125
public boolean onIdleExpired(TimeoutException timeout)
136126
{
137127
getEndPoint().close(timeout);
138-
139-
@SuppressWarnings("unchecked")
140-
Promise<Connection> promise = (Promise<Connection>)context.get(ClientConnector.CONNECTION_PROMISE_CONTEXT_KEY);
141-
promise.failed(timeout);
142-
128+
failConnectionPromise(timeout);
143129
return false;
144130
}
145131

@@ -150,4 +136,17 @@ public void close()
150136
getEndPoint().shutdownOutput();
151137
super.close();
152138
}
139+
140+
private void replaceConnection() throws IOException
141+
{
142+
EndPoint endPoint = getEndPoint();
143+
endPoint.upgrade(connectionFactory.newConnection(endPoint, context));
144+
}
145+
146+
private void failConnectionPromise(Throwable failure)
147+
{
148+
@SuppressWarnings("unchecked")
149+
Promise<Connection> promise = (Promise<Connection>)context.get(ClientConnector.CONNECTION_PROMISE_CONTEXT_KEY);
150+
promise.failed(failure);
151+
}
153152
}

0 commit comments

Comments
 (0)