RFR 8079784: Unexpected IllegalAccessError when trying access InnerClasses attribute

[Harold David Seigel]

Hi David,

Thanks for okaying the fix.

I entered an enhancement to look further into this issue: 
https://bugs.openjdk.java.net/browse/JDK-8212056

Harold


On 10/10/2018 5:31 PM, David Holmes wrote:
> Hi Harold,
>
> On 10/10/2018 11:26 PM, Harold David Seigel wrote:
>> Hi David,
>>
>> Fwiw: note that other places in hotspot have a similar sequence to 
>> what is in the proposed fix:
>
> Okay I don't want to hold this fix up but I think we need to be able 
> to answer the question. Maybe it's just defensive incase of weird 
> classloader behaviour as Dean suggested - though I would hope loader 
> constraints to be involved there somewhere.
>
> Thanks,
> David
>
>> 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
>>>>>>>
>>>>>
>>>
>>