Code Review Request for 7146728 and 7130959

[Brad Wetmore]

Minor comment:

// Array too short, pad it w/ leading 0s including the sign bit

Can you pull off:  "including the sign bit"

Looks good to me otherwise.

Brad





On 3/16/2012 1:42 PM, Valerie (Yu-Ching) Peng wrote:
> Brad,
>
> As I was preparing the files for putting back, I noticed some
> now-redundant code in DHKeyAgreement2.java as well as one minor issue
> about resetting the key agreement object in the
> engineGenerateSecret(...) method of DHKeyAgreement.java.
>
> Hope this to be the last version of the webrev:
> http://cr.openjdk.java.net/~valeriep/7146728/webrev.04/
>
> Please let me know if you have any more comment.
> Thanks!
> Valerie
>
> On 03/14/12 16:54, Valerie (Yu-Ching) Peng wrote:
>> Sure, that's good clarification.
>> I'll add the suggested wording and put back the changes.
>> Thanks,
>> Valerie
>>
>> On 03/14/12 16:01, Brad Wetmore wrote:
>>> 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
>>>>
>>
>