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

[coleen.phillimore at oracle.com]

On 10/30/18 2:13 PM, Lois Foltan wrote:
> On 10/30/2018 11:01 AM, Severin Gehwolf wrote:
>
>> 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/

This change looks good to me.  I'd prefer not calling register_loader() 
when one is only needed to produce an error message.  This also avoids 
the lock inversion problem.

Thanks,
Coleen

>> FWIW, I've tested this webrev with our reproducer and this fix works
>> for us.
>
> Thank you Severin for verifying this!
> Lois
>
>>
>> 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
>>>>
>>>>
>>>
>