Eclipse
/
jetty.project
mirror of https://github.com/jetty/jetty.project.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Jetty 9.4.x 3038 ssl connection leak (#3121) 

Issue #3038 - SSL connection leak.

Fixed SSL spin caused when fill had NEED_WRAP, but a flush/wrap
produced 0 bytes and stayed in NEED_WRAP

Removed check of isInputShutdown prior to filling that allowed EOF to
overtake data already read.

Fix for leak by shutting down output in HttpConnection if
filled -1 and the HttpChannelState was no longer processing
current request.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Signed-off-by: Greg Wilkins <gregw@webtide.com>
This commit is contained in:
Greg Wilkins 2018-11-20 13:53:42 +01:00 committed by GitHub
parent 10622f3455
commit 8c4ee8496f
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 507 additions and 168 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
jetty-client/src/main/java/org/eclipse/jetty/client/http/HttpReceiverOverHTTP.java
View File

@ -20,6 +20,7 @@ package org.eclipse.jetty.client.http;
import java.io.EOFException;
import java.nio.ByteBuffer;
import java.util.concurrent.atomic.AtomicReference;
import org.eclipse.jetty.client.HttpClient;
import org.eclipse.jetty.client.HttpExchange;
@ -39,6 +40,7 @@ import org.eclipse.jetty.util.CompletableCallback;
public class HttpReceiverOverHTTP extends HttpReceiver implements HttpParser.ResponseHandler
{
private final AtomicReference<ContentState> handlingContent = new AtomicReference<>(ContentState.IDLE);
private final HttpParser parser;
private ByteBuffer buffer;
private boolean shutdown;
@ -263,8 +265,18 @@ public class HttpReceiverOverHTTP extends HttpReceiver implements HttpParser.Res
if (exchange == null)
return false;
handlingContent.set(ContentState.CONTENT);
CompletableCallback callback = new CompletableCallback()
{
@Override
public void succeeded()
{
boolean messageComplete = !handlingContent.compareAndSet(ContentState.CONTENT, ContentState.IDLE);
super.succeeded();
if (messageComplete)
messageComplete();
}
@Override
public void resume()
{
@ -304,6 +316,9 @@ public class HttpReceiverOverHTTP extends HttpReceiver implements HttpParser.Res
@Override
public boolean messageComplete()
{
if (handlingContent.compareAndSet(ContentState.CONTENT, ContentState.COMPLETE))
return false;
HttpExchange exchange = getHttpExchange();
if (exchange == null)
return false;
@ -375,4 +390,6 @@ public class HttpReceiverOverHTTP extends HttpReceiver implements HttpParser.Res
{
return String.format("%s[%s]", super.toString(), parser);
}
private enum ContentState { IDLE, CONTENT, COMPLETE }
}

21
jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java
View File

@ -767,7 +767,7 @@ public class HttpParser
case LF:
setState(State.HEADER);
handle=_responseHandler.startResponse(_version, _responseStatus, null)||handle;
handle |= _responseHandler.startResponse(_version, _responseStatus, null);
break;
default:
@ -789,7 +789,7 @@ public class HttpParser
handle=_requestHandler.startRequest(_methodString,_uri.toString(), HttpVersion.HTTP_0_9);
setState(State.END);
BufferUtil.clear(buffer);
handle= handleHeaderContentMessage() || handle;
handle |= handleHeaderContentMessage();
break;
case ALPHA:
@ -865,7 +865,7 @@ public class HttpParser
if (_responseHandler!=null)
{
setState(State.HEADER);
handle=_responseHandler.startResponse(_version, _responseStatus, null)||handle;
handle |= _responseHandler.startResponse(_version, _responseStatus, null);
}
else
{
@ -876,7 +876,7 @@ public class HttpParser
handle=_requestHandler.startRequest(_methodString,_uri.toString(), HttpVersion.HTTP_0_9);
setState(State.END);
BufferUtil.clear(buffer);
handle= handleHeaderContentMessage() || handle;
handle |= handleHeaderContentMessage();
}
break;
@ -905,7 +905,7 @@ public class HttpParser
setState(State.HEADER);
handle=_requestHandler.startRequest(_methodString,_uri.toString(), _version)||handle;
handle |= _requestHandler.startRequest(_methodString,_uri.toString(), _version);
continue;
case ALPHA:
@ -927,7 +927,7 @@ public class HttpParser
case LF:
String reason=takeString();
setState(State.HEADER);
handle=_responseHandler.startResponse(_version, _responseStatus, reason)||handle;
handle |= _responseHandler.startResponse(_version, _responseStatus, reason);
continue;
case ALPHA:
@ -1660,14 +1660,16 @@ public class HttpParser
_contentPosition += _contentChunk.remaining();
buffer.position(buffer.position()+_contentChunk.remaining());
if (_handler.content(_contentChunk))
return true;
boolean handle = _handler.content(_contentChunk);
if(_contentPosition == _contentLength)
{
setState(State.END);
return handleContentMessage();
boolean handleContent = handleContentMessage();
return handle || handleContent;
}
else if (handle)
return true;
}
break;
}
@ -1808,7 +1810,6 @@ public class HttpParser
/* ------------------------------------------------------------------------------- */
public boolean isAtEOF()
{
return _eof;
}

24
jetty-http/src/test/java/org/eclipse/jetty/http/HttpTester.java
View File

@ -27,7 +27,6 @@ import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import org.eclipse.jetty.util.BufferUtil;
import org.eclipse.jetty.util.IO;
import org.eclipse.jetty.util.StringUtil;
import org.eclipse.jetty.util.log.Log;
import org.eclipse.jetty.util.log.Logger;
@ -110,13 +109,28 @@ public class HttpTester
public static Response parseResponse(InputStream responseStream) throws IOException
{
ByteArrayOutputStream contentStream = new ByteArrayOutputStream();
IO.copy(responseStream, contentStream);
Response r=new Response();
HttpParser parser =new HttpParser(r);
parser.parseNext(ByteBuffer.wrap(contentStream.toByteArray()));
// Read and parse a character at a time so we never can read more than we should.
byte[] array = new byte[1];
ByteBuffer buffer = ByteBuffer.wrap(array);
buffer.limit(1);
while(true)
{
buffer.position(1);
int l = responseStream.read(array);
if (l<0)
parser.atEOF();
else
buffer.position(0);
if (parser.parseNext(buffer))
return r;
else if (l<0)
return null;
}
}
public abstract static class Input

19
jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java
View File

@ -513,7 +513,12 @@ public class SslConnection extends AbstractConnection
case NEED_WRAP:
if (_flushState == FlushState.IDLE && flush(BufferUtil.EMPTY_BUFFER))
{
if (_sslEngine.isInboundDone())
// TODO this is probably a JVM bug, work around it by -1
return -1;
continue;
}
// handle in needsFillInterest
return filled = 0;
@ -561,11 +566,11 @@ public class SslConnection extends AbstractConnection
{
BufferUtil.flipToFlush(app_in, pos);
}
if (LOG.isDebugEnabled())
LOG.debug("unwrap {} {} unwrapBuffer={} appBuffer={}",
LOG.debug("unwrap net_filled={} {} encryptedBuffer={} unwrapBuffer={} appBuffer={}",
net_filled,
unwrapResult.toString().replace('\n', ' '),
BufferUtil.toSummaryString(_encryptedInput),
BufferUtil.toDetailString(app_in),
BufferUtil.toDetailString(buffer));
@ -862,13 +867,17 @@ public class SslConnection extends AbstractConnection
try
{
wrapResult = _sslEngine.wrap(appOuts, _encryptedOutput);
if (LOG.isDebugEnabled())
LOG.debug("wrap {} {}", wrapResult.toString().replace('\n',' '), BufferUtil.toHexSummary(_encryptedOutput));
}
finally
{
BufferUtil.flipToFlush(_encryptedOutput, pos);
}
if (LOG.isDebugEnabled())
LOG.debug("wrap {} {} ioDone={}/{}",
wrapResult.toString().replace('\n', ' '),
BufferUtil.toSummaryString(_encryptedOutput),
_sslEngine.isInboundDone(),
_sslEngine.isOutboundDone());
// Was all the data consumed?
boolean allConsumed = true;
@ -1142,7 +1151,7 @@ public class SslConnection extends AbstractConnection
@Override
public boolean isInputShutdown()
{
return getEndPoint().isInputShutdown() || isInboundDone();
return BufferUtil.isEmpty(_decryptedInput) && (getEndPoint().isInputShutdown() || isInboundDone());
}
private boolean isInboundDone()

34
jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java
View File

@ -263,14 +263,26 @@ public class HttpConnection extends AbstractConnection implements Runnable, Http
if (suspended || getEndPoint().getConnection() != this)
break;
}
else
else if (filled==0)
{
if (filled <= 0)
{
if (filled == 0)
fillInterested();
break;
}
else if (filled<0)
{
switch(_channel.getState().getState())
{
case COMPLETING:
case COMPLETED:
case IDLE:
case THROWN:
case ASYNC_ERROR:
getEndPoint().shutdownOutput();
break;
default:
break;
}
break;
}
}
}
@ -310,16 +322,6 @@ public class HttpConnection extends AbstractConnection implements Runnable, Http
if (BufferUtil.isEmpty(_requestBuffer))
{
// Can we fill?
if(getEndPoint().isInputShutdown())
{
// No pretend we read -1
_parser.atEOF();
if (LOG.isDebugEnabled())
LOG.debug("{} filled -1 {}",this,BufferUtil.toDetailString(_requestBuffer));
return -1;
}
// Get a buffer
// We are not in a race here for the request buffer as we have not yet received a request,
// so there are not an possible legal threads calling #parseContent or #completed.
@ -411,13 +413,13 @@ public class HttpConnection extends AbstractConnection implements Runnable, Http
if (_input.isAsync())
{
if (LOG.isDebugEnabled())
LOG.debug("unconsumed async input {}", this);
LOG.debug("{}unconsumed input {}",_parser.isChunking()?"Possible ":"", this);
_channel.abort(new IOException("unconsumed input"));
}
else
{
if (LOG.isDebugEnabled())
LOG.debug("unconsumed input {}", this);
LOG.debug("{}unconsumed input {}",_parser.isChunking()?"Possible ":"", this);
// Complete reading the request
if (!_input.consumeAll())
_channel.abort(new IOException("unconsumed input"));

108
jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java
View File

@ -18,16 +18,6 @@
package org.eclipse.jetty.server;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.greaterThanOrEqualTo;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.startsWith;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.jupiter.api.Assertions.assertTrue;
import java.io.BufferedReader;
import java.io.ByteArrayOutputStream;
import java.io.EOFException;
@ -58,9 +48,20 @@ import org.eclipse.jetty.util.log.AbstractLogger;
import org.eclipse.jetty.util.log.Log;
import org.eclipse.jetty.util.log.StacklessLogging;
import org.hamcrest.Matchers;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.condition.DisabledIfSystemProperty;
import org.junit.jupiter.api.condition.DisabledOnJre;
import org.junit.jupiter.api.condition.JRE;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.greaterThanOrEqualTo;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
import static org.hamcrest.Matchers.startsWith;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
public abstract class HttpServerTestBase extends HttpServerTestFixture
{
@ -1692,4 +1693,89 @@ public abstract class HttpServerTestBase extends HttpServerTestFixture
client.close();
}
}
@Test
@DisabledOnJre({JRE.JAVA_8, JRE.JAVA_9, JRE.JAVA_10})
public void testShutdown() throws Exception
{
configureServer(new ReadExactHandler());
byte[] content = new byte[4096];
Arrays.fill(content,(byte)'X');
try (Socket client = newSocket(_serverURI.getHost(), _serverURI.getPort()))
{
OutputStream os = client.getOutputStream();
// Send two persistent pipelined requests and then shutdown output
os.write(("GET / HTTP/1.1\r\n" +
"Host: localhost\r\n" +
"Content-Length: "+content.length+"\r\n" +
"\r\n").getBytes(StandardCharsets.ISO_8859_1));
os.write(content);
os.write(("GET / HTTP/1.1\r\n" +
"Host: localhost\r\n" +
"Content-Length: "+content.length+"\r\n" +
"\r\n").getBytes(StandardCharsets.ISO_8859_1));
os.write(content);
os.flush();
// Thread.sleep(50);
client.shutdownOutput();
// Read the two pipelined responses
HttpTester.Response response = HttpTester.parseResponse(client.getInputStream());
assertThat(response.getStatus(), is(200));
assertThat(response.getContent(), containsString("Read "+content.length));
response = HttpTester.parseResponse(client.getInputStream());
assertThat(response.getStatus(), is(200));
assertThat(response.getContent(), containsString("Read "+content.length));
// Read the close
assertThat(client.getInputStream().read(),is(-1));
}
}
@Test
@DisabledOnJre({JRE.JAVA_8, JRE.JAVA_9, JRE.JAVA_10})
public void testChunkedShutdown() throws Exception
{
configureServer(new ReadExactHandler(4096));
byte[] content = new byte[4096];
Arrays.fill(content,(byte)'X');
try (Socket client = newSocket(_serverURI.getHost(), _serverURI.getPort()))
{
OutputStream os = client.getOutputStream();
// Send two persistent pipelined requests and then shutdown output
os.write(("GET / HTTP/1.1\r\n" +
"Host: localhost\r\n" +
"Transfer-Encoding: chunked\r\n" +
"\r\n" +
"1000\r\n").getBytes(StandardCharsets.ISO_8859_1));
os.write(content);
os.write("\r\n0\r\n\r\n".getBytes(StandardCharsets.ISO_8859_1));
os.write(("GET / HTTP/1.1\r\n" +
"Host: localhost\r\n" +
"Transfer-Encoding: chunked\r\n" +
"\r\n" +
"1000\r\n").getBytes(StandardCharsets.ISO_8859_1));
os.write(content);
os.write("\r\n0\r\n\r\n".getBytes(StandardCharsets.ISO_8859_1));
os.flush();
client.shutdownOutput();
// Read the two pipelined responses
HttpTester.Response response = HttpTester.parseResponse(client.getInputStream());
assertThat(response.getStatus(), is(200));
assertThat(response.getContent(), containsString("Read "+content.length));
response = HttpTester.parseResponse(client.getInputStream());
assertThat(response.getStatus(), is(200));
assertThat(response.getContent(), containsString("Read "+content.length));
// Read the close
assertThat(client.getInputStream().read(),is(-1));
}
}
}

