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

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

2024-05-15 Thread ExE Boss
On Tue, 14 May 2024 18:10:28 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 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

src/java.base/share/classes/java/lang/Module.java line 334:

> 332: System.err.printf("""
> 333: WARNING: A native method in %s has been bound
> 334: WARNING: %s has been called by %s in %s

Note that this line is still not entirely correct, as for code like:

// in module a:
package a;

import b.Foo;

public class Foo {
public static void main(String... args) {
System.load("JNI library implementing Java_b_Bar_nativeMethod");

Bar.nativeMethod();
}
}


// in module b:
package b;

public class Bar {
public static native void nativeMethod();
}


It’ll show `Bar` as the caller of `Bar::nativeMethod()`, even though the caller 
is `Foo` in this case, which is why I initially suggested just omitting the 
caller from **JNI** linkage warnings.

-

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


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

2024-05-14 Thread David Holmes
On Tue, 14 May 2024 18:10:28 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 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

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

-

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


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

2024-05-14 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 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=19213=03
 - incr: https://webrevs.openjdk.org/?repo=jdk=19213=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