[foreign-memaccess+abi] RFR: 8310659: The jar tool should support allowing access to restricted methods from executable jars [v4]

[Jorn Vernee]

On Wed, 28 Jun 2023 05:48:10 GMT, Alan Bateman <alanb at openjdk.org> wrote:

>> src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java line 776:
>> 
>>> 774:     private static final boolean HAS_ENABLE_NATIVE_ACCESS_FLAG;
>>> 775:     private static final Set<String> NATIVE_ACCESS_MODULES;
>>> 776:     private static boolean ENABLE_NATIVE_ACCESS_SET_IN_MANIFEST = false;
>> 
>> We initialize this class before the manifest is read, so I made this flag mutable.
>
> ModuleBootstrap is for initialising the module system early in the startup. It's not meant to have mutable state that is changed post VM initialization by the launcher. So I'd prefer not introduce this coupling.
> 
> If the manifest has `Enable-Native-Access: ALL-UNNAMED` then I would just expect to see the launcher use Modules.addEnableNativeAccessToAllUnnamed(), nothing more. So I think I'd like to understand how setEnableNativeAccessSetInManifest came about. Is there an ordering issue where somehow ModuleBootstrap.hasEnableNativeAccessFlag is called before EnableNativeAccess.isNativeAccessEnabled on the target module?

There are 2 parts of the puzzle:
1. we need to enable native access for a module. This is what `addEnableNativeAccessToAllUnnamed` does
2. we need to track whether an enable native access flag has been set 'externally', through either the command line or the jar manifest. This dictates whether we should produce an error or a warning.

Right now the latter is implemented by decoding a bunch of system properties that are initialized in arguments.cpp [1], [2]. This decoding than produces a set of module names that was specified on the command line. And this is used to initialize the `HAS_ENABLE_NATIVE_ACCESS_FLAG` variable [3].

I just hooked into the latter process by adding the `ENABLE_NATIVE_ACCESS_SET_IN_MANIFEST` flag. This flag is mutable since `ModuleBootstrap` is intialized before we read the jar manifest. i.e. the variable is mutable because it is lazily initialized.  This works out because we only ever call `Module::ensureNativeAccess` after reading the jar manifest.

`ModuleBoostrap::hasEnableNativeAccessFlag` is really `hasEnableNativeAccessFlagOnCommandLine`, and we need another `hasEnableNativeAccessFlagInJarManifest`, and then the call to `hasEnableNativeAccessFlag` in `Module::ensureNativeAccess` would need to be replaced by `hasEnableNativeAccessFlagOnTheCommandLine() || hasEnableNativeAccessFlagInJarManifest()`.

But, I think we would still have a chicken and egg problem, since we start using modules before reading the jar manifest, so before we really know whether we should be throwing an error or warning for native access. So, at least in theory, if someone called `ensureNativeAccess` before the jar manifest is read, then they could observe the incorrect behavior.

So, I'm not sure if we could avoid this issue, other than not letting the flag set in the manifest factor into the decision whether to throw an error or just issue a warning.

Another option which I tried earlier, is for `LauncherHelper` to set a system property, and then initialize a `static final` flag in a holder class based on that property the first time `ModuleBootsrap::hasEnableNativeAccessFlag` is called. But, that seemed like it was just hiding the mutable state in the system property. Though, maybe it's preferable? It would at least show that we only initialize the state once, and it is initialized somewhere before we call `hasEnableNativeAccessFlag`.

[1]: https://github.com/openjdk/panama-foreign/blob/e326dd172e988532102519abe0ba46a9a4e7fca7/src/hotspot/share/runtime/arguments.cpp#L2341-L2344
[2]: https://github.com/openjdk/panama-foreign/blob/e326dd172e988532102519abe0ba46a9a4e7fca7/src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java#L807
[3]: https://github.com/openjdk/panama-foreign/blob/e326dd172e988532102519abe0ba46a9a4e7fca7/src/java.base/share/classes/jdk/internal/module/ModuleBootstrap.java#L783

-------------

PR Review Comment: https://git.openjdk.org/panama-foreign/pull/843#discussion_r1244885184