RFR 8079784: Unexpected IllegalAccessError when trying access InnerClasses attribute

[Harold David Seigel]

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