Caideyipi commented on code in PR #18410:
URL: https://github.com/apache/iotdb/pull/18410#discussion_r3801022767
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/pipe/agent/plugin/service/PipePluginClassLoader.java:
##########
@@ -20,110 +20,218 @@
package org.apache.iotdb.commons.pipe.agent.plugin.service;
import org.apache.iotdb.commons.file.SystemFileFactory;
+import org.apache.iotdb.commons.i18n.PipeMessages;
+import javax.annotation.concurrent.GuardedBy;
import javax.annotation.concurrent.ThreadSafe;
import java.io.IOException;
+import java.io.InputStream;
import java.net.URL;
import java.net.URLClassLoader;
import java.nio.file.Files;
import java.nio.file.Path;
-import java.util.concurrent.atomic.AtomicLong;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Enumeration;
+import java.util.List;
+import java.util.Locale;
+import java.util.Objects;
+import java.util.jar.JarEntry;
+import java.util.jar.JarFile;
import java.util.stream.Collectors;
import java.util.stream.Stream;
+/**
+ * ClassLoader for a pipe plugin. Uses the standard parent-delegation model.
+ *
+ * <p>Before attaching any plugin jar/class URLs, it scans plugin artifacts as
raw bytes (via {@link
+ * JarFile} / filesystem reads) and compares them with resources visible to
the parent ClassLoader
+ * through {@link ClassLoader#getResourceAsStream(String)}. That check never
defines classes into
+ * the parent (or this) ClassLoader; only a later explicit {@link
Class#forName} loads the plugin
+ * entry class.
+ */
@ThreadSafe
public class PipePluginClassLoader extends URLClassLoader {
- private static final String[] PARENT_FIRST_CLASS_PREFIXES = {
- "java.", "javax.", "jdk.", "sun.", "org.slf4j.",
"org.apache.iotdb.pipe.api."
- };
-
- private final String libRoot;
+ private static final String CLASS_SUFFIX = ".class";
+ private static final String JAR_SUFFIX = ".jar";
+ private static final String MODULE_INFO_CLASS = "module-info.class";
+ private static final int MAX_REPORTED_CONFLICTS = 20;
/**
* If activeInstanceCount is equals to 0, it means that there is no instance
using this
* classloader. This classloader can only be closed when activeInstanceCount
is equals to 0.
*/
- private final AtomicLong activeInstanceCount;
+ @GuardedBy("this")
+ private long activeInstanceCount;
/**
* If this classloader is marked as deprecated, then this classloader can be
closed after all
* instances that use this classloader are closed.
*/
- private volatile boolean deprecated;
+ @GuardedBy("this")
+ private boolean deprecated;
public PipePluginClassLoader(String libRoot) throws IOException {
this(libRoot, ClassLoader.getSystemClassLoader());
}
PipePluginClassLoader(String libRoot, ClassLoader parent) throws IOException
{
super(new URL[0], parent);
- this.libRoot = libRoot;
- activeInstanceCount = new AtomicLong(0);
+ Objects.requireNonNull(libRoot,
PipeMessages.EXCEPTION_LIBROOT_CANNOT_BE_NULL_C22EAC78);
+ activeInstanceCount = 0;
deprecated = false;
- addUrls();
+
+ final Path rootPath = SystemFileFactory.INSTANCE.getFile(libRoot).toPath();
+ if (!Files.exists(rootPath)) {
+ throw new IOException(
+ String.format(
+ PipeMessages
+
.EXCEPTION_FAILED_TO_LOAD_PIPE_PLUGIN_FROM_ARG_BECAUSE_THE_PATH_DOES_NOT_EXIST_1AD125AD,
+ rootPath));
+ }
+
+ // Walk once and reuse for conflict check + URL registration.
+ final List<Path> pluginFiles;
+ try (Stream<Path> pathStream = Files.walk(rootPath)) {
+ pluginFiles =
pathStream.filter(Files::isRegularFile).collect(Collectors.toList());
+ }
+
+ validateNoConflictingClassesWithParent(rootPath, pluginFiles, parent);
+ addUrls(pluginFiles);
Review Comment:
[P1] Manifest `Class-Path` dependencies are not included in `pluginFiles`.
`URLClassLoader` follows a JAR manifest's `Class-Path` entries and can load
classes from sibling or out-of-root JARs after `addURL`, but validation only
scans files returned by `Files.walk(rootPath)`. A conflicting class in such a
dependency therefore bypasses this check. Please resolve and scan the effective
URL class path (including manifest dependencies), or explicitly reject/disable
manifest `Class-Path` entries.
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/pipe/agent/plugin/service/PipePluginClassLoader.java:
##########
@@ -20,110 +20,218 @@
package org.apache.iotdb.commons.pipe.agent.plugin.service;
import org.apache.iotdb.commons.file.SystemFileFactory;
+import org.apache.iotdb.commons.i18n.PipeMessages;
+import javax.annotation.concurrent.GuardedBy;
import javax.annotation.concurrent.ThreadSafe;
import java.io.IOException;
+import java.io.InputStream;
import java.net.URL;
import java.net.URLClassLoader;
import java.nio.file.Files;
import java.nio.file.Path;
-import java.util.concurrent.atomic.AtomicLong;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Enumeration;
+import java.util.List;
+import java.util.Locale;
+import java.util.Objects;
+import java.util.jar.JarEntry;
+import java.util.jar.JarFile;
import java.util.stream.Collectors;
import java.util.stream.Stream;
+/**
+ * ClassLoader for a pipe plugin. Uses the standard parent-delegation model.
+ *
+ * <p>Before attaching any plugin jar/class URLs, it scans plugin artifacts as
raw bytes (via {@link
+ * JarFile} / filesystem reads) and compares them with resources visible to
the parent ClassLoader
+ * through {@link ClassLoader#getResourceAsStream(String)}. That check never
defines classes into
+ * the parent (or this) ClassLoader; only a later explicit {@link
Class#forName} loads the plugin
+ * entry class.
+ */
@ThreadSafe
public class PipePluginClassLoader extends URLClassLoader {
- private static final String[] PARENT_FIRST_CLASS_PREFIXES = {
- "java.", "javax.", "jdk.", "sun.", "org.slf4j.",
"org.apache.iotdb.pipe.api."
- };
-
- private final String libRoot;
+ private static final String CLASS_SUFFIX = ".class";
+ private static final String JAR_SUFFIX = ".jar";
+ private static final String MODULE_INFO_CLASS = "module-info.class";
+ private static final int MAX_REPORTED_CONFLICTS = 20;
/**
* If activeInstanceCount is equals to 0, it means that there is no instance
using this
* classloader. This classloader can only be closed when activeInstanceCount
is equals to 0.
*/
- private final AtomicLong activeInstanceCount;
+ @GuardedBy("this")
+ private long activeInstanceCount;
/**
* If this classloader is marked as deprecated, then this classloader can be
closed after all
* instances that use this classloader are closed.
*/
- private volatile boolean deprecated;
+ @GuardedBy("this")
+ private boolean deprecated;
public PipePluginClassLoader(String libRoot) throws IOException {
this(libRoot, ClassLoader.getSystemClassLoader());
}
PipePluginClassLoader(String libRoot, ClassLoader parent) throws IOException
{
super(new URL[0], parent);
- this.libRoot = libRoot;
- activeInstanceCount = new AtomicLong(0);
+ Objects.requireNonNull(libRoot,
PipeMessages.EXCEPTION_LIBROOT_CANNOT_BE_NULL_C22EAC78);
+ activeInstanceCount = 0;
deprecated = false;
- addUrls();
+
+ final Path rootPath = SystemFileFactory.INSTANCE.getFile(libRoot).toPath();
+ if (!Files.exists(rootPath)) {
+ throw new IOException(
+ String.format(
+ PipeMessages
+
.EXCEPTION_FAILED_TO_LOAD_PIPE_PLUGIN_FROM_ARG_BECAUSE_THE_PATH_DOES_NOT_EXIST_1AD125AD,
+ rootPath));
+ }
+
+ // Walk once and reuse for conflict check + URL registration.
+ final List<Path> pluginFiles;
+ try (Stream<Path> pathStream = Files.walk(rootPath)) {
+ pluginFiles =
pathStream.filter(Files::isRegularFile).collect(Collectors.toList());
+ }
+
+ validateNoConflictingClassesWithParent(rootPath, pluginFiles, parent);
+ addUrls(pluginFiles);
}
- private void addUrls() throws IOException {
- try (Stream<Path> pathStream =
- Files.walk(SystemFileFactory.INSTANCE.getFile(libRoot).toPath())) {
- // skip directory
- for (Path path :
- pathStream.filter(path ->
!path.toFile().isDirectory()).collect(Collectors.toList())) {
- super.addURL(path.toUri().toURL());
+ /**
+ * Scan plugin jars/classes and reject those whose fully-qualified class
names already exist on
+ * the parent ClassLoader with different bytecode.
+ *
+ * <p>Implementation constraints:
+ *
+ * <ul>
+ * <li>Never call {@code Class.forName} / {@code loadClass} for plugin
classes.
+ * <li>Never add plugin URLs to the parent ClassLoader.
+ * <li>Only read bytes via {@link JarFile} / {@link Files} and {@link
+ * ClassLoader#getResourceAsStream(String)}.
+ * </ul>
+ */
+ static void validateNoConflictingClassesWithParent(
+ Path rootPath, List<Path> pluginFiles, ClassLoader parent) throws
IOException {
+ final List<String> conflicts = new ArrayList<>();
+
+ for (Path path : pluginFiles) {
+ final String fileName =
path.getFileName().toString().toLowerCase(Locale.ROOT);
+ if (fileName.endsWith(JAR_SUFFIX)) {
Review Comment:
[P1] The scan inventory is narrower than the URLs that are actually loaded.
`addUrls` adds every regular file, and `URLClassLoader` loads a valid JAR
renamed to `.zip`, but this branch only scans names ending in `.jar` (and
`.class`). The SQL path preserves arbitrary URI extensions, so a `.zip`/renamed
JAR containing a conflicting class bypasses the check and is then
parent-delegated. Please detect archives by opening them (or enforce the
artifact type) and add a regression test.
##########
iotdb-core/node-commons/src/test/java/org/apache/iotdb/commons/pipe/agent/plugin/service/PipePluginClassLoaderTest.java:
##########
@@ -41,74 +43,250 @@
import java.util.Map;
import java.util.jar.JarEntry;
import java.util.jar.JarOutputStream;
+import java.util.jar.Manifest;
import java.util.stream.Stream;
public class PipePluginClassLoaderTest {
+ // Verify that a plugin is rejected when it contains different bytecode for
a parent class.
@Test
- public void testPluginClassesShouldOverrideParentClasses() throws Exception {
- final Path tempDir =
Files.createTempDirectory("pipe-plugin-classloader-test");
+ public void testRejectPluginWhenParentHasDifferentBytecode() throws
Exception {
+ final Path tempDir =
Files.createTempDirectory("pipe-plugin-classloader-conflict");
try {
- final Path parentSources =
Files.createDirectory(tempDir.resolve("parent-sources"));
- final Path parentClasses =
Files.createDirectory(tempDir.resolve("parent-classes"));
- final Path childSources =
Files.createDirectory(tempDir.resolve("child-sources"));
- final Path childClasses =
Files.createDirectory(tempDir.resolve("child-classes"));
-
- final String sampleSource =
- "package test.plugin;"
- + "public class Sample {"
- + " public String ping() {"
- + " return test.dep.Helper.value();"
- + " }"
- + "}";
- final String parentHelperSource =
- "package test.dep;"
- + "public class Helper {"
- + " public static String value() {"
- + " return \"parent\";"
- + " }"
- + "}";
- final String childHelperSource =
- "package test.dep;"
- + "public class Helper {"
- + " public static String value() {"
- + " return \"child\";"
- + " }"
- + "}";
+ final Path parentJar = buildJarWithHelper(tempDir, "parent", "parent");
+ final Path childJar = buildJarWithHelper(tempDir, "child", "child");
+ try (final URLClassLoader parentClassLoader =
+ new URLClassLoader(new URL[] {parentJar.toUri().toURL()}, null)) {
+ // Ensure parent has already resolved the class resource.
+
Assert.assertNotNull(parentClassLoader.getResource("test/dep/Helper.class"));
+
+ try {
+ new PipePluginClassLoader(childJar.toString(), parentClassLoader);
+ Assert.fail("Expected IOException for conflicting classes");
+ } catch (final IOException e) {
+ Assert.assertTrue(e.getMessage().contains("test.dep.Helper"));
+ }
+
+ // Conflict check must not define classes into the parent ClassLoader.
+ Assert.assertNull(findLoadedClass(parentClassLoader,
"test.dep.Helper"));
+ Assert.assertNull(findLoadedClass(parentClassLoader,
"test.plugin.Sample"));
+ }
+ } finally {
+ deleteRecursively(tempDir);
+ }
+ }
+
+ // Verify that identical parent and plugin bytecode is allowed and uses
parent delegation.
+ @Test
+ public void testAllowPluginWhenParentHasIdenticalBytecode() throws Exception
{
+ final Path tempDir =
Files.createTempDirectory("pipe-plugin-classloader-same");
+ try {
+ final Path sharedClasses =
Files.createDirectory(tempDir.resolve("shared-classes"));
+ final Path sharedSources =
Files.createDirectory(tempDir.resolve("shared-sources"));
compile(
- parentSources,
- parentClasses,
- createSources(sampleSource, false),
- createSources(parentHelperSource, true));
- compile(
- childSources,
- childClasses,
- createSources(sampleSource, false),
- createSources(childHelperSource, true));
+ sharedSources,
+ sharedClasses,
+ createSources(
+ "package test.dep;"
+ + "public class Helper {"
+ + " public static String value() { return \"same\"; }"
+ + "}",
+ true),
+ createSources(
+ "package test.plugin;"
+ + "public class Sample {"
+ + " public String ping() { return test.dep.Helper.value();
}"
+ + "}",
+ false));
final Path parentJar = tempDir.resolve("parent.jar");
final Path childJar = tempDir.resolve("child.jar");
- createJar(parentJar, parentClasses,
Arrays.asList("test/plugin/Sample.class"));
+ createJar(parentJar, sharedClasses,
Arrays.asList("test/dep/Helper.class"));
createJar(
childJar,
- childClasses,
+ sharedClasses,
Arrays.asList("test/plugin/Sample.class", "test/dep/Helper.class"));
try (final URLClassLoader parentClassLoader =
new URLClassLoader(new URL[] {parentJar.toUri().toURL()}, null);
final PipePluginClassLoader pluginClassLoader =
new PipePluginClassLoader(childJar.toString(),
parentClassLoader)) {
final Class<?> sampleClass = Class.forName("test.plugin.Sample", true,
pluginClassLoader);
+ // Sample is only in the plugin jar → loaded by plugin ClassLoader.
Assert.assertSame(pluginClassLoader, sampleClass.getClassLoader());
+ // Helper is identical and present on parent → parent-delegation loads
parent's copy.
+ final Class<?> helperClass = Class.forName("test.dep.Helper", true,
pluginClassLoader);
+ Assert.assertSame(parentClassLoader, helperClass.getClassLoader());
final Object sample =
sampleClass.getDeclaredConstructor().newInstance();
- Assert.assertEquals("child",
sampleClass.getMethod("ping").invoke(sample));
+ Assert.assertEquals("same",
sampleClass.getMethod("ping").invoke(sample));
+ }
+ } finally {
+ deleteRecursively(tempDir);
+ }
+ }
+
+ // Verify that conflict scanning does not load plugin classes into the
parent loader.
+ @Test
+ public void testConflictCheckDoesNotLoadPluginClasses() throws Exception {
+ final Path tempDir =
Files.createTempDirectory("pipe-plugin-classloader-noload");
+ try {
+ final Path parentJar = buildJarWithHelper(tempDir, "parent", "parent");
+ final Path childJar = buildJarWithHelper(tempDir, "child", "child");
+
+ try (final URLClassLoader parentClassLoader =
+ new URLClassLoader(new URL[] {parentJar.toUri().toURL()}, null)) {
+ try {
+ PipePluginClassLoader.validateNoConflictingClassesWithParent(
+ childJar, List.of(childJar), parentClassLoader);
+ Assert.fail("Expected IOException for conflicting classes");
+ } catch (final IOException expected) {
+ // expected
+ }
+
+ Assert.assertNull(findLoadedClass(parentClassLoader,
"test.dep.Helper"));
+ Assert.assertNull(findLoadedClass(parentClassLoader,
"test.plugin.Sample"));
+ }
+ } finally {
+ deleteRecursively(tempDir);
+ }
+ }
+
+ // Verify that Java core classes cannot be overridden by plugin classes.
+ @Test
+ public void testJavaCoreClassIsLoadedByBootstrapClassLoader() throws
Exception {
+ // Verify that a plugin cannot replace a Java core class through parent
delegation.
+ final Path tempDir = Files.createTempDirectory("pipe-plugin-core-protect");
+ try {
+ final Path childJar = tempDir.resolve("plugin.jar");
+ createJarWithResource(childJar, "java/lang/String.class", new byte[0]);
+
+ try (final URLClassLoader parentClassLoader = new URLClassLoader(new
URL[0], null)) {
+ try {
+ new PipePluginClassLoader(childJar.toString(), parentClassLoader);
+ Assert.fail("Expected IOException for a conflicting Java core
class");
+ } catch (IOException expected) {
+
Assert.assertTrue(expected.getMessage().contains("java.lang.String"));
+ }
+ }
+ } finally {
+ deleteRecursively(tempDir);
+ }
+ }
+
+ // Verify that closing the plugin loader releases the plugin JAR file handle.
+ @Test
+ public void testPluginJarFileHandleReleasedAfterClose() throws Exception {
+ // Verify that closing the plugin class loader releases the underlying JAR
file.
+ final Path tempDir = Files.createTempDirectory("pipe-plugin-file-handle");
+ try {
+ final Path childJar = buildJarWithHelper(tempDir, "close-test", "dummy");
+ try (final URLClassLoader parentClassLoader = new URLClassLoader(new
URL[0], null);
+ final PipePluginClassLoader pluginClassLoader =
+ new PipePluginClassLoader(childJar.toString(),
parentClassLoader)) {
+ Class.forName("test.plugin.Sample", true, pluginClassLoader);
+ pluginClassLoader.close();
+ }
+ Assert.assertTrue(Files.deleteIfExists(childJar));
+ } finally {
+ deleteRecursively(tempDir);
+ }
+ }
+
+ // Verify parent-first resource lookup and enumeration of duplicate
resources.
+ @Test
+ public void testPluginResourceIsolation() throws Exception {
+ // Verify parent-first lookup and enumeration of duplicate resources.
+ final Path tempDir =
Files.createTempDirectory("pipe-plugin-resource-isolation");
+ try {
+ final Path parentJar = tempDir.resolve("parent.jar");
+ final Path childJar = tempDir.resolve("child.jar");
+ createJarWithResource(parentJar, "config.properties", "source=parent");
+ createJarWithResource(childJar, "config.properties", "source=child");
+ try (final URLClassLoader parentClassLoader =
+ new URLClassLoader(new URL[] {parentJar.toUri().toURL()}, null);
+ final PipePluginClassLoader pluginClassLoader =
+ new PipePluginClassLoader(childJar.toString(),
parentClassLoader)) {
+ final URL resourceUrl =
pluginClassLoader.getResource("config.properties");
+ Assert.assertNotNull(resourceUrl);
+ try (InputStream inputStream = resourceUrl.openStream()) {
Review Comment:
[P1] This test currently fails on Windows CI. The latest PR check reports
`FileSystemException: parent.jar ... being used by another process` during
cleanup in `testPluginResourceIsolation`
(https://github.com/apache/iotdb/actions/runs/31354401714/job/93351225887).
`resourceUrl.openStream()` opens a cached `JarURLConnection`; closing the
classloaders does not release that cached JarFile. Use `getResourceAsStream`,
set `useCaches(false)` before opening, or otherwise avoid deleting while the
cached connection is alive.
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/pipe/agent/plugin/service/PipePluginClassLoader.java:
##########
@@ -20,110 +20,218 @@
package org.apache.iotdb.commons.pipe.agent.plugin.service;
import org.apache.iotdb.commons.file.SystemFileFactory;
+import org.apache.iotdb.commons.i18n.PipeMessages;
+import javax.annotation.concurrent.GuardedBy;
import javax.annotation.concurrent.ThreadSafe;
import java.io.IOException;
+import java.io.InputStream;
import java.net.URL;
import java.net.URLClassLoader;
import java.nio.file.Files;
import java.nio.file.Path;
-import java.util.concurrent.atomic.AtomicLong;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Enumeration;
+import java.util.List;
+import java.util.Locale;
+import java.util.Objects;
+import java.util.jar.JarEntry;
+import java.util.jar.JarFile;
import java.util.stream.Collectors;
import java.util.stream.Stream;
+/**
+ * ClassLoader for a pipe plugin. Uses the standard parent-delegation model.
+ *
+ * <p>Before attaching any plugin jar/class URLs, it scans plugin artifacts as
raw bytes (via {@link
+ * JarFile} / filesystem reads) and compares them with resources visible to
the parent ClassLoader
+ * through {@link ClassLoader#getResourceAsStream(String)}. That check never
defines classes into
+ * the parent (or this) ClassLoader; only a later explicit {@link
Class#forName} loads the plugin
+ * entry class.
+ */
@ThreadSafe
public class PipePluginClassLoader extends URLClassLoader {
- private static final String[] PARENT_FIRST_CLASS_PREFIXES = {
- "java.", "javax.", "jdk.", "sun.", "org.slf4j.",
"org.apache.iotdb.pipe.api."
- };
-
- private final String libRoot;
+ private static final String CLASS_SUFFIX = ".class";
+ private static final String JAR_SUFFIX = ".jar";
+ private static final String MODULE_INFO_CLASS = "module-info.class";
+ private static final int MAX_REPORTED_CONFLICTS = 20;
/**
* If activeInstanceCount is equals to 0, it means that there is no instance
using this
* classloader. This classloader can only be closed when activeInstanceCount
is equals to 0.
*/
- private final AtomicLong activeInstanceCount;
+ @GuardedBy("this")
+ private long activeInstanceCount;
/**
* If this classloader is marked as deprecated, then this classloader can be
closed after all
* instances that use this classloader are closed.
*/
- private volatile boolean deprecated;
+ @GuardedBy("this")
+ private boolean deprecated;
public PipePluginClassLoader(String libRoot) throws IOException {
this(libRoot, ClassLoader.getSystemClassLoader());
}
PipePluginClassLoader(String libRoot, ClassLoader parent) throws IOException
{
super(new URL[0], parent);
- this.libRoot = libRoot;
- activeInstanceCount = new AtomicLong(0);
+ Objects.requireNonNull(libRoot,
PipeMessages.EXCEPTION_LIBROOT_CANNOT_BE_NULL_C22EAC78);
+ activeInstanceCount = 0;
deprecated = false;
- addUrls();
+
+ final Path rootPath = SystemFileFactory.INSTANCE.getFile(libRoot).toPath();
+ if (!Files.exists(rootPath)) {
+ throw new IOException(
+ String.format(
+ PipeMessages
+
.EXCEPTION_FAILED_TO_LOAD_PIPE_PLUGIN_FROM_ARG_BECAUSE_THE_PATH_DOES_NOT_EXIST_1AD125AD,
+ rootPath));
+ }
+
+ // Walk once and reuse for conflict check + URL registration.
+ final List<Path> pluginFiles;
+ try (Stream<Path> pathStream = Files.walk(rootPath)) {
+ pluginFiles =
pathStream.filter(Files::isRegularFile).collect(Collectors.toList());
+ }
+
+ validateNoConflictingClassesWithParent(rootPath, pluginFiles, parent);
+ addUrls(pluginFiles);
}
- private void addUrls() throws IOException {
- try (Stream<Path> pathStream =
- Files.walk(SystemFileFactory.INSTANCE.getFile(libRoot).toPath())) {
- // skip directory
- for (Path path :
- pathStream.filter(path ->
!path.toFile().isDirectory()).collect(Collectors.toList())) {
- super.addURL(path.toUri().toURL());
+ /**
+ * Scan plugin jars/classes and reject those whose fully-qualified class
names already exist on
+ * the parent ClassLoader with different bytecode.
+ *
+ * <p>Implementation constraints:
+ *
+ * <ul>
+ * <li>Never call {@code Class.forName} / {@code loadClass} for plugin
classes.
+ * <li>Never add plugin URLs to the parent ClassLoader.
+ * <li>Only read bytes via {@link JarFile} / {@link Files} and {@link
+ * ClassLoader#getResourceAsStream(String)}.
+ * </ul>
+ */
+ static void validateNoConflictingClassesWithParent(
+ Path rootPath, List<Path> pluginFiles, ClassLoader parent) throws
IOException {
+ final List<String> conflicts = new ArrayList<>();
+
+ for (Path path : pluginFiles) {
+ final String fileName =
path.getFileName().toString().toLowerCase(Locale.ROOT);
+ if (fileName.endsWith(JAR_SUFFIX)) {
+ collectJarConflicts(path, parent, conflicts);
+ } else if (fileName.endsWith(CLASS_SUFFIX)) {
+ collectLooseClassConflict(rootPath, path, parent, conflicts);
}
}
- }
- public synchronized void acquire() {
- activeInstanceCount.incrementAndGet();
+ if (!conflicts.isEmpty()) {
+ final String reported =
+
conflicts.stream().limit(MAX_REPORTED_CONFLICTS).collect(Collectors.joining(",
"));
+ throw new IOException(
+ String.format(
+ PipeMessages
+
.EXCEPTION_FAILED_TO_LOAD_PIPE_PLUGIN_FROM_ARG_BECAUSE_THE_FOLLOWING_CLASSES_CONFLICT_WITH_THE_PARENT_CLASSLOADER_SAME_FULLY_QUALIFIED_NAME_BUT_DIFFERENT_BYTECODE_ARG_0647E8F3,
+ rootPath,
+ reported));
+ }
}
- public synchronized void release() throws IOException {
- activeInstanceCount.decrementAndGet();
- closeIfPossible();
+ private static void collectJarConflicts(Path jarPath, ClassLoader parent,
List<String> conflicts)
+ throws IOException {
+ try (JarFile jarFile =
+ new JarFile(jarPath.toFile(), true, JarFile.OPEN_READ,
Runtime.version())) {
+ final Enumeration<JarEntry> entries = jarFile.entries();
+ while (entries.hasMoreElements()) {
+ final JarEntry entry = entries.nextElement();
+ if (entry.isDirectory()
+ || entry.getName().startsWith("META-INF/versions/")
+ || !isComparableClassEntry(entry.getName())) {
+ continue;
+ }
+ // Resolve the logical entry through the runtime-aware view of a
multi-release JAR.
+ final JarEntry runtimeEntry = jarFile.getJarEntry(entry.getName());
+ try (InputStream pluginIn = jarFile.getInputStream(runtimeEntry)) {
+ maybeAddConflict(entry.getName(), readAllBytes(pluginIn), parent,
conflicts);
+ }
+ }
+ }
}
- public synchronized void markAsDeprecated() throws IOException {
- deprecated = true;
- closeIfPossible();
+ private static void collectLooseClassConflict(
+ Path libRoot, Path classFile, ClassLoader parent, List<String>
conflicts) throws IOException {
+ final Path relative = libRoot.relativize(classFile);
+ final String resourceName = relative.toString().replace('\\', '/');
+ if (!isComparableClassEntry(resourceName)) {
+ return;
+ }
+ maybeAddConflict(resourceName, Files.readAllBytes(classFile), parent,
conflicts);
}
- @Override
- protected Class<?> loadClass(String name, boolean resolve) throws
ClassNotFoundException {
- synchronized (getClassLoadingLock(name)) {
- Class<?> loadedClass = findLoadedClass(name);
- if (loadedClass == null) {
- loadedClass =
- shouldLoadFromParentFirst(name) ? super.loadClass(name, false) :
loadClassLocally(name);
+ private static void maybeAddConflict(
+ String resourceName, byte[] pluginBytes, ClassLoader parent,
List<String> conflicts)
+ throws IOException {
+ // getResourceAsStream locates parent classpath bytes without defining the
Class.
+ try (InputStream parentIn = parent.getResourceAsStream(resourceName)) {
+ if (parentIn == null) {
+ return;
}
- if (resolve) {
- resolveClass(loadedClass);
+ final byte[] parentBytes = readAllBytes(parentIn);
+ if (!Arrays.equals(parentBytes, pluginBytes)) {
Review Comment:
[P1] Identical bytes do not make parent delegation safe for package-private
members. For example, if the plugin contains `p.Entry` and a byte-identical
package-private `p.Helper`, while the parent only has `p.Helper`, this check
passes. The default parent-first loader then defines `Entry` in the plugin
loader and `Helper` in the parent loader; JVM runtime packages include the
defining loader, so `Entry` calling `Helper` throws `IllegalAccessError`. I
reproduced this with identical `Helper.class` bytes. Please keep package
ownership coherent (for example, child-first for plugin-owned packages or
reject split-package duplicates) and add a regression test.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
