SOLR-14457: Fix closing of malformed GZIPInputStream

solr/CHANGES.txt

@@ -431,6 +431,8 @@ Bug Fixes
 
 * SOLR-13209: Fix NPE when no grouping query specified  (Collins Abanda via Mike Drob)
 
+* 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,7 +494,11 @@ private static class DeflateDecompressingEntity extends
     public DeflateDecompressingEntity(final HttpEntity entity) {
       super(entity);
     }
-
+
+    /**
+     * InflaterInputStream does not throw a ZipException in the constructor,
+     * so it does not need the same checks as the GZIPInputStream.
+     */
     @Override
     public InputStream getContent() throws IOException, IllegalStateException {
       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,13 @@
 package org.apache.solr.client.solrj.impl;
 
 import javax.net.ssl.HostnameVerifier;
+import java.io.ByteArrayInputStream;
 import java.io.IOException;
+import java.nio.charset.StandardCharsets;
 
+import org.apache.http.HttpEntity;
+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 +51,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 +89,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 +97,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 +111,21 @@ 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(new ByteArrayInputStream("this is not compressed".getBytes(StandardCharsets.UTF_8)));
+    HttpClientUtil.GzipDecompressingEntity gzipDecompressingEntity = new HttpClientUtil.GzipDecompressingEntity(baseEntity);
+    expectThrows(IOException.class, "An IOException wrapping a ZIPException should be thrown when loading a malformed GZIP Entity Content", gzipDecompressingEntity::getContent);
+    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);
+    expectThrows(IOException.class, "An IOException wrapping a ZIPException should be thrown when loading a malformed GZIP Entity Content", gzipDecompressingEntity::getContent);
+    expectThrows(IOException.class, "An IOException should be thrown again when re-loading a repeatable malformed GZIP Entity Content", gzipDecompressingEntity::getContent);
+  }
 }