RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive
If a base class is package-private then its subclasses should have the same package name and defining class loader, otherwise `IllegalAccessError` is thrown when linking a subclass. Currently when dumping a static archive separate `URLClassLoader`s are used for each unregistered classes' source. Thus if two unregistered classes, a package-private base class and a sub class, from the same package reside in different sources `IllegalAccessError` will be thrown when linking the sub class. This can be unexpected because the app could have used a single class loader for both classes and thus not have seen the error — see `DifferentSourcesApp.java` from this patch for an example of such app. This patch fixes the issue by using a single class loader for all unregistered classes. CDS does not allow classes with the same name making such solution possible. Implementation note: `URLClassLoader` does not allow selecting a specific URL to load a specific class — I used reflection to override a private part of `URLClassLoader` responsible for URL selection while being able to use the rest of its implementation. - Commit messages: - Add regression test - Use a single UnregisteredClassLoader Changes: https://git.openjdk.org/jdk/pull/24223/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=24223&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8315130 Stats: 448 lines in 11 files changed: 276 ins; 98 del; 74 mod Patch: https://git.openjdk.org/jdk/pull/24223.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24223/head:pull/24223 PR: https://git.openjdk.org/jdk/pull/24223
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v4]
On Mon, 7 Apr 2025 16:00:27 GMT, Ioi Lam wrote: >> Timofei Pushkin has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Don't use URLClassPath > > src/hotspot/share/cds/classListParser.cpp line 534: > >> 532: GrowableArray specified_interfaces = >> get_specified_interfaces(); >> 533: >> 534: const char* source_path = ClassLoader::uri_to_path(_source); > > Is `ClassLoader::uri_to_path` necessary? I think `_source` is already a file > path. `_source` is a URL with `file:`removed from the beginning of it, so yes, this is necessary. For example, if a class resides in a directory called "my dir" its `_source` will be "my%20dir" — `uri_to_path` will replace "%20" with " ". - PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2032994904
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v5]
> If a base class is package-private then its subclasses should have the same > package name and defining class loader, otherwise `IllegalAccessError` is > thrown when linking a subclass. Currently when dumping a static archive > separate `URLClassLoader`s are used for each unregistered classes' source. > Thus if two unregistered classes, a package-private base class and a sub > class, from the same package reside in different sources `IllegalAccessError` > will be thrown when linking the sub class. This can be unexpected because the > app could have used a single class loader for both classes and thus not have > seen the error — see `DifferentSourcesApp.java` from this patch for an > example of such app. > > This patch fixes the issue by using a single class loader for all > unregistered classes. CDS does not allow classes with the same name making > such solution possible. Timofei Pushkin has updated the pull request incrementally with one additional commit since the last revision: Remove unnecessary scoping - Changes: - all: https://git.openjdk.org/jdk/pull/24223/files - new: https://git.openjdk.org/jdk/pull/24223/files/16a2f951..dff524c4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24223&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24223&range=03-04 Stats: 19 lines in 1 file changed: 0 ins; 3 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/24223.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24223/head:pull/24223 PR: https://git.openjdk.org/jdk/pull/24223
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v5]
On Wed, 9 Apr 2025 09:03:41 GMT, Timofei Pushkin 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 >> // super: and interfaces: 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=`) 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= 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 > - Let SysL be able to load a registered 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 will become a super > of U instead of — 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 > will not be handeled correctly ( will be used instead of > ). > > 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 and , only that S is not unregistered. The existing implementation (without this PR) will always use . 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
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v6]
> If a base class is package-private then its subclasses should have the same > package name and defining class loader, otherwise `IllegalAccessError` is > thrown when linking a subclass. Currently when dumping a static archive > separate `URLClassLoader`s are used for each unregistered classes' source. > Thus if two unregistered classes, a package-private base class and a sub > class, from the same package reside in different sources `IllegalAccessError` > will be thrown when linking the sub class. This can be unexpected because the > app could have used a single class loader for both classes and thus not have > seen the error — see `DifferentSourcesApp.java` from this patch for an > example of such app. > > This patch fixes the issue by using a single class loader for all > unregistered classes. CDS does not allow classes with the same name making > such solution possible. Timofei Pushkin has updated the pull request incrementally with one additional commit since the last revision: Remove findClass, extend explanation comments - Changes: - all: https://git.openjdk.org/jdk/pull/24223/files - new: https://git.openjdk.org/jdk/pull/24223/files/dff524c4..903f0f97 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24223&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24223&range=04-05 Stats: 25 lines in 1 file changed: 11 ins; 7 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/24223.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24223/head:pull/24223 PR: https://git.openjdk.org/jdk/pull/24223
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v4]
On Mon, 7 Apr 2025 16:15:47 GMT, Ioi Lam wrote:
>> Timofei Pushkin has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Don't use URLClassPath
>
> src/java.base/share/classes/jdk/internal/misc/CDS.java line 444:
>
>> 442: protected Class findClass(String name) throws
>> ClassNotFoundException {
>> 443: // Unregistered classes should be found in load(...),
>> registered classes should be
>> 444: // handeled by parent loaders
>
> Hmm, I wonder how the reference to java.lang.Object is resolved in this case.
> When `CustomLoadee` is parsed, the ClassFileParser needs to resolve is super
> class, which should result into a call to
> `UnregisteredClassLoader::findClass()`.
>
>
> "java/lang/Object id: 0",
> "CustomLoadee id: 1 super: 0 source: .",
`UnregisteredClassLoader::loadClass` will be called, i.e. the default
implementation from `ClassLoader::loadClass`, which will first try to delegate
to parent. The delegation chain will reach the boot loader which will find
`java.lang.Object` and thus `UnregisteredClassLoader::findClass()` won't be
called.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2033015572
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v5]
On Tue, 8 Apr 2025 17:36:57 GMT, Ioi Lam 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 > // super: and interfaces: 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=`) 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= 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 - Let SysL be able to load a registered 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 will become a super of U instead of — 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 will not be handeled correctly ( will be used instead of ). 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
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v4]
> If a base class is package-private then its subclasses should have the same > package name and defining class loader, otherwise `IllegalAccessError` is > thrown when linking a subclass. Currently when dumping a static archive > separate `URLClassLoader`s are used for each unregistered classes' source. > Thus if two unregistered classes, a package-private base class and a sub > class, from the same package reside in different sources `IllegalAccessError` > will be thrown when linking the sub class. This can be unexpected because the > app could have used a single class loader for both classes and thus not have > seen the error — see `DifferentSourcesApp.java` from this patch for an > example of such app. > > This patch fixes the issue by using a single class loader for all > unregistered classes. CDS does not allow classes with the same name making > such solution possible. > > Implementation note: `URLClassLoader` does not allow selecting a specific URL > to load a specific class — I used reflection to override a private part of > `URLClassLoader` responsible for URL selection while being able to use the > rest of its implementation. Timofei Pushkin has updated the pull request incrementally with one additional commit since the last revision: Don't use URLClassPath - Changes: - all: https://git.openjdk.org/jdk/pull/24223/files - new: https://git.openjdk.org/jdk/pull/24223/files/5db07cef..16a2f951 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24223&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24223&range=02-03 Stats: 150 lines in 4 files changed: 62 ins; 67 del; 21 mod Patch: https://git.openjdk.org/jdk/pull/24223.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24223/head:pull/24223 PR: https://git.openjdk.org/jdk/pull/24223
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v4]
On Fri, 4 Apr 2025 21:11:52 GMT, Ioi Lam wrote: >> All the opening and reading is handled by `URLClassPath` (it's not just >> JARs, can also be directories). I used only `defineClass` from >> `URLClassLoader` to minimize the behavior difference with the old code — >> besides defining the class it defines its package and protection domain. But >> it looks like static CDS strips these anyway, so I've removed >> `URLClassLoader` entirely. >> >> `URLClassPath` usage can also be removed — then the small addition to it >> from this PR won't be needed but its functionality will be somewhat >> duplicated in `CDS.UnregisteredLoader`. I've implemented this [in a separate >> branch](https://github.com/openjdk/jdk/compare/master...TimPushkin:jdk:one-loader-v2?expand=1). > > I prefer the changes in your separate branch. URLClassPath is intended for > sequential access. Adding the new method for random access could introduce > extraneous requirements that might affect future evolution of URLClassPath. > As the code for JarSource and Dir Source is quite small, I think it's > acceptable. Agree, I've brought it here - PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2031053093
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v6]
On Tue, 15 Apr 2025 00:58:19 GMT, Ioi Lam wrote: >> This case will work, I've added the test, however I found another case which >> won't be handled correctly, neither before nor after this change. >> >> This is the case you've suggested, it works fine both before and after the >> change. >> >> java/lang/Object id: 0 >> CustomLoadee3 id: 1 >> CustomLoadee3 id: 2 super: 0 source: a.jar >> CustomLoadee3Child id: 3 super: 2 source: a.jar >> >> >> However the below one doesn't work neither before nor after this change. At >> dump time, 2 is loaded instead of 1 as the super of 3 because 2, 3 are >> loaded by the same class loader and 1, 2 have the same name. It is possible >> to get such class list if 2 is loaded by a non-delegating unregistered >> loader while 3 is loaded by a different delegating one. >> >> # Difference with the previous: super of the last class >> java/lang/Object id: 0 >> CustomLoadee3 id: 1 >> CustomLoadee3 id: 2 super: 0 source: a.jar >> CustomLoadee3Child id: 3 super: 1 source: a.jar >> >> >> And the following case is working without this change but will not work with >> it. It is working now because there will be two different unregistered >> loaders for classes 2 and 3 (because they have the same source), however >> with this change there will be one and it will re-use 2 already loaded by it >> when loading 3. >> >> # Difference with the previous: source of the last class >> java/lang/Object id: 0 >> CustomLoadee3 id: 1 >> CustomLoadee3 id: 2 super: 0 source: a.jar >> CustomLoadee3Child id: 3 super: 1 source: b.jar >> >> >> Although the last case is a regression, I am not sure how much of a problem >> this is since in general having a registered super with the same name as >> another unregistered class is not supported anyway as shown by the second >> case. > > Thanks for doing the investigation. I think we are always going to have some > corner cases that cannot be covered: > > - For your second case, we would need a separate ClassLoader per class. > - However, that will result in the IllegalAccessError in the current PR > > The problem is we are trying to reconstruct the classes without > reconstructing the ClassLoaders. A real solution would probably require > updating the classlist format. > > However, future CDS improvements will be based on the AOT cache introduced in > JEP 483. As of JDK 25, we have changed the AOT cache configuration file from > a text file (classlist) to a binary file (see > https://github.com/openjdk/jdk/pull/23484) . Also, the unregistered classes > are saved from the training run, where we still have ClassLoader information > (see https://github.com/openjdk/jdk/pull/23926). Therefore, I think we should > just leave the old classlist format as is, and accept that certain > unregistered classes cannot be supported. I'll try add some test cases > ([JDK-8354557](https://bugs.openjdk.org/browse/JDK-8354557)) for the AOT > cache using the test cases you identified above. > > This PR address the IllegalAccessError problem and simplifies the > implementation of classlist handling. However, it causes incompatibility (in > your third case). I think we should change the error checking code in > `InstanceKlass* ClassListParser::load_class_from_source()` to print a warning > message and continue. These are the rare cases that we cannot support. Since > CDS is just a cache mechanism, it should be acceptable that certain classes > are not supported. We can use your second and third test cases to validate > the warning messages. I fully agree, will implement the warnings - PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2044299087
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v8]
> If a base class is package-private then its subclasses should have the same > package name and defining class loader, otherwise `IllegalAccessError` is > thrown when linking a subclass. Currently when dumping a static archive > separate `URLClassLoader`s are used for each unregistered classes' source. > Thus if two unregistered classes, a package-private base class and a sub > class, from the same package reside in different sources `IllegalAccessError` > will be thrown when linking the sub class. This can be unexpected because the > app could have used a single class loader for both classes and thus not have > seen the error — see `DifferentSourcesApp.java` from this patch for an > example of such app. > > This patch fixes the issue by using a single class loader for all > unregistered classes. CDS does not allow classes with the same name making > such solution possible. Timofei Pushkin has updated the pull request incrementally with one additional commit since the last revision: Remove unused imports - Changes: - all: https://git.openjdk.org/jdk/pull/24223/files - new: https://git.openjdk.org/jdk/pull/24223/files/d2e51dac..fca3c919 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24223&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24223&range=06-07 Stats: 3 lines in 1 file changed: 0 ins; 3 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/24223.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24223/head:pull/24223 PR: https://git.openjdk.org/jdk/pull/24223
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v5]
On Thu, 10 Apr 2025 16:01:20 GMT, Ioi Lam wrote: >> 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) > > In the current JDK (with or without this patch), in your scenario, I think > the name "S" will be printed twice with two different IDs. If a Child class > named "C" is loaded by a custom loader, it will refer to one of the IDs. > Depending on the order of loading, it might refer to the first or the second > ID. > > During dump time, we will try to load this class twice, but both attempts > will result in the same class (defined by the user-defined system class > loader). When resolving the unregistered class, using either ID will give you > the correct super class ... > > Anyway, this seems too fragile. I think we should fix > [JDK-8354315](https://bugs.openjdk.org/browse/JDK-8354315) before integrating > this patch. Or, maybe we can include the fixes of that bug in this PR as well > (and then close that bug as a duplicate of this one). What do you think? In my scenario I actually meant that is in the class list while is not but it doesn't matter much, the incorrect one will be loaded any way. Since the new implementation will behave the same as the existing one in this aspect I think it is OK to fix JDK-8354315 separately. - PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2042346769
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v7]
> If a base class is package-private then its subclasses should have the same > package name and defining class loader, otherwise `IllegalAccessError` is > thrown when linking a subclass. Currently when dumping a static archive > separate `URLClassLoader`s are used for each unregistered classes' source. > Thus if two unregistered classes, a package-private base class and a sub > class, from the same package reside in different sources `IllegalAccessError` > will be thrown when linking the sub class. This can be unexpected because the > app could have used a single class loader for both classes and thus not have > seen the error — see `DifferentSourcesApp.java` from this patch for an > example of such app. > > This patch fixes the issue by using a single class loader for all > unregistered classes. CDS does not allow classes with the same name making > such solution possible. Timofei Pushkin has updated the pull request incrementally with two additional commits since the last revision: - Fix indentation - Extend ClassFromClasspath test - Changes: - all: https://git.openjdk.org/jdk/pull/24223/files - new: https://git.openjdk.org/jdk/pull/24223/files/903f0f97..d2e51dac Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24223&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24223&range=05-06 Stats: 75 lines in 3 files changed: 62 ins; 0 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/24223.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24223/head:pull/24223 PR: https://git.openjdk.org/jdk/pull/24223
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v6]
On Thu, 10 Apr 2025 16:04:59 GMT, Ioi Lam wrote:
>> Timofei Pushkin has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove findClass, extend explanation comments
>
> test/hotspot/jtreg/runtime/cds/appcds/customLoader/ClassFromClasspath.java
> line 53:
>
>> 51: out.shouldContain("unreg CustomLoadee");
>> 52: }
>> 53: }
>
> For completeness, I think we should have a more complicated scenario:
>
> - load CustomLoadee in both the app loader and a custom loader
> - load CustomLoadeeChild in the custom loader. Its super class should be the
> one defined in the custom loader
>
> At run time, verify that CustomLoadeeChild is archived and its super class is
> defined in the custom loader
This case will work, I've added the test, however I found another case which
won't be handled correctly, neither before nor after this change.
This is the case you've suggested, it works fine both before and after the
change.
java/lang/Object id: 0
CustomLoadee3 id: 1
CustomLoadee3 id: 2 super: 0 source: a.jar
CustomLoadee3Child id: 3 super: 2 source: a.jar
However the below one doesn't work neither before nor after this change. At
dump time, 2 is loaded instead of 1 as the super of 3 because 2, 3 are loaded
by the same class loader and 1, 2 have the same name. It is possible to get
such class list if 2 is loaded by a non-delegating unregistered loader while 3
is loaded by a different delegating one.
# Difference with the previous: super of the last class
java/lang/Object id: 0
CustomLoadee3 id: 1
CustomLoadee3 id: 2 super: 0 source: a.jar
CustomLoadee3Child id: 3 super: 1 source: a.jar
And the following case is working without this change but will not work with
it. It is working now because there will be two different unregistered loaders
for classes 2 and 3 (because they have the same source), however with this
change there will be one and it will re-use 2 already loaded by it when loading
3.
# Difference with the previous: source of the last class
java/lang/Object id: 0
CustomLoadee3 id: 1
CustomLoadee3 id: 2 super: 0 source: a.jar
CustomLoadee3Child id: 3 super: 1 source: b.jar
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2042386067
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v10]
On Wed, 16 Apr 2025 14:34:25 GMT, Timofei Pushkin wrote: >> If a base class is package-private then its subclasses should have the same >> package name and defining class loader, otherwise `IllegalAccessError` is >> thrown when linking a subclass. Currently when dumping a static archive >> separate `URLClassLoader`s are used for each unregistered classes' source. >> Thus if two unregistered classes, a package-private base class and a sub >> class, from the same package reside in different sources >> `IllegalAccessError` will be thrown when linking the sub class. This can be >> unexpected because the app could have used a single class loader for both >> classes and thus not have seen the error — see `DifferentSourcesApp.java` >> from this patch for an example of such app. >> >> This patch fixes the issue by using a single class loader for all >> unregistered classes. CDS does not allow classes with the same name making >> such solution possible. > > Timofei Pushkin has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 15 commits: > > - Empty commit to make GH update the PR > - Merge remote-tracking branch 'openjdk-jdk/master' into one-loader > - Implement super overshadowing warning > - Omit Source classes from archive > - Remove unused imports > - Fix indentation > - Extend ClassFromClasspath test > - Remove findClass, extend explanation comments > - Remove unnecessary scoping > - Don't use URLClassPath > - ... and 5 more: https://git.openjdk.org/jdk/compare/465c8e65...595756f3 Besides adding the warnings I noticed that `UnregisteredClassLoader` is omitted from the dump and implemented the same for `UnregisteredClassLoader$Source` and its implementations. - PR Comment: https://git.openjdk.org/jdk/pull/24223#issuecomment-2810027521
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v10]
> If a base class is package-private then its subclasses should have the same > package name and defining class loader, otherwise `IllegalAccessError` is > thrown when linking a subclass. Currently when dumping a static archive > separate `URLClassLoader`s are used for each unregistered classes' source. > Thus if two unregistered classes, a package-private base class and a sub > class, from the same package reside in different sources `IllegalAccessError` > will be thrown when linking the sub class. This can be unexpected because the > app could have used a single class loader for both classes and thus not have > seen the error — see `DifferentSourcesApp.java` from this patch for an > example of such app. > > This patch fixes the issue by using a single class loader for all > unregistered classes. CDS does not allow classes with the same name making > such solution possible. Timofei Pushkin has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 15 commits: - Empty commit to make GH update the PR - Merge remote-tracking branch 'openjdk-jdk/master' into one-loader - Implement super overshadowing warning - Omit Source classes from archive - Remove unused imports - Fix indentation - Extend ClassFromClasspath test - Remove findClass, extend explanation comments - Remove unnecessary scoping - Don't use URLClassPath - ... and 5 more: https://git.openjdk.org/jdk/compare/465c8e65...595756f3 - Changes: https://git.openjdk.org/jdk/pull/24223/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=24223&range=09 Stats: 659 lines in 13 files changed: 475 ins; 122 del; 62 mod Patch: https://git.openjdk.org/jdk/pull/24223.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24223/head:pull/24223 PR: https://git.openjdk.org/jdk/pull/24223
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v9]
> If a base class is package-private then its subclasses should have the same > package name and defining class loader, otherwise `IllegalAccessError` is > thrown when linking a subclass. Currently when dumping a static archive > separate `URLClassLoader`s are used for each unregistered classes' source. > Thus if two unregistered classes, a package-private base class and a sub > class, from the same package reside in different sources `IllegalAccessError` > will be thrown when linking the sub class. This can be unexpected because the > app could have used a single class loader for both classes and thus not have > seen the error — see `DifferentSourcesApp.java` from this patch for an > example of such app. > > This patch fixes the issue by using a single class loader for all > unregistered classes. CDS does not allow classes with the same name making > such solution possible. Timofei Pushkin has updated the pull request incrementally with two additional commits since the last revision: - Implement super overshadowing warning - Omit Source classes from archive - Changes: - all: https://git.openjdk.org/jdk/pull/24223/files - new: https://git.openjdk.org/jdk/pull/24223/files/fca3c919..26371665 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24223&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24223&range=07-08 Stats: 397 lines in 10 files changed: 259 ins; 132 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/24223.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24223/head:pull/24223 PR: https://git.openjdk.org/jdk/pull/24223
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v10]
On Wed, 16 Apr 2025 21:52:46 GMT, Ioi Lam wrote:
>> Timofei Pushkin has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 15 commits:
>>
>> - Empty commit to make GH update the PR
>> - Merge remote-tracking branch 'openjdk-jdk/master' into one-loader
>> - Implement super overshadowing warning
>> - Omit Source classes from archive
>> - Remove unused imports
>> - Fix indentation
>> - Extend ClassFromClasspath test
>> - Remove findClass, extend explanation comments
>> - Remove unnecessary scoping
>> - Don't use URLClassPath
>> - ... and 5 more: https://git.openjdk.org/jdk/compare/465c8e65...595756f3
>
> src/hotspot/share/cds/classListParser.cpp line 517:
>
>> 515: return;
>> 516: }
>> 517: assert(!supertype->is_shared_unregistered_class(), "unregistered
>> supertype cannot be overshadowed");
>
> Does this always prevent an unregistered class to use any other unregistered
> class as its super type?
>
> I think a better check would be here:
>
>
> if (!k->local_interfaces()->contains(specified_interface)) {
> + jio_fprintf(defaultStream::error_stream(), "Specified interface %s (id
> %d) is not directly implemented\n",
> + specified_interface->external_name(), _interfaces->at(i));
> print_specified_interfaces();
> print_actual_interfaces(k);
> + throw exception .
> - error("Specified interface %s (id %d) is not directly implemented",
> - specified_interface->external_name(), _interfaces->at(i));
> }
Overshadowing should be checked before attempting to load the class. Otherwise
if the wrongly used super has a different classfile than the specified super we
may get class loading errors (see JVMS 5.3.5.3 and 5.3.5.4), e.g. if the
specified super wasn't final but we try to use a final class instead of it.
I'll extend the related explanation comments in the code.
> Does this always prevent an unregistered class to use any other unregistered
> class as its super type?
The only way I see for another class to be used instead is for the unregistered
class being loaded to reference supertypes of unexpected names in its
classfile. But this is an error and it will be detected as such in the C++ code
you've cited. If you have any other cases in mind please share.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2048379190
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v10]
On Thu, 17 Apr 2025 05:45:50 GMT, Calvin Cheung wrote:
>> Timofei Pushkin has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains 15 commits:
>>
>> - Empty commit to make GH update the PR
>> - Merge remote-tracking branch 'openjdk-jdk/master' into one-loader
>> - Implement super overshadowing warning
>> - Omit Source classes from archive
>> - Remove unused imports
>> - Fix indentation
>> - Extend ClassFromClasspath test
>> - Remove findClass, extend explanation comments
>> - Remove unnecessary scoping
>> - Don't use URLClassPath
>> - ... and 5 more: https://git.openjdk.org/jdk/compare/465c8e65...595756f3
>
> test/hotspot/jtreg/runtime/cds/appcds/customLoader/RegUnregSuperTest.java
> line 77:
>
>> 75: out.shouldContain("CustomLoadee3Child (id 3) has super-type
>> CustomLoadee3 (id 1) overshadowed by another class with the same name");
>> 76: } else {
>> 77: out.shouldContain("unreg CustomLoadee3Child\n");
>
> Could you remove the linefeed ('\n') chars?
> I'm seeing test failure when running with the `-XX:+AOTClassLinking` option.
>
>
> java.lang.RuntimeException: 'app CustomLoadee3
> ' missing from stdout/stderr
> at
> jdk.test.lib.process.OutputAnalyzer.shouldContain(OutputAnalyzer.java:253)
> at RegUnregSuperTest.main(RegUnregSuperTest.java:71)
> at
> java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:104)
> at java.base/java.lang.reflect.Method.invoke(Method.java:565)
> at
> com.sun.javatest.regtest.agent.MainActionHelper$AgentVMRunnable.run(MainActionHelper.java:335)
> at java.base/java.lang.Thread.run(Thread.java:1447)
>
> The expected output is there:
> `[1.686s][debug ][cds,class] klasses[ 1548] = 0x0008004c7518 app
> CustomLoadee3 aot-linked`
Do I understand correctly that you modified the test's code to use
`-XX:+AOTClassLinking` to get this error? Do you think there should be another
test variant with `-XX:+AOTClassLinking` (it is easy to add)?
"\n" is used after "CustomLoadee3" because `shouldContain("unreg
CustomLoadee3")` will accept either "unreg CustomLoadee3" or "unreg
CustomLoadee3Child" and the second case is not what I want to detect.
Anyway, I've changed the test to pass when `-XX:+AOTClassLinking` is used.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2048413693
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v8]
On Tue, 15 Apr 2025 17:59:41 GMT, Calvin Cheung wrote:
>> Timofei Pushkin has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove unused imports
>
> src/java.base/share/classes/jdk/internal/misc/CDS.java line 390:
>
>> 388: @Override
>> 389: public byte[] readClassFile(String className) throws
>> IOException {
>> 390: final var subPath = className.replace('.',
>> File.separatorChar).concat(".class");
>
> Should File.separatorChar be ‘/‘ ? (just like at line 369)
No, this is an intentional difference.
Here the string is then passed to `Path.of(...)` so it should definitely be
`File.separatorChar` because this is what `Path.of(...)` expects.
Regarding line 369, the string is then passed to `JarFile.getEntry(String
name)` — I wasn't able to find the description of the format of the `name`
parameter so I followed the way `URLClassLoader` does it, it uses `/` for
loading from a JAR (and `File.separatorChar` for loading from a directory).
This seems logical since JAR format is platform-independent.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2046316431
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v11]
> If a base class is package-private then its subclasses should have the same > package name and defining class loader, otherwise `IllegalAccessError` is > thrown when linking a subclass. Currently when dumping a static archive > separate `URLClassLoader`s are used for each unregistered classes' source. > Thus if two unregistered classes, a package-private base class and a sub > class, from the same package reside in different sources `IllegalAccessError` > will be thrown when linking the sub class. This can be unexpected because the > app could have used a single class loader for both classes and thus not have > seen the error — see `DifferentSourcesApp.java` from this patch for an > example of such app. > > This patch fixes the issue by using a single class loader for all > unregistered classes. CDS does not allow classes with the same name making > such solution possible. Timofei Pushkin has updated the pull request incrementally with two additional commits since the last revision: - Make RegUnregSuperTest AOTClassLinking-compatible - Extend overshadowing explanation comments - Changes: - all: https://git.openjdk.org/jdk/pull/24223/files - new: https://git.openjdk.org/jdk/pull/24223/files/595756f3..915987b0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24223&range=10 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24223&range=09-10 Stats: 19 lines in 3 files changed: 9 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/24223.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24223/head:pull/24223 PR: https://git.openjdk.org/jdk/pull/24223
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v2]
> If a base class is package-private then its subclasses should have the same > package name and defining class loader, otherwise `IllegalAccessError` is > thrown when linking a subclass. Currently when dumping a static archive > separate `URLClassLoader`s are used for each unregistered classes' source. > Thus if two unregistered classes, a package-private base class and a sub > class, from the same package reside in different sources `IllegalAccessError` > will be thrown when linking the sub class. This can be unexpected because the > app could have used a single class loader for both classes and thus not have > seen the error — see `DifferentSourcesApp.java` from this patch for an > example of such app. > > This patch fixes the issue by using a single class loader for all > unregistered classes. CDS does not allow classes with the same name making > such solution possible. > > Implementation note: `URLClassLoader` does not allow selecting a specific URL > to load a specific class — I used reflection to override a private part of > `URLClassLoader` responsible for URL selection while being able to use the > rest of its implementation. Timofei Pushkin has updated the pull request incrementally with one additional commit since the last revision: Test classpath case - Changes: - all: https://git.openjdk.org/jdk/pull/24223/files - new: https://git.openjdk.org/jdk/pull/24223/files/3be4c5f9..784c1a83 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24223&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24223&range=00-01 Stats: 53 lines in 1 file changed: 53 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/24223.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24223/head:pull/24223 PR: https://git.openjdk.org/jdk/pull/24223
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v3]
On Fri, 4 Apr 2025 05:58:47 GMT, Ioi Lam wrote:
>> Timofei Pushkin has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Make getResource concurrent-friendly
>> - Don't use URLClassLoader
>
> src/java.base/share/classes/jdk/internal/misc/CDS.java line 446:
>
>> 444: protected Class findClass(String name) throws
>> ClassNotFoundException {
>> 445: // Unregistered classes should be found in load(...),
>> registered classes should be
>> 446: // handeled by parent loaders
>
> This does seem like a good simplification, as we no longer need to check the
> `currentClassName`, `currentSuperClass`, etc.
>
> We had a plan for supported multiple classes for the same name, but we never
> go around doing it (even though the classlist format supports that with the
> use of IDs).
>
> I am not sure if the current tests includes a case where a class name `Foo`
> exists both in the classpath as well as in a custom loader. If not, I think
> we should add such a case (to ensure that UnregisteredClassLoader never
> delegates to the application class loader and finds the wrong `Foo`).
Thank you for the review!
> We had a plan for supported multiple classes for the same name, but we never
> go around doing it (even though the classlist format supports that with the
> use of IDs).
With this approach having multiple unregistered classes with the same name is
not possible (JVM will not allow this for a single class loader) so if you are
still planning to support this in the near future it would be better to go with
the approach originally suggested in the issue: continue using multiple
unregistered class loaders but record class loader ID in class list and create
only one archiving class loader per recorded ID. But that would be a larger
change since it requires class list format to be changed.
> I am not sure if the current tests includes a case where a class name Foo
> exists both in the classpath as well as in a custom loader
Pre-existing `ClassFromSystemModule` test seems to test the boot classpath
case. I've also added a new test for the app classpath case.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2029248783
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v3]
> If a base class is package-private then its subclasses should have the same > package name and defining class loader, otherwise `IllegalAccessError` is > thrown when linking a subclass. Currently when dumping a static archive > separate `URLClassLoader`s are used for each unregistered classes' source. > Thus if two unregistered classes, a package-private base class and a sub > class, from the same package reside in different sources `IllegalAccessError` > will be thrown when linking the sub class. This can be unexpected because the > app could have used a single class loader for both classes and thus not have > seen the error — see `DifferentSourcesApp.java` from this patch for an > example of such app. > > This patch fixes the issue by using a single class loader for all > unregistered classes. CDS does not allow classes with the same name making > such solution possible. > > Implementation note: `URLClassLoader` does not allow selecting a specific URL > to load a specific class — I used reflection to override a private part of > `URLClassLoader` responsible for URL selection while being able to use the > rest of its implementation. Timofei Pushkin has updated the pull request incrementally with two additional commits since the last revision: - Make getResource concurrent-friendly - Don't use URLClassLoader - Changes: - all: https://git.openjdk.org/jdk/pull/24223/files - new: https://git.openjdk.org/jdk/pull/24223/files/784c1a83..5db07cef Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24223&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24223&range=01-02 Stats: 57 lines in 2 files changed: 4 ins; 37 del; 16 mod Patch: https://git.openjdk.org/jdk/pull/24223.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24223/head:pull/24223 PR: https://git.openjdk.org/jdk/pull/24223
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v3]
On Fri, 4 Apr 2025 06:15:27 GMT, Ioi Lam wrote: >> Timofei Pushkin has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Make getResource concurrent-friendly >> - Don't use URLClassLoader > > src/java.base/share/classes/jdk/internal/misc/CDS.java line 420: > >> 418: // class loader. Thus it is safe to delegate their >> loading to system class loader >> 419: // (our parent) - this is what the default >> implementation of loadClass() will do. >> 420: return (Class) DEFINE_CLASS.invoke(this, name, res); > > Instead of subclassing from `URLClassLoader` and go through the trouble of > calling its `defineClass()` method, maybe we should just subclass from > `ClassLoader` and maintain a hashtable of java.util.jar.JarFiles. > > > HashMap map = ; > JarFile jar = map.get(source) ... or open a new JarFile if not found > byte[] buffer = read jar file for the given class name; > call ClassLoader.defineClass(buffer, ...) All the opening and reading is handled by `URLClassPath` (it's not just JARs, can also be directories). I used only `defineClass` from `URLClassLoader` to minimize the behavior difference with the old code — besides defining the class it defines its package and protection domain. But it looks like static CDS strips these anyway, so I've removed `URLClassLoader` entirely. `URLClassPath` usage can also be removed — then the small addition to it from this PR won't be needed but its functionality will be somewhat duplicated in `CDS.UnregisteredLoader`. I've implemented this [in a separate branch](https://github.com/openjdk/jdk/compare/master...TimPushkin:jdk:one-loader-v2?expand=1). - PR Review Comment: https://git.openjdk.org/jdk/pull/24223#discussion_r2029253444
Integrated: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive
On Tue, 25 Mar 2025 11:08:24 GMT, Timofei Pushkin wrote: > If a base class is package-private then its subclasses should have the same > package name and defining class loader, otherwise `IllegalAccessError` is > thrown when linking a subclass. Currently when dumping a static archive > separate `URLClassLoader`s are used for each unregistered classes' source. > Thus if two unregistered classes, a package-private base class and a sub > class, from the same package reside in different sources `IllegalAccessError` > will be thrown when linking the sub class. This can be unexpected because the > app could have used a single class loader for both classes and thus not have > seen the error — see `DifferentSourcesApp.java` from this patch for an > example of such app. > > This patch fixes the issue by using a single class loader for all > unregistered classes. CDS does not allow classes with the same name making > such solution possible. This pull request has now been integrated. Changeset: 46a12e78 Author:Timofei Pushkin Committer: Ioi Lam URL: https://git.openjdk.org/jdk/commit/46a12e781edcbe9da7bd39eb9e101fc680053cef Stats: 671 lines in 13 files changed: 486 ins; 122 del; 63 mod 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive Reviewed-by: iklam, ccheung - PR: https://git.openjdk.org/jdk/pull/24223
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v12]
> If a base class is package-private then its subclasses should have the same > package name and defining class loader, otherwise `IllegalAccessError` is > thrown when linking a subclass. Currently when dumping a static archive > separate `URLClassLoader`s are used for each unregistered classes' source. > Thus if two unregistered classes, a package-private base class and a sub > class, from the same package reside in different sources `IllegalAccessError` > will be thrown when linking the sub class. This can be unexpected because the > app could have used a single class loader for both classes and thus not have > seen the error — see `DifferentSourcesApp.java` from this patch for an > example of such app. > > This patch fixes the issue by using a single class loader for all > unregistered classes. CDS does not allow classes with the same name making > such solution possible. Timofei Pushkin has updated the pull request incrementally with one additional commit since the last revision: Make supertype obstruction check easier to understand - Changes: - all: https://git.openjdk.org/jdk/pull/24223/files - new: https://git.openjdk.org/jdk/pull/24223/files/915987b0..a3b7dd96 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=24223&range=11 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=24223&range=10-11 Stats: 16 lines in 3 files changed: 3 ins; 0 del; 13 mod Patch: https://git.openjdk.org/jdk/pull/24223.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/24223/head:pull/24223 PR: https://git.openjdk.org/jdk/pull/24223
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v12]
On Wed, 14 May 2025 08:18:39 GMT, Timofei Pushkin wrote: >> If a base class is package-private then its subclasses should have the same >> package name and defining class loader, otherwise `IllegalAccessError` is >> thrown when linking a subclass. Currently when dumping a static archive >> separate `URLClassLoader`s are used for each unregistered classes' source. >> Thus if two unregistered classes, a package-private base class and a sub >> class, from the same package reside in different sources >> `IllegalAccessError` will be thrown when linking the sub class. This can be >> unexpected because the app could have used a single class loader for both >> classes and thus not have seen the error — see `DifferentSourcesApp.java` >> from this patch for an example of such app. >> >> This patch fixes the issue by using a single class loader for all >> unregistered classes. CDS does not allow classes with the same name making >> such solution possible. > > Timofei Pushkin has updated the pull request incrementally with one > additional commit since the last revision: > > Make supertype obstruction check easier to understand Sorry, had to pause working on this for some time because of other stuff. I believe I have addressed all the existing comments now. - PR Comment: https://git.openjdk.org/jdk/pull/24223#issuecomment-2879622953
Re: RFR: 8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive [v12]
On Wed, 14 May 2025 08:18:39 GMT, Timofei Pushkin wrote: >> If a base class is package-private then its subclasses should have the same >> package name and defining class loader, otherwise `IllegalAccessError` is >> thrown when linking a subclass. Currently when dumping a static archive >> separate `URLClassLoader`s are used for each unregistered classes' source. >> Thus if two unregistered classes, a package-private base class and a sub >> class, from the same package reside in different sources >> `IllegalAccessError` will be thrown when linking the sub class. This can be >> unexpected because the app could have used a single class loader for both >> classes and thus not have seen the error — see `DifferentSourcesApp.java` >> from this patch for an example of such app. >> >> This patch fixes the issue by using a single class loader for all >> unregistered classes. CDS does not allow classes with the same name making >> such solution possible. > > Timofei Pushkin has updated the pull request incrementally with one > additional commit since the last revision: > > Make supertype obstruction check easier to understand Thank you! > Once this PR has 2 reviews and is final, I will run some tests in our test > pipeline. @iklam It is final, please run the tests. - PR Comment: https://git.openjdk.org/jdk/pull/24223#issuecomment-2882738562
