jmx-dev Integrated: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI
On Mon, 13 May 2024 10:42:26 GMT, Maurizio Cimadamore wrote: > This PR implements [JEP 472](https://openjdk.org/jeps/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. This pull request has now been integrated. Changeset: 20d8f58c Author:Maurizio Cimadamore URL: https://git.openjdk.org/jdk/commit/20d8f58c92009a46dfb91b951e7d87b4cb8e8b41 Stats: 532 lines in 107 files changed: 341 ins; 52 del; 139 mod 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI Reviewed-by: jpai, prr, ihse, kcr, alanb - PR: https://git.openjdk.org/jdk/pull/19213
Re: jmx-dev RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v10]
> This PR implements [JEP 472](https://openjdk.org/jeps/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. Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 13 additional commits since the last revision: - Merge branch 'master' into restricted_jni - Merge branch 'master' into restricted_jni - Address review comments - Add note on --illegal-native-access default value in the launcher help - Address review comment - Refine warning text for JNI method binding - Address review comments Improve warning for JNI methods, similar to what's described in JEP 472 Beef up tests - Address review comments - Fix another typo - Fix typo - ... and 3 more: https://git.openjdk.org/jdk/compare/f7ea738c...04622748 - Changes: - all: https://git.openjdk.org/jdk/pull/19213/files - new: https://git.openjdk.org/jdk/pull/19213/files/ff51ac6a..04622748 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=09 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=08-09 Stats: 51278 lines in 1477 files changed: 28775 ins; 15348 del; 7155 mod Patch: https://git.openjdk.org/jdk/pull/19213.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19213/head:pull/19213 PR: https://git.openjdk.org/jdk/pull/19213
Re: jmx-dev RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]
On Fri, 17 May 2024 13:38:25 GMT, Maurizio Cimadamore wrote: >> This PR implements [JEP 472](https://openjdk.org/jeps/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. > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comments keep alive - PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2228489298
Re: jmx-dev RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v9]
> This PR implements [JEP 472](https://openjdk.org/jeps/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. Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since the last revision: - Merge branch 'master' into restricted_jni - Address review comments - Add note on --illegal-native-access default value in the launcher help - Address review comment - Refine warning text for JNI method binding - Address review comments Improve warning for JNI methods, similar to what's described in JEP 472 Beef up tests - Address review comments - Fix another typo - Fix typo - Add more comments - ... and 2 more: https://git.openjdk.org/jdk/compare/2ced23fe...ff51ac6a - Changes: - all: https://git.openjdk.org/jdk/pull/19213/files - new: https://git.openjdk.org/jdk/pull/19213/files/789bdf48..ff51ac6a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=08 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=07-08 Stats: 168976 lines in 3271 files changed: 114666 ins; 38249 del; 16061 mod Patch: https://git.openjdk.org/jdk/pull/19213.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19213/head:pull/19213 PR: https://git.openjdk.org/jdk/pull/19213
Re: jmx-dev RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]
On Tue, 21 May 2024 07:20:05 GMT, Alan Bateman wrote: > > 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. - PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2122095444
Re: jmx-dev RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]
> This PR implements [JEP 472](https://openjdk.org/jeps/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. Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Address review comments - Changes: - all: https://git.openjdk.org/jdk/pull/19213/files - new: https://git.openjdk.org/jdk/pull/19213/files/3a0db276..789bdf48 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=07 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=06-07 Stats: 28 lines in 10 files changed: 8 ins; 2 del; 18 mod Patch: https://git.openjdk.org/jdk/pull/19213.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19213/head:pull/19213 PR: https://git.openjdk.org/jdk/pull/19213
Re: jmx-dev RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v7]
On Thu, 16 May 2024 18:39:57 GMT, Alan Bateman wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Add note on --illegal-native-access default value in the launcher help
>
> src/java.base/share/classes/java/lang/System.java line 2023:
>
>> 2021: * @throws NullPointerException if {@code filename} is {@code
>> null}
>> 2022: * @throws IllegalCallerException If the caller is in a module
>> that
>> 2023: * does not have native access enabled.
>
> The exception description is fine, just noticed the other exception
> descriptions start with a lowercase "if", this one is different.
I'll fix this. Note that in `ModuleLayer.Controller`, all `@throws` start with
capital letter, which is probably where I copied/pasted this from. I'll fix
all, except for `ModuleLayer` where, for consistency, I think upper case is
better.
> src/java.base/share/man/java.1 line 587:
>
>> 585: \f[V]deny\f[R]: This mode disables all illegal native access except for
>> 586: those modules enabled by the \f[V]--enable-native-access\f[R]
>> 587: command-line option.
>
> "This mode disable all illegal native access except for those modules enabled
> the --enable-native-access command-line option".
>
> This can be read to mean that modules granted native access with the command
> line option is also illegal native access An alternative is to make the
> second part of the sentence a new sentence, something like "Only modules
> enabled by the --enable-native-access command line option may perform native
> access.
I've simplified the text to:
This mode disables illegal native access. That is, any illegal native access
causes an `IllegalCallerException`.
This mode will become the default in a future release.
I think it's not necessary to state again the dependency on
`--enable-native-access` as we already defined what "illegal native access"
means.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1604994928
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1604993505
Re: jmx-dev RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v7]
> This PR implements [JEP 472](https://openjdk.org/jeps/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. Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Add note on --illegal-native-access default value in the launcher help - Changes: - all: https://git.openjdk.org/jdk/pull/19213/files - new: https://git.openjdk.org/jdk/pull/19213/files/1c45e5d5..3a0db276 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=06 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=05-06 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19213.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19213/head:pull/19213 PR: https://git.openjdk.org/jdk/pull/19213
Re: jmx-dev RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v6]
On Thu, 16 May 2024 11:55:35 GMT, Jaikiran Pai wrote: >> We already do, see >> https://github.com/openjdk/jdk/pull/19213/files/1c45e5d56c429205ab8185481bc1044a86ab3bc6#diff-d05029afe6aed86f860a10901114402f1f6af4fe1e4b46d883141ab1d2a527b8R582 > > This is slightly different from what we do in the other PR for unsafe memory > access where we specify the default in the launcher's help text too > https://github.com/openjdk/jdk/pull/19174/files#diff-799093930b698e97b23ead98c6496261af1e2e33ec7aa9261584870cbee8a5eaR219. > > I don't have a strong opinion on this, I think either one is fine. Ah, apologies, I was looking in the wrong place. I agree that we should specify default in the launcher, as well as in the man pages. - PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603233038
Re: jmx-dev RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v6]
On Thu, 16 May 2024 11:42:48 GMT, Jaikiran Pai wrote: > 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/classes/sun/launcher/resources/launcher.properties line > 72: > >> 70: \ by code in modules for which native access is not >> explicitly enabled.\n\ >> 71: \ is one of "deny", "warn" or "allow".\n\ >> 72: \ This option will be removed in a future release.\n\ > > Should this specify the current default value for this option if it isn't set? We already do, see https://github.com/openjdk/jdk/pull/19213/files/1c45e5d56c429205ab8185481bc1044a86ab3bc6#diff-d05029afe6aed86f860a10901114402f1f6af4fe1e4b46d883141ab1d2a527b8R582 - PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2115012361 PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1603195671
Re: jmx-dev RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v6]
> This PR implements [JEP 472](https://openjdk.org/jeps/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. Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Address review comment - Changes: - all: https://git.openjdk.org/jdk/pull/19213/files - new: https://git.openjdk.org/jdk/pull/19213/files/daf729f4..1c45e5d5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=05 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19213.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19213/head:pull/19213 PR: https://git.openjdk.org/jdk/pull/19213
Re: jmx-dev RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v5]
> This PR implements [JEP 472](https://openjdk.org/jeps/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. Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Refine warning text for JNI method binding - Changes: - all: https://git.openjdk.org/jdk/pull/19213/files - new: https://git.openjdk.org/jdk/pull/19213/files/0d21bf99..daf729f4 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=03-04 Stats: 4 lines in 2 files changed: 0 ins; 0 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/19213.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19213/head:pull/19213 PR: https://git.openjdk.org/jdk/pull/19213
Re: jmx-dev RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]
On Wed, 15 May 2024 06:15:35 GMT, Alan Bateman wrote: >> So my recollection/understanding is that we use this mechanism to convert >> module-related `--` flags passed to the VM into system properties that the >> Java code can then read, but we set them up such that you are not allowed to >> specify them directly via `-D`. Is the question here whether this new >> property should be in the `jdk.module` namespace? > > That's my recollection too. The usage here isn' related to modules which > makes me wonder if this function should be renamed (not by this PR of course) > of if we should be using PropertyList_unique_add (with AddProperty, > WriteableProperty, InternalProperty) instead. There will be further GNU style > options coming that will likely need to map to an internal system property in > the same way. I don't fully agree that this option is not module related (which is why I gave it that name). The very definition of illegal native access is related to native access occurring from a module that is outside a specific set. So I think it's 50/50 as to whether this option is module-related or not. Of course I can fix the code if there's something clearly better. - PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1601386336
Re: jmx-dev RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v4]
On Wed, 15 May 2024 07:55:27 GMT, ExE Boss wrote: > Note that this line is still not entirely correct, as for code like: You are correct - the message is however consistent with what written in JEP 472. I'll discuss with @pron - PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1601335120
Re: jmx-dev RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v4]
> This PR implements [JEP 472](https://openjdk.org/jeps/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. Maurizio Cimadamore has updated the pull request incrementally with two additional commits since the last revision: - Address review comments Improve warning for JNI methods, similar to what's described in JEP 472 Beef up tests - Address review comments - Changes: - all: https://git.openjdk.org/jdk/pull/19213/files - new: https://git.openjdk.org/jdk/pull/19213/files/bad10942..0d21bf99 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=02-03 Stats: 84 lines in 15 files changed: 42 ins; 14 del; 28 mod Patch: https://git.openjdk.org/jdk/pull/19213.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19213/head:pull/19213 PR: https://git.openjdk.org/jdk/pull/19213
Re: jmx-dev RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]
> This PR implements [JEP 472](https://openjdk.org/jeps/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. Maurizio Cimadamore has updated the pull request incrementally with three additional commits since the last revision: - Fix another typo - Fix typo - Add more comments - Changes: - all: https://git.openjdk.org/jdk/pull/19213/files - new: https://git.openjdk.org/jdk/pull/19213/files/c4938dc7..bad10942 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=01-02 Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/19213.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19213/head:pull/19213 PR: https://git.openjdk.org/jdk/pull/19213
Re: jmx-dev RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v2]
On Mon, 13 May 2024 11:38:40 GMT, Maurizio Cimadamore
wrote:
>> This PR implements [JEP 472](https://openjdk.org/jeps/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.
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Avoid call to VM::isModuleSystemInited
> Use initial error stream
src/java.base/share/classes/jdk/internal/reflect/Reflection.java line 124:
> 122: if (module != null) {
> 123: // not in init phase
> 124: 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1598328283
Re: jmx-dev RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v2]
> This PR implements [JEP 472](https://openjdk.org/jeps/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. Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Avoid call to VM::isModuleSystemInited Use initial error stream - Changes: - all: https://git.openjdk.org/jdk/pull/19213/files - new: https://git.openjdk.org/jdk/pull/19213/files/d9fe9a71..c4938dc7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=00-01 Stats: 11 lines in 2 files changed: 3 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/19213.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19213/head:pull/19213 PR: https://git.openjdk.org/jdk/pull/19213
Re: jmx-dev RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI
On Mon, 13 May 2024 10:42:26 GMT, Maurizio Cimadamore
wrote:
> This PR implements [JEP 472](https://openjdk.org/jeps/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.
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
make/conf/module-loader-map.conf line 105:
> 103: java.smartcardio \
> 104: jdk.accessibility \
> 105: 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.
src/hotspot/share/prims/nativeLookup.cpp line 277:
> 275:
> 276: Klass* klass = vmClasses::ClassLoader_klass();
> 277: 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.
src/java.base/share/classes/java/lang/Module.java line 311:
> 309: Module target = moduleForNativeAccess();
> 310: ModuleBootstrap.IllegalNativeAccess illegalNativeAccess =
> ModuleBootstrap.illegalNativeAccess();
> 311: 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
src/java.base/share/classes/jdk/internal/reflect/Reflection.java line 115:
> 113: @ForceInline
> 114: public static void ensureNativeAccess(Class currentClass,
> Class owner, String methodName) {
> 115: if (VM.isModuleSystemInited()) {
If we call this code too early, we can see cases where `module` is `null`.
src/java.desktop/macosx/classes/com/apple/eio/FileManager.java line 61:
> 59: }
> 60:
> 61: @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.
test/jdk/java/foreign/enablenativeaccess/panama_jni_load_module/module-info.java
line 24:
> 22: */
> 23:
> 24: 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).
-
PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2107272261
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1598269825
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1598271285
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1598274987
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1598276455
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1598277853
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1598279827
jmx-dev RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI
This PR implements [JEP 472](https://openjdk.org/jeps/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. - Commit messages: - Initial push Changes: https://git.openjdk.org/jdk/pull/19213/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19213&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8331671 Stats: 466 lines in 99 files changed: 301 ins; 53 del; 112 mod Patch: https://git.openjdk.org/jdk/pull/19213.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19213/head:pull/19213 PR: https://git.openjdk.org/jdk/pull/19213
Re: jmx-dev RFR: 8015831: Add lint check for calling overridable methods from a constructor
On Fri, 6 Jan 2023 15:38:31 GMT, Alan Bateman wrote: >>> The associated JBS issue has been dormant for 6+ years and this is a very >>> intrusive change affecting many, many files. Has the resurrection of this >>> project previously been discussed somewhere? >> >> Hi @dholmes-ora, >> >> The work to add this warning has been guided by @briangoetz in discussions >> on the amber-dev mailing list. See that thread for how we came up with the >> current design, underlying motivation, etc. >> >> Regarding intrusiveness (assuming you're referring simply to the number of >> files touched), as mentioned this was one of two options. The other option >> would be to patch to about ~30 `Java.gmk` files in `make/modules` to exclude >> `this-escape` from `-Xlint` during the various module builds. >> >> Going this route is fine with me, but it has the downside that any new code >> being developed would not benefit from the new warning. This was in fact my >> original approach (and it was a lot easier :) but Brian rightly pointed out >> that adding `@SuppressWarnings` annotations was the the safer (i.e, more >> conservative) approach. >> >>> If you haven't done so already then you probably should socialize on >>> compiler-dev and get agreement on the semantics and other details. >> >> Hi @AlanBateman, >> >> As mentioned this has been under discussion on amber-dev for a while. Happy >> to continue that discussion here as well. >> >>> I think you will also need to separate the addition of the lint warning >>> from the changes to the wider JDK. It might be okay to add the feature but >>> have it disabled for the JDK build initially. >> >> Sounds reasonable... so I take it you would also be in favor of patching >> `make/modules` instead of adding `@SuppressWarnings` annotations >> everywhere... is that correct? >> >> If this is generally agreed as a better route then let me know and I'll >> update the patch. >> >> Thanks for both of your comments. > >> Sounds reasonable... so I take it you would also be in favor of patching >> `make/modules` instead of adding `@SuppressWarnings` annotations >> everywhere... is that correct? >> >> If this is generally agreed as a better route then let me know and I'll >> update the patch. > > Yes, I think that would be better. It would remove most of the noise, 1200+ > files, and 10+ mailing lists from this PR. I assume there will be at least > some iteration on compiler-dev about the details and changes to javac. Once > you get to the JDK changes then I suspect that some areas may want to fix > issues rather than adding SW. Sadly, I see a few examples in your list where > there have been attempts to avoid leaking "this" but where we backed away out > of concern that 3rd party code was extending some class and overriding a > method known to be invoked by the constructor. Also we have places that > register themselves to cleaners. I suspect some of the suggestions to > document leaking this in implNotes will need discussion too because they > amount to documenting "hooks" that people will rely on, e.g. documenting in > ArrayDeque that its constructor invokes addList could be read as an invite to > override it. I agree with @AlanBateman. When we introduced similar warnings in the past, we have enabled support in javac (with some test) in a separate PR, and then followed up with small dedicated PR for each of the various areas (enabling new warnings one by one). See this great umbrella JBS issue (created by @asotona) which has details on all the issues that were filed when we added an extra Lint warning for lossy conversions: https://bugs.openjdk.org/browse/JDK-8286374 They all refer to this: https://bugs.openjdk.org/browse/JDK-8244681 Which was submitted as a separate javac-only PR: https://github.com/openjdk/jdk/pull/8599 - PR: https://git.openjdk.org/jdk/pull/11874
