On Thu, 6 Jun 2024 20:10:10 GMT, Valerie Peng <valer...@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