On Wed, 14 Sep 2022 21:04:58 GMT, Sean Mullan <[email protected]> wrote:
>>> If the key is null, the following condition could bypass the checking, and
>>> result in NPE.
>>>
>>> ` if (!MessageDigest.isEqual(key, lastKey)) {`
>>>
>>> Although it is unlikely to happen as the caller should has already been
>>> checked that the key cannot be null, but the code logic here is not that
>>> clear to read. In the new patch, I have the null checking in the init()
>>> method, and the validity checking in the makeSessionKey() method.
>>
>> I agree, the provider layer should have rejected a null `Key` or a null
>> `Key.getEncoded` prior to this, but best to not change what this code
>> previously did (at least not for this change).
>
>> > Actually, NM, init still has to call MessageDigest.isEqual so eliminating
>> > keys of invalid length before that is probably more efficient.
>>
>> The key should be valid for common cases. For valid key, it is more
>> efficient to have the checking in makeSessionKey() as there is less
>> checking. For invalid key, it is more efficient to have the checking before
>> calling MessageDigest.isEqual(). Exception itself is costly, I would prefer
>> to have better performance for common cases (valid key).
>>
>> I updated the patch before I read the comment. Please let me know your
>> preference. I'm fine to rollback if you prefer the old patch.
>
> Yes, I think your current fix should be fine too. No need to rollback.
Speaking of MessageDigest.isEqual, we don't need constant time comparison here.
We could use Arrays.equals for some extra performance.
-------------
PR: https://git.openjdk.org/jdk/pull/10263
