NETWORKING: Fix Netty Leaks by upgrading to 4.1.28 (#32511)

* Upgrade to `4.1.28` since the problem reported in #32487 is a bug in Netty itself (see netty/netty#7337) * Fixed other leaks in test code that now showed up due to fixes improvements in leak reporting in the newer version * Needed to extend permissions for netty common package because it now sets a classloader at runtime after changes in netty/netty@63bae09 * Adjusted forbidden APIs check accordingly * Closes #32487
elastic · Aug 1, 2018 · 4b199dd · 4b199dd
1 parent cc6d6ca
commit 4b199dd
Show file tree

Hide file tree

Showing 40 changed files with 137 additions and 76 deletions.
diff --git a/modules/transport-netty4/build.gradle b/modules/transport-netty4/build.gradle
@@ -34,13 +34,13 @@ compileTestJava.options.compilerArgs << "-Xlint:-cast,-deprecation,-rawtypes,-tr
 
 dependencies {
     // network stack
-    compile "io.netty:netty-buffer:4.1.16.Final"
-    compile "io.netty:netty-codec:4.1.16.Final"
-    compile "io.netty:netty-codec-http:4.1.16.Final"
-    compile "io.netty:netty-common:4.1.16.Final"
-    compile "io.netty:netty-handler:4.1.16.Final"
-    compile "io.netty:netty-resolver:4.1.16.Final"
-    compile "io.netty:netty-transport:4.1.16.Final"
+    compile "io.netty:netty-buffer:4.1.28.Final"
+    compile "io.netty:netty-codec:4.1.28.Final"
+    compile "io.netty:netty-codec-http:4.1.28.Final"
+    compile "io.netty:netty-common:4.1.28.Final"
+    compile "io.netty:netty-handler:4.1.28.Final"
+    compile "io.netty:netty-resolver:4.1.28.Final"
+    compile "io.netty:netty-transport:4.1.28.Final"
 }
 
 dependencyLicenses {
@@ -134,7 +134,6 @@ thirdPartyAudit.excludes = [
         'net.jpountz.xxhash.StreamingXXHash32',
         'net.jpountz.xxhash.XXHashFactory',
         'io.netty.internal.tcnative.CertificateRequestedCallback',
-        'io.netty.internal.tcnative.CertificateRequestedCallback$KeyMaterial',
         'io.netty.internal.tcnative.CertificateVerifier',
         'io.netty.internal.tcnative.SessionTicketKey',
         'io.netty.internal.tcnative.SniHostNameMatcher',
@@ -161,6 +160,6 @@ thirdPartyAudit.excludes = [
 
         'org.conscrypt.AllocatedBuffer',
         'org.conscrypt.BufferAllocator',
-        'org.conscrypt.Conscrypt$Engines',
+        'org.conscrypt.Conscrypt',
         'org.conscrypt.HandshakeListener'
 ]
diff --git a/modules/transport-netty4/licenses/netty-buffer-4.1.16.Final.jar.sha1 b/modules/transport-netty4/licenses/netty-buffer-4.1.16.Final.jar.sha1
diff --git a/modules/transport-netty4/licenses/netty-buffer-4.1.28.Final.jar.sha1 b/modules/transport-netty4/licenses/netty-buffer-4.1.28.Final.jar.sha1
@@ -0,0 +1 @@
+d6c2d13492778009d33f60e05ed90bcb535d1fd1
diff --git a/modules/transport-netty4/licenses/netty-codec-4.1.16.Final.jar.sha1 b/modules/transport-netty4/licenses/netty-codec-4.1.16.Final.jar.sha1
diff --git a/modules/transport-netty4/licenses/netty-codec-4.1.28.Final.jar.sha1 b/modules/transport-netty4/licenses/netty-codec-4.1.28.Final.jar.sha1
@@ -0,0 +1 @@
+a38361d893900947524f8a9da980555950e73d6a
diff --git a/modules/transport-netty4/licenses/netty-codec-http-4.1.16.Final.jar.sha1 b/modules/transport-netty4/licenses/netty-codec-http-4.1.16.Final.jar.sha1
diff --git a/modules/transport-netty4/licenses/netty-codec-http-4.1.28.Final.jar.sha1 b/modules/transport-netty4/licenses/netty-codec-http-4.1.28.Final.jar.sha1
@@ -0,0 +1 @@
+897100c1022c780b0a436b9349e507e8fa9800dc
diff --git a/modules/transport-netty4/licenses/netty-common-4.1.16.Final.jar.sha1 b/modules/transport-netty4/licenses/netty-common-4.1.16.Final.jar.sha1
diff --git a/modules/transport-netty4/licenses/netty-common-4.1.28.Final.jar.sha1 b/modules/transport-netty4/licenses/netty-common-4.1.28.Final.jar.sha1
@@ -0,0 +1 @@
+df69ce8bb9b544a71e7bbee290253cf7c93e6bad
diff --git a/modules/transport-netty4/licenses/netty-handler-4.1.16.Final.jar.sha1 b/modules/transport-netty4/licenses/netty-handler-4.1.16.Final.jar.sha1
diff --git a/modules/transport-netty4/licenses/netty-handler-4.1.28.Final.jar.sha1 b/modules/transport-netty4/licenses/netty-handler-4.1.28.Final.jar.sha1
@@ -0,0 +1 @@
+a035784682da0126bc25f10713dac732b5082a6d
diff --git a/modules/transport-netty4/licenses/netty-resolver-4.1.16.Final.jar.sha1 b/modules/transport-netty4/licenses/netty-resolver-4.1.16.Final.jar.sha1
diff --git a/modules/transport-netty4/licenses/netty-resolver-4.1.28.Final.jar.sha1 b/modules/transport-netty4/licenses/netty-resolver-4.1.28.Final.jar.sha1
@@ -0,0 +1 @@
+f33557dcb31fa20da075ac05e4808115e32ef9b7
diff --git a/modules/transport-netty4/licenses/netty-transport-4.1.16.Final.jar.sha1 b/modules/transport-netty4/licenses/netty-transport-4.1.16.Final.jar.sha1
diff --git a/modules/transport-netty4/licenses/netty-transport-4.1.28.Final.jar.sha1 b/modules/transport-netty4/licenses/netty-transport-4.1.28.Final.jar.sha1
@@ -0,0 +1 @@
+d2ef28f49d726737f0ffe84bf66529b3bf6e0c0d
diff --git a/modules/transport-netty4/src/main/plugin-metadata/plugin-security.policy b/modules/transport-netty4/src/main/plugin-metadata/plugin-security.policy
@@ -23,6 +23,9 @@ grant codeBase "${codebase.netty-common}" {
 
    // netty makes and accepts socket connections
    permission java.net.SocketPermission "*", "accept,connect";
+
+   // Netty sets custom classloader for some of its internal threads
+   permission java.lang.RuntimePermission "*", "setContextClassLoader";
 };
 
 grant codeBase "${codebase.netty-transport}" {

diff --git a/...s/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java b/...s/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4BadRequestTests.java
@@ -20,6 +20,7 @@
 package org.elasticsearch.http.netty4;
 
 import io.netty.handler.codec.http.FullHttpResponse;
+import io.netty.util.ReferenceCounted;
 import org.elasticsearch.ElasticsearchException;
 import org.elasticsearch.common.network.NetworkService;
 import org.elasticsearch.common.settings.Settings;
@@ -92,15 +93,19 @@ public void dispatchBadRequest(RestRequest request, RestChannel channel, ThreadC
             try (Netty4HttpClient nettyHttpClient = new Netty4HttpClient()) {
                 final Collection<FullHttpResponse> responses =
                         nettyHttpClient.get(transportAddress.address(), "/_cluster/settings?pretty=%");
-                assertThat(responses, hasSize(1));
-                assertThat(responses.iterator().next().status().code(), equalTo(400));
-                final Collection<String> responseBodies = Netty4HttpClient.returnHttpResponseBodies(responses);
-                assertThat(responseBodies, hasSize(1));
-                assertThat(responseBodies.iterator().next(), containsString("\"type\":\"bad_parameter_exception\""));
-                assertThat(
+                try {
+                    assertThat(responses, hasSize(1));
+                    assertThat(responses.iterator().next().status().code(), equalTo(400));
+                    final Collection<String> responseBodies = Netty4HttpClient.returnHttpResponseBodies(responses);
+                    assertThat(responseBodies, hasSize(1));
+                    assertThat(responseBodies.iterator().next(), containsString("\"type\":\"bad_parameter_exception\""));
+                    assertThat(
                         responseBodies.iterator().next(),
                         containsString(
-                                "\"reason\":\"java.lang.IllegalArgumentException: unterminated escape sequence at end of string: %\""));
+                            "\"reason\":\"java.lang.IllegalArgumentException: unterminated escape sequence at end of string: %\""));
+                } finally {
+                    responses.forEach(ReferenceCounted::release);
+                }
             }
         }
     }

diff --git a/...port-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpRequestSizeLimitIT.java b/...port-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpRequestSizeLimitIT.java
@@ -20,6 +20,7 @@
 package org.elasticsearch.http.netty4;
 
 import io.netty.handler.codec.http.FullHttpResponse;
+import io.netty.util.ReferenceCounted;
 import org.elasticsearch.ESNetty4IntegTestCase;
 import org.elasticsearch.common.collect.Tuple;
 import org.elasticsearch.common.settings.Settings;
@@ -88,12 +89,20 @@ public void testLimitsInFlightRequests() throws Exception {
 
         try (Netty4HttpClient nettyHttpClient = new Netty4HttpClient()) {
             Collection<FullHttpResponse> singleResponse = nettyHttpClient.post(transportAddress.address(), requests[0]);
-            assertThat(singleResponse, hasSize(1));
-            assertAtLeastOnceExpectedStatus(singleResponse, HttpResponseStatus.OK);
-
-            Collection<FullHttpResponse> multipleResponses = nettyHttpClient.post(transportAddress.address(), requests);
-            assertThat(multipleResponses, hasSize(requests.length));
-            assertAtLeastOnceExpectedStatus(multipleResponses, HttpResponseStatus.SERVICE_UNAVAILABLE);
+            try {
+                assertThat(singleResponse, hasSize(1));
+                assertAtLeastOnceExpectedStatus(singleResponse, HttpResponseStatus.OK);
+
+                Collection<FullHttpResponse> multipleResponses = nettyHttpClient.post(transportAddress.address(), requests);
+                try {
+                    assertThat(multipleResponses, hasSize(requests.length));
+                    assertAtLeastOnceExpectedStatus(multipleResponses, HttpResponseStatus.SERVICE_UNAVAILABLE);
+                } finally {
+                    multipleResponses.forEach(ReferenceCounted::release);
+                }
+            } finally {
+                singleResponse.forEach(ReferenceCounted::release);
+            }
         }
     }
 
@@ -113,8 +122,12 @@ public void testDoesNotLimitExcludedRequests() throws Exception {
 
         try (Netty4HttpClient nettyHttpClient = new Netty4HttpClient()) {
             Collection<FullHttpResponse> responses = nettyHttpClient.put(transportAddress.address(), requestUris);
-            assertThat(responses, hasSize(requestUris.length));
-            assertAllInExpectedStatus(responses, HttpResponseStatus.OK);
+            try {
+                assertThat(responses, hasSize(requestUris.length));
+                assertAllInExpectedStatus(responses, HttpResponseStatus.OK);
+            } finally {
+                responses.forEach(ReferenceCounted::release);
+            }
         }
     }
 

diff --git a/...t-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpServerPipeliningTests.java b/...t-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpServerPipeliningTests.java
@@ -29,6 +29,7 @@
 import io.netty.handler.codec.http.FullHttpRequest;
 import io.netty.handler.codec.http.FullHttpResponse;
 import io.netty.handler.codec.http.HttpHeaderNames;
+import io.netty.util.ReferenceCounted;
 import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.network.NetworkService;
 import org.elasticsearch.common.settings.Settings;
@@ -98,8 +99,12 @@ public void testThatHttpPipeliningWorks() throws Exception {
 
             try (Netty4HttpClient nettyHttpClient = new Netty4HttpClient()) {
                 Collection<FullHttpResponse> responses = nettyHttpClient.get(transportAddress.address(), requests.toArray(new String[]{}));
-                Collection<String> responseBodies = Netty4HttpClient.returnHttpResponseBodies(responses);
-                assertThat(responseBodies, contains(requests.toArray()));
+                try {
+                    Collection<String> responseBodies = Netty4HttpClient.returnHttpResponseBodies(responses);
+                    assertThat(responseBodies, contains(requests.toArray()));
+                } finally {
+                    responses.forEach(ReferenceCounted::release);
+                }
             }
         }
     }

diff --git a/...rt-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpServerTransportTests.java b/...rt-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4HttpServerTransportTests.java
@@ -207,14 +207,23 @@ public void dispatchBadRequest(RestRequest request, RestChannel channel, ThreadC
                 HttpUtil.setContentLength(request, contentLength);
 
                 final FullHttpResponse response = client.post(remoteAddress.address(), request);
-                assertThat(response.status(), equalTo(expectedStatus));
-                if (expectedStatus.equals(HttpResponseStatus.CONTINUE)) {
-                    final FullHttpRequest continuationRequest =
-                        new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/", Unpooled.EMPTY_BUFFER);
-                    final FullHttpResponse continuationResponse = client.post(remoteAddress.address(), continuationRequest);
-
-                    assertThat(continuationResponse.status(), is(HttpResponseStatus.OK));
-                    assertThat(new String(ByteBufUtil.getBytes(continuationResponse.content()), StandardCharsets.UTF_8), is("done"));
+                try {
+                    assertThat(response.status(), equalTo(expectedStatus));
+                    if (expectedStatus.equals(HttpResponseStatus.CONTINUE)) {
+                        final FullHttpRequest continuationRequest =
+                            new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.POST, "/", Unpooled.EMPTY_BUFFER);
+                        final FullHttpResponse continuationResponse = client.post(remoteAddress.address(), continuationRequest);
+                        try {
+                            assertThat(continuationResponse.status(), is(HttpResponseStatus.OK));
+                            assertThat(
+                                new String(ByteBufUtil.getBytes(continuationResponse.content()), StandardCharsets.UTF_8), is("done")
+                            );
+                        } finally {
+                            continuationResponse.release();
+                        }
+                    }
+                } finally {
+                    response.release();
                 }
             }
         }
