diff --git a/crypto/engine/engine.h b/crypto/engine/engine.h index 258ec6ec43..3cb52254ff 100644 --- a/crypto/engine/engine.h +++ b/crypto/engine/engine.h @@ -363,8 +363,12 @@ int ENGINE_cpy(ENGINE *dest, const ENGINE *src); int ENGINE_get_ex_new_index(long argl, void *argp, CRYPTO_EX_new *new_func, CRYPTO_EX_dup *dup_func, CRYPTO_EX_free *free_func); int ENGINE_set_ex_data(ENGINE *e, int idx, void *arg); -/* Cleans the internal engine structure. This should only be used when the - * application is about to exit. */ +/* Cleans the internal engine list. This should only be used when the + * application is about to exit or restart operation (the next operation + * requiring the ENGINE list will re-initialise it with defaults). NB: Dynamic + * ENGINEs will only truly unload (including any allocated data or loaded + * shared-libraries) if all remaining references are released too - so keys, + * certificates, etc all need to be released for an in-use ENGINE to unload. */ void ENGINE_cleanup(void); /* These return values from within the ENGINE structure. These can be useful @@ -445,6 +449,12 @@ int ENGINE_set_default_BN_mod_exp_crt(ENGINE *e); * ENGINE_METHOD_*** defines above. */ int ENGINE_set_default(ENGINE *e, unsigned int flags); +/* This function resets all the internal "default" ENGINEs (there's one for each + * of the various algorithms) to NULL, releasing any references as appropriate. + * This function is called as part of the ENGINE_cleanup() function, so there's + * no need to call both (although no harm is done). */ +int ENGINE_clear_defaults(void); + /* Obligatory error function. */ void ERR_load_ENGINE_strings(void); diff --git a/crypto/engine/engine_all.c b/crypto/engine/engine_all.c index 43da60483b..4d0244f351 100644 --- a/crypto/engine/engine_all.c +++ b/crypto/engine/engine_all.c @@ -62,12 +62,14 @@ static int engine_add(ENGINE *e) { + int toret = 1; if (!ENGINE_by_id(ENGINE_get_id(e))) { (void)ERR_get_error(); - return ENGINE_add(e); + toret = ENGINE_add(e); } - return 1; + ENGINE_free(e); + return toret; } void ENGINE_load_cswift(void) diff --git a/crypto/engine/engine_int.h b/crypto/engine/engine_int.h index d44f648559..54cfe47af7 100644 --- a/crypto/engine/engine_int.h +++ b/crypto/engine/engine_int.h @@ -66,6 +66,29 @@ extern "C" { #endif +#define ENGINE_REF_COUNT_DEBUG + +/* If we compile with this symbol defined, then both reference counts in the + * ENGINE structure will be monitored with a line of output on stderr for each + * change. This prints the engine's pointer address (truncated to unsigned int), + * "struct" or "funct" to indicate the reference type, the before and after + * reference count, and the file:line-number pair. The "engine_ref_debug" + * statements must come *after* the change. */ +#ifdef ENGINE_REF_COUNT_DEBUG + +#define engine_ref_debug(e, isfunct, diff) \ + fprintf(stderr, "blargle: %08x %s from %d to %d (%s:%d)\n", \ + (unsigned int)(e), (isfunct ? "funct" : "struct"), \ + ((isfunct) ? ((e)->funct_ref - (diff)) : ((e)->struct_ref - (diff))), \ + ((isfunct) ? (e)->funct_ref : (e)->struct_ref), \ + (__FILE__), (__LINE__)); + +#else + +#define engine_ref_debug(e, isfunct, diff) + +#endif + /* NB: Bitwise OR-able values for the "flags" variable in ENGINE are now exposed * in engine.h. */ diff --git a/crypto/engine/engine_lib.c b/crypto/engine/engine_lib.c index 5925a0bc65..90eb5cd6d5 100644 --- a/crypto/engine/engine_lib.c +++ b/crypto/engine/engine_lib.c @@ -103,6 +103,8 @@ static void engine_def_check_util(ENGINE **def, ENGINE *val) *def = val; val->struct_ref++; val->funct_ref++; + engine_ref_debug(val, 0, 1) + engine_ref_debug(val, 1, 1) } /* In a slight break with convention - this static function must be @@ -176,6 +178,8 @@ int ENGINE_init(ENGINE *e) * structural reference. */ e->struct_ref++; e->funct_ref++; + engine_ref_debug(e, 0, 1) + engine_ref_debug(e, 1, 1) } CRYPTO_w_unlock(CRYPTO_LOCK_ENGINE); return to_return; @@ -192,43 +196,38 @@ int ENGINE_finish(ENGINE *e) return 0; } CRYPTO_w_lock(CRYPTO_LOCK_ENGINE); - if((e->funct_ref == 1) && e->finish) -#if 0 - /* This is the last functional reference and the engine - * requires cleanup so we do it now. */ - to_return = e->finish(); - if(to_return) - { - /* Cleanup the functional reference which is also a - * structural reference. */ - e->struct_ref--; - e->funct_ref--; - } - CRYPTO_w_unlock(CRYPTO_LOCK_ENGINE); -#else - /* I'm going to deliberately do a convoluted version of this - * piece of code because we don't want "finish" functions - * being called inside a locked block of code, if at all - * possible. I'd rather have this call take an extra couple - * of ticks than have throughput serialised on a externally- - * provided callback function that may conceivably never come - * back. :-( */ + /* Reduce the functional reference count here so if it's the terminating + * case, we can release the lock safely and call the finish() handler + * without risk of a race. We get a race if we leave the count until + * after and something else is calling "finish" at the same time - + * there's a chance that both threads will together take the count from + * 2 to 0 without either calling finish(). */ + e->funct_ref--; + engine_ref_debug(e, 1, -1) + if((e->funct_ref == 0) && e->finish) { CRYPTO_w_unlock(CRYPTO_LOCK_ENGINE); - /* CODE ALERT: This *IS* supposed to be "=" and NOT "==" :-) */ - if((to_return = e->finish(e))) + if(!(to_return = e->finish(e))) { - CRYPTO_w_lock(CRYPTO_LOCK_ENGINE); - /* Cleanup the functional reference which is also a - * structural reference. */ - e->struct_ref--; - e->funct_ref--; - CRYPTO_w_unlock(CRYPTO_LOCK_ENGINE); + ENGINEerr(ENGINE_F_ENGINE_FINISH,ENGINE_R_FINISH_FAILED); + return 0; } } else CRYPTO_w_unlock(CRYPTO_LOCK_ENGINE); +#ifdef REF_CHECK + if(e->funct_ref < 0) + { + fprintf(stderr,"ENGINE_finish, bad functional reference count\n"); + abort(); + } #endif + /* Release the structural reference too */ + if(!ENGINE_free(e)) + { + ENGINEerr(ENGINE_F_ENGINE_FINISH,ENGINE_R_FINISH_FAILED); + return 0; + } return to_return; } @@ -620,8 +619,8 @@ static ENGINE *engine_get_default_type(ENGINE_TYPE t) ret = engine_def_bn_mod_exp; break; case ENGINE_TYPE_BN_MOD_EXP_CRT: ret = engine_def_bn_mod_exp_crt; break; - default: - break; + default: + break; } /* Unforunately we can't do this work outside the lock with a * call to ENGINE_init() because that would leave a race @@ -630,6 +629,8 @@ static ENGINE *engine_get_default_type(ENGINE_TYPE t) { ret->struct_ref++; ret->funct_ref++; + engine_ref_debug(ret, 0, 1) + engine_ref_debug(ret, 1, 1) } CRYPTO_w_unlock(CRYPTO_LOCK_ENGINE); return ret; @@ -675,12 +676,6 @@ static int engine_set_default_type(ENGINE_TYPE t, ENGINE *e) { ENGINE *old = NULL; - if(e == NULL) - { - ENGINEerr(ENGINE_F_ENGINE_SET_DEFAULT_TYPE, - ERR_R_PASSED_NULL_PARAMETER); - return 0; - } /* engine_def_check is lean and mean and won't replace any * prior default engines ... so we must ensure that it is always * the first function to get to touch the default values. */ @@ -688,7 +683,7 @@ static int engine_set_default_type(ENGINE_TYPE t, ENGINE *e) /* Attempt to get a functional reference (we need one anyway, but * also, 'e' may be just a structural reference being passed in so * this call may actually be the first). */ - if(!ENGINE_init(e)) + if(e && !ENGINE_init(e)) { ENGINEerr(ENGINE_F_ENGINE_SET_DEFAULT_TYPE, ENGINE_R_INIT_FAILED); @@ -721,8 +716,8 @@ static int engine_set_default_type(ENGINE_TYPE t, ENGINE *e) case ENGINE_TYPE_BN_MOD_EXP_CRT: old = engine_def_bn_mod_exp_crt; engine_def_bn_mod_exp_crt = e; break; - default: - break; + default: + break; } CRYPTO_w_unlock(CRYPTO_LOCK_ENGINE); /* If we've replaced a previous value, then we need to remove the @@ -801,3 +796,31 @@ int ENGINE_set_default(ENGINE *e, unsigned int flags) return 1; } +int ENGINE_clear_defaults(void) + { + /* If the defaults haven't even been set yet, don't bother. Any kind of + * "cleanup" has a kind of implicit race-condition if another thread is + * trying to keep going, so we don't address that with locking. The + * first ENGINE_set_default_*** call will actually *create* a standard + * set of default ENGINEs (including init() and functional reference + * counts aplenty) before the rest of this function undoes them all. So + * save some hassle ... */ + if(!engine_def_flag) + return 1; + if((0 == 1) || +#ifndef OPENSSL_NO_RSA + !ENGINE_set_default_RSA(NULL) || +#endif +#ifndef OPENSSL_NO_DSA + !ENGINE_set_default_DSA(NULL) || +#endif +#ifndef OPENSSL_NO_DH + !ENGINE_set_default_DH(NULL) || +#endif + !ENGINE_set_default_RAND(NULL) || + !ENGINE_set_default_BN_mod_exp(NULL) || + !ENGINE_set_default_BN_mod_exp_crt(NULL)) + return 0; + return 1; + } + diff --git a/crypto/engine/engine_list.c b/crypto/engine/engine_list.c index 0f6e9bb242..b41e6e5354 100644 --- a/crypto/engine/engine_list.c +++ b/crypto/engine/engine_list.c @@ -139,6 +139,7 @@ static int engine_list_add(ENGINE *e) /* Having the engine in the list assumes a structural * reference. */ e->struct_ref++; + engine_ref_debug(e, 0, 1) /* However it came to be, e is the last item in the list. */ engine_list_tail = e; e->next = NULL; @@ -177,6 +178,7 @@ static int engine_list_remove(ENGINE *e) engine_list_tail = e->prev; /* remove our structural reference. */ e->struct_ref--; + engine_ref_debug(e, 0, -1) return 1; } @@ -187,13 +189,24 @@ static int engine_list_remove(ENGINE *e) * as it will generate errors itself. */ static int engine_internal_check(void) { + int toret = 1; + ENGINE *def_engine; if(engine_list_flag) return 1; /* This is our first time up, we need to populate the list * with our statically compiled-in engines. */ - if(!engine_list_add(ENGINE_openssl())) - return 0; - engine_list_flag = 1; + def_engine = ENGINE_openssl(); + if(!engine_list_add(def_engine)) + toret = 0; + else + engine_list_flag = 1; +#if 0 + ENGINE_free(def_engine); +#else + /* We can't ENGINE_free() because the lock's already held */ + def_engine->struct_ref--; + engine_ref_debug(def_engine, 0, -1) +#endif return 1; } @@ -207,7 +220,10 @@ ENGINE *ENGINE_get_first(void) { ret = engine_list_head; if(ret) + { ret->struct_ref++; + engine_ref_debug(ret, 0, 1) + } } CRYPTO_r_unlock(CRYPTO_LOCK_ENGINE); return ret; @@ -221,7 +237,10 @@ ENGINE *ENGINE_get_last(void) { ret = engine_list_tail; if(ret) + { ret->struct_ref++; + engine_ref_debug(ret, 0, 1) + } } CRYPTO_r_unlock(CRYPTO_LOCK_ENGINE); return ret; @@ -240,8 +259,11 @@ ENGINE *ENGINE_get_next(ENGINE *e) CRYPTO_r_lock(CRYPTO_LOCK_ENGINE); ret = e->next; if(ret) + { /* Return a valid structural refernce to the next ENGINE */ ret->struct_ref++; + engine_ref_debug(ret, 0, 1) + } CRYPTO_r_unlock(CRYPTO_LOCK_ENGINE); /* Release the structural reference to the previous ENGINE */ ENGINE_free(e); @@ -259,8 +281,11 @@ ENGINE *ENGINE_get_prev(ENGINE *e) CRYPTO_r_lock(CRYPTO_LOCK_ENGINE); ret = e->prev; if(ret) + { /* Return a valid structural reference to the next ENGINE */ ret->struct_ref++; + engine_ref_debug(ret, 0, 1) + } CRYPTO_r_unlock(CRYPTO_LOCK_ENGINE); /* Release the structural reference to the previous ENGINE */ ENGINE_free(e); @@ -349,7 +374,10 @@ ENGINE *ENGINE_by_id(const char *id) } } else + { iterator->struct_ref++; + engine_ref_debug(iterator, 0, 1) + } } } CRYPTO_r_unlock(CRYPTO_LOCK_ENGINE); @@ -371,6 +399,7 @@ ENGINE *ENGINE_new(void) } memset(ret, 0, sizeof(ENGINE)); ret->struct_ref = 1; + engine_ref_debug(ret, 0, 1) CRYPTO_new_ex_data(engine_ex_data_stack, ret, &ret->ex_data); return ret; } @@ -386,14 +415,12 @@ int ENGINE_free(ENGINE *e) return 0; } i = CRYPTO_add(&e->struct_ref,-1,CRYPTO_LOCK_ENGINE); -#ifdef REF_PRINT - REF_PRINT("ENGINE",e); -#endif + engine_ref_debug(e, 0, -1) if (i > 0) return 1; #ifdef REF_CHECK if (i < 0) { - fprintf(stderr,"ENGINE_free, bad reference count\n"); + fprintf(stderr,"ENGINE_free, bad structural reference count\n"); abort(); } #endif @@ -422,18 +449,21 @@ void *ENGINE_get_ex_data(const ENGINE *e, int idx) } void ENGINE_cleanup(void) - { - ENGINE *iterator = engine_list_head; + { + ENGINE *iterator = engine_list_head; - while(iterator != NULL) - { - ENGINE_remove(iterator); - ENGINE_free(iterator); - iterator = engine_list_head; - } - engine_list_flag = 0; - return; - } + while(iterator != NULL) + { + ENGINE_remove(iterator); + iterator = engine_list_head; + } + engine_list_flag = 0; + /* Also unset any "default" ENGINEs that may have been set up (a default + * constitutes a functional reference on an ENGINE and there's one for + * each algorithm). */ + ENGINE_clear_defaults(); + return; + } int ENGINE_set_id(ENGINE *e, const char *id) { @@ -465,7 +495,7 @@ int ENGINE_set_RSA(ENGINE *e, const RSA_METHOD *rsa_meth) e->rsa_meth = rsa_meth; return 1; #else - return 0; + return 0; #endif } @@ -475,7 +505,7 @@ int ENGINE_set_DSA(ENGINE *e, const DSA_METHOD *dsa_meth) e->dsa_meth = dsa_meth; return 1; #else - return 0; + return 0; #endif } @@ -485,7 +515,7 @@ int ENGINE_set_DH(ENGINE *e, const DH_METHOD *dh_meth) e->dh_meth = dh_meth; return 1; #else - return 0; + return 0; #endif }