On Wed, 9 Apr 2025 16:43:39 GMT, Timofei Pushkin <tpush...@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
