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

[Jorn Vernee]

On Wed, 15 Feb 2023 15:50:19 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 one additional commit since the last revision:
> 
>   Fix typo in javadoc

src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 351:

> 349:  * where the size of the array might be provided in a separate parameter. The size of the array is not readily apparent
> 350:  * to the code calling the foreign function and hoping to use its result. In addition to having no insight
> 351:  * into the size of the region of memory backing a pointer returned from a foreign function, also has no insight

Suggestion:

 * into the size of the region of memory backing a pointer returned from a foreign function, it also has no insight

src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 376:

> 374:  * <p>
> 375:  * First, clients can unsafely resize a zero-length memory segment by {@linkplain #reinterpret(long) obtaining} a
> 376:  * memory segment with same base address as the zero-length memory segment, but with the desired size,

Suggestion:

 * memory segment with the same base address as the zero-length memory segment, but with the desired size,

src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 385:

> 383:  *}
> 384:  * <p>
> 385:  * In some cases, a client might additionally want to associate new temporal bounds to a zero-length memory segment.

associate to -> associate with
Suggestion:

 * In some cases, a client might additionally want to associate a zero-length memory segment new temporal bounds.

src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 588:

> 586:      * provided size.
> 587:      * @throws IllegalArgumentException if {@code newSize < 0}.
> 588:      * @throws UnsupportedOperationException if this segment is not a {@linkplain #isNative() native} segment.

This is a nice effect of this patch: it is now harder to mess up and accidentally re-wrap the address() of a heap segment. `reinterpret` has the original segment, and can throw in that case.

src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 2219:

> 2217:          * Returns {@code true}, if the provided object is also a scope, which models the same lifetime as that
> 2218:          * modelled by this scope.
> 2219:          * and the provided scope are the same. In that case, it is always the case that

Seems redundant with the sentence before. I think that should be:
Suggestion:

         * In that case, it is always the case that

src/java.base/share/classes/java/lang/foreign/package-info.java line 201:

> 199:  * foreign data and/or functions to first-class Java API elements which can then be used directly by clients. For instance
> 200:  * the restricted method {@link java.lang.foreign.MemorySegment#reinterpret(long)} ()}
> 201:  * can be used to create a fresh segment with same address and temporal bounds,

Suggestion:

 * can be used to create a fresh segment with the same address and temporal bounds,

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

> 91:             int numSymbols = WindowsFallbackSymbols.values().length;
> 92:             MemorySegment funcs = fallbackLibLookup.find("funcs").orElseThrow()
> 93:                     .reinterpret(ADDRESS.byteSize() * numSymbols);

I realize now that this might be better expressed as a static final `SequenceLayout` in `WindowsFallbackSymbols`, since the number of symbols doesn't change. WDYT?

test/jdk/java/foreign/TestSlices.java line 131:

> 129:             MemorySegment slice = segment.asSlice(0, ValueLayout.JAVA_INT); // size = 4
> 130:             assertThrows(IndexOutOfBoundsException.class, () -> slice.getAtIndex(ValueLayout.JAVA_INT, 1));
> 131:             MemorySegment unbounded = slice.reinterpret(Long.MAX_VALUE, arena.scope(), null);

Looking at this, I'm wondering if the whole test `testUnboundedSlice` is still needed at all, now that `asUnbounded` is removed. I'd say just remove the test.

test/micro/org/openjdk/bench/java/lang/foreign/JavaLayouts.java line 26:

> 24: package org.openjdk.bench.java.lang.foreign;
> 25: 
> 26: import java.lang.foreign.MemoryLayout;

Spurious import?
Suggestion:

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

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