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

[Anthony Scarpino]

On Tue, 1 Dec 2020 23:14:19 GMT, Valerie Peng <valeriep at openjdk.org> wrote:

>> Anthony Scarpino has updated the pull request incrementally with five additional commits since the last revision:
>> 
>>  - test updates
>>  - test check mixup
>>  - Overlap protection
>>  - Updated code comments, all tests pass
>>  - Updated code comments, all tests pass
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 657:
> 
>> 655:      */
>> 656:     int decrypt(byte[] in, int inOfs, int len, byte[] out, int outOfs) {
>> 657:         checkDataLength(ibuffer.size(), len);
> 
> Just curious, why remove this checkDataLength(...) call? Seems inconsistent to other methods?

All decrypt() does is put the data into the ibuffer.  I didn't find it useful to calculate that data lengths for the GCM rollover when decryption was not happening.  It's probably a personal preference to wait until doFinal().  The situation when ibuffer is > 64M is very unlikely.

> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 910:
> 
>> 908:         // Decrypt the all the input data and put it into dst
>> 909:         doLastBlock(buffer, ct, dst);
>> 910:         dst.limit(dst.position());
> 
> Based on Cipher javadoc, the output buffer's limit should remain the same, just its position be advanced by the number of bytes written into it. So, we should not call limit(int) to set a new limit on user-specified ByteBuffer objects.

ok

> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java line 935:
> 
>> 933:      */
>> 934:     ByteBuffer overlapDetection(ByteBuffer src, ByteBuffer dst) {
>> 935:         if (src.isDirect() && dst.isDirect()) {
> 
> originalDst is only set to null at construction time. It should be reset here or in restoreDst(). Otherwise, there may be some strange interaction between various ByteBuffer calls using the same Cipher object. Say, the first call uses an extra copy and set originalDst, then the second call does not, but originalDst is still non-null.

Yeah, that makes sense.

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

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