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

[Lois Foltan]

On 10/30/2018 6:40 PM, coleen.phillimore at oracle.com wrote:

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

Thank you Coleen for the review!  Testing: hs-tier1-4 and jdk-tier1-3 
have completed successfully.
Lois

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