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

[Lois Foltan]

Thank you Martin for the review!
Lois

On 10/30/2018 9:33 PM, Martin Balao wrote:

> Hi Lois,
>
> Thanks for your proposal. I'm not a reviewer but it looks good to me too.
>
> Kind regards,
> Martin.-
>
> On Mon, Oct 29, 2018 at 4:47 PM, Lois Foltan <lois.foltan at oracle.com 
> <mailto:lois.foltan at oracle.com>> 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/
>     <http://cr.openjdk.java.net/%7Elfoltan/bug_jdk8212937/webrev/>
>
>     Thanks,
>     Lois
>
>>
>>     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
>>
>>
>
>