Re: RFR 8209184: JDK8 ResourceBundle vulnerable to GC (fix included)
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 wrote on 15/08/2018 06:44:08:
> From: Hans Boehm
> To: Mandy Chung
> Cc: adam.far...@uk.ibm.com, core-libs-dev 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
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 > > 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
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] 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 wrote on 15/08/2018 06:44:08:
From: Hans Boehm
To: Mandy Chung
Cc: adam.far...@uk.ibm.com, core-libs-dev , 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 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
> > 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
