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

[Daniel Fuchs]

On Mon, 25 Sep 2023 18:12:32 GMT, Weijun Wang <weijun at openjdk.org> wrote:

> A different fix after https://github.com/openjdk/jdk/pull/14506 was closed.
> 
> Still haven't made the attributes set immutable but at least it is populated before an entry is added to `entries` and it will never be modified later.
> 
> I tried the newly added `AttributesMultiThread.java` test hundreds of times and only observed failures before this fix (~%2 failure rate).

src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 1270:

> 1268:         }
> 1269:         Entry entry = entries.get(alias.toLowerCase(Locale.ENGLISH));
> 1270:         return Collections.unmodifiableSet(new HashSet<>(entry.attributes));

I do not think this fix is correct. A call to `engineGetAttribute()` could observe an empty `HashSet` set by `populateAttributes`, but before the entry is populated. If we want to prevent that I believe we need a lock of some kind shared between `engineGetAttribute` and `populateAttributes`. 
Also the constructor of `HashSet` that takes a collection calls `HashSet.addAll()`, which loops over the provided collection. In a multi threaded context, if `populateAttributes` can run concurrently with `engineGetAttribute`, it is not impossible that this loop will get a `ConcurrentModificationException`, like before. Or am I missing something?

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/15909#discussion_r1337337639