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

[Ioi Lam]

On Wed, 9 Apr 2025 16:43:39 GMT, Timofei Pushkin <tpushkin at openjdk.org> wrote:

>> 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 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 will always use <S, SysL> as will the new one. It feels like a flaw to me but it is not a flaw of this patch.

Currently we have some restrictions if`-Djava.system.class.loader=` is specified

- we disable full module graph archiving: https://github.com/openjdk/jdk/blob/0e223f1456c14efdb423595bee3444d5e26db7c6/src/hotspot/share/cds/cdsConfig.cpp#L286
- we disable loading archived classes for platform and system loaders: https://github.com/openjdk/jdk/blob/0e223f1456c14efdb423595bee3444d5e26db7c6/src/hotspot/share/cds/filemap.cpp#L1874-L1886

I think we should extend this limitation further (in a separate issue)

- At dump time, dump only boot classes (no platform, system or unregistered)
- At run time, load only boot classes (no platform, system or unregistered)

I filed [JDK-8354315](https://bugs.openjdk.org/browse/JDK-8354315)

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

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