On Wed, 5 Jun 2024 03:49:31 GMT, Martin Balao <mba...@openjdk.org> wrote:
>> src/jdk.crypto.cryptoki/share/classes/sun/security/pkcs11/P11Cipher.java
>> line 1183:
>>
>>> 1181: // Temporary buffer to the penultimate block
>>> 1182: ciphertextBuf.put(start, tmp);
>>> 1183: } else {
>>
>> Personally, I find it easier to follow if this code block follows the
>> decrypt case (line 1184-1190), the allocated `tmp` could be smaller, e.g.
>> Suggestion:
>>
>> byte[] tmp = new byte[pad];
>> // .... pp[pp] ffff -> .... ffff pp[pp]
>> ciphertextBuf.get(start, tmp);
>> ciphertextBuf.put(start, ciphertextBuf, end - blockSize,
>> blockSize);
>> ciphertextBuf.put(end - pad, tmp);
>>
>> Have you considered this?
>
> I have no personal preference, but would suggest that if we change it to cut
> the pad, we keep the decryption case aligned.
What I like about this suggestion is that it allows unifying the repeated
logic: the two blocks inside `if (encrypt)` and the corresponding `else` would
become almost identical, allowing an additional abstraction. How about the
following?
private void convertCTSVariant(ByteBuffer ciphertextBuf,
byte[] ciphertextArr, int end) {
// [...]
// [...] omitted code
// [...]
if (ciphertextBuf != null) {
pad = pad == 0 ? blockSize : pad;
if (encrypt) {
// .... pp[pp] ffff -> .... ffff pp[pp]
swapLastTwoBlocks(ciphertextBuf, end, pad, blockSize);
} else {
// .... ffff pp[pp] -> .... pp[pp] ffff
swapLastTwoBlocks(ciphertextBuf, end, blockSize, pad);
}
}
}
private static void swapLastTwoBlocks(ByteBuffer buffer, int end,
int prevBlockLen, int lastBlockLen) {
// .... prevBlock lastBlock -> .... lastBlock prevBlock
int prevBlockStart = end - prevBlockLen - lastBlockLen;
byte[] prevBlockBackup = new byte[prevBlockLen];
buffer.get(prevBlockStart, prevBlockBackup);
buffer.put(prevBlockStart, buffer, end - lastBlockLen, lastBlockLen);
buffer.put(end - prevBlockLen, prevBlockBackup);
}
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18898#discussion_r1627704892
