RFR 8079784: Unexpected IllegalAccessError when trying access InnerClasses attribute

[Harold David Seigel]

Hi David,

Fwiw: note that other places in hotspot have a similar sequence to what 
is in the proposed fix:

Lines 769 - 774 of method Reflection::check_for_inner_class():

         if (!inner_is_member && ioff != 0 && ooff == 0 &&
             cp->klass_name_at_matches(inner, ioff)) {
           Klass* i = cp->klass_at(ioff, CHECK);
           if (i == inner) {
             return;
           }
         }
       }

And, in InstanceKlass.cpp:

    bool InstanceKlass::find_inner_classes_attr(int* ooff, int* noff,
    TRAPS) const {
       constantPoolHandle i_cp(THREAD, constants());
       for (InnerClassesIterator iter(this); !iter.done(); iter.next()) {
         int ioff = iter.inner_class_info_index();
         if (ioff != 0) {
           // Check to see if the name matches the class we're looking for
           // before attempting to find the class.
           if (i_cp->klass_name_at_matches(this, ioff)) {
             Klass* inner_klass = i_cp->klass_at(ioff, CHECK_false);
             if (this == inner_klass) {

How about if I push this fix and then open an RFE to look at these cases 
to determine if the calls to 'cp->klass_at() and the subsequent 'if' 
statements are needed?

Thanks, Harold


On 10/10/2018 8:42 AM, Harold David Seigel wrote:
> Hi David,
>
> Would you prefer that I remove the "if (o == outer)" and "if (i == 
> inner)" checks?
>
> Thanks, Harold
>
>
> On 10/10/2018 8:35 AM, David Holmes wrote:
>> On 10/10/2018 9:48 PM, Harold David Seigel wrote:
>>> Hi David,
>>>
>>> Thanks for the review!
>>>
>>> Please see comments inline.
>>>
>>>
>>> On 10/9/2018 9:35 PM, David Holmes wrote:
>>>> Hi Harold,
>>>>
>>>> Looks fine as-is I have one query below ...
>>>>
>>>> One additional nit in jasm file - this purported line of source 
>>>> code is not correct:
>>>>
>>>>  60 //         Class<?> clazz = Buggered.Foo.class;
>>>>
>>>> as you aren't using that class.
>>> The test program was originally called Buggered.  I changed the name 
>>> when creating the JTReg test but forgot to update the comment.  I'll 
>>> fix it before pushing.
>>>>
>>>> On 10/10/2018 12:12 AM, Harold David Seigel wrote:
>>>>> Hi,
>>>>>
>>>>> Please review this fix, proposed by Doug Simon, for JDK-8079784. 
>>>>> The fix prevents classes in the InnerClasses attribute from being 
>>>>> loaded unless they are actually being accessed.
>>>>
>>>> That in itself seems reasonable. I'm surprised we don't do more 
>>>> sanity checking on the classes listed in the inner classes 
>>>> attribute - I would have expected at least a "same package" check.
>>>>
>>>> Looking at the code itself:
>>>>
>>>>       if (inner_is_member && ioff != 0 && ooff != 0) {
>>>> +       if (cp->klass_name_at_matches(outer, ooff) &&
>>>> +           cp->klass_name_at_matches(inner, ioff)) {
>>>>           Klass* o = cp->klass_at(ooff, CHECK);
>>>>           if (o == outer) {
>>>>             Klass* i = cp->klass_at(ioff, CHECK);
>>>>             if (i == inner) {
>>>>               return;
>>>>             }
>>>>           }
>>>>         }
>>>> +     }
>>>>
>>>> I'm wondering how it is possible to have the names match and yet 
>>>> potentially o!=outer and i!=inner ?
>>> Just guessing here but klass_at() resolves the class. So potentially 
>>> it could resolve to a different class with the same name.
>>
>> Yes but how could that be possible? How did you pass in a reference 
>> to a class with a given name which must recognizable by that name 
>> within the current classloader, yet when you resolve the CP entry - 
>> still in the current classloader - you somehow get a different class?
>>
>> I had a similar check in the nestmates code and it was considered 
>> impossible and so removed.
>>
>> Cheers,
>> David
>>
>>>>
>>>>> Also, while looking into this issue, I noticed that method 
>>>>> is_same_package_member() is not used. So, I removed it as part of 
>>>>> this webrev.
>>>>
>>>> In 8u it's called by Reflection::is_same_package_member, which in 
>>>> turn is unused. That was removed in 9 by the cleanup done in 
>>>> JDK-8140485. :)
>>> Hopefully, it's gone for good now.
>>>
>>> Thanks! Harold
>>>>
>>>> Thanks,
>>>> David
>>>>
>>>>>
>>>>> Open Webrev: http://cr.openjdk.java.net/~hseigel/bug_8079784/webrev/
>>>>>
>>>>> JBS Bug:  https://bugs.openjdk.java.net/browse/JDK-8079784
>>>>>
>>>>> The fix was tested with the test in the webrev and by running 
>>>>> Mach5 tiers 1 and 2 tests and builds on Linux-x64, Windows, and 
>>>>> Mac OS X, running tiers 3-5 tests on Linux-x64, and by running 
>>>>> JCK-12 Lang and VM tests on Linux-x64.
>>>>>
>>>>> Thanks, Harold
>>>>>
>>>
>