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

Reply via email to