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

[Karen Kinnear]

Volker,

Thank you so much for the extra round of changes. This looks good, and in fact looks
cleaner to me.

Thank you Coleen for sponsoring the change.

thanks,
Karen

> On Feb 10, 2017, at 3:25 PM, Coleen Phillimore <coleen.phillimore at oracle.com> wrote:
> 
> 
> 
> 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