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