Review Request JDK-8188052: JNI_FindClass needs to specify the class loading context used for library lifecycle hooks

[David Holmes]

Hi Mandy,

On 6/10/2017 10:57 AM, David Holmes wrote:
> On 6/10/2017 7:56 AM, mandy chung wrote:
>> On 10/5/17 1:13 PM, coleen.phillimore at oracle.com wrote:
>>>
>>> I should say, besides these style comments this change looks good.
>>> Coleen
>>>
>>> On 10/5/17 3:28 PM, coleen.phillimore at oracle.com wrote:
>>>>
>>>> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8188052/webrev.00/src/hotspot/share/prims/jni.cpp.udiff.html 
>>>>
>>>>
>>>> The type Klass has a class_loader() and protection_domain() 
>>>> accessors so it is unnecessary to cast to InstanceKlass.  We went 
>>>> through and removed many of these unnecessary InstanceKlass::casts.
>>>>
>>
>> That's from existing code which may not be touched for a long time. 
>> Since I'm on this file, I can clean this up.
>>>> 407     if (loader.is_null() &&
>>>>
>>
>> - if (loader.is_null() &&
>> + if (k->class_loader() == NULL && Already fixed per David's comment.
>>
>>>> I agree with David's comment that this line will always evaluate to 
>>>> true so should be cleaned up also.
>>>>
>>>> 413                              thread);
>>>>  414       if (HAS_PENDING_EXCEPTION) {
>>>>  415         Handle ex(thread, thread->pending_exception());
>>>>  416         CLEAR_PENDING_EXCEPTION;
>>>>  417         THROW_HANDLE_0(ex);
>>>>  418       }
>>>>
>>>>
>>>> This is strange too.  Why doesn't this just pass CHECK_NULL at line 
>>>> 413, which is the same thing as checking for and clearing and 
>>>> throwing the pending exception?
>>>>
>>
>> This is existing code.   I removed these lines and pass CHECK_NULL to 
>> call_static.
> 
> Which, by the way, highlights the existing incorrect use of '0' instead 
> of NULL in this code.
> 
>> How does this version look?
>> http://cr.openjdk.java.net/~mchung/jdk10/webrevs/8188052/webrev.02/src/hotspot/share/prims/jni.cpp.sdiff.html 
>>
>>
>> One other thing I notice: it only sets the protection_domain to the 
>> caller class when FindClass is called from JNI_OnLoad/JNI_OnUnload.
>>
>> For other caller class, it uses null protection_domain. I would expect 
>> it should use the caller's protection domain  to look up a class.  I 
>> think it's a bug, isn't?
> 
> Neither Class.forName nor ClassLoader.loadClass take ProtectionDomains 
> as arguments, so I think the "normal" behaviour of FindClass to not use 
> a PD is correct. That of course begs the question as to why the 
> OnLoad/OnUnload contexts are special. I don't have an answer but can do 
> some research.

The PD change relates to a very old security bug:

https://bugs.openjdk.java.net/browse/JDK-4288452 (not public)

Can't say any more other than "it ain't broken, so don't 'fix' it".

Cheers,
David

> Thanks,
> David
> 
>> Mandy
>>