On Tue, 14 Oct 2025 23:43:39 GMT, Mark Powers <[email protected]> wrote:
>> [JDK-8343232](https://bugs.openjdk.org/browse/JDK-8343232) > > Mark Powers has updated the pull request incrementally with one additional > commit since the last revision: > > remaining comments These are mostly style-related comments. Please add javadoc comments to new methods, especially in `MacData`. src/java.base/share/classes/com/sun/crypto/provider/PBES2Parameters.java line 251: > 249: keysize = kdfParams.getKeyLength(); > 250: > 251: if (pBES2_params.tag != DerValue.tag_Sequence) { You can probably move this check before line 228. Note that the same check has been performed on line 214 and the only reason to do it again is if `pBES2_params` has been reassigned on line 227. src/java.base/share/classes/sun/security/pkcs12/MacData.java line 65: > 63: * } > 64: * > 65: * </pre> Please move the `@author` line to the end of the javadoc comment. src/java.base/share/classes/sun/security/pkcs12/MacData.java line 135: > 133: } > 134: > 135: private static Mac getMac(String macAlgorithm, char[] password, I would rather let this method return `mac.doFinal(data)`. The method cleans up `pbeKey` in a finally block and I somehow cannot guarantee it would not have any negative impact when the `Mac` object is still alive. There are two places this method is called and the `Mac` object is used for other purposes. For one, its `getAlgorithm` is called and used in a debug log. I think the `macAlgorithm` can be used too. The other called `getMacLength` but that is the same of the length of the `doFinal` output. src/java.base/share/classes/sun/security/pkcs12/MacData.java line 147: > 145: if (macAlgorithm.startsWith("pbewith")) { > 146: m = Mac.getInstance(hmac); > 147: int len = keyLength == 0 ? m.getMacLength()*8 : keyLength; Somehow I prefer to use `-1` for the default case. This is consistent with `PBKDF2Parameters`. src/java.base/share/classes/sun/security/pkcs12/MacData.java line 155: > 153: } else { > 154: hmac = macAlgorithm; > 155: m = Mac.getInstance(hmac); Don't assign to `hmac` and just call `Mac.getInstance(macAlgorithm)`. Mutating `hmac` makes people wondering if it has a purpose. src/java.base/share/classes/sun/security/pkcs12/MacData.java line 160: > 158: pbeKey = skf.generateSecret(keySpec); > 159: } > 160: keySpec.clearPassword(); Put the line above into the `finally` block at the end. Exceptions could be thrown in the lines above. This will also help merging the two `if (macAlgorithm.startsWith("pbewith"))` checks into one. src/java.base/share/classes/sun/security/pkcs12/MacData.java line 208: > 206: String macAlgorithm, int macIterationCount, byte[] salt) > 207: throws IOException, NoSuchAlgorithmException { > 208: final byte[] mData; No need for `mData`. Just `return bytes.toByteArray()` at the end. src/java.base/share/classes/sun/security/pkcs12/MacData.java line 210: > 208: final byte[] mData; > 209: final PBEParameterSpec params; > 210: String algName = "PBMAC1"; No need to assign a value now. Assign it in the `if (macAlgorithm.startsWith("pbewith"))` block. src/java.base/share/classes/sun/security/pkcs12/MacData.java line 214: > 212: String hmac; > 213: Mac m; > 214: int keyLength; Useless. src/java.base/share/classes/sun/security/pkcs12/MacData.java line 225: > 223: } else if (macAlgorithm.startsWith("hmacpbe")) { > 224: algName = macAlgorithm.substring(7); > 225: kdfHmac = macAlgorithm; Assign null to `kdfHmac`. People might wonder it's useful in this case. src/java.base/share/classes/sun/security/pkcs12/MacData.java line 240: > 238: DerOutputStream bytes = new DerOutputStream(); > 239: bytes.write(encode(algName, macResult, params, kdfHmac, hmac, > 240: m.getMacLength())); `m.getLength()` is the same as `macResult`. src/java.base/share/classes/sun/security/pkcs12/PBMAC1Parameters.java line 99: > 97: this.hmacAlgo = o.stdName(); > 98: > 99: DerValue kdf = pBMAC1_params.data.getDerValue(); This is just `info[0]`. You have already read all sub-components on line 77. src/java.base/share/classes/sun/security/pkcs12/PKCS12KeyStore.java line 2074: > 2072: MacData macData = new MacData(s); > 2073: int ic = macData.getIterations(); > 2074: byte[] salt = macData.getSalt(); This is useless. src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 80: > 78: private final int keyLength; > 79: > 80: private String prfAlgo; This can be final. src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 86: > 84: * parameter block. > 85: * > 86: * @param keyDerivationFunc the DER encoding of the parameter block The `@param` name should be `pBKDF2_params`. src/java.base/share/classes/sun/security/util/PBKDF2Parameters.java line 152: > 150: .putOctetString(salt) > 151: .putInteger(iterationCount) > 152: .putInteger(keyLength) When this method is called, `keyLength` can never be -1. Please add a comment or `assert` statement. ------------- PR Review: https://git.openjdk.org/jdk/pull/24429#pullrequestreview-3340688324 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432766065 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432772229 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432791173 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432797416 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432800846 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432803553 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432821945 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432810417 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432823775 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432818517 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432827317 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432835982 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432837929 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432839534 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432841221 PR Review Comment: https://git.openjdk.org/jdk/pull/24429#discussion_r2432845455
