RFR: 8293779: redundant checking in AESCrypt.makeSessionKey() method [v3]

[Sean Mullan]

On Wed, 14 Sep 2022 18:23:51 GMT, Xue-Lei Andrew Fan <xuelei at 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