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

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

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.

Improve warning for JNI methods, similar to what's described in JEP 472 Beef up tests

dholmes-ora

Hotspot changes look good - notwithstanding discussion about properlty namespace placement. Manpage changes also look good.

src/hotspot/share/runtime/arguments.cpp

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

src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java

src/java.base/share/classes/sun/launcher/resources/launcher.properties

jaikiran · 2024-05-16T11:42:48Z

Hello Maurizio, in the current mainline, we have code in LauncherHelper https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/launcher/LauncherHelper.java#L636 where we enable native access to all unnamed modules if an executable jar with Enable-Native-Access: ALL-UNNAMED manifest is being launched. For such executable jars, what is the expected semantics when the launch also explicitly has a --enable-native-access=M1,M2 option. Something like:

java --enable-native-access=M1,M2 -jar foo.jar

where foo.jar has Enable-Native-Access: ALL-UNNAMED in its manifest.

mcimadamore · 2024-05-16T11:46:05Z

Hello Maurizio, in the current mainline, we have code in LauncherHelper https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/sun/launcher/LauncherHelper.java#L636 where we enable native access to all unnamed modules if an executable jar with Enable-Native-Access: ALL-UNNAMED manifest is being launched. For such executable jars, what is the expected semantics when the launch also explicitly has a --enable-native-access=M1,M2 option. Something like:
java --enable-native-access=M1,M2 -jar foo.jar
where foo.jar has Enable-Native-Access: ALL-UNNAMED in its manifest.

The options are additive - e.g. the enable-native-access in the manifest will add up to the enable-native-access in the command line, so effectively it will be as if you were running with --enable-native-access=M1,M2,ALL-UNNAMED

src/java.base/share/man/java.1

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

src/java.base/share/classes/java/lang/foreign/package-info.java

src/java.base/share/classes/jdk/internal/access/JavaLangAccess.java

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

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.

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

Address review comments

0d21bf9

Improve warning for JNI methods, similar to what's described in JEP 472 Beef up tests

dholmes-ora reviewed May 15, 2024

View reviewed changes

src/hotspot/share/runtime/arguments.cpp Show resolved Hide resolved

ExE-Boss reviewed May 15, 2024

View reviewed changes

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

Refine warning text for JNI method binding

daf729f

AlanBateman reviewed May 15, 2024

View reviewed changes

src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java Outdated Show resolved Hide resolved

Address review comment

1c45e5d

jaikiran reviewed May 16, 2024

View reviewed changes

src/java.base/share/classes/sun/launcher/resources/launcher.properties Show resolved Hide resolved

Add note on --illegal-native-access default value in the launcher help

3a0db27

AlanBateman reviewed May 16, 2024

View reviewed changes

src/java.base/share/man/java.1 Outdated Show resolved Hide resolved

AlanBateman reviewed May 16, 2024

View reviewed changes

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