On Fri, 8 Mar 2024 21:15:45 GMT, Hai-May Chao <hc...@openjdk.org> wrote:

>> Change was made to engineGetEntry() in PKCS12KeyStore to extract the key and 
>> certificate chain from Entry only once. This is because the entry may get 
>> updated between engineGetKey() and engineGetCertificateChain() which causes 
>> inconsistent result. A new test was added to assess and manipulate 
>> PKCS12KeyStore with read and write operations concurrently from multiple 
>> threads. Thanks!
>
> 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.

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.

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`.

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.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18156#discussion_r1518348960
PR Review Comment: https://git.openjdk.org/jdk/pull/18156#discussion_r1518349769
PR Review Comment: https://git.openjdk.org/jdk/pull/18156#discussion_r1518350934
PR Review Comment: https://git.openjdk.org/jdk/pull/18156#discussion_r1518355653

Reply via email to