RFR 8209184: JDK8 ResourceBundle vulnerable to GC (fix included)

[Adam Farley8]

Hi Mandy, Hans,

Tried out the below (after removing my previous fix, the volatile 
compare), and it appears to solve the problem just fine. 

If I generate a webrev and get it attached to the bug, would one of you 
mind approving the change?

----------...at the end of getBundleImpl in jdk8  ------------
        keepAlive(loader);
        return bundle;
}

//To keep the argument ClassLoader alive.
private static void keepAlive(ClassLoader loaderone){
            //Do nothing.
}
----------------------

Best Regards

Adam Farley 

Hans Boehm <hboehm at google.com> wrote on 15/08/2018 06:44:08:

> From: Hans Boehm <hboehm at google.com>
> To: Mandy Chung <mandy.chung at oracle.com>
> Cc: adam.farley at uk.ibm.com, core-libs-dev <core-libs-
> dev at openjdk.java.net>, i18n-dev at openjdk.java.net
> Date: 15/08/2018 06:44
> Subject: Re: RFR 8209184: JDK8 ResourceBundle vulnerable to GC (fix 
included)
> 
> FWIW, since there was agreement that empty static methods should 
> work in this context, that seems like the best option to me.
> 
> On Tue, Aug 14, 2018 at 4:54 PM mandy chung <mandy.chung at oracle.com> 
wrote:
> Thanks for the information.  Not sure what's the best option we can do
> in 8u.  I think it's acceptable to have a fix that works in the
> current context (like empty static method).
> 
> Thoughts?
> 
> Mandy
> 
> On 8/14/18 4:22 PM, Hans Boehm wrote:
> > I haven't looked at the details here, but comparing against a volatile 

> > is not in general a portable way to keep an object live. Like empty 
> > static methods, it may be fine in the current (OpenJDK) context.
> > 
> > Volatile loads have roach motel semantics and can be advanced past 
other 
> > operations. The comparison can be done early, too, keeping only the 
> > condition code. There is no guarantee the reference will still be 
around 
> > when you expect it.
> > 
> > I haven't come up with a great compiler-independent replacement for 
> > reachabilityFence.
> > 
> > On Tue, Aug 14, 2018 at 8:25 AM mandy chung <mandy.chung at oracle.com 
> > <mailto:mandy.chung at oracle.com>> wrote:
> > 
> >     Hi Adam,
> > 
> >     Have you tried Peter's suggestion if an empty static method taking 
an
> >     Object parameter?  Does it work for you?
> > 
> >     Your proposed approach seems fine and I would suggest to put the
> >     check in a static keepAlive method that will make it explicitly.
> > 
> >     Mandy
> > 
> >     On 8/10/18 8:42 AM, Adam Farley8 wrote:
> >      > Hi All,
> >      >
> >      > This bug could be fixed by comparing the Class Loader with a 
blank
> >      > static volatile Object (defined outside the method scope) at 
the
> >      > end of the getBundleImpl class.
> >      >
> >      > E.g.
> >      >
> >      > -----------------------------------------
> >      > +1322
> >      >      /**
> >      >       * volatile reference object to guard the ClassLoader 
object
> >      >       * being garbage collected before getBundleImpl() method
> >     completes
> >      >       * the caching and retrieving of requested Resourcebundle 
object
> >      >       */
> >      >      private static volatile Object vo = new Object();
> >      >
> >      >
> >      > +1400
> >      > //Should never be true. Using the loader here prevents it being 
GC'd.
> >      >              if (loader == vo) throw new Error("Unexpected 
error.");
> >      > -----------------------------------------
> >      >
> >      > Will upload a webrev after debate.
> >      >
> >      > - Adam
> >      > Unless stated otherwise above:
> >      > IBM United Kingdom Limited - Registered in England and Wales 
with
> >     number
> >      > 741598.
> >      > Registered office: PO Box 41, North Harbour, Portsmouth,
> >     Hampshire PO6 3AU
> >      >
> > 
Unless stated otherwise above:
IBM United Kingdom Limited - Registered in England and Wales with number 
741598. 
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU