From: Matt Caswell Date: Wed, 4 Nov 2020 11:34:15 +0000 (+0000) Subject: Don't clear the whole error stack when loading engines X-Git-Tag: openssl-3.0.0-alpha9~166 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=b9b2135d22b93f949fd77f293925fc66158416ff;p=thirdparty%2Fopenssl.git Don't clear the whole error stack when loading engines Loading the various built-in engines was unconditionally clearing the whole error stack. During config file processing processing a .include directive which fails results in errors being added to the stack - but we carry on anyway. These errors were then later being removed by the engine loading code, meaning that problems with the .include directive never get shown. Reviewed-by: Paul Dale Reviewed-by: Tomas Mraz (Merged from https://github.com/openssl/openssl/pull/13311) --- diff --git a/crypto/conf/conf_mod.c b/crypto/conf/conf_mod.c index e7fb8903789..f287cb28eb5 100644 --- a/crypto/conf/conf_mod.c +++ b/crypto/conf/conf_mod.c @@ -208,7 +208,6 @@ DEFINE_RUN_ONCE_STATIC(do_load_builtin_modules) /* Need to load ENGINEs */ ENGINE_load_builtin_engines(); #endif - ERR_clear_error(); return 1; } diff --git a/crypto/engine/eng_dyn.c b/crypto/engine/eng_dyn.c index 01935578c26..3b0d8eb91f7 100644 --- a/crypto/engine/eng_dyn.c +++ b/crypto/engine/eng_dyn.c @@ -257,6 +257,8 @@ void engine_load_dynamic_int(void) ENGINE *toadd = engine_dynamic(); if (!toadd) return; + + ERR_set_mark(); ENGINE_add(toadd); /* * If the "add" worked, it gets a structural reference. So either way, we @@ -268,7 +270,7 @@ void engine_load_dynamic_int(void) * already added (eg. someone calling ENGINE_load_blah then calling * ENGINE_load_builtin_engines() perhaps). */ - ERR_clear_error(); + ERR_pop_to_mark(); } static int dynamic_init(ENGINE *e) diff --git a/crypto/engine/eng_openssl.c b/crypto/engine/eng_openssl.c index 2374af8ae9e..a51ccf129fc 100644 --- a/crypto/engine/eng_openssl.c +++ b/crypto/engine/eng_openssl.c @@ -152,13 +152,20 @@ void engine_load_openssl_int(void) ENGINE *toadd = engine_openssl(); if (!toadd) return; + + ERR_set_mark(); ENGINE_add(toadd); /* * If the "add" worked, it gets a structural reference. So either way, we * release our just-created reference. */ ENGINE_free(toadd); - ERR_clear_error(); + /* + * If the "add" didn't work, it was probably a conflict because it was + * already added (eg. someone calling ENGINE_load_blah then calling + * ENGINE_load_builtin_engines() perhaps). + */ + ERR_pop_to_mark(); } /* diff --git a/crypto/engine/eng_rdrand.c b/crypto/engine/eng_rdrand.c index 39e4055a90c..f46a5145974 100644 --- a/crypto/engine/eng_rdrand.c +++ b/crypto/engine/eng_rdrand.c @@ -87,9 +87,19 @@ void engine_load_rdrand_int(void) ENGINE *toadd = ENGINE_rdrand(); if (!toadd) return; + ERR_set_mark(); ENGINE_add(toadd); + /* + * If the "add" worked, it gets a structural reference. So either way, we + * release our just-created reference. + */ ENGINE_free(toadd); - ERR_clear_error(); + /* + * If the "add" didn't work, it was probably a conflict because it was + * already added (eg. someone calling ENGINE_load_blah then calling + * ENGINE_load_builtin_engines() perhaps). + */ + ERR_pop_to_mark(); } } #else diff --git a/engines/e_afalg.c b/engines/e_afalg.c index 24a1aa900c5..9480d7c24b0 100644 --- a/engines/e_afalg.c +++ b/engines/e_afalg.c @@ -851,9 +851,19 @@ void engine_load_afalg_int(void) toadd = engine_afalg(); if (toadd == NULL) return; + ERR_set_mark(); ENGINE_add(toadd); + /* + * If the "add" worked, it gets a structural reference. So either way, we + * release our just-created reference. + */ ENGINE_free(toadd); - ERR_clear_error(); + /* + * If the "add" didn't work, it was probably a conflict because it was + * already added (eg. someone calling ENGINE_load_blah then calling + * ENGINE_load_builtin_engines() perhaps). + */ + ERR_pop_to_mark(); } # endif diff --git a/engines/e_capi.c b/engines/e_capi.c index 8e5693d25e0..dd66518d3f5 100644 --- a/engines/e_capi.c +++ b/engines/e_capi.c @@ -600,9 +600,19 @@ void engine_load_capi_int(void) ENGINE *toadd = engine_capi(); if (!toadd) return; + ERR_set_mark(); ENGINE_add(toadd); + /* + * If the "add" worked, it gets a structural reference. So either way, we + * release our just-created reference. + */ ENGINE_free(toadd); - ERR_clear_error(); + /* + * If the "add" didn't work, it was probably a conflict because it was + * already added (eg. someone calling ENGINE_load_blah then calling + * ENGINE_load_builtin_engines() perhaps). + */ + ERR_pop_to_mark(); } # endif diff --git a/engines/e_dasync.c b/engines/e_dasync.c index b817b2ba5f5..4eb50d055cf 100644 --- a/engines/e_dasync.c +++ b/engines/e_dasync.c @@ -348,9 +348,19 @@ void engine_load_dasync_int(void) ENGINE *toadd = engine_dasync(); if (!toadd) return; + ERR_set_mark(); ENGINE_add(toadd); + /* + * If the "add" worked, it gets a structural reference. So either way, we + * release our just-created reference. + */ ENGINE_free(toadd); - ERR_clear_error(); + /* + * If the "add" didn't work, it was probably a conflict because it was + * already added (eg. someone calling ENGINE_load_blah then calling + * ENGINE_load_builtin_engines() perhaps). + */ + ERR_pop_to_mark(); } static int dasync_init(ENGINE *e) diff --git a/engines/e_devcrypto.c b/engines/e_devcrypto.c index 729bb1fe955..85815e2e5ae 100644 --- a/engines/e_devcrypto.c +++ b/engines/e_devcrypto.c @@ -1287,9 +1287,20 @@ void engine_load_devcrypto_int(void) return; } + ERR_set_mark(); ENGINE_add(e); + /* + * If the "add" worked, it gets a structural reference. So either way, we + * release our just-created reference. + */ ENGINE_free(e); /* Loose our local reference */ - ERR_clear_error(); + /* + * If the "add" didn't work, it was probably a conflict because it was + * already added (eg. someone calling ENGINE_load_blah then calling + * ENGINE_load_builtin_engines() perhaps). + */ + ERR_pop_to_mark(); +} } #else diff --git a/engines/e_padlock.c b/engines/e_padlock.c index 713a79a3681..572ff909350 100644 --- a/engines/e_padlock.c +++ b/engines/e_padlock.c @@ -49,9 +49,19 @@ void engine_load_padlock_int(void) ENGINE *toadd = ENGINE_padlock(); if (!toadd) return; + ERR_set_mark(); ENGINE_add(toadd); + /* + * If the "add" worked, it gets a structural reference. So either way, we + * release our just-created reference. + */ ENGINE_free(toadd); - ERR_clear_error(); + /* + * If the "add" didn't work, it was probably a conflict because it was + * already added (eg. someone calling ENGINE_load_blah then calling + * ENGINE_load_builtin_engines() perhaps). + */ + ERR_pop_to_mark(); # endif }