RFR: 8333364: Minor cleanup could be done in com.sun.crypto.provider

[Mark Powers]

On Thu, 6 Jun 2024 20:10:10 GMT, Valerie Peng <valeriep at openjdk.org> wrote:

>> https://bugs.openjdk.org/browse/JDK-8333364
>
> src/java.base/share/classes/com/sun/crypto/provider/AESKeyWrap.java line 121:
> 
>> 119:     @Override
>> 120:     int encrypt(byte[] pt, int ptOfs, int ptLen, byte[] ct, int ctOfs) {
>> 121:         throw new UnsupportedOperationException("multi-part not supported");
> 
> Change the error message as well?

Yes. None of our tests check for this error message.

> src/java.base/share/classes/com/sun/crypto/provider/AESKeyWrap.java line 127:
> 
>> 125:     @Override
>> 126:     int decrypt(byte[] ct, int ctOfs, int ctLen, byte[] pt, int ptOfs) {
>> 127:         throw new UnsupportedOperationException("multi-part not supported");
> 
> Change the error message as well?

Yes.

> src/java.base/share/classes/com/sun/crypto/provider/AESKeyWrapPadded.java line 159:
> 
>> 157:     @Override
>> 158:     int encrypt(byte[] pt, int ptOfs, int ptLen, byte[] ct, int ctOfs) {
>> 159:         throw new UnsupportedOperationException("multi-part not supported");
> 
> Change the error message as well?

Yes.

> src/java.base/share/classes/com/sun/crypto/provider/AESKeyWrapPadded.java line 165:
> 
>> 163:     @Override
>> 164:     int decrypt(byte[] ct, int ctOfs, int ctLen, byte[] pt, int ptOfs) {
>> 165:         throw new UnsupportedOperationException("multi-part not supported");
> 
> Change the error message as well?

Yes.

> src/java.base/share/classes/com/sun/crypto/provider/ChaCha20Cipher.java line 678:
> 
>> 676:     protected int engineUpdate(byte[] in, int inOfs, int inLen,
>> 677:             byte[] out, int outOfs) throws ShortBufferException {
>> 678:         int bytesUpdated;
> 
> Not relevant to this line, but rather down below in the javadoc of `engineUpdate(ByteBuffer, ByteBuffer)`, the javadoc has {@code out} but the variable name is `output`, probably a copy-n-paste error from this method.

Fixed.

> src/java.base/share/classes/com/sun/crypto/provider/ChaCha20Cipher.java line 764:
> 
>> 762:     @Override
>> 763:     protected int engineDoFinal(byte[] in, int inOfs, int inLen, byte[] out,
>> 764:             int outOfs) throws ShortBufferException, AEADBadTagException {
> 
> Not relevant to this line, but down below in the javadoc of `engineDoFinal(ByteBuffer, ByteBuffer)`, the method throws `ShortBufferException`, but the javadoc doesn't have it.

Fixed.

> src/java.base/share/classes/com/sun/crypto/provider/CipherCore.java line 874:
> 
>> 872:         int len = Math.addExact(buffered, inputLen);
>> 873:         // calculate padding length
>> 874:         int totalLen = len;
> 
> Well, personally, I'd prefer to replace `len` with `totalLen`, less changes overall and better naming matching the comment on line 871.

Okay.

> src/java.base/share/classes/com/sun/crypto/provider/DHKeyAgreement.java line 408:
> 
>> 406:             if (keysize >= BlowfishConstants.BLOWFISH_MAX_KEYSIZE)
>> 407:                 keysize = BlowfishConstants.BLOWFISH_MAX_KEYSIZE;
>> 408:             return new SecretKeySpec(secret, 0, keysize,
> 
> nit: merge the following line, i.e. "Blowfish", with this line?

Done.

> src/java.base/share/classes/com/sun/crypto/provider/PBES2Parameters.java line 214:
> 
>> 212:     {
>> 213:         String kdfAlgo;
>> 214:         String cipherAlgo;
> 
> nit: merge these with where they are assigned?

Done.

> src/java.base/share/classes/com/sun/crypto/provider/PBES2Parameters.java line 243:
> 
>> 241: 
>> 242:         this.pbes2AlgorithmName = "PBEWith" +
>> 243:                 kdfAlgo + "And" + cipherAlgo;
> 
> nit: move to one line if less than 80 chars.

75 chars. It's now one line.

> src/java.base/share/classes/com/sun/crypto/provider/RC2Parameters.java line 224:
> 
>> 222: 
>> 223:         if (version != 0) {
>> 224:             sb.append(LINE_SEP).append("version:").append(LINE_SEP).append(version).append(LINE_SEP);
> 
> Well, the original code is easier to read. Probably no difference performance-wise given it's just  a few known strings. Why bother especially when the StringBuilder constructor also uses `+`?

The original code really is easier to read. I'll revert the change.

> src/java.base/share/classes/com/sun/crypto/provider/RSACipher.java line 276:
> 
>> 274:         outputSize = n;
>> 275:         bufOfs = 0;
>> 276:         if (Objects.equals(paddingType, PAD_NONE)) {
> 
> Not sure if this is necessary, with the current impl, `paddingType` is always assigned with one of internal PAD_XXX constants. Did I miss something?

You are correct. IJ ignored details of the implementation . `==` will work fine. I'll revert the change.

> src/java.base/share/classes/com/sun/crypto/provider/TlsKeyMaterialGenerator.java line 109:
> 
>> 107:         SecretKey serverMacKey = null;
>> 108:         SecretKey clientCipherKey;
>> 109:         SecretKey serverCipherKey;
> 
> nit: move them down to line 200, e.g. where these two variables are assigned?

No. The two variables wouldn't be in scope for the `finally` block on line 276.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19535#discussion_r1638199883
PR Review Comment: https://git.openjdk.org/jdk/pull/19535#discussion_r1638200163
PR Review Comment: https://git.openjdk.org/jdk/pull/19535#discussion_r1638200674
PR Review Comment: https://git.openjdk.org/jdk/pull/19535#discussion_r1638201294
PR Review Comment: https://git.openjdk.org/jdk/pull/19535#discussion_r1638201606
PR Review Comment: https://git.openjdk.org/jdk/pull/19535#discussion_r1638201931
PR Review Comment: https://git.openjdk.org/jdk/pull/19535#discussion_r1638203464
PR Review Comment: https://git.openjdk.org/jdk/pull/19535#discussion_r1638203212
PR Review Comment: https://git.openjdk.org/jdk/pull/19535#discussion_r1638203036
PR Review Comment: https://git.openjdk.org/jdk/pull/19535#discussion_r1638202768
PR Review Comment: https://git.openjdk.org/jdk/pull/19535#discussion_r1638202592
PR Review Comment: https://git.openjdk.org/jdk/pull/19535#discussion_r1638202361
PR Review Comment: https://git.openjdk.org/jdk/pull/19535#discussion_r1638202186