On Wed, 9 Apr 2025 09:03:41 GMT, Timofei Pushkin <tpush...@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
