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

[Volker Simonis]

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.

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