On Tue, 9 May 2023 14:37:36 GMT, Weijun Wang <wei...@openjdk.org> wrote:
>> Ferenc Rakoczi has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> agreeing with the newest review comments
>
> src/java.base/share/classes/sun/security/provider/HSS.java line 47:
>
>> 45:
>> 46: @Deprecated
>> 47: protected void engineSetParameter(String param, Object value) {
>
> Better to add `@Override` as much as you can.
Done.
> src/java.base/share/classes/sun/security/provider/HSS.java line 97:
>
>> 95:
>> 96: result &= LMSUtils.lmsVerify(lmsPubKey, sig.siglist[sig.Nspk],
>> messageStream.toByteArray());
>> 97: messageStream.reset();
>
> We still need the `messageStream.reset()` call in a `finally` block even if
> there is no `catch` block. It must be called even if an exception is thrown.
Reintroduced try/finally.
> src/java.base/share/classes/sun/security/provider/HSS.java line 114:
>
>> 112: }
>> 113:
>> 114: public LMSPublicKey(byte[] keyArray, int offset, boolean
>> checkExactLength) throws InvalidKeyException {
>
> Two methods are still public.
One removed, the other changed.
> src/java.base/share/classes/sun/security/provider/HSS.java line 255:
>
>> 253: final int otSigType;
>> 254: final LMOTSParams lmotsParams;
>> 255: final int n;
>
> `n` and `p` can be private.
Changed.
> src/java.base/share/classes/sun/security/provider/HSS.java line 376:
>
>> 374: qArr = Arrays.copyOfRange(sigArray, offset, offset + 4);
>> 375: sigOtsType = LMSUtils.fourBytesToInt(sigArray, offset +
>> 4);
>> 376: lmotsParams = LMOTSParams.of(sigOtsType);
>
> Only this single line needs to be put in the `try` block.
OK.
> src/java.base/share/classes/sun/security/provider/HSS.java line 474:
>
>> 472: sha256Fix = new SHA2.SHA256();
>> 473: } else {
>> 474: sha256Fix = null;
>
> Or, shall we throw a `ProviderException` or even an `AssertionError` here?
Doesn't really matter as now it can never be null. I left in the if() in
preparation of adding the SHAKE version, but I removed it now.
> src/java.base/share/classes/sun/security/provider/HSS.java line 540:
>
>> 538: throw new ProviderException("Digest implementation not
>> found", e);
>> 539: }
>> 540: byte[] result = new byte[md.getDigestLength()];
>
> No need to allocate an array here.
True. Removed allocation.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1190064400
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1190060922
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1190061753
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1190061961
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1190062147
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1190062330
PR Review Comment: https://git.openjdk.org/jdk/pull/13691#discussion_r1190062543
