From 0666924e8c41428adb329e47da73546c96e13825 Mon Sep 17 00:00:00 2001 From: George Agnelli Date: Tue, 7 Oct 2014 14:56:15 +0100 Subject: [PATCH] Don't close the connection whenever Expect: 100-continue is missing. Motivation: The 4.1.0-Beta3 implementation of HttpObjectAggregator.handleOversizedMessage closes the connection if the client sent oversized chunked data with no Expect: 100-continue header. This causes a broken pipe or "connection reset by peer" error in some clients (tested on Firefox 31 OS X 10.9.5, async-http-client 1.8.14). This part of the HTTP 1.1 spec (below) seems to say that in this scenario the connection should not be closed (unless the intention is to be very strict about how data should be sent). http://www.w3.org/Protocols/rfc2616/rfc2616-sec8.html "If an origin server receives a request that does not include an Expect request-header field with the "100-continue" expectation, the request includes a request body, and the server responds with a final status code before reading the entire request body from the transport connection, then the server SHOULD NOT close the transport connection until it has read the entire request, or until the client closes the connection. Otherwise, the client might not reliably receive the response message. However, this requirement is not be construed as preventing a server from defending itself against denial-of-service attacks, or from badly broken client implementations." Modifications: Change HttpObjectAggregator.handleOversizedMessage to close the connection only if keep-alive is off and Expect: 100-continue is missing. Update test to reflect the change. Result: Broken pipe and connection reset errors on the client are avoided when oversized data is sent. --- .../codec/http/HttpObjectAggregator.java | 5 +- .../codec/http/HttpObjectAggregatorTest.java | 68 ++++++++++++++++++- 2 files changed, 68 insertions(+), 5 deletions(-) diff --git a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java index ab1f5ef16c..bbb073a449 100644 --- a/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java +++ b/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java @@ -161,10 +161,9 @@ public class HttpObjectAggregator }); // If the client started to send data already, close because it's impossible to recover. - // If 'Expect: 100-continue' is missing, close becuase it's impossible to recover. - // If keep-alive is off, no need to leave the connection open. + // If keep-alive is off and 'Expect: 100-continue' is missing, no need to leave the connection open. if (oversized instanceof FullHttpMessage || - !HttpHeaders.is100ContinueExpected(oversized) || !HttpHeaders.isKeepAlive(oversized)) { + (!HttpHeaders.is100ContinueExpected(oversized) && !HttpHeaders.isKeepAlive(oversized))) { future.addListener(ChannelFutureListener.CLOSE); } diff --git a/codec-http/src/test/java/io/netty/handler/codec/http/HttpObjectAggregatorTest.java b/codec-http/src/test/java/io/netty/handler/codec/http/HttpObjectAggregatorTest.java index 276c305a27..21d9b865ae 100644 --- a/codec-http/src/test/java/io/netty/handler/codec/http/HttpObjectAggregatorTest.java +++ b/codec-http/src/test/java/io/netty/handler/codec/http/HttpObjectAggregatorTest.java @@ -223,6 +223,53 @@ public class HttpObjectAggregatorTest { assertFalse(embedder.finish()); } + @Test + public void testRequestAfterOversized100ContinueAndDecoder() { + EmbeddedChannel embedder = new EmbeddedChannel(new HttpRequestDecoder(), new HttpObjectAggregator(15)); + + // Write first request with Expect: 100-continue + HttpRequest message = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.PUT, "http://localhost"); + HttpHeaders.set100ContinueExpected(message); + HttpHeaders.setContentLength(message, 16); + + HttpContent chunk1 = releaseLater(new DefaultHttpContent(Unpooled.copiedBuffer("some", CharsetUtil.US_ASCII))); + HttpContent chunk2 = releaseLater(new DefaultHttpContent(Unpooled.copiedBuffer("test", CharsetUtil.US_ASCII))); + HttpContent chunk3 = LastHttpContent.EMPTY_LAST_CONTENT; + + // Send a request with 100-continue + large Content-Length header value. + assertFalse(embedder.writeInbound(message)); + + // The agregator should respond with '413 Request Entity Too Large.' + FullHttpResponse response = embedder.readOutbound(); + assertEquals(HttpResponseStatus.REQUEST_ENTITY_TOO_LARGE, response.status()); + assertEquals("0", response.headers().get(Names.CONTENT_LENGTH)); + + // An ill-behaving client could continue to send data without a respect, and such data should be discarded. + assertFalse(embedder.writeInbound(chunk1)); + + // The aggregator should not close the connection because keep-alive is on. + assertTrue(embedder.isOpen()); + + // Now send a valid request. + HttpRequest message2 = new DefaultHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.PUT, "http://localhost"); + + assertFalse(embedder.writeInbound(message2)); + assertFalse(embedder.writeInbound(chunk2)); + assertTrue(embedder.writeInbound(chunk3)); + + FullHttpRequest fullMsg = embedder.readInbound(); + assertNotNull(fullMsg); + + assertEquals( + chunk2.content().readableBytes() + chunk3.content().readableBytes(), + HttpHeaders.getContentLength(fullMsg)); + + assertEquals(HttpHeaders.getContentLength(fullMsg), fullMsg.content().readableBytes()); + + fullMsg.release(); + assertFalse(embedder.finish()); + } + private static void checkOversizedRequest(HttpRequest message) { EmbeddedChannel embedder = new EmbeddedChannel(new HttpObjectAggregator(4)); @@ -230,8 +277,25 @@ public class HttpObjectAggregatorTest { HttpResponse response = embedder.readOutbound(); assertEquals(HttpResponseStatus.REQUEST_ENTITY_TOO_LARGE, response.status()); assertEquals("0", response.headers().get(Names.CONTENT_LENGTH)); - assertFalse(embedder.isOpen()); - assertFalse(embedder.finish()); + + if (serverShouldCloseConnection(message)) { + assertFalse(embedder.isOpen()); + assertFalse(embedder.finish()); + } else { + assertTrue(embedder.isOpen()); + } + } + + private static boolean serverShouldCloseConnection(HttpRequest message) { + // The connection should only be kept open if Expect: 100-continue is set, + // or if keep-alive is on. + if (HttpHeaders.is100ContinueExpected(message)) { + return false; + } + if (HttpHeaders.isKeepAlive(message)) { + return false; + } + return true; } @Test