From 3e47caff4830d2a117eda15b57a5feab89b846ae Mon Sep 17 00:00:00 2001 From: Rich Salz Date: Tue, 28 Apr 2015 10:50:54 -0400 Subject: [PATCH] ERR_ cleanup Remove ERR_[gs]et_implementation as they were not undocumented and useless (the data structure was opaque). Halve the number of lock/unlock calls in almost all ERR_ functions by letting the caller of get_hash or int_thread_set able to lock. Very useful when looping, such as adding errors, or when getting the hash and immediately doing a lookup on it. Reviewed-by: Richard Levitte --- crypto/engine/eng_dyn.c | 1 - crypto/err/err.c | 311 +++++++++++-------------------------- include/openssl/engine.h | 2 - include/openssl/err.h | 13 -- include/openssl/ossl_typ.h | 2 - util/indent.pro | 1 - util/libeay.num | 4 +- 7 files changed, 89 insertions(+), 245 deletions(-) diff --git a/crypto/engine/eng_dyn.c b/crypto/engine/eng_dyn.c index 3169b09ad8..31ec324422 100644 --- a/crypto/engine/eng_dyn.c +++ b/crypto/engine/eng_dyn.c @@ -519,7 +519,6 @@ static int dynamic_load(ENGINE *e, dynamic_data_ctx *ctx) * would also increase opaqueness. */ fns.static_state = ENGINE_get_static_state(); - fns.err_fns = ERR_get_implementation(); fns.ex_data_fns = CRYPTO_get_ex_data_implementation(); CRYPTO_get_mem_functions(&fns.mem_fns.malloc_cb, &fns.mem_fns.realloc_cb, &fns.mem_fns.free_cb); diff --git a/crypto/err/err.c b/crypto/err/err.c index 50865b8b26..b078442109 100644 --- a/crypto/err/err.c +++ b/crypto/err/err.c @@ -223,109 +223,23 @@ static ERR_STRING_DATA ERR_str_reasons[] = { }; #endif -/* Define the predeclared (but externally opaque) "ERR_FNS" type */ -struct st_ERR_FNS { - /* Works on the "error_hash" string table */ - LHASH_OF(ERR_STRING_DATA) *(*cb_err_get) (int create); - void (*cb_err_del) (void); - ERR_STRING_DATA *(*cb_err_get_item) (const ERR_STRING_DATA *); - ERR_STRING_DATA *(*cb_err_set_item) (ERR_STRING_DATA *); - ERR_STRING_DATA *(*cb_err_del_item) (ERR_STRING_DATA *); - /* Works on the "thread_hash" error-state table */ - LHASH_OF(ERR_STATE) *(*cb_thread_get) (int create); - void (*cb_thread_release) (LHASH_OF(ERR_STATE) **hash); - ERR_STATE *(*cb_thread_get_item) (const ERR_STATE *); - ERR_STATE *(*cb_thread_set_item) (ERR_STATE *); - void (*cb_thread_del_item) (const ERR_STATE *); - /* Returns the next available error "library" numbers */ - int (*cb_get_next_lib) (void); -}; - /* Predeclarations of the "err_defaults" functions */ -static LHASH_OF(ERR_STRING_DATA) *int_err_get(int create); -static void int_err_del(void); +static LHASH_OF(ERR_STRING_DATA) *get_hash(int create, int lockit); static ERR_STRING_DATA *int_err_get_item(const ERR_STRING_DATA *); -static ERR_STRING_DATA *int_err_set_item(ERR_STRING_DATA *); -static ERR_STRING_DATA *int_err_del_item(ERR_STRING_DATA *); -static LHASH_OF(ERR_STATE) *int_thread_get(int create); +static LHASH_OF(ERR_STATE) *int_thread_get(int create, int lockit); static void int_thread_release(LHASH_OF(ERR_STATE) **hash); static ERR_STATE *int_thread_get_item(const ERR_STATE *); static ERR_STATE *int_thread_set_item(ERR_STATE *); static void int_thread_del_item(const ERR_STATE *); -static int int_err_get_next_lib(void); -/* The static ERR_FNS table using these defaults functions */ -static const ERR_FNS err_defaults = { - int_err_get, - int_err_del, - int_err_get_item, - int_err_set_item, - int_err_del_item, - int_thread_get, - int_thread_release, - int_thread_get_item, - int_thread_set_item, - int_thread_del_item, - int_err_get_next_lib -}; - -/* The replacable table of ERR_FNS functions we use at run-time */ -static const ERR_FNS *err_fns = NULL; - -/* Eg. rather than using "err_get()", use "ERRFN(err_get)()". */ -#define ERRFN(a) err_fns->cb_##a /* - * The internal state used by "err_defaults" - as such, the setting, reading, - * creating, and deleting of this data should only be permitted via the - * "err_defaults" functions. This way, a linked module can completely defer - * all ERR state operation (together with requisite locking) to the - * implementations and state in the loading application. + * The internal state */ static LHASH_OF(ERR_STRING_DATA) *int_error_hash = NULL; static LHASH_OF(ERR_STATE) *int_thread_hash = NULL; static int int_thread_hash_references = 0; static int int_err_library_number = ERR_LIB_USER; -/* - * Internal function that checks whether "err_fns" is set and if not, sets it - * to the defaults. - */ -static void err_fns_check(void) -{ - if (err_fns) - return; - - CRYPTO_w_lock(CRYPTO_LOCK_ERR); - if (!err_fns) - err_fns = &err_defaults; - CRYPTO_w_unlock(CRYPTO_LOCK_ERR); -} - -/* API functions to get or set the underlying ERR functions. */ - -const ERR_FNS *ERR_get_implementation(void) -{ - err_fns_check(); - return err_fns; -} - -int ERR_set_implementation(const ERR_FNS *fns) -{ - int ret = 0; - - CRYPTO_w_lock(CRYPTO_LOCK_ERR); - /* - * It's too late if 'err_fns' is non-NULL. BTW: not much point setting an - * error is there?! - */ - if (!err_fns) { - err_fns = fns; - ret = 1; - } - CRYPTO_w_unlock(CRYPTO_LOCK_ERR); - return ret; -} - /* * These are the callbacks provided to "lh_new()" when creating the LHASH * tables internal to the "err_defaults" implementation. @@ -335,8 +249,6 @@ static unsigned long get_error_values(int inc, int top, const char **file, int *line, const char **data, int *flags); -/* The internal functions used in the "err_defaults" implementation */ - static unsigned long err_string_data_hash(const ERR_STRING_DATA *a) { unsigned long ret, l; @@ -356,84 +268,39 @@ static int err_string_data_cmp(const ERR_STRING_DATA *a, static IMPLEMENT_LHASH_COMP_FN(err_string_data, ERR_STRING_DATA) -static LHASH_OF(ERR_STRING_DATA) *int_err_get(int create) +static LHASH_OF(ERR_STRING_DATA) *get_hash(int create, int lockit) { LHASH_OF(ERR_STRING_DATA) *ret = NULL; - CRYPTO_w_lock(CRYPTO_LOCK_ERR); + if (lockit) + CRYPTO_w_lock(CRYPTO_LOCK_ERR); if (!int_error_hash && create) { - CRYPTO_push_info("int_err_get (err.c)"); + CRYPTO_push_info("get_hash (err.c)"); int_error_hash = lh_ERR_STRING_DATA_new(); CRYPTO_pop_info(); } if (int_error_hash) ret = int_error_hash; - CRYPTO_w_unlock(CRYPTO_LOCK_ERR); + if (lockit) + CRYPTO_w_unlock(CRYPTO_LOCK_ERR); return ret; } -static void int_err_del(void) -{ - CRYPTO_w_lock(CRYPTO_LOCK_ERR); - if (int_error_hash) { - lh_ERR_STRING_DATA_free(int_error_hash); - int_error_hash = NULL; - } - CRYPTO_w_unlock(CRYPTO_LOCK_ERR); -} - static ERR_STRING_DATA *int_err_get_item(const ERR_STRING_DATA *d) { - ERR_STRING_DATA *p; + ERR_STRING_DATA *p = NULL; LHASH_OF(ERR_STRING_DATA) *hash; - err_fns_check(); - hash = ERRFN(err_get) (0); - if (!hash) - return NULL; - CRYPTO_r_lock(CRYPTO_LOCK_ERR); - p = lh_ERR_STRING_DATA_retrieve(hash, d); + hash = get_hash(0, 0); + if (hash) + p = lh_ERR_STRING_DATA_retrieve(hash, d); CRYPTO_r_unlock(CRYPTO_LOCK_ERR); return p; } -static ERR_STRING_DATA *int_err_set_item(ERR_STRING_DATA *d) -{ - ERR_STRING_DATA *p; - LHASH_OF(ERR_STRING_DATA) *hash; - - err_fns_check(); - hash = ERRFN(err_get) (1); - if (!hash) - return NULL; - - CRYPTO_w_lock(CRYPTO_LOCK_ERR); - p = lh_ERR_STRING_DATA_insert(hash, d); - CRYPTO_w_unlock(CRYPTO_LOCK_ERR); - - return p; -} - -static ERR_STRING_DATA *int_err_del_item(ERR_STRING_DATA *d) -{ - ERR_STRING_DATA *p; - LHASH_OF(ERR_STRING_DATA) *hash; - - err_fns_check(); - hash = ERRFN(err_get) (0); - if (!hash) - return NULL; - - CRYPTO_w_lock(CRYPTO_LOCK_ERR); - p = lh_ERR_STRING_DATA_delete(hash, d); - CRYPTO_w_unlock(CRYPTO_LOCK_ERR); - - return p; -} - static unsigned long err_state_hash(const ERR_STATE *a) { return CRYPTO_THREADID_hash(&a->tid) * 13; @@ -448,11 +315,12 @@ static int err_state_cmp(const ERR_STATE *a, const ERR_STATE *b) static IMPLEMENT_LHASH_COMP_FN(err_state, ERR_STATE) -static LHASH_OF(ERR_STATE) *int_thread_get(int create) +static LHASH_OF(ERR_STATE) *int_thread_get(int create, int lockit) { LHASH_OF(ERR_STATE) *ret = NULL; - CRYPTO_w_lock(CRYPTO_LOCK_ERR); + if (lockit) + CRYPTO_w_lock(CRYPTO_LOCK_ERR); if (!int_thread_hash && create) { CRYPTO_push_info("int_thread_get (err.c)"); int_thread_hash = lh_ERR_STATE_new(); @@ -462,7 +330,8 @@ static LHASH_OF(ERR_STATE) *int_thread_get(int create) int_thread_hash_references++; ret = int_thread_hash; } - CRYPTO_w_unlock(CRYPTO_LOCK_ERR); + if (lockit) + CRYPTO_w_unlock(CRYPTO_LOCK_ERR); return ret; } @@ -491,76 +360,59 @@ static void int_thread_release(LHASH_OF(ERR_STATE) **hash) static ERR_STATE *int_thread_get_item(const ERR_STATE *d) { - ERR_STATE *p; + ERR_STATE *p = NULL; LHASH_OF(ERR_STATE) *hash; - err_fns_check(); - hash = ERRFN(thread_get) (0); - if (!hash) - return NULL; - CRYPTO_r_lock(CRYPTO_LOCK_ERR); - p = lh_ERR_STATE_retrieve(hash, d); + hash = int_thread_get(0, 0); + if (hash) + p = lh_ERR_STATE_retrieve(hash, d); CRYPTO_r_unlock(CRYPTO_LOCK_ERR); - ERRFN(thread_release) (&hash); + int_thread_release(&hash); return p; } static ERR_STATE *int_thread_set_item(ERR_STATE *d) { - ERR_STATE *p; + ERR_STATE *p = NULL; LHASH_OF(ERR_STATE) *hash; - err_fns_check(); - hash = ERRFN(thread_get) (1); - if (!hash) - return NULL; - CRYPTO_w_lock(CRYPTO_LOCK_ERR); - p = lh_ERR_STATE_insert(hash, d); + hash = int_thread_get(1, 0); + if (hash) + p = lh_ERR_STATE_insert(hash, d); CRYPTO_w_unlock(CRYPTO_LOCK_ERR); - ERRFN(thread_release) (&hash); + int_thread_release(&hash); return p; } static void int_thread_del_item(const ERR_STATE *d) { - ERR_STATE *p; + ERR_STATE *p = NULL; LHASH_OF(ERR_STATE) *hash; - err_fns_check(); - hash = ERRFN(thread_get) (0); - if (!hash) - return; - CRYPTO_w_lock(CRYPTO_LOCK_ERR); - p = lh_ERR_STATE_delete(hash, d); - /* make sure we don't leak memory */ - if (int_thread_hash_references == 1 - && int_thread_hash && lh_ERR_STATE_num_items(int_thread_hash) == 0) { - lh_ERR_STATE_free(int_thread_hash); - int_thread_hash = NULL; + hash = int_thread_get(0, 0); + if (hash) { + p = lh_ERR_STATE_delete(hash, d); + /* If there are no other references, and we just removed the + * last item, delete the int_thread_hash */ + if (int_thread_hash_references == 1 + && int_thread_hash + && lh_ERR_STATE_num_items(int_thread_hash) == 0) { + lh_ERR_STATE_free(int_thread_hash); + int_thread_hash = NULL; + } } CRYPTO_w_unlock(CRYPTO_LOCK_ERR); - ERRFN(thread_release) (&hash); + int_thread_release(&hash); if (p) ERR_STATE_free(p); } -static int int_err_get_next_lib(void) -{ - int ret; - - CRYPTO_w_lock(CRYPTO_LOCK_ERR); - ret = int_err_library_number++; - CRYPTO_w_unlock(CRYPTO_LOCK_ERR); - - return ret; -} - #ifndef OPENSSL_NO_ERR # define NUM_SYS_STR_REASONS 127 # define LEN_SYS_STR_REASON 32 @@ -580,8 +432,8 @@ static void build_SYS_str_reasons(void) { /* OPENSSL_malloc cannot be used here, use static storage instead */ static char strerror_tab[NUM_SYS_STR_REASONS][LEN_SYS_STR_REASON]; - int i; static int init = 1; + int i; CRYPTO_r_lock(CRYPTO_LOCK_ERR); if (!init) { @@ -659,7 +511,6 @@ static void ERR_STATE_free(ERR_STATE *s) void ERR_load_ERR_strings(void) { - err_fns_check(); #ifndef OPENSSL_NO_ERR err_load_strings(0, ERR_str_libraries); err_load_strings(0, ERR_str_reasons); @@ -671,12 +522,18 @@ void ERR_load_ERR_strings(void) static void err_load_strings(int lib, ERR_STRING_DATA *str) { - while (str->error) { - if (lib) - str->error |= ERR_PACK(lib, 0, 0); - ERRFN(err_set_item) (str); - str++; + LHASH_OF(ERR_STRING_DATA) *hash; + + CRYPTO_w_lock(CRYPTO_LOCK_ERR); + hash = get_hash(1, 0); + if (hash) { + for (; str->error; str++) { + if (lib) + str->error |= ERR_PACK(lib, 0, 0); + (void)lh_ERR_STRING_DATA_insert(hash, str); + } } + CRYPTO_w_unlock(CRYPTO_LOCK_ERR); } void ERR_load_strings(int lib, ERR_STRING_DATA *str) @@ -687,18 +544,28 @@ void ERR_load_strings(int lib, ERR_STRING_DATA *str) void ERR_unload_strings(int lib, ERR_STRING_DATA *str) { - while (str->error) { - if (lib) - str->error |= ERR_PACK(lib, 0, 0); - ERRFN(err_del_item) (str); - str++; + LHASH_OF(ERR_STRING_DATA) *hash; + + CRYPTO_w_lock(CRYPTO_LOCK_ERR); + hash = get_hash(0, 0); + if (hash) { + for (; str->error; str++) { + if (lib) + str->error |= ERR_PACK(lib, 0, 0); + (void)lh_ERR_STRING_DATA_delete(hash, str); + } } + CRYPTO_w_unlock(CRYPTO_LOCK_ERR); } void ERR_free_strings(void) { - err_fns_check(); - ERRFN(err_del) (); + CRYPTO_w_lock(CRYPTO_LOCK_ERR); + if (int_error_hash) { + lh_ERR_STRING_DATA_free(int_error_hash); + int_error_hash = NULL; + } + CRYPTO_w_unlock(CRYPTO_LOCK_ERR); } /********************************************************/ @@ -932,20 +799,17 @@ char *ERR_error_string(unsigned long e, char *ret) LHASH_OF(ERR_STRING_DATA) *ERR_get_string_table(void) { - err_fns_check(); - return ERRFN(err_get) (0); + return get_hash(0, 1); } LHASH_OF(ERR_STATE) *ERR_get_err_state_table(void) { - err_fns_check(); - return ERRFN(thread_get) (0); + return int_thread_get(0, 1); } void ERR_release_err_state_table(LHASH_OF(ERR_STATE) **hash) { - err_fns_check(); - ERRFN(thread_release) (hash); + int_thread_release(hash); } const char *ERR_lib_error_string(unsigned long e) @@ -953,10 +817,9 @@ const char *ERR_lib_error_string(unsigned long e) ERR_STRING_DATA d, *p; unsigned long l; - err_fns_check(); l = ERR_GET_LIB(e); d.error = ERR_PACK(l, 0, 0); - p = ERRFN(err_get_item) (&d); + p = int_err_get_item(&d); return ((p == NULL) ? NULL : p->string); } @@ -965,11 +828,10 @@ const char *ERR_func_error_string(unsigned long e) ERR_STRING_DATA d, *p; unsigned long l, f; - err_fns_check(); l = ERR_GET_LIB(e); f = ERR_GET_FUNC(e); d.error = ERR_PACK(l, f, 0); - p = ERRFN(err_get_item) (&d); + p = int_err_get_item(&d); return ((p == NULL) ? NULL : p->string); } @@ -978,14 +840,13 @@ const char *ERR_reason_error_string(unsigned long e) ERR_STRING_DATA d, *p = NULL; unsigned long l, r; - err_fns_check(); l = ERR_GET_LIB(e); r = ERR_GET_REASON(e); d.error = ERR_PACK(l, 0, r); - p = ERRFN(err_get_item) (&d); + p = int_err_get_item(&d); if (!p) { d.error = ERR_PACK(0, 0, r); - p = ERRFN(err_get_item) (&d); + p = int_err_get_item(&d); } return ((p == NULL) ? NULL : p->string); } @@ -998,12 +859,11 @@ void ERR_remove_thread_state(const CRYPTO_THREADID *id) CRYPTO_THREADID_cpy(&tmp.tid, id); else CRYPTO_THREADID_current(&tmp.tid); - err_fns_check(); /* * thread_del_item automatically destroys the LHASH if the number of * items reaches zero. */ - ERRFN(thread_del_item) (&tmp); + int_thread_del_item(&tmp); } #ifndef OPENSSL_NO_DEPRECATED @@ -1020,10 +880,9 @@ ERR_STATE *ERR_get_state(void) int i; CRYPTO_THREADID tid; - err_fns_check(); CRYPTO_THREADID_current(&tid); CRYPTO_THREADID_cpy(&tmp.tid, &tid); - ret = ERRFN(thread_get_item) (&tmp); + ret = int_thread_get_item(&tmp); /* ret == the error state, if NULL, make a new one */ if (ret == NULL) { @@ -1037,9 +896,9 @@ ERR_STATE *ERR_get_state(void) ret->err_data[i] = NULL; ret->err_data_flags[i] = 0; } - tmpp = ERRFN(thread_set_item) (ret); + tmpp = int_thread_set_item(ret); /* To check if insertion failed, do a get. */ - if (ERRFN(thread_get_item) (ret) != ret) { + if (int_thread_get_item(ret) != ret) { ERR_STATE_free(ret); /* could not insert it */ return (&fallback); } @@ -1055,8 +914,12 @@ ERR_STATE *ERR_get_state(void) int ERR_get_next_error_library(void) { - err_fns_check(); - return ERRFN(get_next_lib) (); + int ret; + + CRYPTO_w_lock(CRYPTO_LOCK_ERR); + ret = int_err_library_number++; + CRYPTO_w_unlock(CRYPTO_LOCK_ERR); + return ret; } void ERR_set_error_data(char *data, int flags) diff --git a/include/openssl/engine.h b/include/openssl/engine.h index e2f3e5cd9e..fa1d694784 100644 --- a/include/openssl/engine.h +++ b/include/openssl/engine.h @@ -776,7 +776,6 @@ typedef struct st_dynamic_LOCK_fns { /* The top-level structure */ typedef struct st_dynamic_fns { void *static_state; - const ERR_FNS *err_fns; const CRYPTO_EX_DATA_IMPL *ex_data_fns; dynamic_MEM_fns mem_fns; dynamic_LOCK_fns lock_fns; @@ -837,7 +836,6 @@ typedef int (*dynamic_bind_engine) (ENGINE *e, const char *id, CRYPTO_set_dynlock_destroy_callback(fns->lock_fns.dynlock_destroy_cb); \ if(!CRYPTO_set_ex_data_implementation(fns->ex_data_fns)) \ return 0; \ - if(!ERR_set_implementation(fns->err_fns)) return 0; \ skip_cbs: \ if(!fn(e,id)) return 0; \ return 1; } diff --git a/include/openssl/err.h b/include/openssl/err.h index 577a121475..e17706c253 100644 --- a/include/openssl/err.h +++ b/include/openssl/err.h @@ -362,19 +362,6 @@ int ERR_get_next_error_library(void); int ERR_set_mark(void); int ERR_pop_to_mark(void); -/* Already defined in ossl_typ.h */ -/* typedef struct st_ERR_FNS ERR_FNS; */ -/* - * An application can use this function and provide the return value to - * loaded modules that should use the application's ERR state/functionality - */ -const ERR_FNS *ERR_get_implementation(void); -/* - * A loaded module should call this function prior to any ERR operations - * using the application's "ERR_FNS". - */ -int ERR_set_implementation(const ERR_FNS *fns); - #ifdef __cplusplus } #endif diff --git a/include/openssl/ossl_typ.h b/include/openssl/ossl_typ.h index 85fb7b9b40..b32ce66843 100644 --- a/include/openssl/ossl_typ.h +++ b/include/openssl/ossl_typ.h @@ -172,8 +172,6 @@ typedef struct store_method_st STORE_METHOD; typedef struct ui_st UI; typedef struct ui_method_st UI_METHOD; -typedef struct st_ERR_FNS ERR_FNS; - typedef struct engine_st ENGINE; typedef struct ssl_st SSL; typedef struct ssl_ctx_st SSL_CTX; diff --git a/util/indent.pro b/util/indent.pro index c2427a3379..87e2b3ba4a 100644 --- a/util/indent.pro +++ b/util/indent.pro @@ -251,7 +251,6 @@ -T ENGINE_SSL_CLIENT_CERT_PTR -T ENGINE_TABLE -T ENUMERATED_NAMES --T ERR_FNS -T ERR_STATE -T ERR_STRING_DATA -T ESS_CERT_ID diff --git a/util/libeay.num b/util/libeay.num index c5d6ae96c2..553a160096 100755 --- a/util/libeay.num +++ b/util/libeay.num @@ -2033,7 +2033,7 @@ EC_POINT_set_compr_coords_GFp 2597 EXIST:VMS:FUNCTION:EC OCSP_response_status_str 2598 EXIST::FUNCTION: d2i_OCSP_REVOKEDINFO 2599 EXIST::FUNCTION: OCSP_basic_add1_cert 2600 EXIST::FUNCTION: -ERR_get_implementation 2601 EXIST::FUNCTION: +ERR_get_implementation 2601 NOEXIST::FUNCTION: EVP_CipherFinal_ex 2602 EXIST::FUNCTION: OCSP_CERTSTATUS_new 2603 EXIST::FUNCTION: CRYPTO_cleanup_all_ex_data 2604 EXIST::FUNCTION: @@ -2337,7 +2337,7 @@ UI_get0_result_string 2845 EXIST::FUNCTION: ASN1_GENERALSTRING_new 2846 EXIST::FUNCTION: X509_SIG_it 2847 EXIST:!EXPORT_VAR_AS_FUNCTION:VARIABLE: X509_SIG_it 2847 EXIST:EXPORT_VAR_AS_FUNCTION:FUNCTION: -ERR_set_implementation 2848 EXIST::FUNCTION: +ERR_set_implementation 2848 NOEXIST::FUNCTION: ERR_load_EC_strings 2849 EXIST::FUNCTION:EC UI_get0_action_string 2850 EXIST::FUNCTION: OCSP_ONEREQ_get_ext 2851 EXIST::FUNCTION: -- GitLab