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.ch...@oracle.com> wrote on 15/08/2018 17:49:51:
> From: mandy chung <mandy.ch...@oracle.com>
> To: Adam Farley8 <adam.far...@uk.ibm.com>, Hans Boehm
<hbo...@google.com>
> Cc: core-libs-dev <core-libs-...@openjdk.java.net>,
i18n-dev@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 <hbo...@google.com> wrote on 15/08/2018 06:44:08:
> >
> >> From: Hans Boehm <hbo...@google.com>
> >> To: Mandy Chung <mandy.ch...@oracle.com>
> >> Cc: adam.far...@uk.ibm.com, core-libs-dev <core-libs-
> >> d...@openjdk.java.net>, i18n-dev@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.ch...@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.ch...@oracle.com
> >> > <mailto:mandy.ch...@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
