I have no more comment.
As for fill(passwd, '\0'), maybe JVM can translate it to ZeroMemory() or
memset(0). In fact, I don't know why originally it was fill(passwd, '0'). I can
only guess that it can still be printed out as a normal string and if someone
misuse it there won't be a NPE. Who knows?
Thanks
Max
> On Aug 23, 2018, at 6:01 AM, Seán Coffey <sean.cof...@oracle.com> wrote:
>
>
>
> 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 :
>>>>>
>>>
>>>
