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