[foreign-abi] minimizing the surface of restricted native access

[Jorn Vernee]

I really like how the patch looks. It makes more operations safe, and 
allows users to apply safety where possible (as shown by the tests).

I noticed that there is a bounds check in `rebase`, but this is not 
needed -> if an address is OOB, we still can not access it after a 
rebase any ways. I can see how this can help to fail fast in some cases, 
but we've already soon OOB pointers being useful, so sooner or later we 
might encounter a use-case where this bounds check would be prohibitive. 
e.g. even if a MemoryAddress is inaccessible because it's out of bounds, 
it could still be useful to rebase it onto another segment in order to 
get a MemoryAddress object tied to the lifetime of that segment.

One minor suggestion I have wrt the patch is that you could create a 
Util::resizeNativeUnchecked(MemoryAddress, long) that does the resize, 
so that the cast can be removed from callers (looks a little less 
cluttered imo).

Jorn

On 02/12/2019 15:55, Maurizio Cimadamore wrote:
> Following up on this piece of feedback, Jorn and I came up with a more 
> robust strategy; instead of converting an address to an unchecked 
> address, the API will instead allow creation of an 'unsafe' native 
> memory segment with its own temporal bounds and spatial bounds:
>
>     /**
>      * Returns a new native memory segment with given base address and 
> size. The returned segment has its own temporal
>      * bounds, and can therefore be closed; closing such a segment 
> does <em>not</em> result in any resource being
>      * deallocated.
>      * @param base the desired base address
>      * @param byteSize the desired size.
>      * @return a new native memory segment with given base address and 
> size.
>      * @throws IllegalArgumentException if {@code base} does not 
> encapsulate a native memory address.
>      */
>     public static MemorySegment ofNativeUnchecked(MemoryAddress base, 
> long byteSize) { ... }
>
>
> This allows clients to pass an address to be used as a 'base', a 
> desired size and obtain a new segment with that base and size. This of 
> course overrides whatever size information might have been previously 
> available in the input address (hence the unsafety). This method can 
> thus create segments which can be used as a target of a 
> MemoryAddress::rebase operation, in case the user wants to break free 
> from the restriction of the Nothing segment.
>
> I went ahead and implemented the rebase+ofNativeUnchecked approach:
>
> http://cr.openjdk.java.net/~mcimadamore/panama/nothing_segment/
>
> I think the results are pleasing; StdLibTest already showcases a 
> couple of calls which require rebasing:
>
> * strcat - result of strcat needs to be rebased against segment 
> associated with first parameter of strcat (safe)
> * qsort - the upcall needs to rebase its incoming addresses against 
> the segment associated with the native array to be sorted (safe)
> * Tm struct - since this struct is constructed out of a pointer 
> returned by a native call (gmtime), best we can do here is to rebase 
> that pointer against a fresh unchecked segment whose size is the 
> (known) size of the Tm struct (unsafe)
>
> When writing the patch, I liked the fact that things failed if the 
> dereference address was 'not trusted' - and I also appreciated the 
> usefulness of veering into unsafe territory, but in a more gentle way 
> which still allows for bounds and liveness checks.
>
> Maurizio
>
> On 27/11/2019 18:32, Ioannis Tsakpinis wrote:
>>> But what if a client really wants to dereference a_random_  pointer
>>> generated by a library? In this case, the client has no well-known
>>> segment which can be used as a rebase target, so, what can be done? 
>>> This
>>> is where we need to veer into restricted native access territory, and
>>> provide some escape hatch by which an untrusted address can be made
>>> trusted again. There are many ways to do that; perhaps the simplest is
>>> to add a new method on MemoryAddress:
>>>
>>> MemoryAddress asUnchecked()
>>>
>>> which returns a new memory address which point to same location as the
>>> original address, but whose access is neither spatially, nor temporally
>>> checked.
>> This doesn't sound so good.:)