8352001: AOT cache should not contain classes injected into built-in class loaders #24046

iklam · 2025-03-14T05:28:47Z

During an application's training run, it's possible to inject classes into the built-in platform/app class loaders with reflection calls.

Before JDK-8348426, only the names of these classes were recorded in the AOT config file. When the AOT cache is generated, these classes are automatically filtered out.
Since JDK-8348426, these classes are stored as parsed InstanceKlasses in the AOT config file, and will be transferred into the AOT cache. This new behavior may cause some applications to fail, as they may inject bytecodes that have environment dependencies.

For safety, this PR filters out such injected classes from the AOT config file.

Progress

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

Issue

JDK-8352001: AOT cache should not contain classes injected into built-in class loaders (Bug - P3)

Reviewers

Matias Saavedra Silva (@matias9927 - Reviewer) Review applies to 23327c27
Calvin Cheung (@calvinccheung - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24046

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…class loaders

bridgekeeper · 2025-03-14T05:29:31Z

👋 Welcome back iklam! 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-14T05:30:04Z

@iklam This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8352001: AOT cache should not contain classes injected into built-in class loaders

Reviewed-by: ccheung, matsaave

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 22 new commits pushed to the master branch:

7a107e2: 8351603: Change to GCC 14.2.0 for building on Linux at Oracle
84458ec: 8353013: java.net.URI.create(String) may have low performance to scan the host/domain name from URI string when the hostname starts with number
c3de94c: 8352942: jdk/jfr/startupargs/TestMemoryOptions.java fails with 32-bit build
... and 19 more: https://git.openjdk.org/jdk/compare/a2dc9c71e47a1cdf70ab351c557a5f1835eb5f4a...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-03-14T05:30:42Z

@iklam 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 · 2025-03-14T05:33:04Z

Webrevs

bridgekeeper · 2025-04-11T10:20:54Z

@iklam This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

…tin-loaders

matias9927

Looks good! Just one nit

matias9927 · 2025-04-15T19:35:09Z

src/hotspot/share/cds/aotClassLocation.cpp

+      const char* const file_name = ClassLoader::file_name_for_class_name(class_name,
+                                                                          ik->name()->utf8_length());
+      if (!zip->has_entry(current, file_name)) {
+        log_warning(cds)("class %s cannot be archived because it was not define from %s as claimed",


Typo: define -> defined

matias9927 · 2025-04-15T19:49:55Z

test/hotspot/jtreg/runtime/cds/appcds/aotClassLinking/FakeCodeLocation.java

+
+    // Define a class using Lookup.defineClass(). The ClassFileParser should see "__JVM_DefineClass__"
+    // as the source location, so this class will be excluded, as the location is not supported.
+    static void testWithLookup() throws Exception {


Should this test capture some kind of output or exception?

These classes are still loadable (just not from the CDS archive), so there aren't exceptions printed here. These lines check that the classes are not loaded from CDS:

public void checkExecution(OutputAnalyzer out, RunMode runMode) throws Exception { if (isDumping(runMode)) { out.shouldMatch("cds,class.* FakeCodeLocationApp"); out.shouldNotMatch("cds,class.* ClassNotInJar1"); out.shouldNotMatch("cds,class.* ClassNotInJar2"); } if (runMode.isProductionRun()) { out.shouldMatch("class,load.* FakeCodeLocationApp .*source: shared objects file"); out.shouldNotMatch("class,load.* ClassNotInJar1 .*source: shared objects file"); out.shouldNotMatch("class,load.* ClassNotInJar2 .*source: shared objects file"); } }

calvinccheung

Looks good overall. I have a question in classLoaderExt.cpp.

calvinccheung · 2025-04-15T21:54:56Z

src/hotspot/share/classfile/classLoaderExt.cpp

@@ -100,4 +100,8 @@ void ClassLoaderExt::record_result(const s2 classpath_index, InstanceKlass* resu
    CDSConfig::disable_heap_dumping();
  }
 #endif // INCLUDE_CDS_JAVA_HEAP
+
+  if (CDSConfig::is_dumping_preimage_static_archive() || CDSConfig::is_dumping_dynamic_archive()) {
+    AOTClassLocationConfig::dumptime()->check_invalid_classpath_index(classpath_index, result);


In case the classpath_index is invalid, I don't think we should call AOTClassLocationConfig::dumptime_update_max_used_index(). Maybe the check_invalid_classpath_index() function should return a bool and have ClassLoaderExt::record_result() update the classpath index and max used index.

…tin-loaders

calvinccheung

Thanks for the update. Just one minor comment.

calvinccheung · 2025-04-16T16:44:05Z

src/hotspot/share/cds/aotArtifactFinder.cpp

@@ -23,6 +23,7 @@
 */

 #include "cds/aotClassLinker.hpp"
+#include "cds/aotClassLocation.hpp"


Is this include needed?

I added it by mistake. I've removed it. Could you re-approve?

iklam · 2025-04-16T17:51:47Z

Thanks @calvinccheung @calvinccheung for the review
/integrate

openjdk · 2025-04-16T17:52:56Z

Going to push as commit e433fa2.
Since your change was applied there have been 23 commits pushed to the master branch:

d80db00: 8354873: javax/swing/plaf/metal/MetalIconFactory/bug4952462.java failing on CI
7a107e2: 8351603: Change to GCC 14.2.0 for building on Linux at Oracle
84458ec: 8353013: java.net.URI.create(String) may have low performance to scan the host/domain name from URI string when the hostname starts with number
... and 20 more: https://git.openjdk.org/jdk/compare/a2dc9c71e47a1cdf70ab351c557a5f1835eb5f4a...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-04-16T17:53:11Z

@iklam Pushed as commit e433fa2.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8352001: AOT cache should not contain classes injected into built-in …

b5c177e

…class loaders

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

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

Merge branch 'master' into 8352001-exclude-injected-classes-from-buil…

23327c2

…tin-loaders

matias9927 approved these changes Apr 15, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 15, 2025

calvinccheung reviewed Apr 15, 2025

View reviewed changes

iklam added 3 commits April 15, 2025 17:06

@matias9927 comment

ba4d6f5

@calvinccheung review

fc27371

Merge branch 'master' into 8352001-exclude-injected-classes-from-buil…

b5d20c5

…tin-loaders

openjdk bot removed the ready Pull request is ready to be integrated label Apr 16, 2025

calvinccheung approved these changes Apr 16, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 16, 2025

Removed unnecessary include

52f5385

openjdk bot removed the ready Pull request is ready to be integrated label Apr 16, 2025

calvinccheung approved these changes Apr 16, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 16, 2025

openjdk bot added the integrated Pull request has been integrated label Apr 16, 2025

openjdk bot closed this Apr 16, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 16, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8352001: AOT cache should not contain classes injected into built-in class loaders #24046

8352001: AOT cache should not contain classes injected into built-in class loaders #24046

iklam commented Mar 14, 2025 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 14, 2025

openjdk bot commented Mar 14, 2025 •

edited

Loading

openjdk bot commented Mar 14, 2025

mlbridge bot commented Mar 14, 2025 •

edited

Loading

bridgekeeper bot commented Apr 11, 2025

matias9927 left a comment

matias9927 Apr 15, 2025

matias9927 Apr 15, 2025

iklam Apr 16, 2025

calvinccheung left a comment

calvinccheung Apr 15, 2025

iklam Apr 16, 2025

calvinccheung left a comment

calvinccheung Apr 16, 2025

iklam Apr 16, 2025

iklam commented Apr 16, 2025

openjdk bot commented Apr 16, 2025

openjdk bot commented Apr 16, 2025

8352001: AOT cache should not contain classes injected into built-in class loaders #24046

8352001: AOT cache should not contain classes injected into built-in class loaders #24046

Conversation

iklam commented Mar 14, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Mar 14, 2025

openjdk bot commented Mar 14, 2025 • edited Loading

openjdk bot commented Mar 14, 2025

mlbridge bot commented Mar 14, 2025 • edited Loading

Webrevs

bridgekeeper bot commented Apr 11, 2025

matias9927 left a comment

Choose a reason for hiding this comment

matias9927 Apr 15, 2025

Choose a reason for hiding this comment

matias9927 Apr 15, 2025

Choose a reason for hiding this comment

iklam Apr 16, 2025

Choose a reason for hiding this comment

calvinccheung left a comment

Choose a reason for hiding this comment

calvinccheung Apr 15, 2025

Choose a reason for hiding this comment

iklam Apr 16, 2025

Choose a reason for hiding this comment

calvinccheung left a comment

Choose a reason for hiding this comment

calvinccheung Apr 16, 2025

Choose a reason for hiding this comment

iklam Apr 16, 2025

Choose a reason for hiding this comment

iklam commented Apr 16, 2025

openjdk bot commented Apr 16, 2025

openjdk bot commented Apr 16, 2025

iklam commented Mar 14, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Mar 14, 2025 •

edited

Loading

mlbridge bot commented Mar 14, 2025 •

edited

Loading