Code Review Request for 7146728 and 7130959

Brad Wetmore bradford.wetmore at oracle.com
Fri Mar 9 01:15:32 UTC 2012


 >> DHKeyAgreement:
 >> ===============

 > I added more comments to both code blocks to explain what's going on.

A couple more requested comments.

Around line 299, could please add that the sign bit is always 0.

Around line 303, could you put in a comment that since there is enough 
room here for the sign bit in this array, it just becomes a leading zero.

> Good point, I modified the engineGenerateSecret() to leverage
> engineGenerateSecret(byte[], int) method.

Great, thanks!

>> P11KeyAgreement.java
>> ====================
>>
>> // Solaris
>>
>> Shouldn't this be "// Solaris/NSS", for the case in which there isn't
>> a leading zero?
> Well, I think putting NSS in both places seems somewhat confusing. What
> I tried to point out is that NSS (at least in the older versions, I have
> not tried the newer version to see if they switched behavior) trims off
> the leading 0s whenever possible. I've made some modification hoping to
> make it clearer.

Much better, thanks!

Minor nit (take or leave) you could adjust the logic around 208 to only 
create newSecret if secret.length < secretLen.

     if (secret.length > secretLen) {
         throw new ProviderException(...
     }
     byte [] newSecret = new byte[secretLen];
     System.arraycopy(...);
     return newSecret;

>

>> TestShort.java
>> ==============
>> New bugid
>>
>> @bug 4942494 7146728
> Done.
>
>> Indention problem around the try.
> Fixed.

Thanks for taking out the commented out code, I almost mentioned it and 
then didn't.

Brad



More information about the security-dev mailing list