[foreign] RFR: simplify implementation classes

Tue May 29 12:07:21 UTC 2018

On 29/05/18 06:18, Samuel Audet wrote:
> That's an interesting restriction indeed. I must admit I have never 
> read that document from start to finish. :) Thanks for the link!
>
> If Panama can eventually support enough C++ such that we don't need 
> any boilerplate wrappers and this gets fixed, then I think we're good.
>
> But in the event this doesn't happen, there should be an easy 
> (standard) way to get classes that need JNI loaded by the system class 
> loader. Is there an existing standard practice for this right now? All 
> I can find are workarounds whose procedures vary from framework to 
> framework...
Not sure if we can call it standard practice, but many of the approaches 
I've come across so far reach out to dlopen directly, bypassing the VM. 
Panama was doing this (with its library loader), JNR is also doing 
something similar:

https://github.com/jnr/jffi/blob/master/src/main/java/com/kenai/jffi/Library.java#L151

Now, one of the benefits of getting C interop right is that we could 
start experimenting at defining some Panama Posix interfaces; if we get 
there, it will be easier for (expert) developers to explicitly handle 
library life-cycle according to the needs of their application.

Maurizio
>
> Samuel
>
> On 05/23/2018 12:48 AM, Henry Jen wrote:
>> We will look into this.
>>
>> There are two type of native libraries, one is JNI(library implements 
>> native functions for Java), the other knows nothing about Java.
>>
>> Security concerns mentioned here, if I understand correctly, applies 
>> to JNI native libraries. For Panama, the target is the later, we want 
>> to be able to load libraries for the binder generated classes and 
>> should load proper version. That’s also means load different versions 
>> of shared library if so required and underlying OS supports it.
>>
>> Cheers,
>> Henry
>>
>>> On May 22, 2018, at 3:16 AM, Maurizio Cimadamore 
>>> <maurizio.cimadamore at oracle.com> wrote:
>>>
>>> Hi Samuel,
>>> loading a native library from two different classloaders is an 
>>> usability issue, I agree. We have looked into it, and the rationale 
>>> behind the introduction of this restriction is that it would be a 
>>> security issue do be able to do otherwise: the runtime type system 
>>> treats classes as a pair of (classname, classloader) - so, if you 
>>> have two types named A from two different classloaders, they are two 
>>> distinct runtime types (their Class are not equals()).
>>>
>>> If you allow a native library to be shared by different 
>>> classloaders, you effectively expose the application to the risk of 
>>> type spoofing - e.g. the native library could consume types coming 
>>> from one classloader, and give them back to the other classloader. 
>>> This would be, needless to say, very undesirable.
>>>
>>> This is explained in this note:
>>>
>>> https://docs.oracle.com/javase/7/docs/technotes/guides/jni/jni-12.html
>>>
>>> (see section "Library and Version Management")
>>>
>>> In other words, what is now perceived as an usability feature, is 
>>> the result of fixing a security hole in the Java platform.
>>>
>>> Now, this doesn't mean that we will never be able to touch this area 
>>> again; but I think any fix we might want to do in this area have to 
>>> take into account the context in which the restriction has come into 
>>> existence. Perhaps (I'm handwaving here) priviliged code in the 
>>> binder would be allowed to load libraries multiple times; after all 
>>> JNI and Panama are different in at least one respect: you can't pass 
>>> arbitrary objects down to native libraries (the interaction between 
>>> Java and native code is always mediated by the binder). But there 
>>> need to be guarantees that the security of the platform would not be 
>>> compromised (and the code with LdLoader/LibraryLoader was not 
>>> providing such guarantees).
>>>
>>> Maurizio
>>>
>>> On 22/05/18 03:19, Samuel Audet wrote:
>>>> Hi, Maurizio,
>>>>
>>>> Do the changes to LdLoader/LibraryLoader help in any way deal with 
>>>> the same library getting loaded by multiple class loaders? With 
>>>> JNI, it's not currently possible to load the same library twice by 
>>>> different class loaders and this kind of problem keeps coming up 
>>>> over and over again. Even projects like sbt and TensorFlow are 
>>>> still scratching their heads about what to do with this:
>>>> https://github.com/tensorflow/tensorflow/issues/19298
>>>>
>>>> IMO, I think that fixing these kinds of usability issues is a lot 
>>>> more important than coming up with an alternative way to call 
>>>> native functions.
>>>>
>>>> I can see 2 ways to fix this:
>>>> 1. Do not try to associate native libraries with class loaders, and 
>>>> provide a System.unload() to let users manage manually the lifetime 
>>>> of native libraries, if they want.
>>>> 2. Instead of throwing exceptions about native libraries being 
>>>> already loaded, increment a reference counter somewhere in the 
>>>> system class loader, and return success on System.load(), from 
>>>> wherever it gets called.
>>>>
>>>> Would there be another way to deal with this? And what is Panama 
>>>> doing about this? These kinds of "little problems" is severely 
>>>> hurting the Java platform and should be fixed in priority over 
>>>> things like what to do with varargs and what not! What do you think?
>>>>
>>>> Samuel
>>>>
>>>> On 05/12/2018 02:15 AM, Maurizio Cimadamore wrote:
>>>>> Hi,
>>>>> it's spring cleaning in Panama-land :-)
>>>>>
>>>>> Before moving forward with the revised design, I thought it would 
>>>>> be better to consolidate the internal implementation a bit. So I 
>>>>> started removing classes that didn't seem necessary - I ended up 
>>>>> with a much bigger changeset than I anticipated, but I think this 
>>>>> is not bad news, in fact I believe this is a big simplification 
>>>>> over what we had. Here's some highlights:
>>>>>
>>>>> * I removed the PLatform class and all its OS-dependent Host 
>>>>> subclasses. The reality is that, at least for now, we only support 
>>>>> SystemV ABI, and little endianness everywhere. So having all these 
>>>>> intermediate steps seemed way too convoluted (and error prone as 
>>>>> we learned earlier this week). Now SystemABI has a getInstance 
>>>>> method which returns the default for that platform. For library 
>>>>> loading, see below.
>>>>>
>>>>> * Errno. This is also gone. Errno should be part of a wider Posix 
>>>>> implementation effort; there's little value in having it there. It 
>>>>> was only used by a test, (UnixSystem) which is easily fixable with 
>>>>> 3-4 lines of code.
>>>>>
>>>>> * LdLoader/LibraryLoader. This is probably the biggest change. As 
>>>>> we've discussed we have a lot of custom logic that 'just' loads 
>>>>> native libraries. In reality the JDK already has code to do this, 
>>>>> but the problem is that it doesn't expose the library entry point 
>>>>> back to the Java code. I looked at how we might massage the JDK 
>>>>> implementation, and it's actually quite easy: ClassLoader stores 
>>>>> loaded libraries in a class called NativeLibrary, which has a 
>>>>> (native) method to lookup entries, etc. We can have this class 
>>>>> implement our nicl/Library interface, and then have 
>>>>> ClassLoader.loadLibrary returns one of these.
>>>>>
>>>>> There's one caveat: when we load  a new library, we need to pass a 
>>>>> class whose loader is used to 'hook' the native library (so that 
>>>>> when the classloader is GCed the native library is unloaded). 
>>>>> Runtime.loadLibrary achieves this with a @CallerSensitive, but 
>>>>> that's only useful if the client calls that code. Here we also 
>>>>> need the binder to call that code - so a saner approach is to pass 
>>>>> a MethodHandles.Lookup - and use the lookup class as the class 
>>>>> whose loader is hooked to the native lib. That works very well.
>>>>>
>>>>> * As a result of the above class loader cleanup, we could get rid 
>>>>> of UnixLibrary and UnixDynamicLibraries too
>>>>>
>>>>> * BindingRegistry: this is not used, and if we need something like 
>>>>> it, it will come back in a different shape
>>>>>
>>>>> * ContainerSizeInfo: this is an interface which is only 
>>>>> implemented by AbstractABI. Seems like a case of premature 
>>>>> abstraction.
>>>>>
>>>>> * UncheckedPointer: this is used to create pointers w/o bounds - 
>>>>> this is just a factory method in BoundedPointer (which happened to 
>>>>> be there already: createNativeVoidPointer).
>>>>>
>>>>> In total, this patch removes 14 classes, and, crucially, it 
>>>>> removes the duplication of the library loading logic. I suspect 
>>>>> further cleanup is also possible in Libraries/LibrariesHelper, but 
>>>>> I'll leave this to Sundar who's more familiar with that code and 
>>>>> its interaction with jextract. (I suspect some of the 
>>>>> SecurityManager checks are not needed anymore, since those would 
>>>>> be carried out by the main class loader logic).
>>>>>
>>>>> Webrev:
>>>>>
>>>>> http://cr.openjdk.java.net/~mcimadamore/panama/cleanup/
>>>>>
>>>>> Cheers
>>>>> Maurizio
>>>>>