[foreign] RFR: simplify implementation classes

Tue May 29 05:18:14 UTC 2018

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...

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
>>>>