On Tue, 26 Sep 2023 15:04:44 GMT, Daniel Jeliński <djelin...@openjdk.org> wrote:
>> 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? > > Entries is a synchronized map, and populateAttributes is only called on > freshly created entries before they are added to the map. I don't see how > this could fail. Oh - I see - I did miss that. LGTM then. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/15909#discussion_r1337447708
