On Wed, 14 Sep 2022 20:49:34 GMT, Sean Mullan <[email protected]> wrote:
>>> 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).
> > 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.
-------------
PR: https://git.openjdk.org/jdk/pull/10263
