RFR (Unraised): JDK8 ResourceBundle vulnerable to GC

[Peter Levart]

Hi,

On 07/16/2018 02:10 PM, Alan Bateman wrote:
> On 11/07/2018 14:27, Adam Farley8 wrote:
>> Hi All,
>>
>> -- Summary:
>>
>> When calling "ResourceBundle.getBundle(String, Locale, ClassLoader)" on
>> JDK8, the ClassLoader can get GC'd before we're finished with it.
>>
>> This can result in us getting the wrong result back, like if we asked 
>> for
>> "Stuff" with the locale "fr, CA" and got back "Stuff_fr.class" 
>> instead of
>> "Stuff_fr_CA.class".
>>
>> -- Likely Cause:
>>
>> Inside the CacheKey object in the getBundleImpl method, we use a soft
>> reference that can result in the ClassLoader object getting GC'd before
>> we're done with it.
>>
>> -- Example Fix:
>>
>> One fix proven to work is to create a static, volatile Object with 
>> global
>> scope in ResourceBundle, and compare it ("==" style) to the loader right
>> before it gets returned from getBundleImpl.
>>
>> The result of the compare can be something direct like "throw new
>> Error("Unexpected error.")", as this compare should never return true.
>>
>> -- Only seen on...
>>
>> This defect is only seen on JDK8, on ppcle and zLinux, and hasn't yet 
>> been
>> seen using the Hotspot VM (defect found using OpenJ9 VM).
>>
> ResourceBundle has changed significantly since JDK 8 so I'm curious if 
> you have studied the implementation in the mainline (jdk/jdk) to see 
> if the issue is possible there. If it's still a bug then I assume we 
> would want to fix it in jdk/jdk first before thinking about a back port.
>
> -Alan

Checking the differences shows that instead of 
WeakReference<ClassLoader>, CacheKey uses two WeakReference<Module> 
instead in JDK 9+. One is the caller module and the other is the module 
of the bundle (unnamed module if ClassLoader is used to search for the 
bundle). The caller module is guaranteed to stay reachable for the 
entire call (as it holds the calling code), but the Module of the bundle 
could go out of scope during the call, so I think there is a theoretical 
possibility that the lookup CacheKey becomes non-equal to a key in the 
cache before the cache lookup is executed.

Placing Reference.reachabilityFence(module) at the right place(s) should 
help eliminate the possibility. The fix for JDK 8 can not use 
Reference.reachabilityFence(module) as it is JDK 9+ API, but as we 
learned, an empty static method taking an Object parameter is equivalent 
to Reference.reachabilityFence() at least in the HotSpot VM. That might 
not be true for J9 VM though...

Regards, Peter