8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive #24223

TimPushkin · 2025-03-25T11:08:24Z

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 URLClassLoaders 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.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24223/head:pull/24223
$ git checkout pull/24223

Update a local copy of the PR:
$ git checkout pull/24223
$ git pull https://git.openjdk.org/jdk.git pull/24223/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24223

View PR using the GUI difftool:
$ git pr show -t 24223

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24223.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2025-03-25T11:08:57Z

👋 Welcome back tpushkin! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-03-25T11:09:25Z

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

openjdk · 2025-03-25T11:10:08Z

@TimPushkin The following labels will be automatically applied to this pull request:

core-libs
hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-03-25T11:14:09Z

Webrevs

iklam

Hi Timofei, thanks for fixing this.

(Sorry I didn't notice this PR until now ...)

I have a suggestion for further simplification.

src/java.base/share/classes/jdk/internal/misc/CDS.java

src/hotspot/share/cds/classListParser.cpp

src/hotspot/share/cds/unregisteredClasses.cpp

src/java.base/share/classes/jdk/internal/misc/CDS.java

test/hotspot/jtreg/runtime/cds/appcds/customLoader/ClassFromClasspath.java

iklam · 2025-04-15T00:59:12Z

/reviewers 2

openjdk · 2025-04-15T00:59:42Z

@iklam
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

calvinccheung

I have one comment in CDS.java.

src/java.base/share/classes/jdk/internal/misc/CDS.java

openjdk · 2025-04-16T14:12:50Z

@TimPushkin this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout one-loader
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

TimPushkin · 2025-04-16T15:57:02Z

Besides adding the warnings I noticed that UnregisteredClassLoader is omitted from the dump and implemented the same for UnregisteredClassLoader$Source and its implementations.

iklam · 2025-04-16T21:52:46Z

src/hotspot/share/cds/classListParser.cpp

+// If an unregistered class U is specified to have a registered supertype S1
+// named SN but an unregistered class S2 also named SN has already been loaded
+// S2 will be incorrectly used as the supertype of U instead of S1 due to
+// limitations in the loading mechanism of unregistered classes.
+void ClassListParser::check_supertype_overshadowing(int supertype_id, const InstanceKlass* supertype, TRAPS) {
+  const InstanceKlass* overshadower = SystemDictionaryShared::get_unregistered_class(supertype->name());
+  if (overshadower == nullptr || overshadower == supertype) {
+    return;
+  }
+  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.

Sorry I didn't notice the overshadower == supertype check. Now I understand how it works.

However, I think the logic of the code isn't expressed in the same way as mentioned in the comments. That's why I misread it. I would suggest restructuring it like this for clarity.

Also, the opposite to "unregistered" is "built-in". We used to have a third type (classes for "registered" custom class loaders) but that has been removed, leaving us the odd terminology. Maybe we'll clean that up when more support is added to custom loaders in Leyden.

BTW, I think we should use "shadow" instead of "overshadow" for brevity.

void ClassListParser::check_supertype_overshadowing(int supertype_id, const InstanceKlass* specified_supertype, TRAPS) { if (!specified_supertype->is_shared_unregistered_class()) { const InstanceKlass* overshadower = SystemDictionaryShared::get_unregistered_class(specified_supertype->name()); if (overshadower != nullptr) { // If an unregistered class U is specified to have a built-in supertype S1 // named SN but an unregistered class S2 also named SN has already been loaded // S2 will be incorrectly resolved as the supertype of U instead of S1 due to // limitations in the loading mechanism of unregistered classes. // This happens very rarely and we don't support such use cases in the CDS archive. ResourceMark rm; THROW_MSG(vmSymbols::java_lang_UnsupportedOperationException(), err_msg("%s (id %d) has super-type %s (id %d) overshadowed by another class with the same name", _class_name, _id, supertype->external_name(), supertype_id)); } } }

calvinccheung

Found an issue on the RegUnregSuperTest.java.

calvinccheung · 2025-04-17T05:45:50Z

test/hotspot/jtreg/runtime/cds/appcds/customLoader/RegUnregSuperTest.java

+        out.shouldContain("app   CustomLoadee3\n");
+        out.shouldContain("unreg CustomLoadee3\n");
+        if ("reg".equals(variant)) {
+            out.shouldNotContain("unreg CustomLoadee3Child\n");
+            out.shouldContain("CustomLoadee3Child (id 3) has super-type CustomLoadee3 (id 1) overshadowed by another class with the same name");
+        } else {
+            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] = 0x00000008004c7518 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.

No, I didn't modify the test code but ran your patch through our internal testing tiers. One of the tier specifies the -XX:+AOTClassLinking option. You can also run it via jtreg command line using the -javaoptions, e.g. -javaoptions:"-XX:+AOTClassLinking"

iklam

Looks good to me overall. I just have a request for cleaning up ClassListParser::check_supertype_overshadowing()

TimPushkin added 2 commits March 25, 2025 09:53

Use a single UnregisteredClassLoader

3eccfbe

Add regression test

3be4c5f

openjdk bot added the rfr Pull request is ready for review label Mar 25, 2025

openjdk bot added hotspot-runtime [email protected] core-libs [email protected] labels Mar 25, 2025

iklam reviewed Apr 4, 2025

View reviewed changes

src/java.base/share/classes/jdk/internal/misc/CDS.java Outdated Show resolved Hide resolved

src/java.base/share/classes/jdk/internal/misc/CDS.java Outdated Show resolved Hide resolved

TimPushkin added 4 commits April 4, 2025 14:22

Test classpath case

784c1a8

Don't use URLClassLoader

4889593

Make getResource concurrent-friendly

5db07ce

Don't use URLClassPath

16a2f95

iklam reviewed Apr 7, 2025

View reviewed changes

src/hotspot/share/cds/classListParser.cpp Show resolved Hide resolved

src/hotspot/share/cds/unregisteredClasses.cpp Outdated Show resolved Hide resolved

src/java.base/share/classes/jdk/internal/misc/CDS.java Outdated Show resolved Hide resolved

Remove unnecessary scoping

dff524c

iklam suggested changes Apr 8, 2025

View reviewed changes

src/java.base/share/classes/jdk/internal/misc/CDS.java Outdated Show resolved Hide resolved

src/java.base/share/classes/jdk/internal/misc/CDS.java Outdated Show resolved Hide resolved

Remove findClass, extend explanation comments

903f0f9

TimPushkin requested a review from iklam April 10, 2025 06:12

iklam reviewed Apr 10, 2025

View reviewed changes

test/hotspot/jtreg/runtime/cds/appcds/customLoader/ClassFromClasspath.java Outdated Show resolved Hide resolved

TimPushkin added 3 commits April 14, 2025 15:32

Extend ClassFromClasspath test

df497f4

Fix indentation

d2e51da

Remove unused imports

fca3c91

calvinccheung reviewed Apr 15, 2025

View reviewed changes

src/java.base/share/classes/jdk/internal/misc/CDS.java Show resolved Hide resolved

TimPushkin added 2 commits April 16, 2025 15:36

Omit Source classes from archive

ad76966

Implement super overshadowing warning

2637166

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Apr 16, 2025

openjdk bot removed the rfr Pull request is ready for review label Apr 16, 2025

TimPushkin added 2 commits April 16, 2025 16:22

Merge remote-tracking branch 'openjdk-jdk/master' into one-loader

d5e5565

Empty commit to make GH update the PR

595756f

openjdk bot added rfr Pull request is ready for review and removed merge-conflict Pull request has merge conflict with target branch labels Apr 16, 2025

iklam reviewed Apr 16, 2025

View reviewed changes

calvinccheung suggested changes Apr 17, 2025

View reviewed changes

TimPushkin added 2 commits April 17, 2025 09:10

Extend overshadowing explanation comments

28f1a41

Make RegUnregSuperTest AOTClassLinking-compatible

915987b

iklam reviewed Apr 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive #24223

8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive #24223

TimPushkin commented Mar 25, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 25, 2025

openjdk bot commented Mar 25, 2025

openjdk bot commented Mar 25, 2025

mlbridge bot commented Mar 25, 2025 •

edited

Loading

iklam left a comment

iklam commented Apr 15, 2025

openjdk bot commented Apr 15, 2025

calvinccheung left a comment

openjdk bot commented Apr 16, 2025

TimPushkin commented Apr 16, 2025

iklam Apr 16, 2025

TimPushkin Apr 17, 2025

iklam Apr 17, 2025

calvinccheung left a comment

calvinccheung Apr 17, 2025

TimPushkin Apr 17, 2025

calvinccheung Apr 17, 2025

iklam left a comment

8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive #24223

Are you sure you want to change the base?

8315130: java.lang.IllegalAccessError when processing classlist to create CDS archive #24223

Conversation

TimPushkin commented Mar 25, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewing

bridgekeeper bot commented Mar 25, 2025

openjdk bot commented Mar 25, 2025

openjdk bot commented Mar 25, 2025

mlbridge bot commented Mar 25, 2025 • edited Loading

Webrevs

iklam left a comment

Choose a reason for hiding this comment

iklam commented Apr 15, 2025

openjdk bot commented Apr 15, 2025

calvinccheung left a comment

Choose a reason for hiding this comment

openjdk bot commented Apr 16, 2025

TimPushkin commented Apr 16, 2025

iklam Apr 16, 2025

Choose a reason for hiding this comment

TimPushkin Apr 17, 2025

Choose a reason for hiding this comment

iklam Apr 17, 2025

Choose a reason for hiding this comment

calvinccheung left a comment

Choose a reason for hiding this comment

calvinccheung Apr 17, 2025

Choose a reason for hiding this comment

TimPushkin Apr 17, 2025

Choose a reason for hiding this comment

calvinccheung Apr 17, 2025

Choose a reason for hiding this comment

iklam left a comment

Choose a reason for hiding this comment

TimPushkin commented Mar 25, 2025 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 25, 2025 •

edited

Loading