On Tue, 24 Sep 2024 18:26:06 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Kevin Driver has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> additional DPS tests, removal of private method
>
> src/java.base/share/classes/javax/crypto/KDF.java line 546:
>
>> 544: if (checkSpiNonNull(theOne)) return;
>> 545:
>> 546: synchronized (lock) {
>
> Sorry I missed this last time. The `if (checkSpiNonNull(theOne)` check also
> must be repeated inside the `synchronized` block.
>
> Suppose 2 threads, one calling `deriveData` and one calling
> `getProviderName`, are running at the same time. Both start with `theOne`
> being null and reach the block. The `deriveData` thread gets the lock,
> chooses a provider, and assigns `theOne`. When it releases the lock the
> `getProviderName` thread enters this block and updated `theOne`. This should
> not happen since `theOne` should only be assigned once.
>
> The `KDFDelayedProviderSyncTest.java` hasn't been able to catch this. Since
> we don't have states inside the HKDF implementation and there is only one
> candidate, the test is likely to always succeed even if the DPS process is
> not implemented correctly. I can imagine there is a case that there are 2
> HKDF implementations and the 1st one (the `candidate`) always fails at
> `deriveData`. In my supposed scenario above, the 1st `deriveData` succeeds
> but if `theOne` was reset to `candidate` later, the next `deriveData` would
> fail.
Addressed in:
https://github.com/openjdk/jdk/pull/20301/commits/52ef5b0095dda55491286ede5ad18e38d700124d.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1774124340
