On Fri, Sep 5, 2025 at 11:10 AM Pádraig Brady <[email protected]> wrote:
> 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. The man page for OPENSSL_init_crypto may help. By default, OpenSSL initializes everything. Maybe a call to OPENSSL_no_config might help, and a call to OPENSSL_init_crypto with just digests. I seem to recall the error strings are often not needed, so a call to init with OPENSSL_INIT_NO_LOAD_CRYPTO_STRINGS and OPENSSL_INIT_NO_LOAD_SSL_STRINGS may be helpful. Also see the other OPENSSL_INIT_NO_LOAD_* parameters. Jeff
