RFR 6642881: Improve performance of Class.getClassLoader()

[David Holmes]

On 24/06/2014 11:44 AM, Coleen Phillimore wrote:
>
> On 6/23/14, 9:36 PM, Mandy Chung wrote:
>> Coleen,
>>
>> On 6/23/2014 4:45 PM, Coleen Phillimore wrote:
>>>
>>> Please review a change to the JDK code for adding classLoader field
>>> to the instances of java/lang/Class.  This change restricts
>>> reflection from changing access to the classLoader field.  In the
>>> spec, AccessibleObject.setAccessible() may throw SecurityException if
>>> the accessibility of an object may not be changed:
>>>
>>> http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean)
>>>
>>>
>>> Only AccessibleObject.java has changes from the previous version of
>>> this change.
>>>
>>
>> This change looks reasonable.    As a side note:  Joel mentions about
>> the mechanism to hide class members from reflection.   I discussed
>> with Joel offline before he went on parental leave and suggest that we
>> should revisit the two mechanisms that both effectively disallow
>> access to private members in the future.
>
> Thanks, Mandy.  Yes, let me know what you come up with and we can
> improve this.  Thank you for the help fixing this bug.

Note that completely hiding something from reflection is different to 
controlling its accessability via reflection. I think what is being done 
here is the right way to go.

The changes look okay to me.

Thanks,
David


> Coleen
>
>>
>> Mandy
>>
>>> open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/
>>> bug link https://bugs.openjdk.java.net/browse/JDK-6642881
>>>
>>> Thanks,
>>> Coleen
>>>
>>> On 6/19/14, 11:01 PM, David Holmes wrote:
>>>> On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote:
>>>>> Hi Mandy,
>>>>>
>>>>> On 19 jun 2014, at 22:34, Mandy Chung <mandy.chung at oracle.com> wrote:
>>>>>
>>>>>> On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 19 jun 2014, at 20:46, Coleen Phillimore
>>>>>>> <coleen.phillimore at oracle.com> wrote:
>>>>>>>> On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote:
>>>>>>>>> Have you considered hiding the Class.classLoader field from
>>>>>>>>> reflection? I’m not sure it is necessary but I’m not to keen on
>>>>>>>>> the idea of people poking at this field with Unsafe (which
>>>>>>>>> should go away in 9 but …).
>>>>>>>> I don't know how to hide the field from reflection. It's a
>>>>>>>> private final field so you need to get priviledges to make it
>>>>>>>> setAccessible.  If you mean injecting it on the JVM side, the
>>>>>>>> reason for this change is so that the JIT compilers can inline
>>>>>>>> this call and not have to call into the JVM to get the class
>>>>>>>> loader.
>>>>>>>>
>>>>>>> There is sun.reflect.Reflection.registerFieldsToFilter() that
>>>>>>> hides a field from being found using reflection. It might very
>>>>>>> well be the case that private and final is enough, I haven’t
>>>>>>> thought this through 100%. On the other hand, is there a reason
>>>>>>> to give users access through the field instead of having to use
>>>>>>> Class.getClassLoader()?
>>>>>>>
>>>>>> There are many getter methods that returns a private final field.
>>>>>> I'm not sure if hiding the field is necessary nor a good precedence
>>>>>> to set. Accessing a private field requires "accessDeclaredMember"
>>>>>> permission although it's a different security check (vs
>>>>>> "getClassLoader"
>>>>>> permission).  Arguably before this new classLoader field, one could
>>>>>> call Class.getClassLoader0() via reflection to get a hold of class
>>>>>> loader.
>>>>>>
>>>>>> Perhaps you are concerned that the "accessDeclaredMember" permission
>>>>>> is too coarse-grained?  I think the security team is looking into
>>>>>> the improvement in that regards.
>>>>>
>>>>> I think I’m a bit worried about two things, first as you wrote,
>>>>> “accessDeclaredMember” isn’t the same as “getClassLoader”, but
>>>>> since you could get the class loader through getClassLoader0() that
>>>>> shouldn’t be a new issue.
>>>>>
>>>>> The second thing is that IIRC there are ways to set a final field
>>>>> after initialization. I’m not sure we need to care about that
>>>>> either if you need Unsafe to do it.
>>>>
>>>> Normal reflection can set a final field if you can successfully call
>>>> setAccessible(true) on the Field object.
>>>>
>>>> David
>>>> -----
>>>>
>>>>
>>>>> cheers
>>>>> /Joel
>>>>>
>>>
>>
>