RFR: 8253821: Improve ByteBuffer performance with GCM [v2]

[Valerie Peng]

On Thu, 8 Oct 2020 06:51:08 GMT, Anthony Scarpino <ascarpino at openjdk.org> 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