<i18n dev> RFR 8209184: JDK8 ResourceBundle vulnerable to GC (fix included)

[Adam Farley8]

Hi Mandy,

I request that you review this for 8u.

------------------------------------------------------
@@ -1398,10 +1398,18 @@
             bundle = baseBundle;
         }

+        keepAlive(loader);
         return bundle;
     }

     /**
+     * Keeps the argument ClassLoader alive.
+     */
+    private static void keepAlive(ClassLoader loaderone){
+        //Do nothing.
+    }
+
+    /**
      * Checks if the given <code>List</code> is not null, not empty,
      * not having null in its elements.
      */
------------------------------------------------------


Best Regards

Adam Farley 


mandy chung <mandy.chung at oracle.com> wrote on 15/08/2018 17:49:51:

> From: mandy chung <mandy.chung at oracle.com>
> To: Adam Farley8 <adam.farley at uk.ibm.com>, Hans Boehm 
<hboehm at google.com>
> Cc: core-libs-dev <core-libs-dev at openjdk.java.net>, 
i18n-dev at openjdk.java.net
> Date: 15/08/2018 17:50
> Subject: Re: RFR 8209184: JDK8 ResourceBundle vulnerable to GC (fix 
included)
> 
> Hi Adam,
> 
> This fix is JDK 8u only and so you will need to request for 8u approval.
> 
> The proposed empty static method is fine with me.  Please fix the format 

> and indentation before you post the review.
> 
> Since this patch is small, you can inline the diff in the RFR mail.
> I can review it when you send the review request out.
> 
> Mandy
> [1] INVALID URI REMOVED
> 
u=http-3A__openjdk.java.net_projects_jdk8u_approval-2Dtemplate.html&d=DwID-
> g&c=jf_iaSHvJObTbx-siA1ZOg&r=P5m8KWUXJf-
> 
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=LNli6mMzg3GKad9dsE3XbXBnzTk9woUBJ7J4LmWSCoM&s=7qqktTwgCMn4ZPhTnzh9Dfla2kn9iTtZxT47FUZ6otQ&e=
> 
> On 8/15/18 8:45 AM, Adam Farley8 wrote:
> > 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
> 

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