Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-19 Thread Valerie Peng
On Wed, 18 Nov 2020 05:14:11 GMT, Anthony Scarpino  
wrote:

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
>> line 560:
>> 
>>> 558: System.arraycopy(buffer, 0, block, 0, blockSize);
>>> 559: buflen -= block.length;
>>> 560: return 0;
>> 
>> Will bufLen > blockSize? Judging from the context of this method, 
>> buffer.length should always <= blockSize and  never be larger? If bufLen == 
>> blockSize, perhaps we can just use buffer directly and no need to copy.
>
> This method is being removed

Ok.

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-19 Thread Valerie Peng
On Wed, 18 Nov 2020 04:28:38 GMT, Anthony Scarpino  
wrote:

>> The current impl of constructBlock() seems to have code handling ibuffer >= 
>> blocksize scenario. Could you fix that and also pass the input offset into 
>> constructBlock() for this RFE?
>
> I redid the whole code

Sure, I will be waiting for the next commit then.

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-17 Thread Anthony Scarpino
On Fri, 13 Nov 2020 21:00:20 GMT, Valerie Peng  wrote:

>> Anthony Scarpino has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Code review comment update
>>   Major change to test to detect corruption with incremental buffers test
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 560:
> 
>> 558: System.arraycopy(buffer, 0, block, 0, blockSize);
>> 559: buflen -= block.length;
>> 560: return 0;
> 
> Will bufLen > blockSize? Judging from the context of this method, 
> buffer.length should always <= blockSize and  never be larger? If bufLen == 
> blockSize, perhaps we can just use buffer directly and no need to copy.

This method is being removed

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-17 Thread Anthony Scarpino
On Mon, 16 Nov 2020 19:23:20 GMT, Valerie Peng  wrote:

>> You don't know what 'len', which includes ibuffer data, until this point in 
>> the code.
>
> Well, didn't you already calcuate the 'len' value in the beginning of the 
> copied-from-GCTR RuntimeException check?

I'm removing the runtime check in the next update, it was duplicate of the 
ShortBufferException.  Calculating 'len' through the if() above line 818 while 
it is prepares the bytebuffers is good enough.

>> I thought so too, but that isn't what GCTR returns.  All the GCTR checks are 
>> RuntimeExceptions.  This check was original inside of GCTR, but I had to 
>> bring it out because of the ibuffer lengths.  I don't mind changing, it's 
>> just a strange inconsistency.
>
> GaloisCounterMode is the caller class of GCTR and this API is meant to throw 
> ShortBufferException I think (based on the check at lines below at 820-822)...

Yes.. this check is not necessary given the ShortBufferException was already 
checked.  I am removing this as part of the next update

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-17 Thread Anthony Scarpino
On Mon, 16 Nov 2020 19:17:55 GMT, Valerie Peng  wrote:

>> This is a very seldom used code path,  There was only one existing test that 
>> accesses this code and it has not offset.
>> 
>> constructBlock() is a generic method.  There should be no case where ibuffer 
>> is >= blocksize during this method.   For encryption the two encrypt() only 
>> put code in ibuffer.  This method is for normal update() operations.  The 
>> other encrypt is for any of the CipherCore internal buffer needs sent to GCM 
>> before doing the doFinal.  At that point, ibuffer can be >= blocksize and 
>> doFinal will handle it.  Some of this complication will go away with phase 2 
>> of separating GCM from CipherCore
>
> The current impl of constructBlock() seems to have code handling ibuffer >= 
> blocksize scenario. Could you fix that and also pass the input offset into 
> constructBlock() for this RFE?

I redid the whole code

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-17 Thread Anthony Scarpino
On Thu, 12 Nov 2020 20:17:39 GMT, Valerie Peng  wrote:

