RFR 8079784: Unexpected IllegalAccessError when trying access InnerClasses attribute

[David Holmes]

On 11/10/2018 3:27 AM, dean.long at oracle.com wrote:
> On 10/10/18 5: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?
>>
> 
> Do we necessarily know that inner's defining classloader is the same as 
> outer's defining classloader here?  It seems like they could be 
> different if outer's classloader is not well-behaved.  Even so, loader 
> constraints should make sure that outer and inner resolve the names 
> Outer and Outer$Inner to the same klasses, respectively, right?

That's what I'm trying to figure out. :) I suppose we could have 
something like:

Class<?> c1 = loader1.loadClass("Outer");
Class<?> c2 = loader2.loadClass("Outer$Inner");

where the two classes are completely unrelated and then we do:

c1.<some operation>(c2);

which results in the code in question being executed.

David

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