Niels Möller nisse@lysator.liu.se writes:
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.
I agree that the current convention is a bit awkward, so in the attached patch I changed to use index == 0 as the indicator where buffering is needed. That actually makes the code simpler as we can defer buffering until when the data is read. One drawback though is that it causes additional memcpy in a corner case where the _shake_output is used to retrieve data smaller than the block size.
- 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.
Right, I can do that after the current patch is settled.
- 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.
I'm actually not sure how this works. For example, if unsigned int is 32-bit and index is 3, wouldn't ~index turn to 0xfffffffc, while index | INDEX_HIGH_BIT is 0x80000003?
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.
Yes.
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.
I agree; we probably shouldn't expose sha3_128_digest et. al., from the API.
- 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.
Indeed, fixed.
Regards,