Pipe: Reject plugin jars that conflict with parent ClassLoader bytecode - #18410
Pipe: Reject plugin jars that conflict with parent ClassLoader bytecode#18410luoluoyuyu wants to merge 5 commits into
Conversation
Restore standard parent-delegation for PipePluginClassLoader and fail fast when a plugin ships the same class name with different bytecode, without defining classes into the parent during the check.
…sloader-bytecode-conflict
| closeIfPossible(); | ||
| private static void collectJarConflicts(Path jarPath, ClassLoader parent, List<String> conflicts) | ||
| throws IOException { | ||
| try (JarFile jarFile = new JarFile(jarPath.toFile())) { |
There was a problem hiding this 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.
Caideyipi
left a comment
Caideyipi
left a comment
There was a problem hiding this comment.
I found several correctness issues in the new class-loading validation. Please address the inline findings before merging.
| if (resolve) { | ||
| resolveClass(loadedClass); | ||
| final byte[] parentBytes = readAllBytes(parentIn); | ||
| if (!Arrays.equals(parentBytes, pluginBytes)) { |
There was a problem hiding this 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.
|
|
||
| for (Path path : pluginFiles) { | ||
| final String fileName = path.getFileName().toString().toLowerCase(Locale.ROOT); | ||
| if (fileName.endsWith(JAR_SUFFIX)) { |
There was a problem hiding this 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.
| } | ||
|
|
||
| validateNoConflictingClassesWithParent(rootPath, pluginFiles, parent); | ||
| addUrls(pluginFiles); |
There was a problem hiding this 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.
| new PipePluginClassLoader(childJar.toString(), parentClassLoader)) { | ||
| final URL resourceUrl = pluginClassLoader.getResource("config.properties"); | ||
| Assert.assertNotNull(resourceUrl); | ||
| try (InputStream inputStream = resourceUrl.openStream()) { |
There was a problem hiding this 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.
2 participants
Description
Restore standard parent-delegation for PipePluginClassLoader and fail fast when a plugin ships the same class name with different bytecode, without defining classes into the parent during the check.
This PR has:
for an unfamiliar reader.
for code coverage.
Key changed/added classes (or packages if there are too many classes) in this PR