[foreign-memaccess+abi] RFR: 8264386: LibraryLookup should be more friendly with implicit unloading [v2]

[Jorn Vernee]

On Mon, 29 Mar 2021 15:45:06 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

>> Quoting from the PR:
>> 
>> Dereferencing a segment from a LibraryLookup is possible, in the following form:
>> 
>> LibraryLookup libLookup = ...
>> MemorySegment segment = libLookup.lookup("foo").asSegmentRestricted(....)
>> 
>> The above code has a problem: there is no link between the produced segment and the original library lookup - meaning that, in principle, it's possible for the library to be unloaded when dereference occurs.
>> 
>> After having discussed many alternatives with Jorn, the best way to address this conundrum would be to add an overload of `LibraryLookup::lookup` which takes a lyout and returns a segment.
>> 
>> As part of this change I have also removed the LibraryLookup.Symbol API which seems unnecessary - now you can lookup an address or a segment. Of course, looking up a segment is creating a restricted segment (so it should be treated as unsafe).
>> 
>> I've added a test to make sure that the new API works as expected.
>
> Maurizio Cimadamore has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains three additional commits since the last revision:
> 
>  - Add @NativeAccess annotations and @CallerSensitive check on LibraryLookup::lookup(String, MemoryLayout)
>  - Merge branch 'foreign-memaccess+abi' into resourceScope+libraries
>  - Initial push

LGTM, apart from the native access checks (see comments inline).

src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/LibraryLookup.java line 69:

> 67:     /**
> 68:      * Looks up a symbol with given name in this library. The returned memory address maintains a strong reference to this lookup object.
> 69:      * @param name the symbol name. This method can be useful to lookup function symbols in a foreign library.

"This method can be useful to lookup function symbols in a foreign library." seems like this wasn't meant to be put on the parameter?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/LibrariesHelper.java line 119:

> 117:         @Override
> 118:         public Optional<MemoryAddress> lookup(String name) {
> 119:             try {

I think this is missing the restricted access check?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/LibrariesHelper.java line 131:

> 129:         @CallerSensitive
> 130:         public Optional<MemorySegment> lookup(String name, MemoryLayout layout) {
> 131:             Reflection.ensureNativeAccess(Reflection.getCallerClass());

Should this method and the other overload have the `@NativeAccess` annotation?

src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/LibrariesHelper.java line 140:

> 138:                 }
> 139:                 return Optional.of(librarySegment.asSlice(addr)
> 140:                         .asSlice(0L, layout.byteSize()));

Why not simply one `asSlice` call with `addr` and `layout.byteSize()` ?

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

Marked as reviewed by jvernee (Committer).

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