On Wed, 19 May 2021 20:21:23 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 one
> additional commit since the last revision:
>
> Fix perf problem by reorganizing doLastBlock()
Here are comments on the remaining src changes. Will continue looking at the
tests.
Thanks,
Valerie
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
628:
> 626: }
> 627:
> 628: int mergeBlock(byte[] buffer, int bufOfs, byte[] in, int inOfs,
Can be made 'static'? No javadoc for this one, maybe move the javadoc for the
other one up and use "Methods" in the javadoc to cover both?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
637:
> 635: * The method takes two buffers to create one block of data
> 636: *
> 637: * This in only called when buffer length is less that a
> blockSize
nit: typo in->is, that->than
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
640:
> 638: * @return number of bytes used from 'in'
> 639: */
> 640: int mergeBlock(byte[] buffer, int bufOfs, int bufLen, byte[] in,
Can be made 'static'?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
648:
> 646:
> 647: System.arraycopy(buffer, bufOfs, block, 0, bufLen);
> 648: int inUsed = Math.min(block.length - bufLen,
Seems equivalent to `int inUsed = Math.min((block.length - bufLen), inLen);`?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
741:
> 739: } else {
> 740: // If the remaining in buffer + data does
> not fill a
> 741: // block, complete the gctr operation
This comment seems more suited for the else block below at line 753. In
addition, the criteria for completing the gctr operation should be whether src
still has remaining bytes left. It's possible that the (src.remaining() ==
blockSize - over) and happen to fill up the block, right? The current flow for
this particular case would be an op.update(...) then continue down to line
770-778, maybe you can adjust the if-check on line748 so this would go through
line 754-759 and return, i.e. the else block?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
752:
> 750: if (dst != null) {
> 751: dst.put(block, 0, blockSize);
> 752: }
not counting this blockSize value into "processed"?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
752:
> 750: if (dst != null) {
> 751: dst.put(block, 0, blockSize);
> 752: }
Why not just do `op.update(block, 0, blockSize, dst); `?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
757:
> 755: if (dst != null) {
> 756: dst.put(block, 0, Math.min(block.length,
> len));
> 757: }
Would this method work correctly if dst is null? Shouldn't this be checked in
the beginning of this method?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
805:
> 803: /**
> 804: * This segments large data into smaller chunks so hotspot will
> start
> 805: * using CTR and GHASH intrinsics sooner. This is a problem for
> app
nit: typo: CTR->GCTR? This is to improve performance rather than a problem, no?
Same comment applies to the other throttleData() method above.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
874:
> 872: } else if (!src.isDirect() && !dst.isDirect()) {
> 873: // if src is read only, then we need a copy
> 874: if (!src.isReadOnly()) {
Do you mean if (src.hasArray() && dst.hasArray())? Even if src is not read
only, but array()/arrayOffset() methods are optional and can only be safely
invoked after the hasArray() call.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
908:
> 906: * If an intermediate array is needed, the whole original out
> array
> 907: * length is allocated because it's simpler than keep the same
> offset
> 908: * and hope the expected output is
The comment seems incomplete? Perhaps you mean "if an intermediate array is
needed, it will be allocated using the whole original out array length, so that
the same out array offset can be used for code simplicity."
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
942:
> 940:
> 941: System.arraycopy(out, originalOutOfs, originalOut,
> originalOutOfs,
> 942: len);
Don't you need to do `originalOut = null;` after copying the bytes over?
Otherwise, if you have two overlapping calls with the same engine, the 2nd
restoreOut(...) call may lead to data corruption, i.e. it will duplicate the
output bytes to the original output buffer (in the 1st overlapping call).
Same applies for the ByteBuffer case, i.e. restoreDst(...).
If time permits, please add a regression test to cover this.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
975:
> 973: doUpdate(in, inOff, inLen, output, 0);
> 974: } catch (ShortBufferException e) {
> 975: // update decryption has no output
The comment does not seems to make sense? This is GCMEncrypt class. The right
reason should be that the output array is allocated by the provider and should
have the right size. However, it seems safer to throw AssertionException()
instead of swallowing the SBE...
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1014:
> 1012: len += gctrghash.update(buffer, 0, bLen, out,
> outOfs);
> 1013: outOfs += bLen;
> 1014: }
For encryption, would the ibuffer contain more than one blocksize of data?
Isn't the existing impl only put the remaining input (less than a block) into
it? Line 1013: the `outOfs += bLen;`, shouldn't 'bLen' be 'len'?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1016:
> 1014: }
> 1015:
> 1016: // blen is now the offset for 'buffer'
nit: typo, blen->bLen
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1030:
> 1028: inOfs += inLenUsed;
> 1029: inLen -= inLenUsed;
> 1030: outOfs += blockSize;
'blockSize' should be 'len'?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1033:
> 1031: ibuffer.reset();
> 1032: // Code below will write the remainder from 'in' to
> ibuffer
> 1033: } else if (remainder > 0) {
If bLen == 0, there is no need to put the rest of 'buffer' into 'ibuffer'.
It looks strange that the remaining buffer data is stored back into 'ibuffer',
then the code proceeds to encrypt 'in' from line 1043-1046. This looks
incorrect as all prior buffered input should be processed before process
current input.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1036:
> 1034: // If a block or more was encrypted from 'buffer'
> only, but
> 1035: // the rest of 'buffer' with 'in' could not
> construct a
> 1036: // block, then put the rest of 'buffer' back into
> ibuffer.
If 'ibuffer' is always less than a block long, then some of these are dead code
and can be trimmed. Same goes for the doUpdate() w/ ByteBuffer arguments.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1044:
> 1042:
> 1043: // Encrypt the remaining blocks inside of 'in'
> 1044: if (inLen > 0) {
If encrypting the remaining blocks, should use (inLen >= blockSize)?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1055:
> 1053: // remainder offset is based on original buffer length
> 1054: ibuffer.write(in, inOfs + inLen, remainder);
> 1055: }
I wonder if these update(byte[], int, int, byte[], int) calls (such as the one
on line 1045) should take ibuffer and stores the unprocessed bytes into it.
This way seems more robust and you won't need separate logic. Same comment for
the doUpdate() taking ByteBuffer arguments.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1163:
> 1161: r = doUpdate(buffer, 0, bufLen, out, outOfs);
> 1162: bufLen -= r;
> 1163: inOfs += r;
Would bufLen >= blockSize? Regardless, 'inOfs' should not be touched as 'in' is
not used in the above doUpdate() call.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1174:
> 1172: inLen -= r;
> 1173: r = gctrghash.update(block, 0, blockSize, out,
> 1174: outOfs + resultLen);
I don't follow why you don't update the 'outOfs' after the line 1161 doUpdate()
call and then add the resultLen when calling gctrhash.update(...) here. Seems
fragile and difficult to maintain?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1184:
> 1182: block = new byte[bufLen + inLen];
> 1183: System.arraycopy(buffer, 0, block, 0, bufLen);
> 1184: System.arraycopy(in, inOfs, block, bufLen, inLen);
Not needed if inLen == 0.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1213:
> 1211:
> 1212: // copy the tag to the end of the buffer
> 1213: System.arraycopy(block, 0, out, resultLen + outOfs,
> tagLenBytes);
Now that the tag is copied to the output, why not increment resultLen w/
tagLenBytes? This way, you don't have to keep repeating the (resultLen +
tagLenBytes) for another two times?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1317:
> 1315: * If tagOfs = 0, 'in' contains only the tag
> 1316: * if tagOfs = blockSize, there is no data in 'in' and all the
> tag
> 1317: * is in ibuffer
Is this correct? mergeBlock() returns the number of used bytes from 'in', if no
data is in 'in' and all the tag is from 'ibuffer', tagOfs should equal to
-tagLenBytes. The next line should be moved up as the tag position gradually
shifts from 'in' toward 'ibuffer'. The tagOfs for the next line should be
-tagLenBytes < tagOfs < 0?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1401:
> 1399: ShortBufferException {
> 1400: GHASH save = null;
> 1401:
Should do ArrayUtil.nullAndBoundsCheck() on 'in'?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1412:
> 1410: }
> 1411:
> 1412: checkDataLength(len - tagLenBytes);
The impl of checkDataLength(...) is based on the "processed" variable which is
always 0 for doUpdate() calls. This suits encryption better, but does not suit
decryption? It seems that the doUpdate() calls would just keep buffering the
data into 'ibuffer' without checking the size. It seems to me that this
checkDataLength() call on line 1412 would always pass.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1437:
> 1435: } catch (ArrayIndexOutOfBoundsException aiobe) {
> 1436: throw new ShortBufferException("Output buffer invalid");
> 1437: }
I think this should be moved to the very beginning before all the processing
and if the output capacity is less than 'len-tagLenBytes' value, then no need
to proceed? IIRC, the save/restore is more for algorithms which use padding,
may not be needed for GCM?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1442:
> 1440: throw new ShortBufferException("Output buffer too
> small, must" +
> 1441: "be at least " + (len - tagLenBytes) + " bytes
> long");
> 1442: }
This looks like a half-baked save/restore functionality?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1475:
> 1473:
> 1474: // Length of the input
> 1475: ByteBuffer tag;
Is the comment obsolete? I don't quite follow how it's related to 'tag'.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1503:
> 1501: tag.put(ct);
> 1502: tag.flip();
> 1503: // Limit is how much of the ibuffer has been chopped
> off.
The comment seems incorrect? Limit is not how much the ibuffer has been chopped
off, but rather what has not been chopped off, no?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1510:
> 1508:
> 1509: // 'len' contains the length in ibuffer and src
> 1510: checkDataLength(len);
Is this really useful given 'processed' is 0 and there is only one argument
'len'. Should always pass?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1515:
> 1513: // the dst buffer is too short. Context will be restored
> so the
> 1514: // method can be called again with the proper sized dst
> buffer.
> 1515: if (len > dst.remaining()) {
We should check the dst capacity in the beginning of the method, if its
capacity is too small, i.e. less than the overall length - tag length, don't
proceed further.
As in the doFinal() w/ byte[] arguments, I am not sure if the save/restore is
really necessary.
Maybe add a test if it's not already covered by existing tests if time permits.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1557:
> 1555:
> 1556: // Decrypt the all the input data and put it into dst
> 1557: //decryptBlocks(buffer, ct, dst);
nit: remove obsolete commented out code?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1610:
> 1608: // update the input parameters for what was taken
> out of 'in'
> 1609: inOfs += inUsed;
> 1610: inLen -= inUsed;
This merge block code won't be needed if inLen == 0, i.e. can just assign in to
be buffer, inOfs to 0, and inLen to bufRemaining.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1615:
> 1613: if (inLen > 0) {
> 1614: int resultLen;
> 1615: resultLen = op.update(block, 0, blockSize,
nit: can just do `int resultLen = op.update(...);`
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
1665:
> 1663: /**
> 1664: * This class is for encryption operations when both GCTR and GHASH
> 1665: * can operation in parallel.
nit: typo "operation"->"operate"
src/java.base/share/classes/com/sun/crypto/provider/SunJCE.java line 255:
> 253: attrs.put("SupportedKeyFormats", "RAW");
> 254: ps("Cipher", "DESedeWrap",
> 255: "com.sun.crypto.provider.DESedeWrapCipher", null, attrs);
fix indentation to be consistent?
src/java.base/share/classes/com/sun/crypto/provider/SunJCE.java line 264:
> 262: "com.sun.crypto.provider.ARCFOURCipher", attrs);
> 263: ps("Cipher", "AESWrap",
> "com.sun.crypto.provider.AESWrapCipher$General",
> 264: null, attrs);
fix indentation to be consistent?
-------------
PR: https://git.openjdk.java.net/jdk/pull/4072
