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

[Timofei Pushkin]

On Wed, 9 Apr 2025 09:03:41 GMT, Timofei Pushkin <tpushkin at openjdk.org> wrote:

>> 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);
>
> I've actually noticed a problem here. I assumed that registered classes are always loaded by JDK's built-in class loaders (bootstrap, `jdk.internal.loader.ClassLoaders$PlatformClassLoader` or `jdk.internal.loader.ClassLoaders$AppClassLoader`). But in reality when the system class loader is user-provided (via `-Djava.system.class.loader=<class>`) classes loaded by it are still considered registered. This is a problem because such user-provided class loader may not delegate to real built-in class loaders.
> 
> For example:
> - Let C=<N, L> be a class C named N defined by class loader L
> - Let AppL be the built-in system class loader and SysL be the user-provided system class loader
> - Let U be an unregistered class which extends a registered <S, AppL>
> - Let SysL be able to load a registered <S, SysL> when requested (i.e. SysL doesn't delegate when loading a class named S)
> - When `UnregisteredClassLoader` will be loading U it will delegate the loading of its super named S to SysL and thus <S, SysL> will become a super of U instead of <S, AppL> — this is not correct
> 
> This won't be a problem if only classes loaded by the real built-in class loaders would be considered registered because such class loaders always delegate first (the 4th bullet above won't be possible), and thus it doesn't matter which of these loaders was used for delegation by the class loader which defined U.
> 
> This can't be fixed by just making `jdk.internal.loader.ClassLoaders$AppClassLoader` a parent of `UnregisteredClassLoader` and making `UnregisteredClassLoader.findClass(name)` return `getSystemClassLoader().loadClass(name)` because then the case when U extends <S, SysL> will not be handeled correctly (<S, AppL> will be used instead of <S, SysL>).
> 
> So it looks like I have to revert this delegation-based approach and use the old way. I'll also write a test from the above example to see if I am correct first.

I've realized that my example above cannot be expressed in the current class list format since the format doesn't distinguish between <S, SysL> and <S, AppL>, only that S is not unregistered. The existing implementation (without this PR) will always use <S, SysL>. It feels like a flaw to me but it is not a flaw of this patch.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2035755124