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]
