[foreign-abi] RFR: 8237585: Dismantle ForeignUnsafe

Tue Feb 25 18:10:38 UTC 2020

> On Feb 25, 2020, at 9:50 AM, Maurizio Cimadamore <maurizio.cimadamore at oracle.com> wrote:
> 
> 
> On 25/02/2020 17:24, Paul Sandoz wrote:
>> Hi,
>> 
>> I don’t think we should be referencing sun.misc.Unsafe in the documentation of a public API.  It should stand on its own presenting access to the unsafe address pair for double-register addressing.  Any implication that this information can be consumed by sun.misc.Unsafe is coincidental :-)
>> 
>> How is is the address pair intended to be used by the Java caller other than pumping the values into Unsafe methods?
> 
> The address value is definitively useful for whenever you need to need to break free of MemoryAddress and just get your hands on the raw pointer value (a long) which you can then pass e.g. to some native method. So, not just unsafe.get/putXYZ.
> 

Yes, that's like JNI code getting access to the underlying address of a direct byte buffer.

> I think the getBase is the one that is more suspicious, and the one that is more linked to the unsafe addressing scheme. I think we could probably just remove it and have a single method for turning a MemoryAddress into a long raw address?

That seems a better option for now, and in addition to the current checks the method would barf if the underlying base is not null.

> 
> 
>> 
>> Perhaps with inline types we could model this as an UnsafeMemoryAddress.  Is that possible to do now by appealing to a value-based property in the interim?
>> (Internally we don’t need to use that.)
>> 
>> —
>> 
>> This is for a longer discussion…. In past times we might have appealed to a security manager to perform such a permission check.  In current times I think we are more talking about checks to safe guard the integrity of the program.  (Unwarranted System.exit calls might also fit into this approach.)
> 
> Yep - and note that this is a step gap solution (**)

Understood, thanks for sharing the pragmatic rational and vision.

Paul.

> ; in the long run, we want to have a per-module launcher flag, so that restricted native methods can be allowed on a per module basis. We'd also like to keep track of whether a module made access to restricted methods and record that property in some attribute of the module-info file, so that we can fail early when loading the module graph if:
> 
> 1) we are trying to load a module which 'requires native'
> 2) the correct 'allow native to module XYZ' flag is not passed to the command line
> 
> Another thing we discussed in this area is as to whether JNI should or not be covered by same treatment; in principle it should, given that an application calling into a JNI native method can break safety in many possible ways, and if your application depends on 200 jars it is not obvious to understand if (or to protect against) any of them is attempting to do something that you have not agreed upon. Yes, security manager would be one way to do these things, but, as you point out, we've been slowing moving to other approaches (and the module system gives us more ammo to check these things, both at compile- and run-time).
> 
> (**) - the stop gap solution is needed because some of these restricted methods are interesting and useful when working with bindings, and it is kind of unfortunate that, by restricting access to them by using a non-exported package, there's no way e.g. to get javadoc info about these methods (which impacts API discoverability) - and use of non-exported packages also severely limits adoption as IDEs are not always smart to suggest the --add-export flags.
> 
> Maurizio
> 
>> 
>> Paul.
>> 
>> 
>>> On Feb 25, 2020, at 7:09 AM, Jorn Vernee <jvernee at openjdk.java.net> wrote:
>>> 
>>> Removes ForeignUnsafe, moves `getUnsafeBase` and `getUnsafeAccess` to `MemoryAddress`, and moves `ofNativeUnchecked` to `MemorySegment`.
>>> 
>>> I've added a warning about the safety of these methods to the javadoc.
>>> 
>>> Use of the methods is guarded by a check against a system property, `jdk.incubator.foreign.permitUnsafeInterop` for the first 2, and `jdk.incubator.foreign.permitUncheckedSegments` for the latter. This needs to be set to `true` when running an application, or otherwise the use of these methods results in an `IllegalAccessError` being thrown.
>>> 
>>> I've updated the test accordingly, and also remove the FASTPATH option from the SysV ABI implementation, since it was not actually being used, but still causing the tests to be run multiple times.
>>> 
>>> -------------
>>> 
>>> Commits:
>>> - 5c58877a: Slightly touch up javadoc
>>> - 6abdd478: Remove ForeignUnsafe.java, and move it's method to MemoryAddress and MemorySegment
>>> 
>>> Changes: https://git.openjdk.java.net/panama-foreign/pull/31/files
>>> Webrev: https://webrevs.openjdk.java.net/panama-foreign/31/webrev.00
>>>  Issue: https://bugs.openjdk.java.net/browse/JDK-8237585
>>>  Stats: 176 lines in 10 files changed: 78 ins; 91 del; 7 mod
>>>  Patch: https://git.openjdk.java.net/panama-foreign/pull/31.diff
>>>  Fetch: git fetch https://git.openjdk.java.net/panama-foreign pull/31/head:pull/31
>>> 
>>> PR: https://git.openjdk.java.net/panama-foreign/pull/31