Re: 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: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]
On Mon, 13 May 2024 11:47:38 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 three > additional commits since the last revision: > > - Fix another typo > - Fix typo > - Add more comments Build changes look good. - PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2107563120
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]
On Mon, 13 May 2024 11:47:38 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 three > additional commits since the last revision: > > - Fix another typo > - Fix typo > - Add more comments security changes (`java.security.jgss`, `jdk.crypto.cryptoki`, `jdk.crypto.mscapi`, and `jdk.security.auth`) look good. - PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2107621474
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]
On Mon, 13 May 2024 11:47:38 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 three > additional commits since the last revision: > > - Fix another typo > - Fix typo > - Add more comments Changes to jdk.net and jdk.sctp look ok. - PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2107695217
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]
On Mon, 13 May 2024 11:47:38 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 three
> additional commits since the last revision:
>
> - Fix another typo
> - Fix typo
> - Add more comments
src/hotspot/share/runtime/arguments.cpp line 2271:
> 2269: } else if (match_option(option, "--illegal-native-access=", &tail))
> {
> 2270: if (!create_module_property("jdk.module.illegal.native.access",
> tail, InternalProperty)) {
> 2271: return JNI_ENOMEM;
I think it would be helpful to get guidance on if this is the right way to add
this system property, only because this one not a "module property". The
configuration (WriteableProperty + InternalProperty) look right.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1598673962
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]
On Mon, 13 May 2024 11:47:38 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 three
> additional commits since the last revision:
>
> - Fix another typo
> - Fix typo
> - Add more comments
src/hotspot/share/prims/nativeLookup.cpp line 275:
> 273:
> 274: // Otherwise call static method findNative in ClassLoader
> 275:
Suggestion:
src/hotspot/share/prims/nativeLookup.cpp line 419:
> 417: if (entry != nullptr) return entry;
> 418:
> 419:
Suggestion:
src/hotspot/share/prims/nativeLookup.cpp line 426:
> 424: return nullptr;
> 425: }
> 426: }
Suggestion:
}
src/java.base/share/classes/java/lang/Module.java line 331:
> 329: String modflag = isNamed() ? getName() : "ALL-UNNAMED";
> 330: String caller = currentClass != null ?
> currentClass.getName() : "code";
> 331: System.err.printf("""
This message should probably be different when linking native methods, since
otherwise it’ll be:
WARNING: A restricted method in foo has been called
WARNING: bar has been called by Baz in Baz
WARNING: Use --enable-native-access=foo to avoid a warning for callers in this
module
WARNING: Restricted methods will be blocked in a future release unless native
access is enabled
when it should really be something like:
WARNING: A JNI native method in foo has been linked
WARNING: bar has been linked in Baz
WARNING: Use --enable-native-access=foo to avoid a warning for native methods
in this module
WARNING: Native methods will be blocked in a future release unless native
access is enabled
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1599248442
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1599248501
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1599248577
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1599253428
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]
On Mon, 13 May 2024 15:32:27 GMT, Alan Bateman wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - Fix another typo
>> - Fix typo
>> - Add more comments
>
> src/hotspot/share/runtime/arguments.cpp line 2271:
>
>> 2269: } else if (match_option(option, "--illegal-native-access=",
>> &tail)) {
>> 2270: if (!create_module_property("jdk.module.illegal.native.access",
>> tail, InternalProperty)) {
>> 2271: return JNI_ENOMEM;
>
> I think it would be helpful to get guidance on if this is the right way to
> add this system property, only because this one not a "module property". The
> configuration (WriteableProperty + InternalProperty) look right.
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?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1600819327
Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]
On Wed, 15 May 2024 00:54:43 GMT, David Holmes wrote:
>> src/hotspot/share/runtime/arguments.cpp line 2271:
>>
>>> 2269: } else if (match_option(option, "--illegal-native-access=",
>>> &tail)) {
>>> 2270: if (!create_module_property("jdk.module.illegal.native.access",
>>> tail, InternalProperty)) {
>>> 2271: return JNI_ENOMEM;
>>
>> I think it would be helpful to get guidance on if this is the right way to
>> add this system property, only because this one not a "module property". The
>> configuration (WriteableProperty + InternalProperty) look right.
>
> 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.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1601002132
Re: 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: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v3]
On Wed, 15 May 2024 10:34:01 GMT, Maurizio Cimadamore wrote: > 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. It maps here to a jdk.module.* property so I suppose it's okay. The functions were introduced to create jdk.module.* properties where the values were a set module names or a module path. Maybe these functions should be renamed at some point (not here) as they are more widely useful now. - PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1601421535
