On Sun, 22 Sep 2024 13:37:11 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Kevin Driver has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> spec wording changes and a few tweaks to DPS
>
> src/java.base/share/classes/javax/crypto/KDF.java line 141:
>
>> 139: // @param kdfParameters the parameters
>> 140: private KDF(Delegate delegate, String algorithm,
>> 141: KDFParameters kdfParameters) {
>
> There is really no need to pass in `kdfParameters` in this case. Set
> `this.kdfParameters` to `null`.
Addressed in
https://github.com/openjdk/jdk/pull/20301/commits/315f90e6e8572dfaea0fdb4f5d94452637a7e423.
> src/java.base/share/classes/javax/crypto/KDF.java line 554:
>
>> 552: } catch (NoSuchAlgorithmException e) {
>> 553: // this will never be thrown in the deriveData case
>> 554: throw new RuntimeException(e);
>
> If this will never happen, throw an `AssertionError`.
Addressed in
https://github.com/openjdk/jdk/pull/20301/commits/315f90e6e8572dfaea0fdb4f5d94452637a7e423.
> src/java.base/share/classes/javax/crypto/KDF.java line 584:
>
>> 582: boolean isDeriveData = (algorithm == null);
>> 583:
>> 584: if (checkSpiNonNull(theOne)) {
>
> This check is still needed inside the `synchronized` block below, otherwise
> another thread waiting on the same lock with perform DPS again after the
> first one exits.
>
> In fact, since you already check for the same condition before each
> `chooseProvider` call, it is only necessary inside the `synchronized` block.
Addressed in
https://github.com/openjdk/jdk/pull/20301/commits/315f90e6e8572dfaea0fdb4f5d94452637a7e423.
> src/java.base/share/classes/javax/crypto/KDF.java line 593:
>
>> 591: Exception lastException = null;
>> 592: if (!checkSpiNonNull(candidate)) {
>> 593: throw new RuntimeException("Unexpected Error: candidate
>> is null!");
>
> This should not happen. Make it an `AssertionError`.
Addressed in
https://github.com/openjdk/jdk/pull/20301/commits/315f90e6e8572dfaea0fdb4f5d94452637a7e423.
> src/java.base/share/classes/javax/crypto/KDF.java line 607:
>
>> 605: return result;
>> 606: } catch (Exception e) {
>> 607: if (lastException == null) {
>
> Add some debug output since `e` can be silently discarded.
Addressed in
https://github.com/openjdk/jdk/pull/20301/commits/315f90e6e8572dfaea0fdb4f5d94452637a7e423.
> src/java.base/share/classes/javax/crypto/KDF.java line 618:
>
>> 616: throw e; // getNext reached end and have seen IAPE
>> 617: } catch (NoSuchAlgorithmException e) {
>> 618: // getNext reached end without finding an implementation
>
> Print debug info on `e`. Each time an exception is not added as a cause, it
> makes sense to print it out in the debug log.
Addressed in
https://github.com/openjdk/jdk/pull/20301/commits/315f90e6e8572dfaea0fdb4f5d94452637a7e423.
> src/java.base/share/classes/javax/crypto/KDF.java line 653:
>
>> 651: pdebug.println(
>> 652: "The evaluated provider does not support
>> the "
>> 653: + "specified algorithm and parameters");
>
> `nsae.printStackTrace(pdebug.getPrintStream());`
Addressed in
https://github.com/openjdk/jdk/pull/20301/commits/315f90e6e8572dfaea0fdb4f5d94452637a7e423.
> src/java.base/share/classes/javax/crypto/KDF.java line 662:
>
>> 660: + "specified algorithm and parameters");
>> 661: }
>> 662: if (hasOne) throw new InvalidAlgorithmParameterException();
>
> Since this is an IAPE, there should be a reason what "AP" is "I".
Addressed in
https://github.com/openjdk/jdk/pull/20301/commits/315f90e6e8572dfaea0fdb4f5d94452637a7e423.
> src/java.base/share/classes/javax/crypto/KDF.java line 669:
>
>> 667: return (d != null && d.spi() != null);
>> 668: }
>> 669: }
>
> Add an empty line at the end of this file.
Addressed in
https://github.com/openjdk/jdk/pull/20301/commits/315f90e6e8572dfaea0fdb4f5d94452637a7e423.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1771850266
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1771850411
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1771850571
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1771852068
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1771852332
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1771852496
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1771852776
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1771852666
PR Review Comment: https://git.openjdk.org/jdk/pull/20301#discussion_r1771852908
