RFR 8079784: Unexpected IllegalAccessError when trying access InnerClasses attribute (original) (raw)

Harold David Seigel harold.seigel at oracle.com
Thu Oct 11 15:52:41 UTC 2018


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::checkforinnerclass(): if (!innerismember && ioff != 0 && ooff == 0 && cp->klassnameatmatches(inner, ioff)) { Klass* i = cp->klassat(ioff, CHECK); if (i == inner) { return; } } } And, in InstanceKlass.cpp: bool InstanceKlass::findinnerclassesattr(int* ooff, int* noff, TRAPS) const { constantPoolHandle icp(THREAD, constants()); for (InnerClassesIterator iter(this); !iter.done(); iter.next()) { int ioff = iter.innerclassinfoindex(); if (ioff != 0) { // Check to see if the name matches the class we're looking for // before attempting to find the class. if (icp->klassnameatmatches(this, ioff)) { Klass* innerklass = icp->klassat(ioff, CHECKfalse); if (this == innerklass) { How about if I push this fix and then open an RFE to look at these cases to determine if the calls to 'cp->klassat() 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 (innerismember && ioff != 0 && ooff != 0) { +       if (cp->klassnameatmatches(outer, ooff) && +           cp->klassnameatmatches(inner, ioff)) { Klass* o = cp->klassat(ooff, CHECK); if (o == outer) { Klass* i = cp->klassat(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 klassat() 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 issamepackagemember() is not used. So, I removed it as part of this webrev. In 8u it's called by Reflection::issamepackagemember, 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/bug8079784/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



More information about the hotspot-runtime-dev mailing list