Daiki Ueno ueno@gnu.org writes:
Yes, this makes the code a lot simpler. I'm attaching an updated patch.
Thanks, looks good to me. Some details I'm thinking about that might be improvements:
* One could perhaps use index == 0 instead of index == block_size for the case that there is no buffered data. But the current convention does make your "if (length <= left)" nice and simple.
* It looks a bit backwards to me that each iteration *first* copies data to the digest, and *then* calls sha3_permute. In case no more data is to be output, that sha3_permute call is wasted. It would be more natural to me to not call sha3_permute until we know the output is needed. But to fix that and still keep things nice for the first output block, I think one would need to reorganize _nettle_sha3_pad to not imply a call to sha3_permute (via sha3_absorb). So that's better done in a separate change.
* I'm still tempted to use ctx->index = ~index rather than ctx->index = index | INDEX_HIGH_BIT. But maybe that would just be too obfuscated.
Anything about that you agree with, or that you think should be done now?
In next step, to also support shake128, we should generalize your code using an internal function _sha3_shake_output taking block and block size as arguments.
I'm also not sure about proper naming for shake128. If I read the Instances table at https://en.wikipedia.org/wiki/SHA-3 right, there's no standard regular hash function corresponding to shake128. We could still name it sha3_128_shake, but that might be confusing (there's no corresponding sha3_128_digest, would there be any use for that?). The alternative could be to use names sha3_shakeN_init, sha3_shakeN_update, sha3_shakeN_digest, sha3_shakeN_output (with some of the shake256 functions, as well as the context struct, being aliases to corresponding sha3_256 names). But aliases also have a cost in potential confusion.
- if (length > 0)
- {
/* Fill in the buffer for next call. */_nettle_write_le64 (sizeof (ctx->block), ctx->block, ctx->state.a);sha3_permute (&ctx->state);memcpy (digest, ctx->block, length);ctx->index = length | INDEX_HIGH_BIT;- }
- else
- ctx->index = sizeof (ctx->block) | INDEX_HIGH_BIT;
+}
If I read your code right, we actually always have length > 0 at this place. So either delete the if conditional, or change the condition of the loop above from (length > sizeof (ctx->block)) to (length >= sizeof (ctx->block)). The latter option would avoid a memcpy in the case that the requested digest ends with a full block.
Regards, /Niels