@@ -280,10 +289,14 @@ public void dispatchBadRequest(final RestRequest request,
                 final FullHttpRequest request = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, url);
 
                 final FullHttpResponse response = client.post(remoteAddress.address(), request);
-                assertThat(response.status(), equalTo(HttpResponseStatus.BAD_REQUEST));
-                assertThat(
+                try {
+                    assertThat(response.status(), equalTo(HttpResponseStatus.BAD_REQUEST));
+                    assertThat(
                         new String(response.content().array(), Charset.forName("UTF-8")),
                         containsString("you sent a bad request and you should feel bad"));
+                } finally {
+                    response.release();
+                }
             }
         }
 

diff --git a/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4PipeliningIT.java b/modules/transport-netty4/src/test/java/org/elasticsearch/http/netty4/Netty4PipeliningIT.java
@@ -20,6 +20,7 @@
 package org.elasticsearch.http.netty4;
 
 import io.netty.handler.codec.http.FullHttpResponse;
+import io.netty.util.ReferenceCounted;
 import org.elasticsearch.ESNetty4IntegTestCase;
 import org.elasticsearch.common.transport.TransportAddress;
 import org.elasticsearch.http.HttpServerTransport;
@@ -45,14 +46,18 @@ public void testThatNettyHttpServerSupportsPipelining() throws Exception {
 
         HttpServerTransport httpServerTransport = internalCluster().getInstance(HttpServerTransport.class);
         TransportAddress[] boundAddresses = httpServerTransport.boundAddress().boundAddresses();
-        TransportAddress transportAddress = (TransportAddress) randomFrom(boundAddresses);
+        TransportAddress transportAddress = randomFrom(boundAddresses);
 
         try (Netty4HttpClient nettyHttpClient = new Netty4HttpClient()) {
             Collection<FullHttpResponse> responses = nettyHttpClient.get(transportAddress.address(), requests);
-            assertThat(responses, hasSize(5));
+            try {
+                assertThat(responses, hasSize(5));
 
-            Collection<String> opaqueIds = Netty4HttpClient.returnOpaqueIds(responses);
-            assertOpaqueIdsInOrder(opaqueIds);
+                Collection<String> opaqueIds = Netty4HttpClient.returnOpaqueIds(responses);
+                assertOpaqueIdsInOrder(opaqueIds);
+            } finally {
+                responses.forEach(ReferenceCounted::release);
+            }
         }
     }
 

