From b688d952190534204bc4332dc16f7b172831da5e Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Mon, 18 Jan 2021 17:32:44 +0100 Subject: [PATCH 1/9] Issue #5859 Fix Classloader leak from QueuedThreadPool Signed-off-by: Jan Bartel --- .../java/org/eclipse/jetty/util/thread/QueuedThreadPool.java | 1 + 1 file changed, 1 insertion(+) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java index cbc0ad23409e..47ceda7891cb 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java @@ -689,6 +689,7 @@ public Thread newThread(Runnable runnable) thread.setDaemon(isDaemon()); thread.setPriority(getThreadsPriority()); thread.setName(_name + "-" + thread.getId()); + thread.setContextClassLoader(QueuedThreadPool.class.getClassLoader()); return thread; } From f60cf57801727e74def58766c625cf569289299c Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Mon, 18 Jan 2021 17:45:10 +0100 Subject: [PATCH 2/9] Issue #5859 Add trivial test Signed-off-by: Jan Bartel --- .../util/thread/QueuedThreadPoolTest.java | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java index d2f0e745762a..bdc8430aeea7 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java @@ -27,6 +27,7 @@ import org.eclipse.jetty.util.log.Logger; import org.eclipse.jetty.util.log.StacklessLogging; import org.eclipse.jetty.util.thread.ThreadPool.SizedThreadPool; +import org.hamcrest.Matchers; import org.junit.jupiter.api.Test; import static org.hamcrest.MatcherAssert.assertThat; @@ -833,6 +834,24 @@ public void testDump() throws Exception assertThat(count(dump, "QueuedThreadPoolTest.lambda$testDump$"), is(1)); } + @Test + public void testContextClassLoader() throws Exception + { + QueuedThreadPool tp = new QueuedThreadPool(); + tp.setMinThreads(1); + tp.setMaxThreads(2); + tp.setIdleTimeout(1000); + tp.setThreadsPriority(Thread.NORM_PRIORITY - 1); + try (StacklessLogging stackless = new StacklessLogging(QueuedThreadPool.class)) + { + tp.start(); + tp.execute(() -> + { + assertThat(Thread.currentThread().getContextClassLoader(), Matchers.equalTo(QueuedThreadPool.class.getClassLoader())); + }); + } + } + private int count(String s, String p) { int c = 0; From 596444f958d3a6eab2883fb177ad74da15ff993a Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Mon, 18 Jan 2021 18:46:03 +0100 Subject: [PATCH 3/9] Issue #5859 Post review changes Signed-off-by: Jan Bartel --- .../jetty/util/thread/QueuedThreadPool.java | 2 +- .../util/thread/QueuedThreadPoolTest.java | 18 +++++++++++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java index 47ceda7891cb..82b71036048e 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java @@ -689,7 +689,7 @@ public Thread newThread(Runnable runnable) thread.setDaemon(isDaemon()); thread.setPriority(getThreadsPriority()); thread.setName(_name + "-" + thread.getId()); - thread.setContextClassLoader(QueuedThreadPool.class.getClassLoader()); + thread.setContextClassLoader(this.getClass().getClassLoader()); return thread; } diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java index bdc8430aeea7..f84c512fadb0 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java @@ -19,6 +19,8 @@ package org.eclipse.jetty.util.thread; import java.io.Closeable; +import java.net.URL; +import java.net.URLClassLoader; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; @@ -839,16 +841,26 @@ public void testContextClassLoader() throws Exception { QueuedThreadPool tp = new QueuedThreadPool(); tp.setMinThreads(1); - tp.setMaxThreads(2); + tp.setMaxThreads(3); tp.setIdleTimeout(1000); tp.setThreadsPriority(Thread.NORM_PRIORITY - 1); try (StacklessLogging stackless = new StacklessLogging(QueuedThreadPool.class)) { - tp.start(); - tp.execute(() -> + //change the current thread's classloader to something else + Thread.currentThread().setContextClassLoader(new URLClassLoader(new URL[] {})); + + //create a new thread + Thread t = tp.newThread(() -> { + //the executing thread should be still set to the classloader of the QueuedThreadPool, + //not that of the thread that created this thread. assertThat(Thread.currentThread().getContextClassLoader(), Matchers.equalTo(QueuedThreadPool.class.getClassLoader())); }); + + //new thread should be set to the classloader of the QueuedThreadPool + assertThat(t.getContextClassLoader(), Matchers.equalTo(QueuedThreadPool.class.getClassLoader())); + + t.start(); } } From 7f270a0cb9a189a6cdc8ae34bf6deec389033c90 Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Wed, 20 Jan 2021 15:16:03 +0100 Subject: [PATCH 4/9] Issue #5859 Ensure inheritedAccessContext doesn't pin classloader Signed-off-by: Jan Bartel --- .../jetty/util/thread/QueuedThreadPool.java | 21 +++++++++++++------ .../jetty/util/thread/ShutdownThread.java | 12 ++++++++++- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java index 82b71036048e..371f46a91c7e 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java @@ -20,6 +20,8 @@ import java.io.Closeable; import java.io.IOException; +import java.security.AccessController; +import java.security.PrivilegedAction; import java.util.ArrayList; import java.util.List; import java.util.Set; @@ -685,12 +687,19 @@ private boolean addCounts(int deltaThreads, int deltaIdle) @Override public Thread newThread(Runnable runnable) { - Thread thread = new Thread(_threadGroup, runnable); - thread.setDaemon(isDaemon()); - thread.setPriority(getThreadsPriority()); - thread.setName(_name + "-" + thread.getId()); - thread.setContextClassLoader(this.getClass().getClassLoader()); - return thread; + return (AccessController.doPrivileged(new PrivilegedAction() + { + @Override + public Thread run() + { + Thread thread = new Thread(_threadGroup, runnable); + thread.setDaemon(isDaemon()); + thread.setPriority(getThreadsPriority()); + thread.setName(_name + "-" + thread.getId()); + thread.setContextClassLoader(this.getClass().getClassLoader()); + return thread; + } + })); } protected void removeThread(Thread thread) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ShutdownThread.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ShutdownThread.java index 8aba8713ddd6..27cacaec5727 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ShutdownThread.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ShutdownThread.java @@ -18,6 +18,8 @@ package org.eclipse.jetty.util.thread; +import java.security.AccessController; +import java.security.PrivilegedAction; import java.util.Arrays; import java.util.List; import java.util.concurrent.CopyOnWriteArrayList; @@ -36,7 +38,15 @@ public class ShutdownThread extends Thread { private static final Logger LOG = Log.getLogger(ShutdownThread.class); - private static final ShutdownThread _thread = new ShutdownThread(); + private static final ShutdownThread _thread = AccessController.doPrivileged(new PrivilegedAction() + { + @Override + public ShutdownThread run() + { + return new ShutdownThread(); + } + + }); private boolean _hooked; private final List _lifeCycles = new CopyOnWriteArrayList(); From 4a93a000d8a3991f7b9d2b8b9cb663b9903d400d Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Wed, 20 Jan 2021 17:13:22 +0100 Subject: [PATCH 5/9] Issue #5859 Make safe creation of threads a utility class. Signed-off-by: Jan Bartel --- .../jetty/util/thread/QueuedThreadPool.java | 20 +++---- .../jetty/util/thread/ShutdownThread.java | 13 ++--- .../jetty/util/thread/ThreadCreator.java | 53 +++++++++++++++++++ .../util/thread/QueuedThreadPoolTest.java | 6 --- 4 files changed, 65 insertions(+), 27 deletions(-) create mode 100644 jetty-util/src/main/java/org/eclipse/jetty/util/thread/ThreadCreator.java diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java index 371f46a91c7e..2f61081e5fac 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java @@ -687,19 +687,15 @@ private boolean addCounts(int deltaThreads, int deltaIdle) @Override public Thread newThread(Runnable runnable) { - return (AccessController.doPrivileged(new PrivilegedAction() + return ThreadCreator.create(() -> { - @Override - public Thread run() - { - Thread thread = new Thread(_threadGroup, runnable); - thread.setDaemon(isDaemon()); - thread.setPriority(getThreadsPriority()); - thread.setName(_name + "-" + thread.getId()); - thread.setContextClassLoader(this.getClass().getClassLoader()); - return thread; - } - })); + Thread thread = new Thread(_threadGroup, runnable); + thread.setDaemon(isDaemon()); + thread.setPriority(getThreadsPriority()); + thread.setName(_name + "-" + thread.getId()); + thread.setContextClassLoader(this.getClass().getClassLoader()); + return thread; + }); } protected void removeThread(Thread thread) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ShutdownThread.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ShutdownThread.java index 27cacaec5727..98f109e9b3ca 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ShutdownThread.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ShutdownThread.java @@ -38,15 +38,10 @@ public class ShutdownThread extends Thread { private static final Logger LOG = Log.getLogger(ShutdownThread.class); - private static final ShutdownThread _thread = AccessController.doPrivileged(new PrivilegedAction() - { - @Override - public ShutdownThread run() - { - return new ShutdownThread(); - } - - }); + private static final ShutdownThread _thread = ThreadCreator.create(() -> + { + return new ShutdownThread(); + }); private boolean _hooked; private final List _lifeCycles = new CopyOnWriteArrayList(); diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ThreadCreator.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ThreadCreator.java new file mode 100644 index 000000000000..407e6de80b60 --- /dev/null +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ThreadCreator.java @@ -0,0 +1,53 @@ +// +// ======================================================================== +// Copyright (c) 1995-2020 Mort Bay Consulting Pty Ltd and others. +// ------------------------------------------------------------------------ +// 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.util.thread; + +import java.security.AccessController; +import java.security.PrivilegedAction; + +/** + * ThreadCreator + * + * Convenience class to ensure that a new Thread is created + * inside a privileged block. This prevents the Thread constructor + * from pinning the caller's context classloader. This happens + * when the Thread constructor takes a snapshot of the current + * calling context - which contains ProtectionDomains that may + * reference the context classloader - and remembers it for the + * lifetime of the Thread. + */ +class ThreadCreator +{ + interface Factory + { + T newThread(); + } + + static T create(Factory maker) + { + return AccessController.doPrivileged(new PrivilegedAction() + { + @Override + public T run() + { + return maker.newThread(); + } + }); + } +} diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java index f84c512fadb0..6e8a6792d546 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/thread/QueuedThreadPoolTest.java @@ -840,10 +840,6 @@ public void testDump() throws Exception public void testContextClassLoader() throws Exception { QueuedThreadPool tp = new QueuedThreadPool(); - tp.setMinThreads(1); - tp.setMaxThreads(3); - tp.setIdleTimeout(1000); - tp.setThreadsPriority(Thread.NORM_PRIORITY - 1); try (StacklessLogging stackless = new StacklessLogging(QueuedThreadPool.class)) { //change the current thread's classloader to something else @@ -859,8 +855,6 @@ public void testContextClassLoader() throws Exception //new thread should be set to the classloader of the QueuedThreadPool assertThat(t.getContextClassLoader(), Matchers.equalTo(QueuedThreadPool.class.getClassLoader())); - - t.start(); } } From a7847d3c54900e7d8b13c879dd6446d44502980b Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Thu, 21 Jan 2021 17:29:01 +0100 Subject: [PATCH 6/9] Issue #5859 Changes after review Signed-off-by: Jan Bartel --- ...ator.java => PrivilegedThreadFactory.java} | 21 +++++++++++-------- .../jetty/util/thread/QueuedThreadPool.java | 2 +- .../jetty/util/thread/ShutdownThread.java | 2 +- 3 files changed, 14 insertions(+), 11 deletions(-) rename jetty-util/src/main/java/org/eclipse/jetty/util/thread/{ThreadCreator.java => PrivilegedThreadFactory.java} (75%) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ThreadCreator.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/PrivilegedThreadFactory.java similarity index 75% rename from jetty-util/src/main/java/org/eclipse/jetty/util/thread/ThreadCreator.java rename to jetty-util/src/main/java/org/eclipse/jetty/util/thread/PrivilegedThreadFactory.java index 407e6de80b60..ce3dd7e0d58a 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ThreadCreator.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/PrivilegedThreadFactory.java @@ -20,9 +20,10 @@ import java.security.AccessController; import java.security.PrivilegedAction; +import java.util.function.Supplier; /** - * ThreadCreator + * PrivilegedThreadFactory * * Convenience class to ensure that a new Thread is created * inside a privileged block. This prevents the Thread constructor @@ -32,21 +33,23 @@ * reference the context classloader - and remembers it for the * lifetime of the Thread. */ -class ThreadCreator +class PrivilegedThreadFactory { - interface Factory - { - T newThread(); - } - - static T create(Factory maker) + /** + * Use a Supplier to make a new thread, calling it within + * a privileged block to prevent classloader pinning. + * + * @param newThreadSupplier a Supplier to create a fresh thread + * @return a new thread, protected from classloader pinning. + */ + static T newThread(Supplier newThreadSupplier) { return AccessController.doPrivileged(new PrivilegedAction() { @Override public T run() { - return maker.newThread(); + return newThreadSupplier.get(); } }); } diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java index 2f61081e5fac..c4a37f850c3c 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java @@ -687,7 +687,7 @@ private boolean addCounts(int deltaThreads, int deltaIdle) @Override public Thread newThread(Runnable runnable) { - return ThreadCreator.create(() -> + return PrivilegedThreadFactory.newThread(() -> { Thread thread = new Thread(_threadGroup, runnable); thread.setDaemon(isDaemon()); diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ShutdownThread.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ShutdownThread.java index 98f109e9b3ca..f870ee93efe7 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ShutdownThread.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/ShutdownThread.java @@ -38,7 +38,7 @@ public class ShutdownThread extends Thread { private static final Logger LOG = Log.getLogger(ShutdownThread.class); - private static final ShutdownThread _thread = ThreadCreator.create(() -> + private static final ShutdownThread _thread = PrivilegedThreadFactory.newThread(() -> { return new ShutdownThread(); }); From eed0c2ed2cbb73581af38e294521742893f8804a Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Thu, 21 Jan 2021 17:34:26 +0100 Subject: [PATCH 7/9] Issue #5859 Change after review Signed-off-by: Jan Bartel --- .../java/org/eclipse/jetty/util/thread/QueuedThreadPool.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java index c4a37f850c3c..c39879837a7d 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/QueuedThreadPool.java @@ -693,7 +693,7 @@ public Thread newThread(Runnable runnable) thread.setDaemon(isDaemon()); thread.setPriority(getThreadsPriority()); thread.setName(_name + "-" + thread.getId()); - thread.setContextClassLoader(this.getClass().getClassLoader()); + thread.setContextClassLoader(getClass().getClassLoader()); return thread; }); } From c23544f84267fa5330f8d1dde6322f0177629a75 Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Thu, 21 Jan 2021 17:36:05 +0100 Subject: [PATCH 8/9] Issue #5859 Changes after review Signed-off-by: Jan Bartel --- .../eclipse/jetty/util/thread/PrivilegedThreadFactory.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/PrivilegedThreadFactory.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/PrivilegedThreadFactory.java index ce3dd7e0d58a..e27969b59715 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/PrivilegedThreadFactory.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/PrivilegedThreadFactory.java @@ -23,10 +23,10 @@ import java.util.function.Supplier; /** - * PrivilegedThreadFactory - * * Convenience class to ensure that a new Thread is created - * inside a privileged block. This prevents the Thread constructor + * inside a privileged block. + * + * This prevents the Thread constructor * from pinning the caller's context classloader. This happens * when the Thread constructor takes a snapshot of the current * calling context - which contains ProtectionDomains that may From d1eef4922325d9af8058a0ab8e46b7851c5d5a35 Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Tue, 26 Jan 2021 16:56:49 +0100 Subject: [PATCH 9/9] Issue #5859 Update license header Signed-off-by: Jan Bartel --- .../org/eclipse/jetty/util/thread/PrivilegedThreadFactory.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/PrivilegedThreadFactory.java b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/PrivilegedThreadFactory.java index e27969b59715..bc6ad33c8920 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/thread/PrivilegedThreadFactory.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/thread/PrivilegedThreadFactory.java @@ -1,6 +1,6 @@ // // ======================================================================== -// Copyright (c) 1995-2020 Mort Bay Consulting Pty Ltd and others. +// Copyright (c) 1995-2021 Mort Bay Consulting Pty Ltd and others. // ------------------------------------------------------------------------ // All rights reserved. This program and the accompanying materials // are made available under the terms of the Eclipse Public License v1.0