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.cof...@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 >>>>>>> >>>>>> >>>>> >>>> >>> >> >
