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

[David Holmes]

Forgot to add webrev.02 looks fine to me too.

Thanks,
David
-----

On 6/10/2017 11:15 AM, David Holmes wrote:
> 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
>>>