On Fri, 21 May 2021 21:18:34 GMT, Anthony Scarpino <[email protected]>
wrote:
>> Hi,
>>
>> I need a review of this rather large change to GCM. GCM will no longer use
>> CipherCore, and AESCrypt to handle it's buffers and other objects. It is
>> also a major code redesign limits the amount of data copies and make some
>> performance-based decisions.
>>
>> Thanks
>>
>> Tony
>
> Anthony Scarpino has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Review comments update
Here are comments for the test changes.
Thanks,
Valerie
test/jdk/com/sun/crypto/provider/Cipher/AEAD/Encrypt.java line 236:
> 234: HexFormat hex = HexFormat.of().withUpperCase();
> 235: if (!Arrays.equals(output, outputTexts.get(k))) {
> 236: System.out.println("Combination #" + k + 1 + "\nresult
> " +
nit: Why "+1"? The number should match the exception message, otherwise, very
confusing.
test/jdk/com/sun/crypto/provider/Cipher/AEAD/GCMBufferTest.java line 673:
> 671:
> 672: // Test update-update-doFinal with byte arrays and preset data
> sizes
> 673: t = new GCMBufferTest("AES/GCM/NoPadding",
This change seems un-necessary? Why separating the declaration to line 631?
test/jdk/com/sun/crypto/provider/Cipher/AEAD/GCMShortBuffer.java line 34:
> 32: /*
> 33: * @test
> 34: * @summary Call decrypt doFinal() with different output values to see if
> the
Missing bug id?
test/jdk/com/sun/crypto/provider/Cipher/AEAD/GCMShortBuffer.java line 56:
> 54: throw e;
> 55: }
> 56: c.doFinal(cipherText, 1, len, pt, 0);
Should check the return value of doFinal() to match the expected output size?
test/jdk/com/sun/crypto/provider/Cipher/AEAD/GCMShortBuffer.java line 78:
> 76: throw e;
> 77: }
> 78: c.doFinal(ByteBuffer.wrap(cipherText, 1, len), out);
Should check the return value of doFinal() to match the expected output size?
test/jdk/com/sun/crypto/provider/Cipher/AES/TestAESCipher.java line 90:
> 88: i++;
> 89: }
> 90: return b;
The generated data seems too similar, all starts with 0 and increment. Maybe
use i = len to start with? Or take some additional argument for starting value?
test/jdk/com/sun/crypto/provider/Cipher/AES/TestAESCipher.java line 98:
> 96:
> 97: byte[] iv = makeData(16);
> 98: AlgorithmParameterSpec aps = new GCMParameterSpec(128, new
> byte[16]);
Use the already generated iv instead of new byte[16]?
test/jdk/com/sun/crypto/provider/Cipher/AES/TestAESCipher.java line 117:
> 115: //int offset = ci.update(plainText, 0, plainText.length,
> cipherText, 0);
> 116: //ci.doFinal(cipherText, offset);
> 117: ci.doFinal(plainText, 0, plainText.length, cipherText, 0);
This change seems un-necessary? I think it's better to not change it so
multi-part enc/dec are tested.
-------------
PR: https://git.openjdk.java.net/jdk/pull/4072
