jdk icon indicating copy to clipboard operation
jdk copied to clipboard

Published 20 hours ago • verified publisher icon openjdk

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

Open mcimadamore opened this issue 1 year ago • 10 comments
trafficstars

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

  • [x] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • [x] Change must not contain extraneous whitespace
  • [x] 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

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

mcimadamore avatar May 13 '24 10:05 mcimadamore

:wave: 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.

bridgekeeper[bot] avatar May 13 '24 10:05 bridgekeeper[bot]

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

  • a5e28005fa95426f811e1ed98a7d726cbdbe196d: 8338834: Remove unused import declarations in java.compiler
  • 9cbf685b0b1ade5e6ddebfeec225b2efb5cf4cfc: 8337658: ZGC: Move soft reference handling out of the driver loop function
  • 8e0d0190ed19bc1a9d4ec0c6ee3aa6454542989f: 8338630: Test java/nio/channels/DatagramChannel/SendReceiveMaxSize.java timeout
  • e06652ad3c02dfe54104eaa04eaa3d117699b27f: 8338810: PPC, s390x: LightweightSynchronizer::exit asserts, missing lock
  • ea3370982bfd3da4b200b738dd3b8c16cebb3a34: 8328880: Events::log_exception should limit the size of the logging message
  • c89a1c35bda9002ee687b3fa267f3ef9cba78b00: 8338696: (fs) BasicFileAttributes.creationTime() falls back to epoch if birth time is unavailable (Linux)
  • 813546f9236d278c380888f1f90cd49b23792d92: 8338856: [BACKOUT] JDK-8337828: CDS: Trim down minimum GC region alignment
  • 0b5c8870e5aa4fd0889d60faa9b1f65a9c338fff: 8338380: Update TLSCommon/interop/AbstractServer to specify an interface to listen for connections
  • 6041c936d6dd39c5b3a89ed2823b25a8aef42b9f: 8335664: Parsing jsr broken: assert(bci>= 0 && bci < c->method()->code_size()) failed: index out of bounds
  • 6cf7f9c4a76b99ed7aa4dc7ee54692331fc73408: 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[bot] avatar May 13 '24 10:05 openjdk[bot]

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

openjdk[bot] avatar May 13 '24 10:05 openjdk[bot]

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

mcimadamore avatar May 13 '24 10:05 mcimadamore

Webrevs

mlbridge[bot] avatar May 13 '24 11:05 mlbridge[bot]

Build changes look good.

erikj79 avatar May 13 '24 13:05 erikj79

security changes (java.security.jgss, jdk.crypto.cryptoki, jdk.crypto.mscapi, and jdk.security.auth) look good.

wangweij avatar May 13 '24 13:05 wangweij

Changes to jdk.net and jdk.sctp look ok.

dfuch avatar May 13 '24 14:05 dfuch

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.

jaikiran avatar May 16 '24 11:05 jaikiran

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

mcimadamore avatar May 16 '24 11:05 mcimadamore

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.

AlanBateman avatar May 21 '24 07:05 AlanBateman

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.

mcimadamore avatar May 21 '24 08:05 mcimadamore

jdk.jpackage changes look good

alexeysemenyukoracle avatar May 21 '24 15:05 alexeysemenyukoracle

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.

alexeysemenyukoracle avatar May 21 '24 16:05 alexeysemenyukoracle

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.

AlanBateman avatar May 23 '24 06:05 AlanBateman

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.

kevinrushforth avatar May 23 '24 13:05 kevinrushforth

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

bridgekeeper[bot] avatar Jun 20 '24 17:06 bridgekeeper[bot]

keep alive

mcimadamore avatar Jul 15 '24 13:07 mcimadamore

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

bridgekeeper[bot] avatar Aug 12 '24 19:08 bridgekeeper[bot]

/integrate

mcimadamore avatar Aug 26 '24 09:08 mcimadamore

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

  • ce83f6af64efd673b83c945765f68e8a3bf89774: 8338844: C2: remove useless code in PhaseIdealLoop::place_outside_loop() after 8335709
  • 0c14579fef902f0501d0510bdc32e8cece34834a: 8336830: C2: assert(get_loop(lca)->_nest < n_loop->_nest || lca->in(0)->is_NeverBranch()) failed: must not be moved into inner loop
  • 5671f836039ef1683e3e9ce5b7cf0fa2f1860e2d: 8338785: The java.awt.datatransfer.SystemFlavorMap#FLAVOR_MAP_KEY field is not used
  • 32b3d707c1b3a9a0d127684e245e5c975ac5566a: 8338925: ProblemList runtime/interpreter/LastJsrTest.java on linux-all
  • 5d12ac3fcb076bf701d7a572942f57f4de7a9ca0: 8337715: Update --release 23 symbol information for JDK 23 build 37
  • 23dc3b02468836f4c9b4303f2c7c0a7305461ce1: 8324048: (fc) Make FileKey fields final
  • a461369f16a2d92ab428d14c36dd69fa5942bbc5: 8338700: AttributeMapper type parameter should be bounded by Attribute
  • 916f1aa04f6fcc6da9bf9d725e3639cf4c0755a1: 8329756: [macos] "javax/swing/JTable/KeyBoardNavigation.java" fail because most combinations of navigational keys with the Ctrl key do not work
  • 21d1e4d8039ecccbf60138ede574e0177ee5550f: 8338819: JFR: Internal events causes crash when no other events are in use
  • 965dd1acd0ce5b225d85e2c55cc097856e0e9f3c: 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[bot] avatar Aug 26 '24 09:08 openjdk[bot]

@mcimadamore Pushed as commit 20d8f58c92009a46dfb91b951e7d87b4cb8e8b41.

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

openjdk[bot] avatar Aug 26 '24 09:08 openjdk[bot]