On Wed, 7 Oct 2020 22:38:21 GMT, Valerie Peng <valer...@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
