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

[Anthony Scarpino]

On Wed, 7 Oct 2020 22:38:21 GMT, Valerie Peng <valeriep at openjdk.org> wrote:

>> 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/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...

- I do not understand where the corruption comes from.  The user provides a buffer that output is suppose to be placed
  into, what could it be corrupting?  The existing tests (SameBuffer, in particular) works fine with this and the
  ByteBuffer calls.  I spent a lot of time trying to get those same buffer tests to pass.
- It was my expectation that checkOutputCapacity() is making sure all the inOfs ==<> outOfs are going to work. Does that
  not catch all cases?
- outWithPadding" is excessive because it doubles the allocation for every operation followed by a copy to the original
  buffer, even if the original buffer was adequate.  I'm ok with doing the extra alloc & copy in certain situations, but
  not everytime.  Can you be more specific what things may fail that we don't already check for in the regression tests?

> 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?

Yes. thanks.. The IDE covered that up by going to the one in CipherSpi and I thought it was calling the AES one.  TLS
doesn't use update() so the perf numbers won't change.  I'll have to run the tests again.

> 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?

I didn't see a way to override this because CipherSpi is a public class, any methods I added would become a new API.
 Also bufferCrypt is private, so I had to copy it.  CipherSpi does not know which mode is being used, but AESCipher
 does.  Maybe I'm missing something, I'd rather it not be a copy, but I couldn't see a better way. If you have a
 specific idea, please give me details.

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

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