[foreign-memaccess] RFR: Preserve memory scope for buffer segments

[Maurizio Cimadamore]

On Thu, 16 Apr 2020 12:50:04 GMT, Jorn Vernee <jvernee at openjdk.org> wrote:

>> Not 100% sure about this, but wanted to share this PR for evaluation. Basically, I've realized that we are also not
>> preserving the memory scope of the original segment when we do a segment -> buffer -> segment transition.
>> This means that, e.g. code like this:
>> 
>> MemorySegment segment = MemorySegment.allocateNative(16);
>> segment = MemorySegment.ofByteBuffer(segment.asByteBuffer());
>> segment.close(); // no cleanup of native memory
>> 
>> will not really 'close' the original segment. Similarly, if the original segment is being worked upon by some other
>> thread (through a spliterator) it is possible for the morphed segment to lose this info, and to allow a close - if the
>> scope is created afresh (as is now).  This patch fixes this, but I'm torn as to whether it makes things better;
>> preserving access modes and thread ownership is mostly about not allowing clients to use the byte buffer escape hatch
>> as a way to defy the segment restrictions. I don't think preserving scope falls into the same bucket - e.g. this is a
>> separate question as to whether a segment S2 obtained through S1 -> BB -> S2 is just a *view* of S1 (with same temporal
>> bounds), or is just an independent segment, with new temporal bounds. I think both answers are legitimate, and it's
>> mostly a question of making up our mind.  My take is that inferring scope like this can be seen a bit too magic - and
>> you are never sure of what happens when you do a MemorySegment::close, so perhaps what we have w/o this patch is
>> preferrable. What do you think?
>
> src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 397:
> 
>> 396:
>> 397:         AbstractMemorySegmentImpl bufferSegment = (AbstractMemorySegmentImpl)nioAccess.bufferSegment(bb);
>> 398:         final MemoryScope bufferScope;
> 
> Wondering if we can't just return `bufferSegment` here? It should have the write type right? I think the only thing
> we'd need to do is drop the WRITE access mode if `bb` is readOnly.

uhmmm - not quite - the buffer could have been sliced?

> Wrt your concerns. I think we need to preserve the scope to prevent dead accesses. Consider this example:
> 
> ```
>         MemorySegment s1 = MemorySegment.allocateNative(intArrayLayout);
>         MemorySegment s2 = MemorySegment.ofByteBuffer(s1.asByteBuffer());
> 
>         s1.close(); // memory freed
> 
>         intElemHandle.set(s2.baseAddress(), (long) 0, 10); // Dead access!
> ```
> 
> It fails with this patch, but wrongly succeeds without it.

Very good point. Aliasing is an issue w.r.t. closing.

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

PR: https://git.openjdk.java.net/panama-foreign/pull/113