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
