Caideyipi commented on code in PR #18410:
URL: https://github.com/apache/iotdb/pull/18410#discussion_r3734291779
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/pipe/agent/plugin/service/PipePluginClassLoader.java:
##########
@@ -20,110 +20,213 @@
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())) {
Review Comment:
[P1] Resolve Multi-Release JAR entries using the runtime version before
comparing bytes
ew JarFile(jarPath.toFile()) opens the JAR with the base (Java 8) view,
while parent.getResourceAsStream(entry.getName()) is runtime-aware and resolves
pkg/Foo.class to (for example) META-INF/versions/17/pkg/Foo.class on JDK 17.
Consequently, even the exact same MR-JAR on the parent and plugin classpaths
can be reported as conflicting: with jackson-core-2.16.2.jar, the base and Java
17 FastDoubleSwar.class entries have different bytes, so this check rejects an
otherwise identical dependency.
Please compare the runtime-selected plugin bytes as well (for example, open
JarFile with Runtime.version()), and skip/deduplicate physical
META-INF/versions/ entries. A regression test with a Multi-Release JAR would
help prevent this false positive.
--
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]
