[foreign-memaccess+abi] RFR: 8302556: Find better way to create unsafe native segments

[Maurizio Cimadamore]

On Wed, 15 Feb 2023 15:10:42 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

> Consider the following code, which is quite common when trying to retroactively secure memory segments originating in native code:
> 
> 
> MemorySegment malloc(long size, Arena arena) {
>      MemorySegment raw = <MALLOC_HANDLE>.invokeExact(size);
>      return MemorySegment.ofAddress(raw.address(), size, arena, () -> StdLib.free(raw));
> }
> 
> 
> There are few issues with this code:
> 
> * Clients need a throwaway local variable (`raw`) where to store the raw segment returned from the linker API;
> * to create a new segment with `MemorySegment::ofAddress`, one has to retrieve the address from the old segment and pass it to the factory (see call to `raw.address()`);
> * the cleanup action has to carefully refer to the `raw` segment - that's because the new segment will be invalidated when the arena is closed, so only `raw` is guaranteed to still be alive at that point.
> 
> This patch introduces an API change to cope with the issues above. More specifically, this patch introduces a new *instance* method, namely `MemorySegment::reinterpret` which can be used to customize an existing native segment by giving it a new size and scope. `MemorySegment::ofAddress` is still there, but this patch removes all the overloads: effectively now `ofAddress` is only used to turn a `long` into a (zero-length) native memory segment.
> 
> With the changes in this patch the above code becomes:
> 
> 
> MemorySegment malloc(long size, Arena arena) {
>      return <MALLOC_HANDLE>.invokeExact(size);
>                            .reinterpret(size, arena.scope(), StdLib::free);
> }
> 
> 
> Not only the new code is more "fluent" than the old one (no extra local variable declaration required), but the story for attaching custom cleanup action has been improved significantly: instead of just taking a `Runnable` the new API takes a `Consumer<MemorySegment>` which is passed a _fresh_ zero-length memory segment whose scope is always alive (in other words, an _alias_ of the memory segment to be cleaned up). This seems acceptable for a restricted method that is meant to be used in very specific situations, and leads to much cleaner code.
> 
> There are several overloads for `reinterpret`, to reset size, scope and both size *and* scope. Note that, since `reinterpret` accepts a scope (and not just an `Arena`), it is possible to use this method to rescue the co-allocation use case (albeit at a lower level) - that is, when reading a pointer from another memory segment, a client could set the scope on the read segment to be the same as that of the containing segment.

Sample javadoc:
http://cr.openjdk.java.net/~mcimadamore/panama/segment_reinterpret_v2/

src/java.base/share/classes/jdk/internal/foreign/SystemLookup.java line 50:

> 48: 
> 49:     /* A fallback lookup, used when creation of system lookup fails. */
> 50:     private static final SymbolLookup FALLBACK_LOOKUP = name -> {

I realized that some of these lookups were not checking for null values - so I've fixed that here (since I was touching the code anyways).

src/java.base/share/classes/jdk/internal/foreign/abi/UpcallStubs.java line 60:

> 58:             }
> 59:         });
> 60:         return MemorySegment.ofAddress(entry).reinterpret(arena.scope(), null);

In principle we could just express this using public API if we tweak the semantics for cleanup action to always run in case the scope is found to be already closed.

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

PR: https://git.openjdk.org/panama-foreign/pull/797