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

[Volker Simonis]

Coleen, Karen, thanks for your support!
Volker

Karen Kinnear <karen.kinnear at oracle.com> schrieb am Fr., 10. Feb. 2017 um
23:12:

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