RFR 8079784: Unexpected IllegalAccessError when trying access InnerClasses attribute

[David Holmes]

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