Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]

2024-07-15 Thread Maurizio Cimadamore
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: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]

2024-05-23 Thread Kevin Rushforth
On Thu, 23 May 2024 06:20:51 GMT, Alan Bateman  wrote:

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

-

PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2127188783


Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]

2024-05-23 Thread Alan Bateman
On Wed, 22 May 2024 21:42:14 GMT, Kevin Rushforth  wrote:

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

-

PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2126320311


Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]

2024-05-22 Thread Kevin Rushforth
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

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.

-

Marked as reviewed by kcr (Author).

PR Review: https://git.openjdk.org/jdk/pull/19213#pullrequestreview-2072430338


Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]

2024-05-22 Thread Magnus Ihse Bursie
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

Build changes look good. Thanks for trimming down NATIVE_ACCESS_MODULES.

-

Marked as reviewed by ihse (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19213#pullrequestreview-2070573791


Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]

2024-05-21 Thread Alexey Semenyuk
On Tue, 21 May 2024 08:44:47 GMT, Maurizio Cimadamore  
wrote:

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

-

PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2123054154


Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]

2024-05-21 Thread Phil Race
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

client parts look fine.

-

Marked as reviewed by prr (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19213#pullrequestreview-2069134455


Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]

2024-05-21 Thread Alexey Semenyuk
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

`jdk.jpackage` changes look good

-

PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2122942586


Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]

2024-05-21 Thread Maurizio Cimadamore
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: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]

2024-05-21 Thread Alan Bateman
On Mon, 20 May 2024 18:47:35 GMT, Phil Race  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.

-

PR Comment: https://git.openjdk.org/jdk/pull/19213#issuecomment-2121927727


Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]

2024-05-20 Thread Alan Bateman
On Mon, 20 May 2024 18:39:31 GMT, Phil Race  wrote:

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

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1607147983


Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]

2024-05-20 Thread Phil Race
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

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.

-

PR Review: https://git.openjdk.org/jdk/pull/19213#pullrequestreview-2066794950


Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]

2024-05-20 Thread Phil Race
On Mon, 13 May 2024 10:49:30 GMT, Maurizio Cimadamore  
wrote:

>> Maurizio Cimadamore has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Address review comments
>
> 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.

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 ?

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

Sounds reasonable.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1607136237
PR Review Comment: https://git.openjdk.org/jdk/pull/19213#discussion_r1607136808


Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]

2024-05-18 Thread Jaikiran Pai
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

Marked as reviewed by jpai (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19213#pullrequestreview-2064736036


Re: RFR: 8331671: Implement JEP 472: Prepare to Restrict the Use of JNI [v8]

2024-05-17 Thread Maurizio Cimadamore
> 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=19213=07
 - incr: https://webrevs.openjdk.org/?repo=jdk=19213=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