[foreign] RFR: simplify implementation classes

Tue May 22 10:16:54 UTC 2018

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