I stumbled across the same crashes that Philip did in crypto/err/err.c when used in a multithreaded application. I initially observed the problem in 0.9.7b under Linux, but found it was much easier to reproduce when running under the debugger of MSVC 6.0. In particular, I would often see a crash in ERR_clear_error() when called before every SSL_read() or SSL_write().
I agree with Philip's analysis of the problem. Attached is a patch in which I've made some changes to ensure mutually exclusive access to int_thread_lash and int_error_hash in critical areas. I've stress-tested this version in my application for several days continuously and have not seen any problems yet (previously, I'd see a crash within 3-4 hours). After examining how int_thread_hash is used (particularly in the external ERR_* API functions (pretty much everything that uses ERR_get_state()), I agree it simplifies things greatly to allocate it once and leave it in place until shutdown. Even a reference count scheme will be problematic, given functions like ERR_get_err_state_table(). Regarding the potential for a memory leak- I ensured int_thread_hash is ultimately freed in ERR_free_strings(). While it's slightly non-intuitive to do this in ERR_free_strings(), the advantage is that existing apps already leak-tested and using ERR_free_strings() in their cleanup sequences will remain leak-free without changes. I just joined openssl-dev and discovered Richard has been working on a fix as well. I'll have a look at that now. Mark Fontana Electronics For Imaging diff -ur openssl-0.9.7b/crypto/err/err.c openssl-0.9.7b-fix/crypto/err/err.c --- openssl-0.9.7b/crypto/err/err.c Tue Feb 18 06:15:13 2003 +++ openssl-0.9.7b-fix/crypto/err/err.c Thu Sep 25 13:16:54 2003 @@ -225,6 +225,7 @@ ERR_STRING_DATA *(*cb_err_del_item)(ERR_STRING_DATA *); /* Works on the "thread_hash" error-state table */ LHASH *(*cb_thread_get)(int create); + void (*cb_thread_del)(void); 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 *); @@ -239,6 +240,7 @@ static ERR_STRING_DATA *int_err_set_item(ERR_STRING_DATA *); static ERR_STRING_DATA *int_err_del_item(ERR_STRING_DATA *); static LHASH *int_thread_get(int create); +static void int_thread_del(void); 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 *); @@ -252,6 +254,7 @@ int_err_set_item, int_err_del_item, int_thread_get, + int_thread_del, int_thread_get_item, int_thread_set_item, int_thread_del_item, @@ -325,22 +328,18 @@ /* The internal functions used in the "err_defaults" implementation */ + +/* NOTE: CRYPTO_LOCK_ERR must be locked while calling this function */ static LHASH *int_err_get(int create) { - LHASH *ret = NULL; - - CRYPTO_w_lock(CRYPTO_LOCK_ERR); if (!int_error_hash && create) { CRYPTO_push_info("int_err_get (err.c)"); int_error_hash = lh_new(err_hash, err_cmp); CRYPTO_pop_info(); } - if (int_error_hash) - ret = int_error_hash; - CRYPTO_w_unlock(CRYPTO_LOCK_ERR); - return ret; + return int_error_hash; } static void int_err_del(void) @@ -356,33 +355,27 @@ static ERR_STRING_DATA *int_err_get_item(const ERR_STRING_DATA *d) { - ERR_STRING_DATA *p; + ERR_STRING_DATA *p = NULL; LHASH *hash; err_fns_check(); - hash = ERRFN(err_get)(0); - if (!hash) - return NULL; - - CRYPTO_r_lock(CRYPTO_LOCK_ERR); - p = (ERR_STRING_DATA *)lh_retrieve(hash, d); - CRYPTO_r_unlock(CRYPTO_LOCK_ERR); + CRYPTO_w_lock(CRYPTO_LOCK_ERR); + if ((hash = ERRFN(err_get)(0)) != NULL) + p = (ERR_STRING_DATA *)lh_retrieve(hash, d); + CRYPTO_w_unlock(CRYPTO_LOCK_ERR); return p; } static ERR_STRING_DATA *int_err_set_item(ERR_STRING_DATA *d) { - ERR_STRING_DATA *p; + ERR_STRING_DATA *p = NULL; LHASH *hash; err_fns_check(); - hash = ERRFN(err_get)(1); - if (!hash) - return NULL; - CRYPTO_w_lock(CRYPTO_LOCK_ERR); - p = (ERR_STRING_DATA *)lh_insert(hash, d); + if ((hash = ERRFN(err_get)(1)) != NULL) + p = (ERR_STRING_DATA *)lh_insert(hash, d); CRYPTO_w_unlock(CRYPTO_LOCK_ERR); return p; @@ -390,67 +383,66 @@ static ERR_STRING_DATA *int_err_del_item(ERR_STRING_DATA *d) { - ERR_STRING_DATA *p; + ERR_STRING_DATA *p = NULL; LHASH *hash; err_fns_check(); - hash = ERRFN(err_get)(0); - if (!hash) - return NULL; - CRYPTO_w_lock(CRYPTO_LOCK_ERR); - p = (ERR_STRING_DATA *)lh_delete(hash, d); + if ((hash = ERRFN(err_get)(0)) == NULL) + p = (ERR_STRING_DATA *)lh_delete(hash, d); CRYPTO_w_unlock(CRYPTO_LOCK_ERR); return p; } + +/* NOTE: CRYPTO_LOCK_ERR must be locked while calling this function */ static LHASH *int_thread_get(int create) { - LHASH *ret = NULL; - - CRYPTO_w_lock(CRYPTO_LOCK_ERR); if (!int_thread_hash && create) { CRYPTO_push_info("int_thread_get (err.c)"); int_thread_hash = lh_new(pid_hash, pid_cmp); CRYPTO_pop_info(); } + + return int_thread_hash; + } + +static void int_thread_del(void) + { + CRYPTO_w_lock(CRYPTO_LOCK_ERR); if (int_thread_hash) - ret = int_thread_hash; + { + lh_free(int_thread_hash); + int_thread_hash = NULL; + } CRYPTO_w_unlock(CRYPTO_LOCK_ERR); - return ret; } static ERR_STATE *int_thread_get_item(const ERR_STATE *d) { - ERR_STATE *p; + ERR_STATE *p = NULL; LHASH *hash; err_fns_check(); - hash = ERRFN(thread_get)(0); - if (!hash) - return NULL; - - CRYPTO_r_lock(CRYPTO_LOCK_ERR); - p = (ERR_STATE *)lh_retrieve(hash, d); - CRYPTO_r_unlock(CRYPTO_LOCK_ERR); + CRYPTO_w_lock(CRYPTO_LOCK_ERR); + if ((hash = ERRFN(thread_get)(0)) != NULL) + p = (ERR_STATE *)lh_retrieve(hash, d); + CRYPTO_w_unlock(CRYPTO_LOCK_ERR); return p; } static ERR_STATE *int_thread_set_item(ERR_STATE *d) { - ERR_STATE *p; + ERR_STATE *p = NULL; LHASH *hash; err_fns_check(); - hash = ERRFN(thread_get)(1); - if (!hash) - return NULL; - CRYPTO_w_lock(CRYPTO_LOCK_ERR); - p = (ERR_STATE *)lh_insert(hash, d); + if ((hash = ERRFN(thread_get)(1)) != NULL) + p = (ERR_STATE *)lh_insert(hash, d); CRYPTO_w_unlock(CRYPTO_LOCK_ERR); return p; @@ -458,22 +450,13 @@ static void int_thread_del_item(const ERR_STATE *d) { - ERR_STATE *p; + ERR_STATE *p = NULL; LHASH *hash; err_fns_check(); - hash = ERRFN(thread_get)(0); - if (!hash) - return; - CRYPTO_w_lock(CRYPTO_LOCK_ERR); - p = (ERR_STATE *)lh_delete(hash, d); - /* make sure we don't leak memory */ - if (int_thread_hash && (lh_num_items(int_thread_hash) == 0)) - { - lh_free(int_thread_hash); - int_thread_hash = NULL; - } + if ((hash = ERRFN(thread_get)(0)) != NULL) + p = (ERR_STATE *)lh_delete(hash, d); CRYPTO_w_unlock(CRYPTO_LOCK_ERR); if (p) @@ -611,6 +594,7 @@ { err_fns_check(); ERRFN(err_del)(); + ERRFN(thread_del)(); } /********************************************************/ @@ -835,14 +819,24 @@ LHASH *ERR_get_string_table(void) { + LHASH *hash; + err_fns_check(); - return ERRFN(err_get)(0); + CRYPTO_w_lock(CRYPTO_LOCK_ERR); + hash = ERRFN(err_get)(0); + CRYPTO_w_unlock(CRYPTO_LOCK_ERR); + return hash; } LHASH *ERR_get_err_state_table(void) { + LHASH *hash; + err_fns_check(); - return ERRFN(thread_get)(0); + CRYPTO_w_lock(CRYPTO_LOCK_ERR); + hash = ERRFN(thread_get)(0); + CRYPTO_w_unlock(CRYPTO_LOCK_ERR); + return hash; } const char *ERR_lib_error_string(unsigned long e) @@ -926,8 +920,6 @@ if (pid == 0) pid=(unsigned long)CRYPTO_thread_id(); tmp.pid=pid; - /* thread_del_item automatically destroys the LHASH if the number of - * items reaches zero. */ ERRFN(thread_del_item)(&tmp); } ______________________________________________________________________ OpenSSL Project http://www.openssl.org Development Mailing List [EMAIL PROTECTED] Automated List Manager [EMAIL PROTECTED]