RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive

[Ioi Lam]

On Tue, 25 Mar 2025 11:08:24 GMT, Timofei Pushkin <tpushkin at openjdk.org> wrote:

> If a base class is package-private then its subclasses should have the same package name and defining class loader, otherwise `IllegalAccessError` is thrown when linking a subclass. Currently when dumping a static archive separate `URLClassLoader`s are used for each unregistered classes' source. Thus if two unregistered classes, a package-private base class and a sub class, from the same package reside in different sources `IllegalAccessError` will be thrown when linking the sub class. This can be unexpected because the app could have used a single class loader for both classes and thus not have seen the error — see `DifferentSourcesApp.java` from this patch for an example of such app.
> 
> This patch fixes the issue by using a single class loader for all unregistered classes. CDS does not allow classes with the same name making such solution possible.
> 
> Implementation note: `URLClassLoader` does not allow selecting a specific URL to load a specific class — I used reflection to override a private part of `URLClassLoader` responsible for URL selection while being able to use the rest of its implementation.

Hi Timofei, thanks for fixing this. 

(Sorry I didn't notice this PR until now ...)

I have a suggestion for further simplification.

src/java.base/share/classes/jdk/internal/misc/CDS.java line 420:

> 418:                 // class loader. Thus it is safe to delegate their loading to system class loader
> 419:                 // (our parent) - this is what the default implementation of loadClass() will do.
> 420:                 return (Class<?>) DEFINE_CLASS.invoke(this, name, res);

Instead of subclassing from `URLClassLoader` and go through the trouble of calling its `defineClass()` method, maybe we should just subclass from `ClassLoader` and maintain a hashtable of java.util.jar.JarFiles.


HashMap<String, JarFile> map = ....;
JarFile jar = map.get(source) ... or open a new JarFile if not found
byte[] buffer = read jar file for the given class name;
call ClassLoader.defineClass(buffer, ...)

src/java.base/share/classes/jdk/internal/misc/CDS.java line 446:

> 444:         protected Class<?> findClass(String name) throws ClassNotFoundException {
> 445:             // Unregistered classes should be found in load(...), registered classes should be
> 446:             // handeled by parent loaders

This does seem like a good simplification, as we no longer need to check the `currentClassName`, `currentSuperClass`, etc. 

We had a plan for supported multiple classes for the same name, but we never go around doing it (even though the classlist format supports that with the use of IDs).

I am not sure if the current tests includes a case where a class name `Foo` exists both in the classpath as well as in a custom loader. If not, I think we should add such a case (to ensure that UnregisteredClassLoader never delegates to the application class loader and finds the wrong `Foo`).

-------------

PR Review: https://git.openjdk.org/jdk/pull/24223#pullrequestreview-2741852934
PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2028155863
PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2028141440