On Mon, 17 May 2021 21:41:37 GMT, Anthony Scarpino <ascarp...@openjdk.org>
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 two
> additional commits since the last revision:
>
> - review comment updates
> - Fixed the lack of overlap detection with GCMEncrypt.update()
Here are some comments. Please discard if it does not apply to the latest
change.
src/java.base/share/classes/com/sun/crypto/provider/GCTR.java line 54:
> 52: * @since 1.8
> 53: */
> 54: final class GCTR extends CounterMode implements GCM {
Not sure if this really needs to implements GCM?
src/java.base/share/classes/com/sun/crypto/provider/GCTR.java line 58:
> 56: // Maximum buffer size rotating ByteBuffer->byte[] intrinsic copy
> 57: private static final int MAX_LEN = 1024;
> 58: byte[] block;
make private?
src/java.base/share/classes/com/sun/crypto/provider/GCTR.java line 86:
> 84: }
> 85:
> 86: void checkBlock() {
make private?
src/java.base/share/classes/com/sun/crypto/provider/GCTR.java line 95:
> 93:
> 94: /**
> 95: * Using the given inLen, this operations only on blockSize data,
> leaving
nit: operates (instead of operations)
src/java.base/share/classes/com/sun/crypto/provider/GCTR.java line 97:
> 95: * Using the given inLen, this operations only on blockSize data,
> leaving
> 96: * the remainder in 'in'.
> 97: * The return value will be (inLen - (inLen - blockSize))
I think you mean (inLen - (inLen % blockSize))?
src/java.base/share/classes/com/sun/crypto/provider/GCTR.java line 153:
> 151: throw new RuntimeException("input length out of bound");
> 152: }
> 153: if (inLen < 0 || inLen % blockSize != 0) {
The 2nd condition, i.e. (inLen % blockSize != 0), should be removed just like
the other update(byte[], int, int, byte[], int) method?
src/java.base/share/classes/com/sun/crypto/provider/GCTR.java line 203:
> 201: // allocating and copying for direct bytebuffers
> 202: if (!src.isDirect() && !dst.isDirect() &&
> 203: !src.isReadOnly() && !dst.isReadOnly()) {
Why do we need to check for src being isReadOnly() since we are not writing
bytes into src? As for dst, if it's read only, then we should probably not
proceed further? The other update method which takes ByteBuffer dst did not
check if it's read only. A bit inconsistent?
src/java.base/share/classes/com/sun/crypto/provider/GCTR.java line 219:
> 217: // roll over incorrectly. Use GCM-specific code instead.
> 218: for (int i = 0; i < numOfCompleteBlocks; i++) {
> 219: checkBlock();
checkBlock() can probably be moved outside of the for-loop?
src/java.base/share/classes/com/sun/crypto/provider/GCTR.java line 268:
> 266: }
> 267: }
> 268: reset();
To ensure no change of behavior, keep the reset() inside the try/finally block?
src/java.base/share/classes/com/sun/crypto/provider/GCTR.java line 283:
> 281: // allocating and copying for direct bytebuffers
> 282: if (!src.isDirect() && !dst.isDirect() &&
> 283: !src.isReadOnly() && !dst.isReadOnly()) {
Same question regarding the isReadOnly() calls as in the update(ByteBuffer,
ByteBuffer) method.
src/java.base/share/classes/com/sun/crypto/provider/GHASH.java line 51:
> 49: */
> 50: final class GHASH implements Cloneable, GCM {
> 51: private static final int AES_BLOCK_SIZE = 16;
nit: add an empty line between this and next.
-------------
PR: https://git.openjdk.java.net/jdk/pull/4072
