8327624: Remove VM implementation that bypass verification for core reflection #21571

mlchung · 2024-10-17T20:45:53Z

The old core reflection implementation generates dynamic classes that are special cases in the VM to bypass bytecode verification to workaround various issues [1] [2] [3].

The old core reflection implementation was removed in JDK 22. It's time to remove these VM hacks along with the old implementation of sun.reflect.ReflectionFactory::newConstructorForSerialization.

After this change, jdk.internal.reflect.DelegatingClassLoader no longer exists. Hence the special metaspace for reflection is no longer needed. GTests will need to be updated when Metaspace::ReflectionMetaspaceType is removed. Such clean up can be done separately (JDK-8342561).

[1] JDK-4486457
[2] JDK-4474172
[3] JDK-6790209

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8342574 to be approved
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issues

JDK-8327624: Remove VM implementation that bypass verification for core reflection (Enhancement - P4)
JDK-8342574: Remove VM implementation that bypass verification for core reflection (CSR)

Reviewers

Chen Liang (@liach - Reviewer)
David Holmes (@dholmes-ora - Reviewer)
John R Rose (@rose00 - Reviewer)
Alan Bateman (@AlanBateman - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21571

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

Using diff file

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

Webrev

Link to Webrev Comment

…eflection

bridgekeeper · 2024-10-17T20:46:29Z

👋 Welcome back mchung! 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 · 2024-10-17T20:46:33Z

@mlchung This change is no longer ready for integration - check the PR body for details.

openjdk · 2024-10-17T20:47:15Z

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

core-libs
hotspot

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 · 2024-10-17T20:49:46Z

Webrevs

01: Full - Incremental (245cfb6f)
00: Full (4753637c)

liach · 2024-10-17T22:03:43Z

It seems we can now remove AccessorGenerator, ByteVector*, ClassFile*, Label, and MagicAccessorImpl Java files from jdk.internal.reflect. Do we plan to do that in another RFE?

mlchung · 2024-10-17T22:39:18Z

Good catch! Patch updated.

liach

/reviewers 2 reviewer

The jdk.internal.reflect changes look good; need other reviewers to review hotspot changes.

openjdk · 2024-10-17T23:54:07Z

@liach
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 2 Reviewers).

dholmes-ora

Nice cleanup! Great to see all this old code go. Just one mistake ...

Thanks

dholmes-ora · 2024-10-18T02:25:16Z

src/hotspot/share/classfile/classFileParser.cpp

-    // If the loader is not the boot loader then throw an exception if its
-    // superclass is in package jdk.internal.reflect and its loader is not a
-    // special reflection class loader
-    if (!this_klass->class_loader_data()->is_the_null_class_loader_data()) {
-      PackageEntry* super_package = super->package();
-      if (super_package != nullptr &&
-          super_package->name()->fast_compare(vmSymbols::jdk_internal_reflect()) == 0 &&
-          !java_lang_ClassLoader::is_reflection_class_loader(this_klass->class_loader())) {
-        ResourceMark rm(THREAD);
-        Exceptions::fthrow(
-          THREAD_AND_LOCATION,
-          vmSymbols::java_lang_IllegalAccessError(),
-          "class %s loaded by %s cannot access jdk/internal/reflect superclass %s",
-          this_klass->external_name(),
-          this_klass->class_loader_data()->loader_name_and_id(),
-          super->external_name());
-        return;
-      }
-    }
-


This code should not be removed. The spec for this code should now be:

// If the loader is not the boot loader then throw an exception if its // superclass is in package jdk.internal.reflect

All we need do is remove the check:

&& !java_lang_ClassLoader::is_reflection_class_loader

Oh that's a good find. Maybe we should write a test for this, if as I assume there isn't one already.
Edit: not with this RFE, just in general.

// If the loader is not the boot loader then throw an exception if its
// superclass is in package jdk.internal.reflect

This should not be needed. jdk.internal.reflect is a non-exported package in java.base module. If another module M defines a class whose superclass is in jdk.internal.reflect package, java.base must export jdk.internal.reflect package to M for access. Otherwise, it will fail the super access check, as done in the check below this deleted code.

Reflection::VerifyClassAccessResults vca_result = Reflection::verify_class_access(this_klass, InstanceKlass::cast(super), false);

test/hotspot/jtreg/runtime/AccessCheckSuper.java is one test that fails the super class access check. @lfoltan may know whether there are more tests besides this one.

This code was necessary back when this was the sun.reflect package and it was not able to be encapsulated (before modules) to prevent untrusted class loaders from leaking the MagicAccessorImpl class hierarchy.

Okay ... so such subclass was and remains illegal and is now detected slightly later. That suggests to me this special case was in the wrong place and should have been inside Reflection::verify_class_access.

Anyway thanks for clarifying full removal is in fact what we want.

rose00

This is a most impressive example of C.D.E. (code deletion engineering).

I remember when this was written, as an accelerator for the JNI methods, long long before we had method handles to do the same thing more flexibly. I think it was Ken Russell that did the work. And of course I remember Mandy's newer work (3 years ago) fitting method handles into reflection (a move I applauded of course).

As it happens I was just reviewing the reflection implementation this week, to understand its interaction with upcoming Leyden changes to bootstrap sequences. Of course I wondered, "when will we ever retire this older implementation?" Happily, that day has come.

