On Wed, 14 Sep 2022 18:23:51 GMT, Xue-Lei Andrew Fan <xue...@openjdk.org> wrote:
>> Good point. Modified to use less checking.
>>
>> 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.
>
>> 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.
> 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).
-------------
PR: https://git.openjdk.org/jdk/pull/10263
