On Thu, 8 Oct 2020 00:13:37 GMT, Anthony Scarpino <ascarp...@openjdk.org> wrote:

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

I wrote a whole new tests to exercise all the buffers with and without offsets. 
 Hopefully that covers the concern.  The test found 4 bugs..

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

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

Reply via email to