On Fri, 16 Jun 2023 09:25:30 GMT, Daniel Fuchs <dfu...@openjdk.org> wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> more cases to cover
>
> 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.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/14506#discussion_r1232205474
