RFR: 8327461: KeyStore getEntry is not thread-safe [v6]

[Hai-May Chao]

On Fri, 8 Mar 2024 21:56:19 GMT, Weijun Wang <weijun at openjdk.org> wrote:

>> Hai-May Chao has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains six additional commits since the last revision:
>> 
>>  - Merge
>>  - remove unneeded checks in engineGetEntry
>>  - Update engineDeleteEntry
>>  - Update engineIsKeyEntry and engineIsCertificateEntry
>>  - Update bug number in the test
>>  - 8327461: KeyStore getEntry is not thread-safe
>
> src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 301:
> 
>> 299:         Entry entry = entries.get(alias.toLowerCase(Locale.ENGLISH));
>> 300:         Key entryKey = internalGetKey(entry, password);
>> 301:         return entryKey;
> 
> Combine the 2 lines above.

Done.

> src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 480:
> 
>> 478:         Entry entry = entries.get(alias.toLowerCase(Locale.ENGLISH));
>> 479:         Certificate[] certChain = internalGetCertificateChain(entry);
>> 480:         return certChain;
> 
> Combine the two lines above.

Done.

> src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 1027:
> 
>> 1025: 
>> 1026:         Entry entry = entries.remove(alias.toLowerCase(Locale.ENGLISH));
>> 1027:         if (entry != null) {
> 
> No need to check `entry != null`.

The API doc states: Returns: the previous value associated with key, or null if there was no mapping for key. It’d be a good practice to check if the entry is not null before proceeding with further operations. Would you please elaborate why it is not needed here?

> src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 1082:
> 
>> 1080:     }
>> 1081: 
>> 1082:     private boolean internalEngineIsKeyEntry(Entry entry) {
> 
> No need to have both `internal` and `Engine` in the method name. Use `internal` only to follow the other method names. Same with `internalEngineIsCertificateEntry` below.

Ok.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18156#discussion_r1518399957
PR Review Comment: https://git.openjdk.org/jdk/pull/18156#discussion_r1518399969
PR Review Comment: https://git.openjdk.org/jdk/pull/18156#discussion_r1518399996
PR Review Comment: https://git.openjdk.org/jdk/pull/18156#discussion_r1518400012