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

[mandy chung]

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] http://openjdk.java.net/projects/jdk8u/approval-template.html

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