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

[Ioi Lam]

On Tue, 8 Apr 2025 12:09:59 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.
>
> Timofei Pushkin has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Remove unnecessary scoping

Changes requested by iklam (Reviewer).

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

> 436:             // class loader. Thus it is safe to delegate their loading to system class loader
> 437:             // (our parent) - this is what the default implementation of loadClass() will do.
> 438:             return defineClass(name, bytes, 0, bytes.length);

I didn't realize that `URLClassLoader` will by default delegate to `ClassLoader::getSystemClassLoader()`. How about rewording the comments like this to clarify?


// defineClass() will internally invoke loadClass() to load supertypes of this unregistered class.
// Any supertype S with the name SN must have already been loaded (enforced by the order
// of classes in the classlist). In addition:
// - if S is an unregistered class, S must have already been have been defined by the current class
//   loader, and will be found by `this.findLoadedClass(SN)`
// - if S is not an unregistered class, S must have already been defined by the built-in boot,
//   platform, or system class loaders, and can be found by this.getParent().loadClass(SN, false)
// See the implementation of ClassLoader::loadClass() for details.
//
// Therefore, we should resolve all supertypes to the expected ones as specified by the
// <code>super:</code> and <code>interfaces:</code> attributes in the classlist. This
// invariance is validated by the C++ function ClassListParser::load_class_from_source()
assert getParent() == getSystemClassLoader();
return defineClass(name, bytes, 0, bytes.length);

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

PR Review: https://git.openjdk.org/jdk/pull/24223#pullrequestreview-2750817568
PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2033724629