Fix ex_data locks issue

Travis identified a problem with freeing the ex_data locks which wasn't
quite right in ff2344052. Trying to fix it identified a further problem:
the ex_data locks are cleaned up by OPENSSL_cleanup(), which is called
explicitly by CRYPTO_mem_leaks(), but then later the BIO passed to
CRYPTO_mem_leaks() is freed. An attempt is then made to use the ex_data
lock already freed.

Reviewed-by: Tim Hudson <tjh@openssl.org>

diff --git a/crypto/bio/bio_lib.c b/crypto/bio/bio_lib.c
index 025869454fa511a1f971692cfab4344652402339..94c97da369d5c038f408e938d97ed3737f47afe6 100644 (file)
--- a/crypto/bio/bio_lib.c
+++ b/crypto/bio/bio_lib.c
@@ -645,6 +645,10 @@ uint64_t BIO_number_written(BIO *bio)
     return 0;
 }
 
+void bio_free_ex_data(BIO *bio)
+{
+    CRYPTO_free_ex_data(CRYPTO_EX_INDEX_BIO, bio, &bio->ex_data);
+}
 
 void bio_cleanup(void)
 {

diff --git a/crypto/ex_data.c b/crypto/ex_data.c
index c607f87d654bc5879a38fb087dbf820201575599..ca1c204f38e116c2f81e0a957d52d92deb383109 100644 (file)
--- a/crypto/ex_data.c
+++ b/crypto/ex_data.c
@@ -134,7 +134,7 @@ typedef struct ex_callbacks_st {
 
 static EX_CALLBACKS ex_data[CRYPTO_EX_INDEX__COUNT];
 
-static CRYPTO_RWLOCK *ex_data_lock;
+static CRYPTO_RWLOCK *ex_data_lock = NULL;
 static CRYPTO_ONCE ex_data_init = CRYPTO_ONCE_STATIC_INIT;
 
 static void do_ex_data_init(void)
@@ -142,12 +142,6 @@ static void do_ex_data_init(void)
     ex_data_lock = CRYPTO_THREAD_lock_new();
 }
 
-void ex_data_cleanup(void)
-{
-    CRYPTO_THREAD_lock_free(ex_data_lock);
-    ex_data_lock = NULL;
-}
-
 /*
  * Return the EX_CALLBACKS from the |ex_data| array that corresponds to
  * a given class.  On success, *holds the lock.*
@@ -163,6 +157,19 @@ static EX_CALLBACKS *get_and_lock(int class_index)
 
     CRYPTO_THREAD_run_once(&ex_data_init, do_ex_data_init);
 
+    if (ex_data_lock == NULL) {
+        /*
+         * This can happen in normal operation when using CRYPTO_mem_leaks().
+         * The CRYPTO_mem_leaks() function calls OPENSSL_cleanup() which cleans
+         * up the locks. Subsequently the BIO that CRYPTO_mem_leaks() uses gets
+         * freed, which also attempts to free the ex_data. However
+         * CRYPTO_mem_leaks() ensures that the ex_data is freed early (i.e.
+         * before OPENSSL_cleanup() is called), so if we get here we can safely
+         * ignore this operation. We just treat it as an error.
+         */
+         return NULL;
+    }
+
     ip = &ex_data[class_index];
     CRYPTO_THREAD_write_lock(ex_data_lock);
     return ip;
@@ -189,6 +196,9 @@ void crypto_cleanup_all_ex_data_int(void)
         sk_EX_CALLBACK_pop_free(ip->meth, cleanup_cb);
         ip->meth = NULL;
     }
+
+    CRYPTO_THREAD_lock_free(ex_data_lock);
+    ex_data_lock = NULL;
 }
 
 

diff --git a/crypto/include/internal/cryptlib_int.h b/crypto/include/internal/cryptlib_int.h
index fd685229365ef000cbaf43668a2161423928340d..a38ac18d1f80da1e8d82ca63f73758b649f87155 100644 (file)
--- a/crypto/include/internal/cryptlib_int.h
+++ b/crypto/include/internal/cryptlib_int.h
@@ -65,7 +65,6 @@ struct thread_local_inits_st {
 };
 
 int ossl_init_thread_start(uint64_t opts);
-void ex_data_cleanup(void);
 
 /*
  * OPENSSL_INIT flags. The primary list of these is in crypto.h. Flags below

diff --git a/crypto/mem_dbg.c b/crypto/mem_dbg.c
index d4d72f25c6f1756ed7f4e2b59002d7ee5068d990..2b8cf73028deac21ff1ec542dd44fb293179620b 100644 (file)
--- a/crypto/mem_dbg.c
+++ b/crypto/mem_dbg.c
@@ -115,7 +115,7 @@
 #include "internal/threads.h"
 #include <openssl/crypto.h>
 #include <openssl/buffer.h>
-#include <openssl/bio.h>
+#include "internal/bio.h"
 #include <openssl/lhash.h>
 
 #ifndef OPENSSL_NO_CRYPTO_MDEBUG_BACKTRACE
@@ -636,6 +636,12 @@ int CRYPTO_mem_leaks(BIO *b)
 {
     MEM_LEAK ml;
 
+    /*
+     * OPENSSL_cleanup() will free the ex_data locks so we can't have any
+     * ex_data hanging around
+     */
+    bio_free_ex_data(b);
+
     /* Ensure all resources are released */
     OPENSSL_cleanup();
 

diff --git a/include/internal/bio.h b/include/internal/bio.h
index ec9dff69d231d1bbc72ca0dec8f1f05a6dcb3c17..31fe1aaaaf08249eb5866c869c183c403fd28a72 100644 (file)
--- a/include/internal/bio.h
+++ b/include/internal/bio.h
@@ -67,4 +67,5 @@ struct bio_method_st {
     long (*callback_ctrl) (BIO *, int, bio_info_cb *);
 };
 
+void bio_free_ex_data(BIO *bio);
 void bio_cleanup(void);