On Tue, 24 Sep 2024 21:39:32 GMT, Kevin Driver <kdri...@openjdk.org> wrote:
>> 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.
Have you thought about the test?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1774219844
