On Thu, 8 Oct 2020 03:21:46 GMT, Anthony Scarpino <ascarp...@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
