On Thu, 8 Oct 2020 06:51:08 GMT, Anthony Scarpino <[email protected]> wrote:
>> 8253821: Improve ByteBuffer performance with GCM
>
> Anthony Scarpino has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Xuelei comments
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
550:
> 548:
> 549: processed += len;
> 550: ghashAllToS.update(src, len);
Isn't input to ghashAllToS always be the produced cipher text? Did I miss
something?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
595:
> 593: }
> 594: GCTR gctrForSToTag = new GCTR(embeddedCipher,
> this.preCounterBlock);
> 595: gctrForSToTag.doFinal(s, 0, s.length, block, 0);
since GCTR output the same length of output as input, (e.g. 'sOut' is same
length as 's'), can't we just re-use 's' as
the output buffer instead of 'block'?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
593:
> 591: if (tagLenBytes > block.length) {
> 592: block = new byte[tagLenBytes];
> 593: }
Is tagLenBytes ever larger than AES block size?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
743:
> 741: }
> 742: GCTR gctrForSToTag = new GCTR(embeddedCipher,
> this.preCounterBlock);
> 743: gctrForSToTag.doFinal(s, 0, tagLenBytes, block, 0);
Same comments as earlier.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
748:
> 746: int mismatch = 0;
> 747: for (int i = 0; i < tagLenBytes; i++) {
> 748: mismatch |= tag[i] ^ block[i];
block->s?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
605:
> 603: int len = src.remaining();
> 604: dst.mark();
> 605: if (len > MAX_BUF_SIZE - tagLenBytes) {
Missing the bytes in ibuffer for this check?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
610:
> 608: }
> 609:
> 610: if (dst.remaining() < len + tagLenBytes) {
Missing the bytes in ibuffer for this check?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
614:
> 612: }
> 613:
> 614: checkDataLength(processed, len);
It seems that both checks (line 605 and 614) can be combined into:
checkDataLength(processed, Math.addExact(len, tagLenBytes));
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
630:
> 628: if (tagLenBytes > block.length) {
> 629: block = new byte[tagLenBytes];
> 630: }
Again, will this ever happen? Can just use 's' instead of 'block' from here and
below?
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
635:
> 633: dst.put(block, 0, tagLenBytes);
> 634:
> 635: return (processed + tagLenBytes);
Is it supposed to return "all data processed + tag len"? Normally, we return
the output size produced by this
particular call instead of all accumulated.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
617:
> 615:
> 616: processAAD();
> 617: if (len > 0) {
Even if (len == 0), we should still process the data stored into 'ibuffer'? It
seems that both of the
encrypt(ByteBuffer) and encryptFinal(ByteBuffer) are adapted from their
counterpart with byte[] arguments. However, the
byte[] methods have different entrant conditions due to the buffering in
CipherCore. So the impl of the ByteBuffer ones
may need additional logic to handle all possible calling sequence.
src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line
621:
> 619: null : ByteBuffer.wrap(ibuffer.toByteArray()), src,
> dst);
> 620: dst.reset();
> 621: ghashAllToS.doLastBlock(dst, processed);
Are we sure about using "processed" here? I'd expect the value is the number of
bytes written into dst in the
doLastBlock(...) call on line 618. Is the "processed" variable used differently
in ByteBuffer case?
-------------
PR: https://git.openjdk.java.net/jdk/pull/411
