On Mon, 2 Nov 2020 19:55:26 GMT, Valerie Peng <valer...@openjdk.org> wrote:
>>> * 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? >> Existing tests may not catch/check all error cases. Especially, in the past, >> all calls w/ ByteBuffer inputs are converted to calls w/ byte[] inputs. >> Thus, testing is focused on byte[] scenarios. In addition, since for >> decryption, internal buffering is done, there may be no test checking if >> padding bytes are written to the output buffer. Please see my comment >> follows. The corruption that I refer to is about the padding bytes which are >> now written into the output buffer with this change. > >> * 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. ------------- PR: https://git.openjdk.java.net/jdk/pull/411
