RFR(S): 8173743: Failures during class definition can lead to memory leaks in metaspace

[Coleen Phillimore]

On 2/10/17 11:39 AM, Volker Simonis wrote:
> On Thu, Feb 9, 2017 at 7:45 PM, Karen Kinnear <karen.kinnear at oracle.com>
> wrote:
>
>> Volker,
>>
>> I agree that today I do not see any path that calls
>> find_or_define_instance_class
>> or define_class for the boot loader that does not go through
>> resolve_instance_class_or_null.
>>
>> In closed code, for other class loaders, there are other calls directly
>> into class definition today.
>> My concern is that if we do not cover this hole, someone will climb into
>> it in the future
>> with an alternative path to defining a class that does not go through a
>> resolve/load class
>> path first. So I would recommend adding the extra check while you are
>> studying the
>> code so carefully.
>>
>>
> I think you are right. Better to be on the safe side and the extra check
> doesn't do any harm. Please find the new version of the change here:
>
> http://cr.openjdk.java.net/~simonis/webrevs/2017/8173743.v3/
>
> It is a little tricky because we have to register the instance klass for
> deallocation in two cases. First if the newly defined klass differs form
> the original one and second if the call to find_or_define_instance_class
> returned with a pending exception.
>
> I've also slightly changed the condition in resolve_from_stream(). Instead
> of checking for (defined_k.not_null() && defined_k() != k()) I now check
> for (!HAS_PENDING_EXCEPTION && defined_k() != k()) and I've put in an
> assertion for defined_k.not_null(). I think find_or_define_instance_class()
> should always return a valid klass if it doesn't return with a pending
> exception.
>
> If you (and the other reviewers) are fine with this version, please feel
> free to sponsor it.

This change looks fine to me and I will sponsor it, if Karen agrees that 
this change looks good.

Thanks,
Coleen
>
> Thank you and best regards,
> Volker
>
> PS: I've again tested this successfully with the JPRT which is running
> behind the jdk9/hs-demo-submit forest
>
>
> I am ok with the code as is if you prefer.
>> thanks,
>> Karen
>>
>>> On Feb 8, 2017, at 2:42 PM, Volker Simonis <volker.simonis at gmail.com>
>> wrote:
>>>>
>>>> A couple of notes:
>>>> 1) \ I think there is also a need for a check
>>>> inside load_instance_klass for the null loader case, i.e. after the
>> call to find_or_define_instance_class.
>>> This does indeed look like another place which needs fixing. I've
>>> tried to come up with a test case, but didn't succeed.
>>> I've than looked at the source code and observed the following:
>>>
>>> - the place you mention inside load_instance_class() is only relevant
>>> for the null (i.e. bootstrap) loader
>>>
>>> - SystemDictionary::resolve_instance_class_or_null(), which is the
>>> caller of load_instance_class() has the following comments and code:
>>>
>>>     // add placeholder entry to record loading instance class
>>>     // Five cases:
>>>     ...
>>>     // case4. Bootstrap classloader - don't own objectLocker
>>>     //    This classloader supports parallelism at the classloader level,
>>>     //    but only allows a single load of a class/classloader pair.
>>>     //    No performance benefit and no deadlock issues.
>>>     ...
>>>               // case 4: bootstrap classloader: prevent futile
>> classloading,
>>>               // wait on first requestor
>>>               if (class_loader.is_null()) {
>>>                 SystemDictionary_lock->wait();
>>>
>>> So it looks to me like  find_or_define_instance_class() can not return
>>> a different instanceKlass for the bootstrap class loader.
>>>
>>> What do you think? Did I missed something? If yes, do you have any
>>> idea how this error could be triggered with a test case?
>>>
>>> Do you want me do add a check for  "defined_k == k" anyway (for any
>> case)?
>>> I've uploaded a new webrev which contains the changes (only for the
>>> test) for all the suggestions I've received until now:
>>>
>>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8173743.v2/
>>>
>>> Thanks,
>>> Volker
>>