Code Review Request for 7146728 and 7130959
Brad Wetmore
bradford.wetmore at oracle.com
Wed Mar 14 23:01:29 UTC 2012
Here's the comment I was trying to suggest in your office yesterday:
/*
* NOTE: BigInteger.toByteArray() returns a byte array containing
* the two's-complement representation of this BigInteger with
* the most significant byte in the zero-th element. This
* contains the minimum number of bytes required to represent
* this BigInteger, including at least one sign bit whose value
* is always 0.
*
* Keys are always positive, and the above sign bit isn't
* actually used when representing keys. (i.e. key = new
* BigInteger(1, byteArray)) To obtain an array containing
* exactly expectedLen bytes of magnitude, we strip any extra
* leading 0's, or pad with 0's in case of a "short" secret.
*/
Hope this helps,
Brad
On 3/9/2012 12:57 PM, Valerie (Yu-Ching) Peng wrote:
>
> Ok, more comments/changes added as suggested.
> Webrev updated at:
> http://cr.openjdk.java.net/~valeriep/7146728/webrev.02/
>
> Thanks,
> Valerie
>
> On 03/08/12 17:15, Brad Wetmore wrote:
>>
>> >> 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