49
jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestFixture.java
View File

@ -27,6 +27,7 @@ import java.net.Socket;
import java.net.URI;
import java.nio.charset.StandardCharsets;
import javax.servlet.RequestDispatcher;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
@ -186,6 +187,54 @@ public class HttpServerTestFixture
}
}
protected static class ReadExactHandler extends AbstractHandler.ErrorDispatchHandler
{
private int expected;
public ReadExactHandler()
{
this(-1);
}
public ReadExactHandler(int expected)
{
this.expected = expected;
}
@Override
public void doNonErrorHandle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
{
baseRequest.setHandled(true);
int len = expected<0?request.getContentLength():expected;
if (len<0)
throw new IllegalStateException();
byte[] content = new byte[len];
int offset = 0;
while (offset<len)
{
int read = request.getInputStream().read(content,offset,len-offset);
if (read<0)
break;
offset+=read;
}
response.setStatus(200);
String reply = "Read " + offset + "\r\n";
response.setContentLength(reply.length());
response.getOutputStream().write(reply.getBytes(StandardCharsets.ISO_8859_1));
}
@Override
protected void doError(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
{
System.err.println("ERROR: "+request.getAttribute(RequestDispatcher.ERROR_MESSAGE));
Throwable th = (Throwable)request.getAttribute(RequestDispatcher.ERROR_EXCEPTION);
if (th!=null)
th.printStackTrace();
super.doError(target, baseRequest, request, response);
}
}
protected static class ReadHandler extends AbstractHandler
{
@Override

162
jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SSLReadEOFAfterResponseTest.java Normal file
View File

@ -0,0 +1,162 @@
//
// ========================================================================
// Copyright (c) 1995-2018 Mort Bay Consulting Pty. Ltd.
// ------------------------------------------------------------------------
// All rights reserved. This program and the accompanying materials
// are made available under the terms of the Eclipse Public License v1.0
// and Apache License v2.0 which accompanies this distribution.
//
// The Eclipse Public License is available at
// http://www.eclipse.org/legal/epl-v10.html
//
// The Apache License v2.0 is available at
// http://www.opensource.org/licenses/apache2.0.php
//
// You may elect to redistribute this code under either of these licenses.
// ========================================================================
//
package org.eclipse.jetty.server.ssl;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.jupiter.api.Assertions.assertEquals;
import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.io.InterruptedIOException;
import java.io.OutputStream;
import java.net.Socket;
import java.nio.charset.StandardCharsets;
import javax.net.ssl.SSLContext;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import org.eclipse.jetty.server.Request;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.ServerConnector;
import org.eclipse.jetty.server.handler.AbstractHandler;
import org.eclipse.jetty.toolchain.test.MavenTestingUtils;
import org.eclipse.jetty.util.resource.Resource;
import org.eclipse.jetty.util.ssl.SslContextFactory;
import org.hamcrest.Matchers;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.condition.DisabledOnJre;
import org.junit.jupiter.api.condition.JRE;
// Only in JDK 11 is possible to use SSLSocket.shutdownOutput().
@DisabledOnJre({JRE.JAVA_8, JRE.JAVA_9, JRE.JAVA_10})
public class SSLReadEOFAfterResponseTest
{
@Test
public void testReadEOFAfterResponse() throws Exception
{
File keystore = MavenTestingUtils.getTestResourceFile("keystore");
SslContextFactory sslContextFactory = new SslContextFactory();
sslContextFactory.setKeyStoreResource(Resource.newResource(keystore));
sslContextFactory.setKeyStorePassword("storepwd");
sslContextFactory.setKeyManagerPassword("keypwd");
Server server = new Server();
ServerConnector connector = new ServerConnector(server, sslContextFactory);
int idleTimeout = 1000;
connector.setIdleTimeout(idleTimeout);
server.addConnector(connector);
String content = "the quick brown fox jumped over the lazy dog";
byte[] bytes = content.getBytes(StandardCharsets.UTF_8);
server.setHandler(new AbstractHandler.ErrorDispatchHandler()
{
@Override
protected void doNonErrorHandle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
{
// First: read the whole content.
InputStream input = request.getInputStream();
int length = bytes.length;
while (length > 0)
{
int read = input.read();
if (read < 0)
throw new IllegalStateException();
--length;
}
// Second: write the response.
response.setContentLength(bytes.length);
response.getOutputStream().write(bytes);
response.flushBuffer();
sleep(idleTimeout / 2);
// Third, read the EOF.
int read = input.read();
if (read >= 0)
throw new IllegalStateException();
}
});
server.start();
try
{
SSLContext sslContext = sslContextFactory.getSslContext();
try (Socket client = sslContext.getSocketFactory().createSocket("localhost", connector.getLocalPort()))
{
client.setSoTimeout(5 * idleTimeout);
OutputStream output = client.getOutputStream();
String request = "" +
"POST / HTTP/1.1\r\n" +
"Host: localhost\r\n" +
"Content-Length: " + content.length() + "\r\n" +
"\r\n";
output.write(request.getBytes(StandardCharsets.UTF_8));
output.write(bytes);
output.flush();
// Read the response.
InputStream input = client.getInputStream();
int crlfs = 0;
while (true)
{
int read = input.read();
assertThat(read, Matchers.greaterThanOrEqualTo(0));
if (read == '\r' || read == '\n')
++crlfs;
else
crlfs = 0;
if (crlfs == 4)
break;
}
for (byte b : bytes)
assertEquals(b, input.read());
// Shutdown the output so the server reads the TLS close_notify.
client.shutdownOutput();
// client.close();
// The connection should now be idle timed out by the server.
int read = input.read();
assertEquals(-1, read);
}
}
finally
{
server.stop();
}
}
private void sleep(long time) throws IOException
{
try
{
Thread.sleep(time);
}
catch (InterruptedException x)
{
throw new InterruptedIOException();
}
}
}

1
jetty-server/src/test/resources/jetty-logging.properties
View File

@ -1,5 +1,4 @@
org.eclipse.jetty.util.log.class=org.eclipse.jetty.util.log.StdErrLog
#org.eclipse.jetty.LEVEL=DEBUG
#org.eclipse.jetty.io.ssl.LEVEL=DEBUG
#org.eclipse.jetty.server.ConnectionLimit.LEVEL=DEBUG
#org.eclipse.jetty.server.AcceptRateLimit.LEVEL=DEBUG