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

[Valerie Peng]

On Thu, 8 Oct 2020 03:21:46 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/AESCipher.java line 658:

> 656:             BadPaddingException {
> 657:         return bufferCrypt(input, output, false);
> 658:     }

Is the override of this method for using a different bufferCrypt impl? There is also engineUpdate(ByteBuffer,
ByteBuffer) in CipherSpi, is there a reason for not overriding that here?

src/java.base/share/classes/com/sun/crypto/provider/AESCipher.java line 744:

> 742:                 } else {
> 743:                     return core.doFinal(input, output);
> 744:                 }

It seems this block is the only difference between this method and CipherSpi.bufferCrypt(). Have you considered moving
this special handling to the overridden engineDoFinal(...) method and not duplicating the whole CipherSpi.bufferCrypt()
method here? BTW, instead of using the generic update/doFinal name and then commenting them for GCM usage only, perhaps
it's more enticing to name them as gcmUpdate/gcmDoFinal?

src/java.base/share/classes/com/sun/crypto/provider/CipherCore.java line 820:

> 818:             return cipher.decrypt(src, dst);
> 819:         }
> 820:         return cipher.encrypt(src, dst);

How about return (decrypting? cipher.decrypt(src, dst) : cipher.encrypt(src, dst));

src/java.base/share/classes/com/sun/crypto/provider/CipherCore.java line 816:

> 814:     }
> 815:
> 816:     int update(ByteBuffer src, ByteBuffer dst) throws ShortBufferException {

Is this also one of the "GCM only" methods? If so, same comment as doFinal(ByteBuffer, ByteBuffer)?
Maybe the name should be more specific to avoid misuse.

src/java.base/share/classes/com/sun/crypto/provider/FeedbackCipher.java line 261:

> 259:
> 260:     int encryptFinal(ByteBuffer src, ByteBuffer dst)
> 261:         throws IllegalBlockSizeException, AEADBadTagException,

Tag is generated during encryption, this can't possibly throw AEADBadTagException, copy-n-paste error maybe?

src/java.base/share/classes/com/sun/crypto/provider/CipherCore.java line 1253:

> 1251:         if (decrypting) {
> 1252:             if (buffered > 0) {
> 1253:                 cipher.decrypt(buffer, 0, buffered, new byte[0], 0);

This looks a bit strange? The output buffer is 0-length which would lead to ShortBufferException when the buffered
bytes is enough to produce some output.

src/java.base/share/classes/com/sun/crypto/provider/CipherCore.java line 1258:

> 1256:         } else {
> 1257:             if (buffered > 0) {
> 1258:                 cipher.encrypt(buffer, 0, buffered, new byte[0], 0);

Same comment as above?

src/java.base/share/classes/com/sun/crypto/provider/CipherCore.java line 939:

> 937:             // if the size of specified output buffer is less than
> 938:             // the length of the cipher text, then the current
> 939:             // content of cipher has to be preserved in order for

This change is somewhat dangerous. When using the supplied output buffer directly, you may corrupt its content w/
padding bytes. This optimization should only be applied when no padding is involved. In addition, input and output can
point to the same buffer with all sorts of index combinations, i.e. inOfs == outOfs, inOfs < outOfs, inOfs > outOfs.
With "outWithPadding" approach, no need to check. However, for "internalOutput", data corruption may happen. This kind
of problem can be very hard to diagnose. Have to be very very careful here as this code may impact all ciphers...

src/java.base/share/classes/com/sun/crypto/provider/GCTR.java line 240:

> 238:         }
> 239:     }
> 240: }

See the above red icon? It warns about missing newline at end of this file.

src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 528:

> 526:         }
> 527:
> 528:         ArrayUtil.blockSizeCheck(src.remaining(), blockSize);

Hmm, I am not sure if this check still applies in ByteBuffer case. You are passing the ByteBuffer objs directly from
AESCipher->CipherCore->GaloisCounterMode. This is different from the byte[] case where CipherCore would chop up the
data into blocks and pass the blocks to the underlying FeedbackCipher impl. Perhaps no existing regression tests covers
ByteBuffer inputs w/ non-blocksize data? Otherwise, this should be caught? BTW, why not just use 'len' again? Seems
unnecessary to keep calling src.remaining() in various places in this method.

-------------

PR: https://git.openjdk.java.net/jdk/pull/411