RFR 8199435 : Unsafe publication of java.util.Properties.map

[Peter Levart]

Hi Claes,

On 06/18/2018 12:02 PM, Claes Redestad wrote:
>
>
> On 2018-06-18 05:45, David Holmes wrote:
>> Making it "final" to fix unsafe publication only works with actual 
>> publication after construction. You're making it "final" but then not 
>> setting it at construction time and instead using UNSAFE.putVolatile 
>> to get around the fact that it is final! The "final" serves no 
>> purpose in that regard. Further a putVolatile without corresponding 
>> getVolatile does not achieve safe publication under the JMM.
>
> Adding a volatile read on every read through Properties is likely to 
> have some performance impact,

On non-Intel architectures in particular. On intel it would just inhibit 
some JIT optimizations like hoisting the read of field out of loop...

> but it could still be better than before 8029891 where such operations 
> were synchronized. Regarding Peter's comments about defaults, I think 
> accesses to that field was implicitly guarded by the synchronized 
> super.get calls before 8029891 and now needs to be re-examined too.

This was pre- 8029891 code of getProperty for example:

     public String getProperty(String key) {
         Object oval = super.get(key);
         String sval = (oval instanceof String) ? (String)oval : null;
         return ((sval == null) && (defaults != null)) ? 
defaults.getProperty(key) : sval;
     }

So 'defaults' field was accessed out of synchronized super.get() method 
and getProperty was not synchronized. This is pre-existing issue. But 
'map' field is new.

The problem with 'defaults' field is also that is is protected and 
subclasses can access it without synchronization. If it is made 
volatile, getProperty() method will be penalized.

So maybe using weaker memory fences in combination with plain 'property' 
field can make the field behave like it was final, so at least accesses 
to field within Properties class can be made non-racy. If this is done 
for 'properties' then same fences will work for 'map' too and it doesn't 
have to be final.

>
>>
>> I think making the actual field(s) volatile is the only JMM compliant 
>> way to correctly address this.
>
> The only issue is serialization-related readHashTable method (clone 
> could be implemented without Unsafe as clone.map.putAll(map)). 

1st you would have to do:

     clone.map = new CHM();

and only then:

     clone.map.putAll(map);

or else you would be adding the elements of the original map to the 
original map (which would actually work with CHM), but then the cloned 
Properties object would share the map with original one. So you do need 
to assign to 'map' field in clone().


> Is there no sanctioned way (using e.g. VarHandles) to safely construct 
> and publish a transient "final" field in the scope of a readObject? 
> I've been shying away from experimenting with VarHandles in places 
> like Properties as I'm certain it could lead to bootstrap issues, but 
> for something like a deserialization hook it might very well be fine.

Paul would know for sure, but I think this was deliberately prevented in 
VarHandle(s).

Regards, Peter

>
> /Claes
>