RFR 8212937: Parent class loader may not have a referred ClassLoaderData instance when obtained in Klass::class_in_module_of_loader

[Severin Gehwolf]

Hi Lois,

On Mon, 2018-10-29 at 15:47 -0400, Lois Foltan wrote:
> On 10/25/2018 11:15 AM, Martin Balao wrote:
> 
> > Hi Lois,
> > 
> > Thanks for having a look at this.
> > 
> > Yes, I have a test that triggers this execution path. Unfortunately, 
> > this is a security test that I cannot publicly share. What I can tell 
> > you, though, is that the parent class loader is a "delegating class 
> > loader" which never defined a class and that's the reason why it 
> > doesn't have a ClassLoaderData instance created.
> > 
> > I'm not sure if this would be needed in other places where the idiom 
> > is not used. The reason is that there might be any other implicit 
> > assumptions that makes it impossible. That's why I limited the scope 
> > of this changeset to the only case that I'm sure it's possible to trigger.
> 
> Hi Martin,
> 
> I wrote a test case that reproduces this failure and demonstrates that 
> invoking SystemDictionary::register_loader() will not work. With a 
> non-product (fastdebug) build, the test case shows that calling 
> register_loader is problematic, a SystemDictionary lock is currently 
> held and acquiring a ClassLoaderDataGraph lock to create the new 
> ClassLoaderData causes an out of order locking fatal error.
> 
> Please consider this test case and proposed change:
>      http://cr.openjdk.java.net/~lfoltan/bug_jdk8212937/webrev/

FWIW, I've tested this webrev with our reproducer and this fix works
for us.

Thanks,
Severin

> > Kind regards,
> > Martin.-
> > 
> > On Wed, Oct 24, 2018 at 5:52 PM, Lois Foltan <lois.foltan at oracle.com 
> > <mailto:lois.foltan at oracle.com>> wrote:
> > 
> >     On 10/24/2018 3:58 PM, Martin Balao wrote:
> > 
> >         Hi,
> > 
> >         Can I have a review for JDK-8212937 [1]?
> > 
> >         Webrev.00:
> > 
> >           *
> >         http://cr.openjdk.java.net/~mbalao/webrevs/8212937/8212937.webrev.00/
> >         <http://cr.openjdk.java.net/%7Embalao/webrevs/8212937/8212937.webrev.00/>
> >           *
> >         http://cr.openjdk.java.net/~mbalao/webrevs/8212937/8212937.webrev.00.zip
> >         <http://cr.openjdk.java.net/%7Embalao/webrevs/8212937/8212937.webrev.00.zip>
> > 
> >         By obtaining the ClassLoaderData instance through a call to
> >         SystemDictionary::register_loader, we make sure that either an
> >         existing
> >         instance is obtained or a new one created. This same idiom has
> >         already been
> >         used in other places where there are no guarantees that a previous
> >         ClassLoaderData instance exists for a given class loader. See
> >         futher
> >         information in JDK-8212937 ticket [1].
> > 
> >         Thanks,
> >         Martin.-
> > 
> >         --
> >         [1] - https://bugs.openjdk.java.net/browse/JDK-8212937
> >         <https://bugs.openjdk.java.net/browse/JDK-8212937>
> > 
> >     Hi Martin,
> > 
> >     I agree that the same idiom is used in other places, but there are
> >     also a couple of occurrences of java_lang_ClassLoader::parent in
> >     classfile/classLoaderStats.cpp and
> >     classfile/classLoaderHierarchyDCmd.cpp that do not.  In order to
> >     better understand, do you have a test case that demonstrates this
> >     issue?
> > 
> >     Thanks,
> >     Lois
> > 
> > 
> 
>