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
