[foreign-abi] RFR: 8237585: Dismantle ForeignUnsafe

[Ty Young]

On 2/25/20 11:50 AM, Maurizio Cimadamore 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.
>
> 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?


Please do. It's very odd that you can't get the raw long address of a 
MemoryAddress in the current API.


>
>
>>
>> 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 (**); 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


No more launcher flags, please. IDEs will either take their sweet time 
to add support for them or might not provide support at all.


This is a project dependency issue and could be handled in module-info 
completely. You could add the ability to declare a module via its 
declaration(module org.foo) as unsafe and/or native and specify which 
package the unsafe and/or native access is from. Any method which uses 
Panama's API will then require an unsafe and/or native annoation which 
will in turn require the package being declared as unsafe and/or native 
in module-info.


(The "unsafe" annoation doesn't even need to be restricted to Panama, it 
could be used for other things too)


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