Trivial patch: avoid redundant calls to ENGINE_register_all_complete
hello all, Nick, per the other thread in tor-talk about RDRAND, this is the minor fix for OpenSSL 1.0.1+ mentioned. i don't know that this is useful, and i am still giving the engine code a thorough review per Nick's other feedback: "Above all, do not assume that you understand how OpenSSL works until you have investigated with a debugger, the source code, and a pot of coffee." :) best regards, --- diff --git a/src/common/crypto.c b/src/common/crypto.c index 5afb98e..7c02ea4 100644 --- a/src/common/crypto.c +++ b/src/common/crypto.c @@ -282,7 +282,10 @@ crypto_global_init(int useAccel, const char *accelName, const char *accelDir) log_info(LD_CRYPTO, "Initializing OpenSSL engine support."); ENGINE_load_builtin_engines(); - ENGINE_register_all_complete(); + /* OpenSSL 1.0.1 and newer register complete when engines loaded. */ + if (SSLeay() < OPENSSL_V_SERIES(1,0,1)) { + ENGINE_register_all_complete(); + } if (accelName) { if (accelDir) {
i was able to confirm the expected behavior using the BADRAND engine attached to same ticket: https://trac.torproject.org/projects/tor/ticket/10402 and also here (do trac tickets ever go away / get deleted?): https://peertech.org/dist/openssl-1.0.1e-badrand-test.patch currently Tor on 1.0.1+ will invoke a chain like: Dec 18 13:59:22.000 [info] crypto_global_init(): Initializing OpenSSL engine support. ENGINE_load_builtin_engines called. ENGINE_register_all_complete invoked. ENGINE_register_complete invoked for rsax. ENGINE_register_complete invoked for dynamic. ENGINE_register_all_complete invoked. ENGINE_register_complete invoked for rsax. ENGINE_register_complete invoked for dynamic. . . due to the redundant call to ENGINE_register_all_complete. once patched per above, the call sequence is the expected: Dec 18 14:32:25.000 [info] crypto_global_init(): Initializing OpenSSL engine support. ENGINE_load_builtin_engines called. ENGINE_register_all_complete invoked. ENGINE_register_complete invoked for rsax. ENGINE_register_complete invoked for dynamic. . . log file from successul run post-patch above: https://peertech.org/dist/tor-no-repeat-register-run.txt best regards, On Wed, Dec 18, 2013 at 9:57 AM, coderman <coderman@gmail.com> wrote:
hello all, Nick,
per the other thread in tor-talk about RDRAND, this is the minor fix for OpenSSL 1.0.1+ mentioned.
i don't know that this is useful, and i am still giving the engine code a thorough review per Nick's other feedback: "Above all, do not assume that you understand how OpenSSL works until you have investigated with a debugger, the source code, and a pot of coffee." :)
best regards,
---
diff --git a/src/common/crypto.c b/src/common/crypto.c index 5afb98e..7c02ea4 100644 --- a/src/common/crypto.c +++ b/src/common/crypto.c @@ -282,7 +282,10 @@ crypto_global_init(int useAccel, const char *accelName, const char *accelDir)
log_info(LD_CRYPTO, "Initializing OpenSSL engine support."); ENGINE_load_builtin_engines(); - ENGINE_register_all_complete(); + /* OpenSSL 1.0.1 and newer register complete when engines loaded. */ + if (SSLeay() < OPENSSL_V_SERIES(1,0,1)) { + ENGINE_register_all_complete(); + }
if (accelName) { if (accelDir) {
On Wed, Dec 18, 2013 at 12:57 PM, coderman <coderman@gmail.com> wrote:
hello all, Nick,
per the other thread in tor-talk about RDRAND, this is the minor fix for OpenSSL 1.0.1+ mentioned.
i don't know that this is useful, and i am still giving the engine code a thorough review per Nick's other feedback: "Above all, do not assume that you understand how OpenSSL works until you have investigated with a debugger, the source code, and a pot of coffee." :)
best regards,
Hi, coderman! Is there any actual harm in the redundant invocation of ENGINE_register_all_complete(), or is this about cleanliness/saving cycles/something else? Assuming it's not actually harmful, I'd prefer to leave the call in, with a comment that it isn't necessary any longer. When crypto libraries make a formerly necessary thing optional, I often find that leaving it in can make us a little less bug prone in the long run. peace, -- Nick
On Wed, Dec 18, 2013 at 7:05 PM, Nick Mathewson <nickm@alum.mit.edu> wrote:
... Is there any actual harm in the redundant invocation of ENGINE_register_all_complete(), or is this about cleanliness/saving cycles/something else?
Assuming it's not actually harmful, I'd prefer to leave the call in, with a comment that it isn't necessary any longer.
no impact. leaving be with a comment is a better idea. best regards,
participants (2)
-
coderman -
Nick Mathewson