diff --git a/plugins/transport-nio/build.gradle b/plugins/transport-nio/build.gradle
@@ -29,13 +29,13 @@ dependencies {
     compile "org.elasticsearch:elasticsearch-nio:${version}"
 
     // network stack
-    compile "io.netty:netty-buffer:4.1.16.Final"
-    compile "io.netty:netty-codec:4.1.16.Final"
-    compile "io.netty:netty-codec-http:4.1.16.Final"
-    compile "io.netty:netty-common:4.1.16.Final"
-    compile "io.netty:netty-handler:4.1.16.Final"
-    compile "io.netty:netty-resolver:4.1.16.Final"
-    compile "io.netty:netty-transport:4.1.16.Final"
+    compile "io.netty:netty-buffer:4.1.28.Final"
+    compile "io.netty:netty-codec:4.1.28.Final"
+    compile "io.netty:netty-codec-http:4.1.28.Final"
+    compile "io.netty:netty-common:4.1.28.Final"
+    compile "io.netty:netty-handler:4.1.28.Final"
+    compile "io.netty:netty-resolver:4.1.28.Final"
+    compile "io.netty:netty-transport:4.1.28.Final"
 }
 
 dependencyLicenses {
@@ -113,7 +113,6 @@ thirdPartyAudit.excludes = [
         'net.jpountz.xxhash.StreamingXXHash32',
         'net.jpountz.xxhash.XXHashFactory',
         'io.netty.internal.tcnative.CertificateRequestedCallback',
-        'io.netty.internal.tcnative.CertificateRequestedCallback$KeyMaterial',
         'io.netty.internal.tcnative.CertificateVerifier',
         'io.netty.internal.tcnative.SessionTicketKey',
         'io.netty.internal.tcnative.SniHostNameMatcher',
@@ -140,6 +139,6 @@ thirdPartyAudit.excludes = [
 
         'org.conscrypt.AllocatedBuffer',
         'org.conscrypt.BufferAllocator',
-        'org.conscrypt.Conscrypt$Engines',
+        'org.conscrypt.Conscrypt',
         'org.conscrypt.HandshakeListener'
 ]
diff --git a/plugins/transport-nio/licenses/netty-buffer-4.1.16.Final.jar.sha1 b/plugins/transport-nio/licenses/netty-buffer-4.1.16.Final.jar.sha1
diff --git a/plugins/transport-nio/licenses/netty-buffer-4.1.28.Final.jar.sha1 b/plugins/transport-nio/licenses/netty-buffer-4.1.28.Final.jar.sha1
@@ -0,0 +1 @@
+d6c2d13492778009d33f60e05ed90bcb535d1fd1
diff --git a/plugins/transport-nio/licenses/netty-codec-4.1.16.Final.jar.sha1 b/plugins/transport-nio/licenses/netty-codec-4.1.16.Final.jar.sha1
diff --git a/plugins/transport-nio/licenses/netty-codec-4.1.28.Final.jar.sha1 b/plugins/transport-nio/licenses/netty-codec-4.1.28.Final.jar.sha1
@@ -0,0 +1 @@
+a38361d893900947524f8a9da980555950e73d6a
diff --git a/plugins/transport-nio/licenses/netty-codec-http-4.1.16.Final.jar.sha1 b/plugins/transport-nio/licenses/netty-codec-http-4.1.16.Final.jar.sha1
diff --git a/plugins/transport-nio/licenses/netty-codec-http-4.1.28.Final.jar.sha1 b/plugins/transport-nio/licenses/netty-codec-http-4.1.28.Final.jar.sha1
@@ -0,0 +1 @@
+897100c1022c780b0a436b9349e507e8fa9800dc
diff --git a/plugins/transport-nio/licenses/netty-common-4.1.16.Final.jar.sha1 b/plugins/transport-nio/licenses/netty-common-4.1.16.Final.jar.sha1
diff --git a/plugins/transport-nio/licenses/netty-common-4.1.28.Final.jar.sha1 b/plugins/transport-nio/licenses/netty-common-4.1.28.Final.jar.sha1
@@ -0,0 +1 @@
+df69ce8bb9b544a71e7bbee290253cf7c93e6bad
diff --git a/plugins/transport-nio/licenses/netty-handler-4.1.16.Final.jar.sha1 b/plugins/transport-nio/licenses/netty-handler-4.1.16.Final.jar.sha1
diff --git a/plugins/transport-nio/licenses/netty-handler-4.1.28.Final.jar.sha1 b/plugins/transport-nio/licenses/netty-handler-4.1.28.Final.jar.sha1
@@ -0,0 +1 @@
+a035784682da0126bc25f10713dac732b5082a6d
diff --git a/plugins/transport-nio/licenses/netty-resolver-4.1.16.Final.jar.sha1 b/plugins/transport-nio/licenses/netty-resolver-4.1.16.Final.jar.sha1
diff --git a/plugins/transport-nio/licenses/netty-resolver-4.1.28.Final.jar.sha1 b/plugins/transport-nio/licenses/netty-resolver-4.1.28.Final.jar.sha1
@@ -0,0 +1 @@
+f33557dcb31fa20da075ac05e4808115e32ef9b7
diff --git a/plugins/transport-nio/licenses/netty-transport-4.1.16.Final.jar.sha1 b/plugins/transport-nio/licenses/netty-transport-4.1.16.Final.jar.sha1
diff --git a/plugins/transport-nio/licenses/netty-transport-4.1.28.Final.jar.sha1 b/plugins/transport-nio/licenses/netty-transport-4.1.28.Final.jar.sha1
@@ -0,0 +1 @@
+d2ef28f49d726737f0ffe84bf66529b3bf6e0c0d
diff --git a/plugins/transport-nio/src/main/plugin-metadata/plugin-security.policy b/plugins/transport-nio/src/main/plugin-metadata/plugin-security.policy
@@ -26,4 +26,6 @@ grant codeBase "${codebase.netty-common}" {
    // This should only currently be required as we use the netty http client for tests
    // netty makes and accepts socket connections
    permission java.net.SocketPermission "*", "accept,connect";
+   // Netty sets custom classloader for some of its internal threads
+   permission java.lang.RuntimePermission "*", "setContextClassLoader";
 };