On Fri, 16 Jun 2023 12:45:58 GMT, Weijun Wang <[email protected]> wrote:
>> src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line
>> 1438:
>>
>>> 1436:
>>> 1437: if (entry.attributes == null) {
>>> 1438: entry.attributes = ConcurrentHashMap.newKeySet();
>>
>> It seems that there is still a risk that two threads may compete here, then
>> you may end up with one thread working with a different copy of the
>> attributes, no longer tied to the entry. E.g:
>>
>> Thread#1 sees attributes is null, Thread#2 sees attributes is null, both set
>> attributes and only one of them win. The caller in the second thread (that
>> lost) is given the "dangling" map that has not been set, and if it modifies
>> it then modifications will be lost.
>> I don't know if this scenario can actually happen - but the possibility is
>> there. Unless there's always some locking further up the stack that would
>> prevent this?
>>
>> Also I see in total four places that do:
>>
>> [entry|this].attributes = new HashSet<>();
>>
>>
>> wouldn't you need to modify all four places in the same manner?
>
> Correct. Done.
>
> I'll look into making the classes immutable in the next release. I hesitate
> to make such a big change in RDP.
I wonder if this would work, if you don't want to use final or volatile + CAS:
class Entry {
...
private Set<KeyStore.Entry.Attribute> createAttributesMap() {
synchronized (this) {
var attributes = this.attributes;
if (attributes == null) {
return attributes = this.attributes =
ConcurrentHashMap.newKeySet();
} else return attributes;
}
}
}
...
if (entry.attributes == null) {
entry.attributes = entry.createAttributesMap();
}
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14506#discussion_r1232315157
