8331671: Implement JEP 472: Prepare to Restrict the Use of JNI #19213

mcimadamore · 2024-05-13T10:42:26Z

This PR implements JEP 472, by restricting the use of JNI in the following ways:

System::load and System::loadLibrary are now restricted methods
Runtime::load and Runtime::loadLibrary are now restricted methods
binding a JNI native method declaration to a native implementation is now considered a restricted operation

This PR slightly changes the way in which the JDK deals with restricted methods, even for FFM API calls. In Java 22, the single --enable-native-access was used both to specify a set of modules for which native access should be allowed and to specify whether illegal native access (that is, native access occurring from a module not specified by --enable-native-access) should be treated as an error or a warning. More specifically, an error is only issued if the --enable-native-access flag is used at least once.

Here, a new flag is introduced, namely illegal-native-access=allow/warn/deny, which is used to specify what should happen when access to a restricted method and/or functionality is found outside the set of modules specified with --enable-native-access. The default policy is warn, but users can select allow to suppress the warnings, or deny to cause IllegalCallerException to be thrown. This aligns the treatment of restricted methods with other mechanisms, such as --illegal-access and the more recent --sun-misc-unsafe-memory-access.

Some changes were required in the package-info javadoc for java.lang.foreign, to reflect the changes in the command line flags described above.

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
Change requires CSR request JDK-8331672 to be approved

Issues

JDK-8331671: Implement JEP 472: Prepare to Restrict the Use of JNI (Enhancement - P4)
JDK-8331672: Implement JEP 472: Prepare to Restrict the Use of JNI (CSR)

Reviewers

Alan Bateman (@AlanBateman - Reviewer) Review applies to 3a0db276
Jaikiran Pai (@jaikiran - Reviewer)
Phil Race (@prrace - Reviewer)
Magnus Ihse Bursie (@magicus - Reviewer)
Kevin Rushforth (@kevinrushforth - Author)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 19213

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-05-13T10:43:12Z

👋 Welcome back mcimadamore! 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-05-13T10:44:05Z

@mcimadamore 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:

8331671: Implement JEP 472: Prepare to Restrict the Use of JNI

Reviewed-by: jpai, prr, ihse, kcr, alanb

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 16 new commits pushed to the master branch:

a5e2800: 8338834: Remove unused import declarations in java.compiler
9cbf685: 8337658: ZGC: Move soft reference handling out of the driver loop function
8e0d019: 8338630: Test java/nio/channels/DatagramChannel/SendReceiveMaxSize.java timeout
e06652a: 8338810: PPC, s390x: LightweightSynchronizer::exit asserts, missing lock
ea33709: 8328880: Events::log_exception should limit the size of the logging message
c89a1c3: 8338696: (fs) BasicFileAttributes.creationTime() falls back to epoch if birth time is unavailable (Linux)
813546f: 8338856: [BACKOUT] JDK-8337828: CDS: Trim down minimum GC region alignment
0b5c887: 8338380: Update TLSCommon/interop/AbstractServer to specify an interface to listen for connections
6041c93: 8335664: Parsing jsr broken: assert(bci>= 0 && bci < c->method()->code_size()) failed: index out of bounds
6cf7f9c: 8338662: Shenandoah: Remove excessive ShenandoahVerifier::verify_during_evacuation
... and 6 more: https://git.openjdk.org/jdk/compare/0e8fe3550b628c6617ac7593d7e17ef7d9bc0869...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 · 2024-05-13T10:44:38Z

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

build
client
core-libs
hotspot
i18n
jmx
net
security
serviceability

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.

mcimadamore · 2024-05-13T10:49:30Z

make/conf/module-loader-map.conf

    jdk.accessibility \
-    jdk.charsets \
+    jdk.attach \


The list of allowed modules has been rewritten from scratch, by looking at the set of modules containing at least one native method declaration.

Should I understand this list to be the set of modules exempt from needing to specific that native access is allowed ?
ie they always have native access without any warnings, and further that any attempt to enable warnings, or to disable native access for these modules is ignored ?

Yes, this was added via JDK-8327218. The changes in this PR are just trimming down the list to only the modules that have native code.

mcimadamore · 2024-05-13T10:50:42Z

