RFR 8079784: Unexpected IllegalAccessError when trying access InnerClasses attribute

[David Holmes]

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
>>>>>>
>>>>
>>
>