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

[Jorn Vernee]

On Wed, 15 Feb 2023 19:16:45 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.
>
> Maurizio Cimadamore has updated the pull request incrementally with five additional commits since the last revision:
> 
>  - Address review comments
>  - Update test/micro/org/openjdk/bench/java/lang/foreign/JavaLayouts.java
>    
>    Co-authored-by: Jorn Vernee <JornVernee at users.noreply.github.com>
>  - Update src/java.base/share/classes/java/lang/foreign/package-info.java
>    
>    Co-authored-by: Jorn Vernee <JornVernee at users.noreply.github.com>
>  - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java
>    
>    Co-authored-by: Jorn Vernee <JornVernee at users.noreply.github.com>
>  - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java
>    
>    Co-authored-by: Jorn Vernee <JornVernee at users.noreply.github.com>

Marked as reviewed by jvernee (Committer).

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

> 88: 
> 89:             MemorySegment funcs = fallbackLibLookup.find("funcs").orElseThrow()
> 90:                     .reinterpret(WindowsFallbackSymbols.LAYOUT.byteSize());

Nice! Thanks.

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

> 205:         }
> 206: 
> 207:         static SequenceLayout LAYOUT = MemoryLayout.sequenceLayout(

Could be `final` as well.

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

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