src/hotspot/share/prims/nativeLookup.cpp

  Klass*   klass = vmClasses::ClassLoader_klass();
-  Handle name_arg = java_lang_String::create_from_str(jni_name, CHECK_NULL);
+  Handle jni_class(THREAD, method->method_holder()->java_mirror());


This is the biggest change in this PR. That is, we need to pass enough arguments to ClassLoader::findNative so that the method can start a restricted check accordingly.

mcimadamore · 2024-05-13T10:53:50Z

src/java.base/share/classes/java/lang/Module.java

-        if (!EnableNativeAccess.isNativeAccessEnabled(target)) {
-            if (ModuleBootstrap.hasEnableNativeAccessFlag()) {
+        ModuleBootstrap.IllegalNativeAccess illegalNativeAccess = ModuleBootstrap.illegalNativeAccess();
+        if (illegalNativeAccess != ModuleBootstrap.IllegalNativeAccess.ALLOW &&


There are some changes in this code:

this code is no-op if --illegal-native-access is set to allow

we also attach the location of the problematic class to the warning message, using CodeSource

we use the "initial error stream" to emit the warning, similarly to what is done for other runtime warnings

mcimadamore · 2024-05-13T10:55:09Z

src/java.base/share/classes/jdk/internal/reflect/Reflection.java

-                ClassLoader.getSystemClassLoader().getUnnamedModule();
-        class Holder {
-            static final JavaLangAccess JLA = SharedSecrets.getJavaLangAccess();
+        if (VM.isModuleSystemInited()) {


If we call this code too early, we can see cases where module is null.

mcimadamore · 2024-05-13T10:56:16Z

src/java.desktop/macosx/classes/com/apple/eio/FileManager.java

@@ -58,7 +58,7 @@ public class FileManager {
        loadOSXLibrary();
    }

-    @SuppressWarnings("removal")
+    @SuppressWarnings({"removal", "restricted"})


There are several of these changes. One option might have been to just disable restricted warnings when building. But on a deeper look, I realized that in all these places we already disabled deprecation warnings for the use of security manager, so I also added a new suppression instead.

Sounds reasonable.

mcimadamore · 2024-05-13T10:57:58Z

test/jdk/java/foreign/enablenativeaccess/panama_jni_load_module/module-info.java

+ * questions.
+ */
+
+module panama_jni_load_module {


This module setup is a bit convoluted, but I wanted to make sure that we got separate warnings for System.loadLibrary and binding of the native method, and that warning on the use of the native method was not generated (typically, all three operations occur in the same module).

mcimadamore · 2024-05-13T10:59:34Z

Javadoc: https://cr.openjdk.org/~mcimadamore/jdk/8331671/v1/javadoc/api/index.html
Specdiff: https://cr.openjdk.org/~mcimadamore/jdk/8331671/v1/specdiff_out/overview-summary.html

mlbridge · 2024-05-13T11:08:49Z

Webrevs

Use initial error stream

mcimadamore · 2024-05-13T11:38:40Z

src/java.base/share/classes/jdk/internal/reflect/Reflection.java

-        Holder.JLA.ensureNativeAccess(module, owner, methodName, currentClass);
+        if (module != null) {
+            // not in init phase
+            Holder.JLA.ensureNativeAccess(module, owner, methodName, currentClass);


In an earlier iteration I had a call to VM::isModuleSystemInited, but I discovered that caused a performance regression, since that method involves a volatile access. Perhaps we should rethink that part of the init code to use stable fields, but it's probably better done separately.

erikj79 · 2024-05-13T13:20:13Z

Build changes look good.

src/java.base/share/classes/java/lang/Runtime.java

src/java.base/share/classes/java/lang/Module.java

AlanBateman

This looks good. Just a few minor comments where future maintainers might appreciate comments that describe parameters.

prrace

Have you looked into / thought about how this will work for jpackaged apps ?
I suspect that both the existing FFM usage and this will be options the application packager will need to supply when building the jpackaged app - the end user cannot pass in command line VM options.
Seems there should be some testing of this as some kind of native access could be a common case for jpackaged apps.

prrace · 2024-05-20T18:39:31Z

make/conf/module-loader-map.conf

    jdk.accessibility \
-    jdk.charsets \
+    jdk.attach \


Should I understand this list to be the set of modules exempt from needing to specific that native access is allowed ?
ie they always have native access without any warnings, and further that any attempt to enable warnings, or to disable native access for these modules is ignored ?

prrace · 2024-05-20T18:40:09Z

src/java.desktop/macosx/classes/com/apple/eio/FileManager.java

@@ -58,7 +58,7 @@ public class FileManager {
        loadOSXLibrary();
    }

-    @SuppressWarnings("removal")
+    @SuppressWarnings({"removal", "restricted"})


Sounds reasonable.

AlanBateman · 2024-05-21T07:20:05Z

Have you looked into / thought about how this will work for jpackaged apps ? I suspect that both the existing FFM usage and this will be options the application packager will need to supply when building the jpackaged app - the end user cannot pass in command line VM options. Seems there should be some testing of this as some kind of native access could be a common case for jpackaged apps.

I don't see any tests in test/jdk/tools/jpackage that creates an application that uses JNI code. Seems like a good idea to add this via another PR and it specify --java-options so that the application launcher enables native access. It could test jpackage using jlink too.

mcimadamore · 2024-05-21T08:44:47Z

Have you looked into / thought about how this will work for jpackaged apps ? I suspect that both the existing FFM usage and this will be options the application packager will need to supply when building the jpackaged app - the end user cannot pass in command line VM options. Seems there should be some testing of this as some kind of native access could be a common case for jpackaged apps.

I don't see any tests in test/jdk/tools/jpackage that creates an application that uses JNI code. Seems like a good idea to add this via another PR and it specify --java-options so that the application launcher enables native access. It could test jpackage using jlink too.

These are all good suggestions. I have not looked into jpackage, but yes, I would expect that the jpackage user would need to provide extra options when packaging the application. The same is true for creating JDK image jlink (which we use in the jextract build) - although, in that case the end user also has the possibility to pass options on the command line.

alexeysemenyukoracle · 2024-05-21T15:54:19Z

jdk.jpackage changes look good

prrace

client parts look fine.

alexeysemenyukoracle · 2024-05-21T16:56:33Z

These are all good suggestions. I have not looked into jpackage, but yes, I would expect that the jpackage user would need to provide extra options when packaging the application.

It would be good to document how jpackage users packaging apps with native access will be affected by this change. Primarily that they need to pass --illegal-native-access parameter to affected jpackage app launchers.

magicus

Build changes look good. Thanks for trimming down NATIVE_ACCESS_MODULES.

kevinrushforth

I tested this with JavaFX and everything is working as I would expect. Without any options, I get the expected warnings, one time per modules for the three javafx.* modules that use JNI. If I pass the --enable-native-access options at runtime, listing those three modules, there is no warning. Further, I confirm that if I pass that option to jlink or jpackage when creating a custom runtime, there is no warning.

AlanBateman · 2024-05-23T06:20:51Z

Further, I confirm that if I pass that option to jlink or jpackage when creating a custom runtime, there is no warning.

Great! What about jpackage without a custom runtime, wondering if --java-options can be tested.

kevinrushforth · 2024-05-23T13:57:31Z

Further, I confirm that if I pass that option to jlink or jpackage when creating a custom runtime, there is no warning.

Great! What about jpackage without a custom runtime, wondering if --java-options can be tested.

Yes, pointing to an existing runtime works, too. In either mode (jpackage using an existing Java runtime vs running jlink to create a new one), the options specified by jpackage --java-options are written to the application's .cfg file and used when the application launcher is run.

bridgekeeper · 2024-06-20T17:27:12Z

@mcimadamore 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!

mcimadamore · 2024-07-15T13:19:38Z

keep alive

bridgekeeper · 2024-08-12T19:14:04Z

@mcimadamore 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!

mcimadamore · 2024-08-26T09:17:12Z

/integrate

openjdk · 2024-08-26T09:17:46Z

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

ce83f6a: 8338844: C2: remove useless code in PhaseIdealLoop::place_outside_loop() after 8335709
0c14579: 8336830: C2: assert(get_loop(lca)->_nest < n_loop->_nest || lca->in(0)->is_NeverBranch()) failed: must not be moved into inner loop
5671f83: 8338785: The java.awt.datatransfer.SystemFlavorMap#FLAVOR_MAP_KEY field is not used
32b3d70: 8338925: ProblemList runtime/interpreter/LastJsrTest.java on linux-all
5d12ac3: 8337715: Update --release 23 symbol information for JDK 23 build 37
23dc3b0: 8324048: (fc) Make FileKey fields final
a461369: 8338700: AttributeMapper type parameter should be bounded by Attribute
916f1aa: 8329756: [macos] "javax/swing/JTable/KeyBoardNavigation.java" fail because most combinations of navigational keys with the Ctrl key do not work
21d1e4d: 8338819: JFR: Internal events causes crash when no other events are in use
965dd1a: 8333334: C2: Make result of Node::dominates more precise to enhance scalar replacement
... and 18 more: https://git.openjdk.org/jdk/compare/0e8fe3550b628c6617ac7593d7e17ef7d9bc0869...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-08-26T09:17:52Z

@mcimadamore Pushed as commit 20d8f58.

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

Initial push

d9fe9a7

openjdk bot added the csr Pull request needs approved CSR before integration label May 13, 2024

mcimadamore commented May 13, 2024

View reviewed changes

mcimadamore marked this pull request as ready for review May 13, 2024 11:04

openjdk bot added the rfr Pull request is ready for review label May 13, 2024

Avoid call to VM::isModuleSystemInited

c4938dc

Use initial error stream

mcimadamore commented May 13, 2024

View reviewed changes

mcimadamore added 3 commits May 13, 2024 12:42

Add more comments

8f02a6b

Fix typo

45c8873

Fix another typo

bad1094

AlanBateman reviewed May 17, 2024

View reviewed changes

src/java.base/share/classes/java/lang/Runtime.java Show resolved Hide resolved

AlanBateman reviewed May 17, 2024

View reviewed changes

src/java.base/share/classes/java/lang/Module.java Outdated Show resolved Hide resolved

AlanBateman approved these changes May 17, 2024

View reviewed changes

Address review comments

789bdf4

jaikiran approved these changes May 18, 2024

View reviewed changes

prrace reviewed May 20, 2024

View reviewed changes

prrace approved these changes May 21, 2024

View reviewed changes

magicus approved these changes May 22, 2024

View reviewed changes

kevinrushforth approved these changes May 22, 2024

View reviewed changes

Merge branch 'master' into restricted_jni

ff51ac6

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

Merge branch 'master' into restricted_jni

0462274

openjdk bot added the integrated Pull request has been integrated label Aug 26, 2024

openjdk bot closed this Aug 26, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Aug 26, 2024

graalvmbot mentioned this pull request Sep 5, 2024

[GR-57740] Update labsjdk to 24+13-jvmci-b01 oracle/graal#9621

Closed

8331671: Implement JEP 472: Prepare to Restrict the Use of JNI #19213

8331671: Implement JEP 472: Prepare to Restrict the Use of JNI #19213

Uh oh!

Conversation

mcimadamore commented May 13, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented May 13, 2024

Uh oh!

openjdk bot commented May 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented May 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcimadamore commented May 13, 2024

Uh oh!

mlbridge bot commented May 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikj79 commented May 13, 2024

Uh oh!

Uh oh!

Uh oh!

AlanBateman left a comment

Choose a reason for hiding this comment

Uh oh!

prrace left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlanBateman commented May 21, 2024

Uh oh!

mcimadamore commented May 21, 2024

Uh oh!

alexeysemenyukoracle commented May 21, 2024

Uh oh!

prrace left a comment

Choose a reason for hiding this comment

Uh oh!

alexeysemenyukoracle commented May 21, 2024

Uh oh!

magicus left a comment

Choose a reason for hiding this comment

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

AlanBateman commented May 23, 2024

Uh oh!

kevinrushforth commented May 23, 2024

Uh oh!

mcimadamore commented May 13, 2024 •

edited by openjdk bot

Loading

openjdk bot commented May 13, 2024 •

edited

Loading

mlbridge bot commented May 13, 2024 •

edited

Loading