From 52172fb3c4a650f5a5be17f02e17a12e9481ee1b Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Mon, 21 Jul 2014 19:21:44 +0200 Subject: [PATCH] 440038 - Content decoding may fail. Properly looping around the decoding step to ensure that the encoded content is fully consumed. --- .../jetty/client/GZIPContentDecoder.java | 2 +- .../eclipse/jetty/client/HttpReceiver.java | 54 ++++-- .../jetty/client/HttpClientGZIPTest.java | 156 ++++++++++++++++++ .../eclipse/jetty/client/HttpClientTest.java | 28 ---- .../client/http/HttpReceiverOverHTTPTest.java | 24 +-- 5 files changed, 200 insertions(+), 64 deletions(-) create mode 100644 jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientGZIPTest.java diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/GZIPContentDecoder.java b/jetty-client/src/main/java/org/eclipse/jetty/client/GZIPContentDecoder.java index ab8f2a40b5f..6a7d6fa8ac0 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/GZIPContentDecoder.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/GZIPContentDecoder.java @@ -251,7 +251,7 @@ public class GZIPContentDecoder implements ContentDecoder else { // Accumulate inflated bytes and loop to see if we have finished - byte[] newOutput = Arrays.copyOf(output, output.length+decoded); + byte[] newOutput = Arrays.copyOf(output, output.length + decoded); System.arraycopy(bytes, 0, newOutput, output.length, decoded); output = newOutput; } diff --git a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpReceiver.java b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpReceiver.java index 356413a04ce..4b8546594cd 100644 --- a/jetty-client/src/main/java/org/eclipse/jetty/client/HttpReceiver.java +++ b/jetty-client/src/main/java/org/eclipse/jetty/client/HttpReceiver.java @@ -21,6 +21,7 @@ package org.eclipse.jetty.client; import java.io.IOException; import java.net.URI; import java.nio.ByteBuffer; +import java.util.ArrayList; import java.util.Collections; import java.util.Enumeration; import java.util.HashMap; @@ -282,7 +283,7 @@ public abstract class HttpReceiver * @param buffer the response HTTP content buffer * @return whether the processing should continue */ - protected boolean responseContent(HttpExchange exchange, ByteBuffer buffer, Callback callback) + protected boolean responseContent(HttpExchange exchange, ByteBuffer buffer, final Callback callback) { out: while (true) { @@ -307,19 +308,46 @@ public abstract class HttpReceiver if (LOG.isDebugEnabled()) LOG.debug("Response content {}{}{}", response, System.lineSeparator(), BufferUtil.toDetailString(buffer)); - ContentDecoder decoder = this.decoder; - if (decoder != null) - { - buffer = decoder.decode(buffer); - - // TODO If the decoder consumes all the content, should we return here? - - if (LOG.isDebugEnabled()) - LOG.debug("Response content decoded ({}) {}{}{}", decoder, response, System.lineSeparator(), BufferUtil.toDetailString(buffer)); - } - ResponseNotifier notifier = getHttpDestination().getResponseNotifier(); - notifier.notifyContent(exchange.getConversation().getResponseListeners(), response, buffer, callback); + List listeners = exchange.getConversation().getResponseListeners(); + + ContentDecoder decoder = this.decoder; + if (decoder == null) + { + notifier.notifyContent(listeners, response, buffer, callback); + } + else + { + List decodeds = new ArrayList<>(2); + while (buffer.hasRemaining()) + { + ByteBuffer decoded = decoder.decode(buffer); + if (!decoded.hasRemaining()) + continue; + decodeds.add(decoded); + if (LOG.isDebugEnabled()) + LOG.debug("Response content decoded ({}) {}{}{}", decoder, response, System.lineSeparator(), BufferUtil.toDetailString(decoded)); + } + + if (decodeds.isEmpty()) + { + callback.succeeded(); + } + else + { + Callback partial = new Callback.Adapter() + { + @Override + public void failed(Throwable x) + { + callback.failed(x); + } + }; + + for (int i = 1, size = decodeds.size(); i <= size; ++i) + notifier.notifyContent(listeners, response, decodeds.get(i - 1), i < size ? partial : callback); + } + } if (!updateResponseState(ResponseState.TRANSIENT, ResponseState.CONTENT)) terminateResponse(exchange, failure); diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientGZIPTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientGZIPTest.java new file mode 100644 index 00000000000..e8b8cfa60b3 --- /dev/null +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientGZIPTest.java @@ -0,0 +1,156 @@ +// +// ======================================================================== +// Copyright (c) 1995-2014 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.client; + +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.InterruptedIOException; +import java.util.Arrays; +import java.util.concurrent.TimeUnit; +import java.util.zip.GZIPOutputStream; +import javax.servlet.ServletException; +import javax.servlet.ServletOutputStream; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.eclipse.jetty.client.api.ContentResponse; +import org.eclipse.jetty.server.Request; +import org.eclipse.jetty.server.handler.AbstractHandler; +import org.eclipse.jetty.util.ssl.SslContextFactory; +import org.junit.Assert; +import org.junit.Test; + +public class HttpClientGZIPTest extends AbstractHttpClientServerTest +{ + public HttpClientGZIPTest(SslContextFactory sslContextFactory) + { + super(sslContextFactory); + } + + @Test + public void testGZIPContentEncoding() throws Exception + { + final byte[] data = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9}; + start(new AbstractHandler() + { + @Override + public void handle(String target, org.eclipse.jetty.server.Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + baseRequest.setHandled(true); + response.setHeader("Content-Encoding", "gzip"); + GZIPOutputStream gzipOutput = new GZIPOutputStream(response.getOutputStream()); + gzipOutput.write(data); + gzipOutput.finish(); + } + }); + + ContentResponse response = client.newRequest("localhost", connector.getLocalPort()) + .scheme(scheme) + .timeout(5, TimeUnit.SECONDS) + .send(); + + Assert.assertEquals(200, response.getStatus()); + Assert.assertArrayEquals(data, response.getContent()); + } + + @Test + public void testGZIPContentOneByteAtATime() throws Exception + { + final byte[] data = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9}; + start(new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + baseRequest.setHandled(true); + response.setHeader("Content-Encoding", "gzip"); + + ByteArrayOutputStream gzipData = new ByteArrayOutputStream(); + GZIPOutputStream gzipOutput = new GZIPOutputStream(gzipData); + gzipOutput.write(data); + gzipOutput.finish(); + + ServletOutputStream output = response.getOutputStream(); + byte[] gzipBytes = gzipData.toByteArray(); + for (byte gzipByte : gzipBytes) + { + output.write(gzipByte); + output.flush(); + sleep(100); + } + } + + private void sleep(long ms) throws IOException + { + try + { + TimeUnit.MILLISECONDS.sleep(ms); + } + catch (InterruptedException x) + { + throw new InterruptedIOException(); + } + } + }); + + ContentResponse response = client.newRequest("localhost", connector.getLocalPort()) + .scheme(scheme) + .send(); + + Assert.assertEquals(200, response.getStatus()); + Assert.assertArrayEquals(data, response.getContent()); + } + + @Test + public void testGZIPContentSentTwiceInOneWrite() throws Exception + { + final byte[] data = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9}; + start(new AbstractHandler() + { + @Override + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + baseRequest.setHandled(true); + response.setHeader("Content-Encoding", "gzip"); + + ByteArrayOutputStream gzipData = new ByteArrayOutputStream(); + GZIPOutputStream gzipOutput = new GZIPOutputStream(gzipData); + gzipOutput.write(data); + gzipOutput.finish(); + + byte[] gzipBytes = gzipData.toByteArray(); + byte[] content = Arrays.copyOf(gzipBytes, 2 * gzipBytes.length); + System.arraycopy(gzipBytes, 0, content, gzipBytes.length, gzipBytes.length); + + ServletOutputStream output = response.getOutputStream(); + output.write(content); + } + }); + + ContentResponse response = client.newRequest("localhost", connector.getLocalPort()) + .scheme(scheme) + .send(); + + Assert.assertEquals(200, response.getStatus()); + + byte[] expected = Arrays.copyOf(data, 2 * data.length); + System.arraycopy(data, 0, expected, data.length, data.length); + Assert.assertArrayEquals(expected, response.getContent()); + } +} diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTest.java index 9b77675ff08..1cab93ae99a 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/HttpClientTest.java @@ -43,8 +43,6 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; -import java.util.zip.GZIPOutputStream; - import javax.servlet.ServletException; import javax.servlet.ServletOutputStream; import javax.servlet.http.HttpServletRequest; @@ -672,32 +670,6 @@ public class HttpClientTest extends AbstractHttpClientServerTest Assert.assertTrue(latch.await(5, TimeUnit.SECONDS)); } - @Test - public void test_GZIP_ContentEncoding() throws Exception - { - final byte[] data = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9}; - start(new AbstractHandler() - { - @Override - public void handle(String target, org.eclipse.jetty.server.Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException - { - baseRequest.setHandled(true); - response.setHeader("Content-Encoding", "gzip"); - GZIPOutputStream gzipOutput = new GZIPOutputStream(response.getOutputStream()); - gzipOutput.write(data); - gzipOutput.finish(); - } - }); - - ContentResponse response = client.newRequest("localhost", connector.getLocalPort()) - .scheme(scheme) - .timeout(5, TimeUnit.SECONDS) - .send(); - - Assert.assertEquals(200, response.getStatus()); - Assert.assertArrayEquals(data, response.getContent()); - } - @Slow @Test public void test_Request_IdleTimeout() throws Exception diff --git a/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpReceiverOverHTTPTest.java b/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpReceiverOverHTTPTest.java index b6662cd1581..32db8dd5eaf 100644 --- a/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpReceiverOverHTTPTest.java +++ b/jetty-client/src/test/java/org/eclipse/jetty/client/http/HttpReceiverOverHTTPTest.java @@ -23,7 +23,6 @@ import java.io.EOFException; import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; import java.util.Collections; -import java.util.concurrent.CountDownLatch; import java.util.concurrent.ExecutionException; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; @@ -222,26 +221,8 @@ public class HttpReceiverOverHTTPTest "Content-Length: " + gzip.length + "\r\n" + "Content-Encoding: gzip\r\n" + "\r\n"); - - HttpRequest request = (HttpRequest)client.newRequest("http://localhost"); - final CountDownLatch latch = new CountDownLatch(1); - FutureResponseListener listener = new FutureResponseListener(request) - { - @Override - public void onContent(Response response, ByteBuffer content) - { - boolean hadRemaining=content.hasRemaining(); - super.onContent(response, content); - - // TODO gzip decoding can pass on empty chunks. Currently ignoring them here, but could be done at the decoder??? - if (hadRemaining) // Ignore empty chunks - latch.countDown(); - } - }; - HttpExchange exchange = new HttpExchange(destination, request, Collections.singletonList(listener)); - connection.getHttpChannel().associate(exchange); - exchange.requestComplete(); - exchange.terminateRequest(null); + HttpExchange exchange = newExchange(); + FutureResponseListener listener = (FutureResponseListener)exchange.getResponseListeners().get(0); connection.getHttpChannel().receive(); endPoint.reset(); @@ -260,7 +241,6 @@ public class HttpReceiverOverHTTPTest ContentResponse response = listener.get(5, TimeUnit.SECONDS); Assert.assertNotNull(response); Assert.assertEquals(200, response.getStatus()); - Assert.assertTrue(latch.await(5, TimeUnit.SECONDS)); Assert.assertArrayEquals(data, response.getContent()); } }