Pádraig Brady <[email protected]> writes: > Wow the openssl docs are bad. Looking at the source I see:
My thoughts were the same, but I did not want to be rude to the openssl folks. :) > 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 Thanks for looking into this and the detailed analysis. I really have zero clue why they deprecated the low-level _Init, _Update, _Final APIs. The EVP stuff really is unessecary for our purposes. > So in summary, I'd revert the previous patch, > and we'll figure out how to proceed from there. Thanks, I'll have a look later today. I'll probably just move SHA-3 to returning bool for errors (e.g. OOM). In coreutils we can xalloc_die, in libraries they decision on how to handle that can be left up to them. Until someone like Emacs tries to use them as a function pointer, it shouldn't matter if the prototypes are different than the other crypto modules. Collin
