On 05/09/2025 05:06, Collin Funk wrote:
Pádraig Brady <[email protected]> writes:
Would it be better to call EVP_MD_CTX_init() rather than memset() ?
Or even simpler, remove the explicit init and use EVP_DigestInit()
rather than EVP_DigestInit_ex(), which the docs imply does an implicit
init of the ctx.
The OpenSSL API naming convention isn't my favorite, to put it nicely.
My understanding based on the manual is that memset + EVP_DigestInit_ex
is more efficient than the other options [1]:
The functions EVP_DigestInit(), EVP_DigestFinal() and
EVP_MD_CTX_copy() are obsolete but are retained to maintain
compatibility with existing code. New applications should use
EVP_DigestInit_ex(), EVP_DigestFinal_ex() and EVP_MD_CTX_copy_ex()
because they can efficiently reuse a digest context instead of
initializing and cleaning it up on each call and allow non default
implementations of digests to be specified.
Collin
[1] https://docs.openssl.org/master/man3/EVP_DigestInit/#notes
Wow the openssl docs are bad. Looking at the source I see:
On the create side we have:
EVP_MD_CTX_create = EVP_MD_CTX_new() + EVP_MD_CTX_init()
EVP_MD_CTX_new = malloc() + memset(0);
So to best match EVP_MD_CTX_create() we want
memset(0) + EVP_MD_CTX_init()
On the destroy side we have:
EVP_MD_CTX_destroy = EVP_MD_CTX_reset() + free()
EVP_MD_CTX_reset = EVP_MD_CTX_init() (in all openssl versions)
So to best match EVP_MD_CTX_destroy() we want
EVP_MD_CTX_init()
So it's better to call EVP_MD_CTX_init() on the create side,
to avoid any future changes where 0 is not an appropriate starting state.
Also calling EVP_MD_CTX_init() on the destroy side is required
so that memory is freed.
The attached does that, and thus fixes a memory leak in the current code.
I've not applied it though as...
The change to use the stack was presuming the openssl implementation
doesn't do significant allocations otherwise, but it is quite the opposite:
$ valgrind src/cksum -a sha2 -l 256 </dev/null 2>&1 | grep 'heap usage'
total heap usage: 71 allocs, 71 frees, 56,052 bytes allocated
# valgrind src/cksum -a sha3 -l 256 </dev/null 2>&1 | grep 'heap usage'
total heap usage: 4,504 allocs, 1,289 frees, 310,929 bytes allocated
So this suggests we should revert the previous change to stack based,
and go back to the standard heap allocation mechanism.
Also are you kidding me openssl, 4500 allocations!
This seems to be due to the whole provider interface openssl queries
when using the EVP interface, and a cursory look suggests it might
be somewhat bypassed with:
OSSL_LIB_CTX *libctx = OSSL_LIB_CTX_new();
OSSL_PROVIDER *default_prov = OSSL_PROVIDER_load(libctx, "default");
EVP_MD *sha3_256 = EVP_MD_fetch(libctx, "SHA3-256", NULL);
...
But that would be more complication and probably wouldn't save much?
We're not doing anything daft anyway, it's just openssl being daft:
$ valgrind openssl dgst -sha2-256 /dev/null 2>&1 | grep 'heap usage'
total heap usage: 4,628 allocs, 1,405 frees, 286,287 bytes allocated
$ valgrind openssl dgst -sha3-256 /dev/null 2>&1 | grep 'heap usage'
total heap usage: 4,628 allocs, 1,405 frees, 286,591 bytes allocated
So in summary, I'd revert the previous patch,
and we'll figure out how to proceed from there.
cheers,
Padraigdiff --git a/lib/sha3.c b/lib/sha3.c
index e30210fb3e..af7c885f70 100644
--- a/lib/sha3.c
+++ b/lib/sha3.c
@@ -323,9 +323,10 @@ sha3_process_block (const void *buffer, size_t len, struct sha3_ctx *ctx)
void \
sha3_##SIZE##_init_ctx (struct sha3_ctx *ctx) \
{ \
- /* EVP_DigestInit_ex expects all bytes to be zero. */ \
+ /* EVP_MD_CTX_new() returns zeroed memory. */ \
memset (ctx, 0, sizeof *ctx); \
EVP_MD_CTX *evp_ctx = (EVP_MD_CTX *) ctx->evp_ctx_buffer; \
+ EVP_MD_CTX_init (evp_ctx); \
int rc = EVP_DigestInit_ex (evp_ctx, EVP_sha3_##SIZE (), NULL); \
/* This should never fail. */ \
if (rc == 0) \
@@ -350,6 +351,7 @@ sha3_finish_ctx (struct sha3_ctx *ctx, void *resbuf)
EVP_MD_CTX *evp_ctx = (EVP_MD_CTX *) ctx->evp_ctx_buffer;
/* This should never fail. */
int result = EVP_DigestFinal_ex (evp_ctx, resbuf, NULL);
+ EVP_MD_CTX_init (evp_ctx);
if (result == 0)
abort ();
return resbuf;
