On 22 August 2018 19:22:49 IST, Ivan Gerasimov <ivan.gerasi...@oracle.com>
wrote:
>Hi Seán!
>
>Just a minor comment.
>
>I don't know if it's even measurable in this context, but I was under
>impression that filling memory with zero *bytes* might be a slightly
>more efficient then filling with any other constant.
>
>Maybe it is better to use Arrays.fill(passwd, '\0') instead of
>Arrays.fill(passwd, '0') to give the JVM a chance to optimize filling
>if
>it's possible?
Interesting comment Ivan. I was not aware of such an effect! If you've further
references on that, I'd appreciate it. '0' is used in other, similar, fill
operations IIRC. Perhaps we can optimize such code across all security libs
code via another JBS issue.
Regards,
Sean.
>
>With kind regards,
>
>Ivan
>
>
>On 8/22/18 9:25 AM, Seán Coffey wrote:
>> Thanks for reviewing. comments inline..
>>
>> On 22/08/18 15:50, Weijun Wang wrote:
>>> PBES2Core.java:
>>>
>>> 181 byte[] passwdBytes = key.getEncoded();
>>> 182 char[] passwdChars = null;
>>> 183 PBEKeySpec pbeSpec;
>>> 184 try {
>>> 185 if ((passwdBytes == null) ||
>>> 186 !(key.getAlgorithm().regionMatches(true, 0, "PBE", 0, 3))) {
>>> 187 throw new InvalidKeyException("Missing
>password");
>>> 188 }
>>> ....
>>> 272 } finally {
>>> 273 if (passwdChars != null) Arrays.fill(passwdChars,
>'
>>> ');
>>> 274 Arrays.fill(passwdBytes, (byte)0x00);
>>> 275 }
>>>
>>> If passwdBytes == null, line 274 would throw an NPE.
>> Good catch. Corrected.
>>>
>>> PBKDF2KeyImpl.java:
>>>
>>> 87 char[] passwd = keySpec.getPassword();
>>> 88 if (passwd == null) {
>>> 89 // Should allow an empty password.
>>> 90 this.passwd = new char[0];
>>> 91 } else {
>>> 92 this.passwd = passwd.clone();
>>> 93 }
>>> 94 // Convert the password from char[] to byte[]
>>> 95 byte[] passwdBytes = getPasswordBytes(this.passwd);
>>>
>>> 96 // remove local copy
>>> 97 Arrays.fill(passwd, '0');
>>>
>>> If passwd == null, line 97 would throw an NPE.
>> Another good catch!
>>
>> updated webrev :
>> http://cr.openjdk.java.net/~coffeys/webrev.8209129.v3/webrev/
>>
>> regards,
>> Sean.
>>
>>>
>>> Otherwise fine.
>>>
>>> Thanks
>>> Max
>>>
>>>
>>>> On Aug 17, 2018, at 12:53 AM, Seán Coffey <sean.cof...@oracle.com>
>>>> wrote:
>>>>
>>>> Find new webrev here Max :
>>>>
>>>> http://cr.openjdk.java.net/~coffeys/webrev.8209129.v2/webrev/
>>>>
>>>> regards :
>>>>
>>
>>
