RFR: 8207775: Better management of CipherCore buffers

Seán Coffey sean.coffey at oracle.com
Fri Aug 3 08:55:25 UTC 2018


Thanks for the review Tony. Yes - no problems with tests. Copyright 
updated and changeset pushed.

regards,
Sean.


On 03/08/2018 05:23, Anthony Scarpino wrote:
> Looks good. Only comment is update the copyright year. No need to regenerate a webrev. Assuming it passes all the tests I say it’s ready to push.
>
> Tony
>
>> On Aug 2, 2018, at 5:15 AM, Seán Coffey <sean.coffey at oracle.com> wrote:
>>
>> Thanks Tony. I was asked off thread to add some comments to help code maintenance also.
>>
>> webrev updated. Hope we're nearly ready to push now.
>>
>> http://cr.openjdk.java.net/~coffeys/webrev.8207775.v4/webrev/
>>
>> regards,
>> Sean.
>>
>>> On 01/08/2018 18:41, Anthony Scarpino wrote:
>>> That looks fine to me.
>>>
>>> Tony
>>>
>>>> On 08/01/2018 08:39 AM, Seán Coffey wrote:
>>>> Thanks again for review Tony. I think you raise a good point and should give some performance gain.
>>>>
>>>> for line 676: maybe this :
>>>>
>>>>                    byte[] copy = Arrays.copyOf(output, len);
>>>>                    if (decrypting) {
>>>>                        Arrays.fill(output, (byte) 0x00);
>>>>                    }
>>>>                    return copy;
>>>>
>>>>
>>>> perhaps the same again for around line 852 :
>>>>                    byte[] copy = Arrays.copyOf(output, len);
>>>>                    if (decrypting) {
>>>>                        Arrays.fill(output, (byte) 0x00);
>>>>                    }
>>>>                    return copy;
>>>>
>>>> ==
>>>>
>>>> Any other reviews/comments from folks before I submit final build/test & webrev ?
>>>>
>>>> Regards,
>>>> Sean.
>>>>
>>>>> On 01/08/18 15:55, Anthony Scarpino wrote:
>>>>>
>>>>>
>>>>> My only comment is if it makes sense to have the change at 676 to also only null out on decrypt?
>>>>>
>>>>> Otherwise I'm fine with the changes
>>>>>
>>>>> Tony
>>>>>
>>>>>> On 07/31/2018 02:04 AM, Seán Coffey wrote:
>>>>>> Thanks for review Tony. Comments inline..
>>>>>>
>>>>>>> On 27/07/18 21:02, Anthony Scarpino wrote:
>>>>>>> If we are going to add more, here are two more ton consider:
>>>>>>>
>>>>>>> - It looks like there is another Arrays.copyOf() on doFinal() line 851
>>>>>> Good point.
>>>>>>> - doFinal()
>>>>>>> at line 897 there might be something that should be done with 'buffer'.  In particular as a result of line 963's arraycopy().
>>>>>> Yes - I've identified two areas where we can be proactive about nulling out 'buffer' contents. That's around the same time where we reset 'buffered' to 0. See lines 777 and 967
>>>>>>
>>>>>> http://cr.openjdk.java.net/~coffeys/webrev.8207775.v3/webrev/
>>>>>>
>>>>>> regards,
>>>>>> Sean.
>>>>>>> Tony
>>>>>>>
>>>>>>>
>>>>>>>> On 07/27/2018 08:29 AM, Seán Coffey wrote:
>>>>>>>> Thanks Tony. If it's alright with you, I'd like to make one more edit for this change.
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~coffeys/webrev.8207775.v2/webrev/
>>>>>>>>
>>>>>>>> There's a condition where we can null out an array early if we're returning a copy. See lines 671-683
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Sean.
>>>>>>>>
>>>>>>>>> On 26/07/18 17:42, Anthony Scarpino wrote:
>>>>>>>>>> On 07/26/2018 07:36 AM, Seán Coffey wrote:
>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8207775
>>>>>>>>>>
>>>>>>>>>> Simple enough fix to null out some internal buffers once they're no longer required.
>>>>>>>>>>
>>>>>>>>>> webrev : http://cr.openjdk.java.net/~coffeys/webrev.8207775/webrev/
>>>>>>>>>>
>>>>>>>>>> regards,
>>>>>>>>>> Sean.
>>>>>>>>>>
>>>>>>>>> that looks fine..
>>>>>>>>>
>>>>>>>>> Tony




More information about the security-dev mailing list