>> checkOutputCapacity:  Yes.. The method includes the offsets for the output 
>> buffer, which I believe would verify that the output area in the buffer with 
>> offsets is large enough.
>> 
>> outWithPadding:  I understand the situation and I am assuming there are 
>> tests that cover this case.  Given it's a generic situation.
>
> Have you tested the outWithPadding situation? Given that the existing impl 
> only write out the final result, I don't think you can assume that existing 
> tests cover it. I have wrote a simple test to check it if you have not done 
> so, can you try it out to be sure?
> 
> import java.io.PrintStream;
> import java.util.*;
> import java.security.*;
> import java.security.spec.*;
> 
> import javax.crypto.*;
> import javax.crypto.spec.*;
> 
> public class TestDoFinal {
> 
> private static String ALGO = "AES";
> private static int BLK_SIZE = 16;
> 
> public static void main(String args[]) throws Exception {
> 
> byte[] in = new byte[32];
> Arrays.fill(in, (byte)8);
> KeyGenerator kg = KeyGenerator.getInstance(ALGO, "SunJCE");
> SecretKey skey = kg.generateKey();
> Cipher ci = Cipher.getInstance(ALGO + "/CBC/PKCS5Padding", "SunJCE");
> ci.init(Cipher.ENCRYPT_MODE, skey);
> int inLen = in.length - BLK_SIZE;
> byte[] out = ci.doFinal(in, 0, inLen);
> System.out.println("=> enc " + inLen + " bytes, ret " +
> (out == null? "null":(out.length + " byte")));
> 
> AlgorithmParameters param = ci.getParameters();
> ci.init(Cipher.DECRYPT_MODE, skey, param);
> int rLen = ci.doFinal(out, 0, out.length, in);
> System.out.println("=> dec " + out.length + " bytes, ret " +
> rLen + " byte");
> // check if more than rLen bytes are written into 'in'
> for (int j = rLen; j < in.length; j++) {
> if (in[j] != (byte)8) {
> throw new Exception("Value check failed at index " + j);
> }
> }
> System.out.println("Test Passed");
> }
> }

I tried to fix this, and I did for this test, but there other situations with 
update() that weren't working.  It would take some reworking of a few common 
methods during the doFinal process to handle this right.  I'm going to put an 
'if()" so non-GCM modes create a new buffer like it did before.  It was a "nice 
to have' for this rfe that can be done with future work for other mode 
optimizations.

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-16 Thread Valerie Peng
On Sat, 14 Nov 2020 00:33:35 GMT, Anthony Scarpino  
wrote:

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
>> line 818:
>> 
>>> 816: // do this check here can also catch the potential integer 
>>> overflow
>>> 817: // scenario for the subsequent output buffer capacity check.
>>> 818: checkDataLength(0, len);
>> 
>> Perhaps this can be moved up to the beginning of this method just like the 
>> dst size check?
>
> You don't know what 'len', which includes ibuffer data, until this point in 
> the code.

Well, didn't you already calcuate the 'len' value in the beginning of the 
copied-from-GCTR RuntimeException check?

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-16 Thread Valerie Peng
On Fri, 13 Nov 2020 22:43:17 GMT, Anthony Scarpino  
wrote:

>> The assumption of this whole block here seems to be that ibuffer would not 
>> contain more than a block of buffered data? If that's the case, maybe we can 
>> just use 'ibuffer' instead of allocating a local 'block' and copy the data 
>> into it every time?
>
> This is a very seldom used code path,  There was only one existing test that 
> accesses this code and it has not offset.
> 
> constructBlock() is a generic method.  There should be no case where ibuffer 
> is >= blocksize during this method.   For encryption the two encrypt() only 
> put code in ibuffer.  This method is for normal update() operations.  The 
> other encrypt is for any of the CipherCore internal buffer needs sent to GCM 
> before doing the doFinal.  At that point, ibuffer can be >= blocksize and 
> doFinal will handle it.  Some of this complication will go away with phase 2 
> of separating GCM from CipherCore

The current impl of constructBlock() seems to have code handling ibuffer >= 
blocksize scenario. Could you fix that and also pass the input offset into 
constructBlock() for this RFE?

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-16 Thread Valerie Peng
On Sat, 14 Nov 2020 00:29:41 GMT, Anthony Scarpino  
wrote:

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
>> line 777:
>> 
>>> 775: if ((src.remaining() + ((ibuffer != null) ? ibuffer.size() : 
>>> 0) -
>>> 776: tagLenBytes) > dst.remaining()) {
>>> 777: throw new RuntimeException("output buffer too small");
>> 
>> Shouldn't this be ShortBufferException instead of RuntimeException?
>
> I thought so too, but that isn't what GCTR returns.  All the GCTR checks are 
> RuntimeExceptions.  This check was original inside of GCTR, but I had to 
> bring it out because of the ibuffer lengths.  I don't mind changing, it's 
> just a strange inconsistency.

GaloisCounterMode is the caller class of GCTR and this API is meant to throw 
ShortBufferException I think (based on the check at lines below at 820-822)...

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
>> line 540:
>> 
>>> 538: // remainder offset is based on original buffer length
>>> 539: ibuffer.write(in, inOfs + inLen, remainder);
>>> 540: }
>> 
>> I wonder if this can be moved down for better readability, i.e. process data 
>> in multiple of blocks, and store the remaining into 'ibuffer'?
>
> I tried to, but I don't like how the variable line up doing the remainder 
> afterwards.  I put some hopefully better comments above each section

