Dmitry Eremin-Solenikov dbaryshkov@gmail.com writes:
--- a/md5.c +++ b/md5.c @@ -56,19 +56,19 @@ md5_init(struct md5_ctx *ctx) 0x98badcfe, 0x10325476, };
- memcpy(ctx->state, iv, sizeof(ctx->state));
- ctx->count = 0;
- memcpy(ctx->state.state, iv, sizeof(ctx->state.state));
- ctx->state.count = 0; ctx->index = 0;
}
Could have the same memcpy handle all of state.
--- a/md5.h +++ b/md5.h @@ -53,10 +53,15 @@ extern "C" { /* Digest is kept internally as 4 32-bit words. */ #define _MD5_DIGEST_LENGTH 4
-struct md5_ctx +struct md5_state { uint32_t state[_MD5_DIGEST_LENGTH]; uint64_t count; /* Block count */ +};
+struct md5_ctx +{
- struct md5_state state; uint8_t block[MD5_BLOCK_SIZE]; /* Block buffer */ unsigned index; /* Into buffer */
};
When reworking these structs, I think we should generally have index before block, since index has a larger required alignment. Even if it doesn't matter in this particular case, with MD5_BLOCK_SIZE == 64.
I wonder if we can find some better naming. "state.state" isn't so pretty, and I think the conventional terminology would be to refer to the 4 32-bit words as the md5 "state", not including the block count. What we're trying to capture actually is somewhat hmac-specific.
It's the part that needs to be copied to the start at an md5_ctx to reset it to some block boundary, and the reason we need a named type (rather than offsetof(struct md5_ctx, block), is that code needs to allocate it. Perhaps
struct md5_init_state { uint32_t state[_MD5_DIGEST_LENGTH]; uint64_t count; /* Block count */ unsigned index; };
or struct md5_internal_ctx_no_buffer or so.
Then both plain md5 reset and md5-hmac could use a single memcpy. And we could use some internal variant of md5_digest to avoid the redundant memcpy done by md5_digest (in your patch set, the hmac code depends on md5_digest resetting the index field, while it overwrites the rest of the state).
I'm starting to think that it probably was a mistake to advertise and document the internal hmac_set_key, hmac_update and hmac_digest methods. Maybe we can deprecate them (without immediately breaking them); I find no usage on codesearch.debian.net. We'de get more flexibility if we could implement hmac_md5_* without going via struct nettle_hash nettle_md5.
I think it would make sense to start with reordering fields in the current context structs.
Regards, /Niels