Thanks Mandy!

AlanBateman

Good cleanup.

I searched the tests as I expected to find some tests exercising -Djdk.reflect.useOldSerializableConstructor but nothing found.

mlchung · 2024-10-18T16:58:41Z

Happily, that day has come.

Yes, feeling happy! I have been waiting to get rid of this last piece!

ExE-Boss

Does this mean that JDK‑4486457 will finally be able to be made public?

ExE-Boss · 2024-10-19T06:33:01Z

src/hotspot/share/classfile/classFileParser.cpp

-    // If the loader is not the boot loader then throw an exception if its
-    // superclass is in package jdk.internal.reflect and its loader is not a
-    // special reflection class loader
-    if (!this_klass->class_loader_data()->is_the_null_class_loader_data()) {
-      PackageEntry* super_package = super->package();
-      if (super_package != nullptr &&
-          super_package->name()->fast_compare(vmSymbols::jdk_internal_reflect()) == 0 &&
-          !java_lang_ClassLoader::is_reflection_class_loader(this_klass->class_loader())) {
-        ResourceMark rm(THREAD);
-        Exceptions::fthrow(
-          THREAD_AND_LOCATION,
-          vmSymbols::java_lang_IllegalAccessError(),
-          "class %s loaded by %s cannot access jdk/internal/reflect superclass %s",
-          this_klass->external_name(),
-          this_klass->class_loader_data()->loader_name_and_id(),
-          super->external_name());
-        return;
-      }
-    }
-


This code was necessary back when this was the sun.reflect package and it was not able to be encapsulated (before modules) to prevent untrusted class loaders from leaking the MagicAccessorImpl class hierarchy.

8327624: Remove VM implementation that bypass verification for core r…

4753637

…eflection

openjdk bot added the rfr Pull request is ready for review label Oct 17, 2024

openjdk bot added hotspot hotspot-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Oct 17, 2024

Remove unused classes

245cfb6

liach approved these changes Oct 17, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 17, 2024

openjdk bot added csr Pull request needs approved CSR before integration and removed ready Pull request is ready to be integrated labels Oct 17, 2024

dholmes-ora suggested changes Oct 18, 2024

View reviewed changes

rose00 approved these changes Oct 18, 2024

View reviewed changes

AlanBateman approved these changes Oct 18, 2024

View reviewed changes

ExE-Boss reviewed Oct 19, 2024

View reviewed changes

graalvmbot mentioned this pull request Oct 19, 2024

[JDK-8342587] Adapt JDK-8327624: Remove VM implementation that bypass verification for core reflection oracle/graal#9927

Open

dholmes-ora approved these changes Oct 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8327624: Remove VM implementation that bypass verification for core reflection #21571

8327624: Remove VM implementation that bypass verification for core reflection #21571

mlchung commented Oct 17, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 17, 2024

openjdk bot commented Oct 17, 2024 •

edited

Loading

openjdk bot commented Oct 17, 2024

mlbridge bot commented Oct 17, 2024 •

edited

Loading

liach commented Oct 17, 2024

mlchung commented Oct 17, 2024

liach left a comment

openjdk bot commented Oct 17, 2024

dholmes-ora left a comment

dholmes-ora Oct 18, 2024

coleenp Oct 18, 2024 •

edited

Loading

mlchung Oct 18, 2024 •

edited

Loading

mlchung Oct 18, 2024

ExE-Boss Oct 19, 2024

dholmes-ora Oct 21, 2024

rose00 left a comment

AlanBateman left a comment

mlchung commented Oct 18, 2024

ExE-Boss left a comment

ExE-Boss Oct 19, 2024

8327624: Remove VM implementation that bypass verification for core reflection #21571

Are you sure you want to change the base?

8327624: Remove VM implementation that bypass verification for core reflection #21571

Conversation

mlchung commented Oct 17, 2024 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Oct 17, 2024

openjdk bot commented Oct 17, 2024 • edited Loading

openjdk bot commented Oct 17, 2024

mlbridge bot commented Oct 17, 2024 • edited Loading

Webrevs

liach commented Oct 17, 2024

mlchung commented Oct 17, 2024

liach left a comment

Choose a reason for hiding this comment

openjdk bot commented Oct 17, 2024

dholmes-ora left a comment

Choose a reason for hiding this comment

dholmes-ora Oct 18, 2024

Choose a reason for hiding this comment

coleenp Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

mlchung Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

mlchung Oct 18, 2024

Choose a reason for hiding this comment

ExE-Boss Oct 19, 2024

Choose a reason for hiding this comment

dholmes-ora Oct 21, 2024

Choose a reason for hiding this comment

rose00 left a comment

Choose a reason for hiding this comment

AlanBateman left a comment

Choose a reason for hiding this comment

mlchung commented Oct 18, 2024

ExE-Boss left a comment

Choose a reason for hiding this comment

ExE-Boss Oct 19, 2024

Choose a reason for hiding this comment

mlchung commented Oct 17, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 17, 2024 •

edited

Loading

mlbridge bot commented Oct 17, 2024 •

edited

Loading

coleenp Oct 18, 2024 •

edited

Loading

mlchung Oct 18, 2024 •

edited

Loading