On Tue, 8 Apr 2025 17:36:57 GMT, Ioi Lam <ik...@openjdk.org> wrote: >> Timofei Pushkin has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove unnecessary scoping > > 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 `-Dsystem.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 an error (which will then be detected in `ClassListParser::load_class_from_source()` but still) 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2034846610
