Commit 1ee7b8b9 authored by Matt Caswell's avatar Matt Caswell
Browse files

Fix ex_data locks issue 



Travis identified a problem with freeing the ex_data locks which wasn't
quite right in ff234405. 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: default avatarTim Hudson <tjh@openssl.org>
parent 6e08e9e7

crypto/bio/bio_lib.c

+4 −0
Original line number Diff line number Diff line
@@ -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)

{

crypto/ex_data.c

+17 −7
Original line number Diff line number Diff line
@@ -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;

}

crypto/include/internal/cryptlib_int.h

+0 −1
Original line number Diff line number Diff line
@@ -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

crypto/mem_dbg.c

+7 −1
Original line number Diff line number Diff line
@@ -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();

include/internal/bio.h

+1 −0
Original line number Diff line number Diff line
@@ -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);