Issue #1045 abort connections with non matching content-length
This commit is contained in:
parent
96804627ee
commit
3e4f7b1fbf
|
@ -30,9 +30,12 @@ import org.eclipse.jetty.io.ByteBufferPool;
|
|||
import org.eclipse.jetty.server.HttpTransport;
|
||||
import org.eclipse.jetty.util.BufferUtil;
|
||||
import org.eclipse.jetty.util.Callback;
|
||||
import org.eclipse.jetty.util.log.Log;
|
||||
import org.eclipse.jetty.util.log.Logger;
|
||||
|
||||
public class HttpTransportOverFCGI implements HttpTransport
|
||||
{
|
||||
private static final Logger LOG = Log.getLogger(HttpTransportOverFCGI.class);
|
||||
private final ServerGenerator generator;
|
||||
private final Flusher flusher;
|
||||
private final int request;
|
||||
|
@ -97,6 +100,8 @@ public class HttpTransportOverFCGI implements HttpTransport
|
|||
|
||||
private void commit(MetaData.Response info, boolean head, ByteBuffer content, boolean lastContent, Callback callback)
|
||||
{
|
||||
if (LOG.isDebugEnabled())
|
||||
LOG.debug("commit {} {} l={}",this,info,lastContent);
|
||||
boolean shutdown = this.shutdown = info.getFields().contains(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE.asString());
|
||||
|
||||
if (head)
|
||||
|
@ -137,7 +142,10 @@ public class HttpTransportOverFCGI implements HttpTransport
|
|||
@Override
|
||||
public void abort(Throwable failure)
|
||||
{
|
||||
if (LOG.isDebugEnabled())
|
||||
LOG.debug("abort {} {}",this,failure);
|
||||
aborted = true;
|
||||
flusher.shutdown();
|
||||
}
|
||||
|
||||
@Override
|
||||
|
|
|
@ -608,7 +608,7 @@ public class HttpClientTest extends AbstractHttpClientServerTest
|
|||
{
|
||||
client.newRequest("localhost", connector.getLocalPort())
|
||||
.scheme(scheme)
|
||||
.timeout(5, TimeUnit.SECONDS)
|
||||
.timeout(60, TimeUnit.SECONDS)
|
||||
.send();
|
||||
Assert.fail();
|
||||
}
|
||||
|
|
|
@ -931,9 +931,10 @@ public class ProxyServletTest
|
|||
Assert.assertArrayEquals(content, response.getContent());
|
||||
}
|
||||
|
||||
@Test(expected = TimeoutException.class)
|
||||
@Test
|
||||
public void testWrongContentLength() throws Exception
|
||||
{
|
||||
|
||||
startServer(new HttpServlet()
|
||||
{
|
||||
@Override
|
||||
|
@ -948,11 +949,17 @@ public class ProxyServletTest
|
|||
startProxy();
|
||||
startClient();
|
||||
|
||||
client.newRequest("localhost", serverConnector.getLocalPort())
|
||||
.timeout(1, TimeUnit.SECONDS)
|
||||
try
|
||||
{
|
||||
ContentResponse response = client.newRequest("localhost", serverConnector.getLocalPort())
|
||||
.timeout(5, TimeUnit.SECONDS)
|
||||
.send();
|
||||
|
||||
Assert.fail();
|
||||
Assert.assertThat(response.getStatus(),Matchers.greaterThanOrEqualTo(500));
|
||||
}
|
||||
catch(ExecutionException e)
|
||||
{
|
||||
Assert.assertThat(e.getCause(),Matchers.instanceOf(IOException.class));
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
|
|
|
@ -402,10 +402,12 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor
|
|||
{
|
||||
if (!_response.isCommitted() && !_request.isHandled())
|
||||
_response.sendError(HttpStatus.NOT_FOUND_404);
|
||||
else if (!_response.isContentComplete(_response.getHttpOutput().getWritten()))
|
||||
_transport.abort(new IOException("insufficient content written"));
|
||||
_response.closeOutput();
|
||||
_request.setHandled(true);
|
||||
|
||||
_state.onComplete();
|
||||
_state.onComplete();
|
||||
|
||||
onCompleted();
|
||||
|
||||
|
|
|
@ -527,6 +527,8 @@ public class HttpConnection extends AbstractConnection implements Runnable, Http
|
|||
@Override
|
||||
public void abort(Throwable failure)
|
||||
{
|
||||
if (LOG.isDebugEnabled())
|
||||
LOG.debug("abort {} {}",this,failure);
|
||||
// Do a direct close of the output, as this may indicate to a client that the
|
||||
// response is bad either with RST or by abnormal completion of chunked response.
|
||||
getEndPoint().close();
|
||||
|
|
|
@ -685,6 +685,7 @@ public class HttpOutput extends ServletOutputStream implements Runnable
|
|||
if (LOG.isDebugEnabled())
|
||||
LOG.debug("sendContent({})", BufferUtil.toDetailString(content));
|
||||
|
||||
_written += content.remaining();
|
||||
write(content, true);
|
||||
closed();
|
||||
}
|
||||
|
@ -766,6 +767,7 @@ public class HttpOutput extends ServletOutputStream implements Runnable
|
|||
if (LOG.isDebugEnabled())
|
||||
LOG.debug("sendContent(buffer={},{})", BufferUtil.toDetailString(content), callback);
|
||||
|
||||
_written += content.remaining();
|
||||
write(content, true, new Callback.Nested(callback)
|
||||
{
|
||||
@Override
|
||||
|
@ -1280,6 +1282,7 @@ public class HttpOutput extends ServletOutputStream implements Runnable
|
|||
// write what we have
|
||||
_buffer.position(0);
|
||||
_buffer.limit(len);
|
||||
_written += len;
|
||||
write(_buffer, _eof, this);
|
||||
return Action.SCHEDULED;
|
||||
}
|
||||
|
@ -1338,6 +1341,7 @@ public class HttpOutput extends ServletOutputStream implements Runnable
|
|||
|
||||
// write what we have
|
||||
BufferUtil.flipToFlush(_buffer, 0);
|
||||
_written += _buffer.remaining();
|
||||
write(_buffer, _eof, this);
|
||||
|
||||
return Action.SCHEDULED;
|
||||
|
|
|
@ -879,13 +879,13 @@ public class Response implements HttpServletResponse
|
|||
if (isCommitted() || isIncluding())
|
||||
return;
|
||||
|
||||
_contentLength = len;
|
||||
if (_contentLength > 0)
|
||||
if (len>0)
|
||||
{
|
||||
long written = _out.getWritten();
|
||||
if (written > len)
|
||||
throw new IllegalArgumentException("setContentLength(" + len + ") when already written " + written);
|
||||
|
||||
_contentLength = len;
|
||||
_fields.putLongField(HttpHeader.CONTENT_LENGTH, len);
|
||||
if (isAllContentWritten(written))
|
||||
{
|
||||
|
@ -899,15 +899,19 @@ public class Response implements HttpServletResponse
|
|||
}
|
||||
}
|
||||
}
|
||||
else if (_contentLength==0)
|
||||
else if (len==0)
|
||||
{
|
||||
long written = _out.getWritten();
|
||||
if (written > 0)
|
||||
throw new IllegalArgumentException("setContentLength(0) when already written " + written);
|
||||
_contentLength = len;
|
||||
_fields.put(HttpHeader.CONTENT_LENGTH, "0");
|
||||
}
|
||||
else
|
||||
{
|
||||
_contentLength = len;
|
||||
_fields.remove(HttpHeader.CONTENT_LENGTH);
|
||||
}
|
||||
}
|
||||
|
||||
public long getContentLength()
|
||||
|
@ -919,6 +923,11 @@ public class Response implements HttpServletResponse
|
|||
{
|
||||
return (_contentLength >= 0 && written >= _contentLength);
|
||||
}
|
||||
|
||||
public boolean isContentComplete(long written)
|
||||
{
|
||||
return (_contentLength < 0 || written >= _contentLength);
|
||||
}
|
||||
|
||||
public void closeOutput() throws IOException
|
||||
{
|
||||
|
|
|
@ -98,6 +98,7 @@ public abstract class AbstractHttpTest
|
|||
writer.write("\r\n");
|
||||
writer.flush();
|
||||
|
||||
// TODO replace the SimpleHttp stuff
|
||||
SimpleHttpResponse response = httpParser.readResponse(reader);
|
||||
if ("HTTP/1.1".equals(httpVersion)
|
||||
&& response.getHeaders().get("content-length") == null
|
||||
|
|
|
@ -22,6 +22,7 @@ import static org.hamcrest.Matchers.is;
|
|||
import static org.hamcrest.Matchers.not;
|
||||
import static org.junit.Assert.assertThat;
|
||||
|
||||
import java.io.EOFException;
|
||||
import java.io.IOException;
|
||||
import java.util.Arrays;
|
||||
import java.util.Collection;
|
||||
|
@ -31,7 +32,9 @@ import javax.servlet.http.HttpServletRequest;
|
|||
import javax.servlet.http.HttpServletResponse;
|
||||
|
||||
import org.eclipse.jetty.http.HttpVersion;
|
||||
import org.eclipse.jetty.server.handler.AbstractHandler;
|
||||
import org.eclipse.jetty.toolchain.test.http.SimpleHttpResponse;
|
||||
import org.hamcrest.Matchers;
|
||||
import org.junit.Test;
|
||||
import org.junit.runner.RunWith;
|
||||
import org.junit.runners.Parameterized;
|
||||
|
@ -418,6 +421,50 @@ public class HttpManyWaysToCommitTest extends AbstractHttpTest
|
|||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testSetContentLengthFlushAndWriteInsufficientBytes() throws Exception
|
||||
{
|
||||
server.setHandler(new SetContentLengthAndWriteInsufficientBytesHandler(true));
|
||||
server.start();
|
||||
try
|
||||
{
|
||||
// TODO This test is compromised by the SimpleHttpResponse mechanism.
|
||||
// Replace with a better client
|
||||
|
||||
SimpleHttpResponse response = executeRequest();
|
||||
String failed_body = ""+(char)-1+(char)-1+(char)-1;
|
||||
assertThat("response code is 200", response.getCode(), is("200"));
|
||||
assertThat(response.getBody(), Matchers.endsWith(failed_body));
|
||||
assertHeader(response, "content-length", "6");
|
||||
}
|
||||
catch(EOFException e)
|
||||
{
|
||||
// possible good response
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testSetContentLengthAndWriteInsufficientBytes() throws Exception
|
||||
{
|
||||
server.setHandler(new SetContentLengthAndWriteInsufficientBytesHandler(false));
|
||||
server.start();
|
||||
|
||||
try
|
||||
{
|
||||
// TODO This test is compromised by the SimpleHttpResponse mechanism.
|
||||
// Replace with a better client
|
||||
SimpleHttpResponse response = executeRequest();
|
||||
String failed_body = ""+(char)-1+(char)-1+(char)-1;
|
||||
assertThat("response code is 200", response.getCode(), is("200"));
|
||||
assertThat(response.getBody(), Matchers.endsWith(failed_body));
|
||||
assertHeader(response, "content-length", "6");
|
||||
}
|
||||
catch(EOFException e)
|
||||
{
|
||||
// expected
|
||||
}
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testSetContentLengthAndWriteExactlyThatAmountOfBytes() throws Exception
|
||||
{
|
||||
|
@ -444,6 +491,25 @@ public class HttpManyWaysToCommitTest extends AbstractHttpTest
|
|||
assertThat("response body is foo", response.getBody(), is("foo"));
|
||||
}
|
||||
|
||||
private class SetContentLengthAndWriteInsufficientBytesHandler extends AbstractHandler
|
||||
{
|
||||
boolean flush;
|
||||
private SetContentLengthAndWriteInsufficientBytesHandler(boolean flush)
|
||||
{
|
||||
this.flush = flush;
|
||||
}
|
||||
|
||||
@Override
|
||||
public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException
|
||||
{
|
||||
baseRequest.setHandled(true);
|
||||
response.setContentLength(6);
|
||||
if (flush)
|
||||
response.flushBuffer();
|
||||
response.getWriter().write("foo");
|
||||
}
|
||||
}
|
||||
|
||||
private class SetContentLengthAndWriteThatAmountOfBytesHandler extends ThrowExceptionOnDemandHandler
|
||||
{
|
||||
private SetContentLengthAndWriteThatAmountOfBytesHandler(boolean throwException)
|
||||
|
|
|
@ -471,6 +471,112 @@ public class AsyncIOServletTest extends AbstractTest
|
|||
assertTrue(clientLatch.await(5, TimeUnit.SECONDS));
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testAsyncWriteLessThanContentLengthFlushed() throws Exception
|
||||
{
|
||||
CountDownLatch complete = new CountDownLatch(1);
|
||||
start(new HttpServlet()
|
||||
{
|
||||
@Override
|
||||
protected void service(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException
|
||||
{
|
||||
response.setContentLength(10);
|
||||
|
||||
AsyncContext async = request.startAsync();
|
||||
ServletOutputStream out = response.getOutputStream();
|
||||
AtomicInteger state = new AtomicInteger(0);
|
||||
|
||||
out.setWriteListener(new WriteListener()
|
||||
{
|
||||
@Override
|
||||
public void onWritePossible() throws IOException
|
||||
{
|
||||
while(true)
|
||||
{
|
||||
if (!out.isReady())
|
||||
return;
|
||||
|
||||
switch(state.get())
|
||||
{
|
||||
case 0:
|
||||
state.incrementAndGet();
|
||||
WriteListener listener = this;
|
||||
new Thread(()->
|
||||
{
|
||||
try
|
||||
{
|
||||
Thread.sleep(50);
|
||||
listener.onWritePossible();
|
||||
}
|
||||
catch(Exception e)
|
||||
{}
|
||||
}).start();
|
||||
return;
|
||||
|
||||
case 1:
|
||||
state.incrementAndGet();
|
||||
out.flush();
|
||||
break;
|
||||
|
||||
case 2:
|
||||
state.incrementAndGet();
|
||||
out.write("12345".getBytes());
|
||||
break;
|
||||
|
||||
case 3:
|
||||
async.complete();
|
||||
complete.countDown();
|
||||
return;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@Override
|
||||
public void onError(Throwable t)
|
||||
{
|
||||
}
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
AtomicBoolean failed = new AtomicBoolean(false);
|
||||
CountDownLatch clientLatch = new CountDownLatch(3);
|
||||
client.newRequest(newURI())
|
||||
.path(servletPath)
|
||||
.onResponseHeaders(response ->
|
||||
{
|
||||
if (response.getStatus() == HttpStatus.OK_200)
|
||||
clientLatch.countDown();
|
||||
})
|
||||
.onResponseContent(new Response.ContentListener()
|
||||
{
|
||||
@Override
|
||||
public void onContent(Response response, ByteBuffer content)
|
||||
{
|
||||
// System.err.println("Content: "+BufferUtil.toDetailString(content));
|
||||
}
|
||||
})
|
||||
.onResponseFailure(new Response.FailureListener()
|
||||
{
|
||||
@Override
|
||||
public void onFailure(Response response, Throwable failure)
|
||||
{
|
||||
clientLatch.countDown();
|
||||
}
|
||||
})
|
||||
.send(result ->
|
||||
{
|
||||
failed.set(result.isFailed());
|
||||
clientLatch.countDown();
|
||||
clientLatch.countDown();
|
||||
clientLatch.countDown();
|
||||
});
|
||||
|
||||
assertTrue(complete.await(10, TimeUnit.SECONDS));
|
||||
assertTrue(clientLatch.await(10, TimeUnit.SECONDS));
|
||||
assertTrue(failed.get());
|
||||
}
|
||||
|
||||
@Test
|
||||
public void testIsReadyAtEOF() throws Exception
|
||||
{
|
||||
|
@ -979,7 +1085,7 @@ public class AsyncIOServletTest extends AbstractTest
|
|||
while (input.isReady() && !input.isFinished())
|
||||
{
|
||||
int read = input.read();
|
||||
System.err.printf("%x%n", read);
|
||||
// System.err.printf("%x%n", read);
|
||||
readLatch.countDown();
|
||||
}
|
||||
}
|
||||
|
|
Loading…
Reference in New Issue