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

[Karen Kinnear]

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