RFR: 8309667: TLS handshake fails because of ConcurrentModificationException in PKCS12KeyStore.engineGetEntry [v3]

[Daniel Fuchs]

On Fri, 16 Jun 2023 12:45:58 GMT, Weijun Wang <weijun at openjdk.org> 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