RFR: JDK-8338587 - Internal XOF Methods for SHAKE128 and SHAKE256 [v4]

[Ferenc Rakoczi]

On Wed, 28 Aug 2024 13:24:04 GMT, Weijun Wang <weijun at openjdk.org> wrote:

>> Ferenc Rakoczi has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Code style changes suggested by Andrey Turbanov
>
> src/java.base/share/classes/sun/security/provider/SHA3.java line 74:
> 
>> 72:     private final byte suffix;
>> 73: 
>> 74:     // the stae matrix flattened into an array
> 
> Typo: `stat` should be `state`.

Fixed.

> src/java.base/share/classes/sun/security/provider/SHA3.java line 84:
> 
>> 82:     // calls) will set it to 0 at its start.
>> 83:     // When a squeeze() call uses up all available bytes from this state
>> 84:     // and so a new keccak() call is made, squeezeOffset is reset to 0.
> 
> The paragraph only mentions `set to 0` and `reset to 0`, and has not talked about other values. Maybe you can say when the squeeze call returns, it's set to a position that states before it have been used.

Well, the comment starts with "The byte offset in the state where the next squeeze() will start."

> src/java.base/share/classes/sun/security/provider/SHA3.java line 136:
> 
>> 134:      */
>> 135:     void implDigest(byte[] out, int ofs) {
>> 136:         // Moving this allocation to the block where t is used causes a little
> 
> Typo, `t` should be `it`.

Fixed.

> src/java.base/share/classes/sun/security/provider/SHA3.java line 170:
> 
>> 168: 
>> 169:     void implSqueeze(byte[]output, int offset, int numBytes) {
>> 170:         // Moving this allocation to the block where t is used causes a little
> 
> Typo. `t` should be `it`.

Fixed.

> src/java.base/share/classes/sun/security/provider/SHA3.java line 225:
> 
>> 223:                 ((squeezeOffset + numBytes - longOffset * 8) + 7) / 8;
>> 224: 
>> 225:         int limit = longOffset + longsToConvert - (numBytes % 8 == 0 ? 0 : 1);
> 
> Can we also do the same simplification here as in `implDigest`?
> 
>         int numLongs = numBytes / 8;
>         for (int i = longOffset; i < longOffset + numLongs; i++) {
> 
> After the `if` block above, `squeezeOffset` should be exactly `longOffset * 8`.

Fixed as suggested.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20631#discussion_r1735008603
PR Review Comment: https://git.openjdk.org/jdk/pull/20631#discussion_r1735008691
PR Review Comment: https://git.openjdk.org/jdk/pull/20631#discussion_r1735008796
PR Review Comment: https://git.openjdk.org/jdk/pull/20631#discussion_r1735008980
PR Review Comment: https://git.openjdk.org/jdk/pull/20631#discussion_r1735009153