On Wed, 4 Sep 2024 23:02:29 GMT, Valerie Peng <valer...@openjdk.org> wrote:
>> Kevin Driver has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> change impl class to use byte arrays rather than SecretKey objects where
>> possible
>
> src/java.base/share/classes/com/sun/crypto/provider/HkdfKeyDerivation.java
> line 358:
>
>> 356: }
>> 357:
>> 358: return Arrays.copyOf(kdfOutput, outLen);
>
> Here is an alternative solution which does not need `Arrays.copyOf()` call:
> Suggestion:
>
> kdfOutput = new byte[outLen];
> int i = 0;
> int offset = 0;
> try {
> while (i < rounds) {
> if (i > 0) {
> hmacObj.update(kdfOutput, offset - hmacLen, hmacLen); //
> add T(i-1)
> }
> hmacObj.update(info); // Add info
> hmacObj.update((byte) ++i); // Add round number
> if (i == rounds && (outLen - offset < hmacLen)) {
> // special handling for last chunk
> byte[] tmp = hmacObj.doFinal();
> System.arraycopy(tmp, 0, kdfOutput, offset,
> outLen - offset);
> offset = outLen;
> } else {
> hmacObj.doFinal(kdfOutput, offset);
> offset += hmacLen;
> }
> }
> } catch (ShortBufferException sbe) {
> // This really shouldn't happen given that we've
> // sized the buffers to their largest possible size up-front,
> // but just in case...
> throw new RuntimeException(sbe);
> }
> return kdfOutput;
My personal opinion is that this adds complexity which is unnecessary. The
`Arrays.copyOf` solution reads more simply and GC will take care of any extra
allocation once it goes out of scope. I appreciate the alternate fix, but in
this case the original solution is more easily understood and thus more
maintainable.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1745932560
