On Fri, 14 Jun 2024 13:11:06 GMT, Mark Powers <mpow...@openjdk.org> wrote:
>> https://bugs.openjdk.org/browse/JDK-8333364 > > Mark Powers has updated the pull request incrementally with one additional > commit since the last revision: > > move variables to above try block Some comments after reviewing part of it. src/java.base/share/classes/com/sun/crypto/provider/ChaCha20Cipher.java line 1261: > 1259: * > 1260: * @throws ShortBufferException if there is insufficient room to > 1261: * write the tag. >From line 751, `engineUpdate` can throw `ShortBufferExc` but wraps it in a >`ProviderException`. Is that case possible? If so, we should change the >@throws spec to a `ProviderException` but remove it from the throws line since >it is a runtime exc. src/java.base/share/classes/com/sun/crypto/provider/ChaCha20Poly1305Parameters.java line 210: > 208: HexDumpEncoder encoder = new HexDumpEncoder(); > 209: return LINE_SEP + "nonce:" + > 210: LINE_SEP + "[" + encoder.encodeBuffer(nonce) + "]"; This is probably not a performance sensitive method, but another fix would be to keep `StringBuilder` but call append() for each part of the String. Causes fewer objects to be created. I could accept either fix though unless there is an obvious performance issue. src/java.base/share/classes/com/sun/crypto/provider/DHKeyAgreement.java line 175: > 173: * Diffie-Hellman between 2 parties, this would be the other party's > 174: * Diffie-Hellman public key. > 175: * @param lastPhase flag which indicates whether this is the last s/whether/if/ src/java.base/share/classes/com/sun/crypto/provider/FeedbackCipher.java line 160: > 158: int encryptFinal(byte[] plain, int plainOffset, int plainLen, > 159: byte[] cipher, int cipherOffset) > 160: throws IllegalBlockSizeException, ShortBufferException { Why was SBE removed? src/java.base/share/classes/com/sun/crypto/provider/GHASH.java line 275: > 273: * This is an intrinsified method. The method's argument list must > match > 274: * the hotspot signature. This method and methods called by it, > cannot > 275: * throw exceptions or allocate arrays as it will break intrinsics Add period to end of sentence. src/java.base/share/classes/com/sun/crypto/provider/HmacCore.java line 86: > 84: } > 85: } > 86: } catch (NoSuchAlgorithmException ignored) { Debatable whether it is ok to remove that line. If code is ever added after this line and within this for block, then this code could behave differently because it would not be the same as continue. ------------- PR Review: https://git.openjdk.org/jdk/pull/19535#pullrequestreview-2136470781 PR Review Comment: https://git.openjdk.org/jdk/pull/19535#discussion_r1651440877 PR Review Comment: https://git.openjdk.org/jdk/pull/19535#discussion_r1651443179 PR Review Comment: https://git.openjdk.org/jdk/pull/19535#discussion_r1651450723 PR Review Comment: https://git.openjdk.org/jdk/pull/19535#discussion_r1651454821 PR Review Comment: https://git.openjdk.org/jdk/pull/19535#discussion_r1651455506 PR Review Comment: https://git.openjdk.org/jdk/pull/19535#discussion_r1651457973
