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

[Claes Redestad]

On 2018-06-18 18:09, Peter Levart wrote:
>
>
> On 06/18/2018 04:47 PM, Claes Redestad wrote:
>>
>>
>> On 2018-06-18 16:23, Peter Levart wrote:
>>> Hi Claes,
>>>
>>> On 06/18/2018 03:54 PM, Claes Redestad wrote:
>>>> I'd suggest something simple like this to ensure correctness, while 
>>>> keeping the number of volatile reads to a minimum:
>>>>
>>>> http://cr.openjdk.java.net/~redestad/8199435.00/
>>>>
>>>> Then file a follow-up RFE to investigate if we can make these 
>>>> fields non-volatile, e.g. using careful fencing as suggested
>>>> by Peter.
>>>
>>> OK, but the constructor will still need a releaseFence at the end to 
>>> prevent a potential write that unsafely publishes Properties 
>>> instance to float before the volatile writes of 'map' and 'defaults'...
>>>
>>> You might want to use Unsafe directly for that since VarHandle could 
>>> cause bootstrap issues.
>>
>> I don't follow.. which constructor needs a fence in the suggested patch?
>>
>> /Claes
>
> The Properties constructor:
>
>
>     private Properties(Properties defaults, int initialCapacity) {
>         // use package-private constructor to
>         // initialize unused fields with dummy values
>         super((Void) null);
>         map = new ConcurrentHashMap<>(initialCapacity);
>         this.defaults = defaults;
>
>         UNSAFE.storeFence(); // <-- HERE!!
>     }
>
>
> Final fields have bigger guarantee than volatile fields in this respect.
>
> Plain write that follows in program a volatile write, can in theory 
> float before the volatile write. So if you publish a Properties 
> instance via data race (via plain write), the observer may still see 
> uninitialized 'map' and 'defaults' fields.
>

Right

http://cr.openjdk.java.net/~redestad/8199435.01/

(Yes, using VarHandle.storeStoreFence would do the exact same thing, but 
is not usable from Properties as the VarHandle impl needs to read some 
system properties...)

/Claes