[foreign-abi] RFR: 8237585: Dismantle ForeignUnsafe

Tue Feb 25 17:50:02 UTC 2020

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?

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

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