Hello Niels,
Thank you for the suggestions, all makes sense to me.
Niels Möller nisse@lysator.liu.se writes:
+void +sha3_256_shake_output(struct sha3_256_ctx *ctx,
size_t length,uint8_t *digest)+{
- unsigned offset;
- unsigned mask = UINT_MAX >> 1;
I think I'd name the local variable "index" rather than "offset", to match the state variable. And I think it would make sense with a define for the high bit, something like
#define INDEX_HIGH_BIT (~((UINT_MAX) >> 1))
(one could also use something like ~0U instead of UINT_MAX, but UINT_MAX may be more readable).
Renamed and introduced the macro.
- /* We use the leftmost bit as a flag to indicate SHAKE is initialized. */
- if (ctx->index & ~mask)
- offset = ctx->index & mask;
The value of offset here is in the range 0 < offset <= SHA3_256_BLOCK_SIZE, right? One could use a representation where
offset = ~ctx->index;
instead of bitwise operations. One would still need the condition if (ctx->index & INDEX_HIGH_BIT), but that would typically be compiled to the same as if ((signed int) ctx->index < 0).
I think it would also make sense with an
assert (ctx->index < SHA3_256_BLOCK_SIZE);
in the start of sha3_256_update, which will trigger if the update function is called after the output function, with no init in between.
- else
- {
- _sha3_pad_shake (&ctx->state, SHA3_256_BLOCK_SIZE, ctx->block,
ctx->index);
/* Point at the end of block to trigger fill in of the buffer. */offset = sizeof (ctx->block);I think this block deserves a comment that this is the first call to sha3_256_shake_output. For the block size, I think it would be nice to consitently use one of SHA3_256_BLOCK_SIZE and sizeof (ctx->block).
I chose the latter to not embed algorithm specific constant, as the same code also needs to support SHAKE128 (to be added).
- }
- for (;;)
- {
/* Write remaining data from the buffer. */if (offset < sizeof (ctx->block))- {
unsigned remaining;remaining = MIN(length, sizeof (ctx->block) - offset);memcpy (digest, &ctx->block[offset], remaining);digest += remaining;offset += remaining;I think handling of the leftover can be moved before the loop, and simplified as
unsigned left = sizeof(ctx->block) - offset; if (length <= left) { memcpy (digest, ctx->block + offset, length); ctx->index = (offset + length) | INDEX_HIGH_BIT; return; } memcpy (digest, ctx->block + offset, left); digest += left; length -= left;
followed by a loop
for (; length >= SHA3_256_BLOCK_SIZE; length -= SHA3_256_BLOCK_SIZE, digest += SHA3_256_BLOCK_SIZE) { ... output a full block ... }
if (length > 0) { ... do final partial block ... ctx->index = length | INDEX_HIGH_BIT; } else ctx->index = SHA3_256_BLOCK_SIZE | INDEX_HIGH_BIT;
Yes, this makes the code a lot simpler. I'm attaching an updated patch.
Regards,