Ok.

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-13 Thread Anthony Scarpino
On Fri, 13 Nov 2020 22:25:13 GMT, Valerie Peng  wrote:

>> Anthony Scarpino has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Code review comment update
>>   Major change to test to detect corruption with incremental buffers test
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 777:
> 
>> 775: if ((src.remaining() + ((ibuffer != null) ? ibuffer.size() : 0) 
>> -
>> 776: tagLenBytes) > dst.remaining()) {
>> 777: throw new RuntimeException("output buffer too small");
> 
> Shouldn't this be ShortBufferException instead of RuntimeException?

I thought so too, but that isn't what GCTR returns.  All the GCTR checks are 
RuntimeExceptions.  This check was original inside of GCTR, but I had to bring 
it out because of the ibuffer lengths.  I don't mind changing, it's just a 
strange inconsistency.

> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 822:
> 
>> 820: if (len > dst.remaining()) {
>> 821: throw new ShortBufferException("Output buffer too small");
>> 822: }
> 
> How is this different from the check at line 775-778 at the beginning of this 
> method?

Ah.. good.. I'll remove the one above from GCTR as I liked this check better 
and it protects the GCTR update ()

> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 818:
> 
>> 816: // do this check here can also catch the potential integer 
>> overflow
>> 817: // scenario for the subsequent output buffer capacity check.
>> 818: checkDataLength(0, len);
> 
> Perhaps this can be moved up to the beginning of this method just like the 
> dst size check?

You don't know what 'len', which includes ibuffer data, until this point in the 
code.

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-13 Thread Anthony Scarpino
On Fri, 13 Nov 2020 20:47:53 GMT, Valerie Peng  wrote:

>> Anthony Scarpino has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Code review comment update
>>   Major change to test to detect corruption with incremental buffers test
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 540:
> 
>> 538: // remainder offset is based on original buffer length
>> 539: ibuffer.write(in, inOfs + inLen, remainder);
>> 540: }
> 
> I wonder if this can be moved down for better readability, i.e. process data 
> in multiple of blocks, and store the remaining into 'ibuffer'?

I tried to, but I don't like how the variable line up doing the remainder 
afterwards.  I put some hopefully better comments above each section

> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 563:
> 
>> 561: } else {
>> 562: System.arraycopy(buffer, 0, block, 0, buflen);
>> 563: System.arraycopy(in, buflen, block, buflen,
> 
> Use 'bufLen' as the offset for 'in' looks incorrect?

Yes, it should be in's offset

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-13 Thread Anthony Scarpino
On Fri, 13 Nov 2020 20:54:15 GMT, Valerie Peng  wrote:

>> Anthony Scarpino has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Code review comment update
>>   Major change to test to detect corruption with incremental buffers test
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 507:
> 
>> 505: processAAD();
>> 506: // 'len' stores the length to use with buffer 'in'.
>> 507: // 'inLen' stores the length returned by the method.
> 
> line 551 has "return len;" which seems conflicting with these two-line 
> comments here?

The comment is backwards.  I wrote the comment before I decided to switch the 
meaning of the variables and never updated the comment

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-13 Thread Valerie Peng
On Thu, 12 Nov 2020 16:34:30 GMT, Anthony Scarpino  
wrote:

>> 8253821: Improve ByteBuffer performance with GCM
>
> Anthony Scarpino has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Code review comment update
>   Major change to test to detect corruption with incremental buffers test

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

> 820: if (len > dst.remaining()) {
> 821: throw new ShortBufferException("Output buffer too small");
> 822: }

How is this different from the check at line 775-778 at the beginning of this 
method?

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

> 816: // do this check here can also catch the potential integer 
> overflow
> 817: // scenario for the subsequent output buffer capacity check.
> 818: checkDataLength(0, len);

Perhaps this can be moved up to the beginning of this method just like the dst 
size check?

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-13 Thread Anthony Scarpino
On Fri, 13 Nov 2020 20:13:42 GMT, Valerie Peng  wrote:

>> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
>> line 518:
>> 
>>> 516: ArrayUtil.nullAndBoundsCheck(out, outOfs, inLen);
>>> 517: byte[] block = new byte[blockSize];
>>> 518: int inLenUsed = constructBlock(ibuffer.toByteArray(), in, 
>>> block);
>> 
>> constructBlock takes 'in' but not 'inOfs'? Wouldn't the data be taken from 
>> the wrong index? No test catches this, strange?
>
> The assumption of this whole block here seems to be that ibuffer would not 
> contain more than a block of buffered data? If that's the case, maybe we can 
> just use 'ibuffer' instead of allocating a local 'block' and copy the data 
> into it every time?

This is a very seldom used code path,  There was only one existing test that 
accesses this code and it has not offset.

constructBlock() is a generic method.  There should be no case where ibuffer is 
>= blocksize during this method.   For encryption the two encrypt() only put 
code in ibuffer.  This method is for normal update() operations.  The other 
encrypt is for any of the CipherCore internal buffer needs sent to GCM before 
doing the doFinal.  At that point, ibuffer can be >= blocksize and doFinal will 
handle it.  Some of this complication will go away with phase 2 of separating 
GCM from CipherCore

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-13 Thread Valerie Peng
On Thu, 12 Nov 2020 16:34:30 GMT, Anthony Scarpino  
wrote:

>> 8253821: Improve ByteBuffer performance with GCM
>
> Anthony Scarpino has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Code review comment update
>   Major change to test to detect corruption with incremental buffers test

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

> 775: if ((src.remaining() + ((ibuffer != null) ? ibuffer.size() : 0) -
> 776: tagLenBytes) > dst.remaining()) {
> 777: throw new RuntimeException("output buffer too small");

Shouldn't this be ShortBufferException instead of RuntimeException?

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-13 Thread Valerie Peng
On Thu, 12 Nov 2020 16:34:30 GMT, Anthony Scarpino  
wrote:

>> 8253821: Improve ByteBuffer performance with GCM
>
> Anthony Scarpino has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Code review comment update
>   Major change to test to detect corruption with incremental buffers test

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

> 558: System.arraycopy(buffer, 0, block, 0, blockSize);
> 559: buflen -= block.length;
> 560: return 0;

Will bufLen > blockSize? Judging from the context of this method, buffer.length 
should always <= blockSize and  never be larger? If bufLen == blockSize, 
perhaps we can just use buffer directly and no need to copy.

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

> 557: if (buflen >= blockSize) {
> 558: System.arraycopy(buffer, 0, block, 0, blockSize);
> 559: buflen -= block.length;

bufLen isn't used, why bother updating its value?

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-13 Thread Valerie Peng
On Thu, 12 Nov 2020 16:34:30 GMT, Anthony Scarpino  
wrote:

>> 8253821: Improve ByteBuffer performance with GCM
>
> Anthony Scarpino has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Code review comment update
>   Major change to test to detect corruption with incremental buffers test

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

> 561: } else {
> 562: System.arraycopy(buffer, 0, block, 0, buflen);
> 563: System.arraycopy(in, buflen, block, buflen,

Use 'bufLen' as the offset for 'in' looks incorrect?

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-13 Thread Valerie Peng
On Thu, 12 Nov 2020 16:34:30 GMT, Anthony Scarpino  
wrote:

>> 8253821: Improve ByteBuffer performance with GCM
>
> Anthony Scarpino has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Code review comment update
>   Major change to test to detect corruption with incremental buffers test

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

> 505: processAAD();
> 506: // 'len' stores the length to use with buffer 'in'.
> 507: // 'inLen' stores the length returned by the method.

line 551 has "return len;" which seems conflicting with these two-line comments 
here?

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-13 Thread Valerie Peng
On Thu, 12 Nov 2020 16:34:30 GMT, Anthony Scarpino  
wrote:

>> 8253821: Improve ByteBuffer performance with GCM
>
> Anthony Scarpino has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Code review comment update
>   Major change to test to detect corruption with incremental buffers test

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

> 538: // remainder offset is based on original buffer length
> 539: ibuffer.write(in, inOfs + inLen, remainder);
> 540: }

I wonder if this can be moved down for better readability, i.e. process data in 
multiple of blocks, and store the remaining into 'ibuffer'?

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-13 Thread Valerie Peng
On Fri, 13 Nov 2020 20:11:45 GMT, Valerie Peng  wrote:

>> Anthony Scarpino has updated the pull request incrementally with one 
>> additional commit since the last revision:
>> 
>>   Code review comment update
>>   Major change to test to detect corruption with incremental buffers test
>
> src/java.base/share/classes/com/sun/crypto/provider/GaloisCounterMode.java 
> line 518:
> 
>> 516: ArrayUtil.nullAndBoundsCheck(out, outOfs, inLen);
>> 517: byte[] block = new byte[blockSize];
>> 518: int inLenUsed = constructBlock(ibuffer.toByteArray(), in, 
>> block);
> 
> constructBlock takes 'in' but not 'inOfs'? Wouldn't the data be taken from 
> the wrong index? No test catches this, strange?

The assumption of this whole block here seems to be that ibuffer would not 
contain more than a block of buffered data? If that's the case, maybe we can 
just use 'ibuffer' instead of allocating a local 'block' and copy the data into 
it every time?

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-13 Thread Valerie Peng
On Thu, 12 Nov 2020 16:34:30 GMT, Anthony Scarpino  
wrote:

>> 8253821: Improve ByteBuffer performance with GCM
>
> Anthony Scarpino has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Code review comment update
>   Major change to test to detect corruption with incremental buffers test

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

> 516: ArrayUtil.nullAndBoundsCheck(out, outOfs, inLen);
> 517: byte[] block = new byte[blockSize];
> 518: int inLenUsed = constructBlock(ibuffer.toByteArray(), in, 
> block);

constructBlock takes 'in' but not 'inOfs'? Wouldn't the data be taken from the 
wrong index? No test catches this, strange?

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-12 Thread Valerie Peng
On Mon, 2 Nov 2020 22:30:45 GMT, Anthony Scarpino  wrote:

>>> * It was my expectation that checkOutputCapacity() is making sure all 
>>> the inOfs ==<> outOfs are going to work. Does that not catch all cases?
>> checkOutputCapacity() as the name has, only changes that the output buffer 
>> is large enough.
>> 
>>> * 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?
>> 
>> For example,
>> 1) various input/output offset combinations, e.g. inOfs <,=,> outOfs, 
>> 2) Given a output buffer of 200-byte and outOfs == 0, assuming the returned 
>> decrypted data length is 160 bytes, the bytes from index 160 and onward 
>> should not be overwritten. GCM has no padding, so you may not notice any 
>> difference if this is what you are testing. This is generic CipherCore code 
>> and changes impact all ciphers.
>
> checkOutputCapacity:  Yes.. The method includes the offsets for the output 
> buffer, which I believe would verify that the output area in the buffer with 
> offsets is large enough.
> 
> outWithPadding:  I understand the situation and I am assuming there are tests 
> that cover this case.  Given it's a generic situation.

Have you tested the outWithPadding situation? Given that the existing impl only 
write out the final result, I don't think you can assume that existing tests 
cover it. I have wrote a simple test to check it if you have not done so, can 
you try it out to be sure?

import java.io.PrintStream;
import java.util.*;
import java.security.*;
import java.security.spec.*;

import javax.crypto.*;
import javax.crypto.spec.*;

public class TestDoFinal {

private static String ALGO = "AES";
private static int BLK_SIZE = 16;

public static void main(String args[]) throws Exception {

byte[] in = new byte[32];
Arrays.fill(in, (byte)8);
KeyGenerator kg = KeyGenerator.getInstance(ALGO, "SunJCE");
SecretKey skey = kg.generateKey();
Cipher ci = Cipher.getInstance(ALGO + "/CBC/PKCS5Padding", "SunJCE");
ci.init(Cipher.ENCRYPT_MODE, skey);
int inLen = in.length - BLK_SIZE;
byte[] out = ci.doFinal(in, 0, inLen);
System.out.println("=> enc " + inLen + " bytes, ret " +
(out == null? "null":(out.length + " byte")));

AlgorithmParameters param = ci.getParameters();
ci.init(Cipher.DECRYPT_MODE, skey, param);
int rLen = ci.doFinal(out, 0, out.length, in);
System.out.println("=> dec " + out.length + " bytes, ret " +
rLen + " byte");
// check if more than rLen bytes are written into 'in'
for (int j = rLen; j < in.length; j++) {
if (in[j] != (byte)8) {
throw new Exception("Value check failed at index " + j);
}
}
System.out.println("Test Passed");
}
}

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-12 Thread Anthony Scarpino
On Fri, 23 Oct 2020 16:35:10 GMT, Anthony Scarpino  
wrote:

>> I will take a look as well.
>
> This is an update for all of Valerie's comments and adding a test to verify 
> different buffer types and configurations

The latest update should address all outstanding comments.  The biggest change 
was to the test, which had major reorganization and created tests that 
increments data sizes for update and doFinal ops byte-by-byte to check for any 
errors.  That should reduce concerns about buffer corruption.

-

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


Re: RFR: 8253821: Improve ByteBuffer performance with GCM [v4]

2020-11-12 Thread Anthony Scarpino
> 8253821: Improve ByteBuffer performance with GCM

Anthony Scarpino has updated the pull request incrementally with one additional 
commit since the last revision:

  Code review comment update
  Major change to test to detect corruption with incremental buffers test

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/411/files
  - new: https://git.openjdk.java.net/jdk/pull/411/files/7c54017f..8580c6ec

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=411=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=411=02-03

  Stats: 690 lines in 8 files changed: 422 ins; 150 del; 118 mod
  Patch: https://git.openjdk.java.net/jdk/pull/411.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/411/head:pull/411

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