SOLR-14457: Fix closing of malformed GZIPInputStream (apache#283)

solr/CHANGES.txt

@@ -442,6 +442,8 @@ Bug Fixes
 
 * SOLR-6156: Fix NullPointerException if group.field grouping is used with rows=0 and timeAllowed. (Modassar Ather, Christine Poerschke)
 
+* SOLR-14457: SolrClient leaks connections on compressed responses if the response is malformed (Houston Putman)
+
 Other Changes
 ---------------------
 * SOLR-15566: Clarify ref guide documentation about SQL queries with `SELECT *` requiring a `LIMIT` clause (Timothy Potter)

solr/solrj/src/java/org/apache/solr/client/solrj/impl/HttpClientUtil.java

@@ -61,6 +61,7 @@
 import org.apache.solr.common.params.ModifiableSolrParams;
 import org.apache.solr.common.params.SolrParams;
 import org.apache.solr.common.util.ObjectReleaseTracker;
+import org.apache.solr.common.util.Utils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -447,14 +448,39 @@ public void process(final HttpResponse response, final HttpContext context)
     }
   }
 
-  private static class GzipDecompressingEntity extends HttpEntityWrapper {
+  protected static class GzipDecompressingEntity extends HttpEntityWrapper {
+    private boolean gzipInputStreamCreated = false;
+    private InputStream gzipInputStream = null;
+
     public GzipDecompressingEntity(final HttpEntity entity) {
       super(entity);
     }
-
+
+    /**
+     * Return a InputStream of uncompressed data.
+     * If there is an issue with the compression of the data, a null InputStream will be returned,
+     * and the underlying compressed InputStream will be closed.
+     *
+     * The same input stream will be returned if the underlying entity is not repeatable.
+     * If the underlying entity is repeatable, then a new input stream will be created.
+     */
     @Override
     public InputStream getContent() throws IOException, IllegalStateException {
-      return new GZIPInputStream(wrappedEntity.getContent());
+      if (!gzipInputStreamCreated || wrappedEntity.isRepeatable()) {
+        gzipInputStreamCreated = true;
+        InputStream wrappedContent = wrappedEntity.getContent();
+        if (wrappedContent != null) {
+          try {
+            gzipInputStream = new GZIPInputStream(wrappedContent);
+          } catch (IOException ioException) {
+            try (wrappedContent) {
+              Utils.readFully(wrappedContent);
+            } catch (IOException ignored) {}
+            throw new IOException("Cannot open GZipInputStream for response", ioException);
+          }
+        }
+      }
+      return gzipInputStream;
     }
 
     @Override
@@ -468,9 +494,11 @@ private static class DeflateDecompressingEntity extends
     public DeflateDecompressingEntity(final HttpEntity entity) {
       super(entity);
     }
-    
+
     @Override
     public InputStream getContent() throws IOException, IllegalStateException {
+      // InflaterInputStream does not throw a ZipException in the constructor,
+      // so it does not need the same checks as the GZIPInputStream.
       return new InflaterInputStream(wrappedEntity.getContent());
     }
   }

solr/solrj/src/java/org/apache/solr/common/util/Utils.java

@@ -648,7 +648,7 @@ public static void consumeFully(HttpEntity entity) {
    * @param is to read
    * @throws IOException on problem with IO
    */
-  private static void readFully(InputStream is) throws IOException {
+  public static void readFully(InputStream is) throws IOException {
     is.skip(is.available());
     while (is.read() != -1) {
     }

solr/solrj/src/test/org/apache/solr/client/solrj/impl/HttpClientUtilTest.java

@@ -17,8 +17,19 @@
 package org.apache.solr.client.solrj.impl;
 
 import javax.net.ssl.HostnameVerifier;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
 import java.io.IOException;
-
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+import java.util.zip.GZIPOutputStream;
+import java.util.zip.ZipException;
+
+import org.apache.commons.io.IOUtils;
+import org.apache.http.HttpEntity;
+import org.apache.http.entity.ByteArrayEntity;
+import org.apache.http.entity.InputStreamEntity;
+import org.apache.http.entity.StringEntity;
 import org.apache.solr.SolrTestCase;
 import org.apache.solr.client.solrj.impl.HttpClientUtil.SocketFactoryRegistryProvider;
 
@@ -46,8 +57,7 @@ public void resetHttpClientBuilder() {
   }
 
   @Test
-  // commented out on: 24-Dec-2018   @BadApple(bugUrl="https://issues.apache.org/jira/browse/SOLR-12028") // added 20-Sep-2018
-  public void testSSLSystemProperties() throws IOException {
+  public void testSSLSystemProperties() {
 
     assertNotNull("HTTPS scheme could not be created using system defaults",
                   HttpClientUtil.getSocketFactoryRegistryProvider().getSocketFactoryRegistry().lookup("https"));
@@ -85,7 +95,6 @@ private void assertSSLHostnameVerifier(Class<? extends HostnameVerifier> expecte
   }
 
   @Test
-  // commented out on: 24-Dec-2018   @BadApple(bugUrl="https://issues.apache.org/jira/browse/SOLR-12028") // added 20-Sep-2018
   public void testToBooleanDefaultIfNull() throws Exception {
     assertFalse(HttpClientUtil.toBooleanDefaultIfNull(Boolean.FALSE, true));
     assertTrue(HttpClientUtil.toBooleanDefaultIfNull(Boolean.TRUE, false));
@@ -94,8 +103,7 @@ public void testToBooleanDefaultIfNull() throws Exception {
   }
 
   @Test
-  // commented out on: 24-Dec-2018   @BadApple(bugUrl="https://issues.apache.org/jira/browse/SOLR-12028") // added 20-Sep-2018
-  public void testToBooleanObject() throws Exception {
+  public void testToBooleanObject() {
     assertEquals(Boolean.TRUE, HttpClientUtil.toBooleanObject("true"));
     assertEquals(Boolean.TRUE, HttpClientUtil.toBooleanObject("TRUE"));
     assertEquals(Boolean.TRUE, HttpClientUtil.toBooleanObject("tRuE"));
@@ -109,4 +117,60 @@ public void testToBooleanObject() throws Exception {
     assertEquals(null, HttpClientUtil.toBooleanObject("foo"));
     assertEquals(null, HttpClientUtil.toBooleanObject(null));
   }
+
+  @Test
+  public void testNonRepeatableMalformedGzipEntityAutoClosed() throws IOException {
+    HttpEntity baseEntity = new InputStreamEntity(IOUtils.toInputStream("this is not compressed", StandardCharsets.UTF_8));
+    HttpClientUtil.GzipDecompressingEntity gzipDecompressingEntity = new HttpClientUtil.GzipDecompressingEntity(baseEntity);
+    Throwable error = expectThrows(IOException.class, "An IOException wrapping a ZIPException should be thrown when loading a malformed GZIP Entity Content", gzipDecompressingEntity::getContent);
+    assertEquals("IOException should be caused by a ZipException", ZipException.class, error.getCause() == null ? null : error.getCause().getClass());
+    assertNull("The second time getContent is called, null should be returned since the underlying entity is non-repeatable", gzipDecompressingEntity.getContent());
+    assertEquals("No more content should be available after the GZIP Entity failed to load", 0, baseEntity.getContent().available());
+  }
+
+  @Test
+  public void testRepeatableMalformedGzipEntity() throws IOException {
+    HttpEntity baseEntity = new StringEntity("this is not compressed");
+    HttpClientUtil.GzipDecompressingEntity gzipDecompressingEntity = new HttpClientUtil.GzipDecompressingEntity(baseEntity);
+    Throwable error = expectThrows(IOException.class, "An IOException wrapping a ZIPException should be thrown when loading a malformed GZIP Entity Content", gzipDecompressingEntity::getContent);
+    assertEquals("IOException should be caused by a ZipException", ZipException.class, error.getCause() == null ? null : error.getCause().getClass());
+    error = expectThrows(IOException.class, "An IOException should be thrown again when re-loading a repeatable malformed GZIP Entity Content", gzipDecompressingEntity::getContent);
+    assertEquals("IOException should be caused by a ZipException", ZipException.class, error.getCause() == null ? null : error.getCause().getClass());
+  }
+
+  @Test
+  public void testRepeatableGzipEntity() throws IOException {
+    String testString = "this is compressed";
+    ByteArrayOutputStream baos = new ByteArrayOutputStream();
+    try (GZIPOutputStream gzipOutputStream = new GZIPOutputStream(baos)) {
+      IOUtils.write(testString, gzipOutputStream, StandardCharsets.UTF_8);
+    }
+    // Use an ByteArrayEntity because it is repeatable
+    HttpEntity baseEntity = new ByteArrayEntity(baos.toByteArray());
+    HttpClientUtil.GzipDecompressingEntity gzipDecompressingEntity = new HttpClientUtil.GzipDecompressingEntity(baseEntity);
+    try (InputStream stream = gzipDecompressingEntity.getContent()){
+      assertEquals("Entity incorrect after decompression", testString, IOUtils.toString(stream, StandardCharsets.UTF_8));
+    }
+    try (InputStream stream = gzipDecompressingEntity.getContent()){
+      assertEquals("Entity incorrect after decompression after repeating", testString, IOUtils.toString(stream, StandardCharsets.UTF_8));
+    }
+  }
+
+  @Test
+  public void testNonRepeatableGzipEntity() throws IOException {
+    String testString = "this is compressed";
+    ByteArrayOutputStream baos = new ByteArrayOutputStream();
+    try (GZIPOutputStream gzipOutputStream = new GZIPOutputStream(baos)) {
+      IOUtils.write(testString, gzipOutputStream, StandardCharsets.UTF_8);
+    }
+    // Use an InputStreamEntity because it is non-repeatable
+    HttpEntity baseEntity = new InputStreamEntity(new ByteArrayInputStream(baos.toByteArray()));
+    HttpClientUtil.GzipDecompressingEntity gzipDecompressingEntity = new HttpClientUtil.GzipDecompressingEntity(baseEntity);
+    try (InputStream stream = gzipDecompressingEntity.getContent()){
+      assertEquals("Entity incorrect after decompression", testString, IOUtils.toString(stream, StandardCharsets.UTF_8));
+    }
+    try (InputStream stream = gzipDecompressingEntity.getContent()){
+      expectThrows(IOException.class, "Entity Content should already be closed since the input is non-repeatable", stream::available);
+    }
+  }
 }