Locking issues.
authorBodo Möller <bodo@openssl.org>
Fri, 15 Dec 2000 16:40:35 +0000 (16:40 +0000)
committerBodo Möller <bodo@openssl.org>
Fri, 15 Dec 2000 16:40:35 +0000 (16:40 +0000)
CHANGES
apps/openssl.c
crypto/cryptlib.c
crypto/ex_data.c
crypto/mem_dbg.c
crypto/x509/x509_vfy.c
ssl/ssl_cert.c
ssl/ssltest.c

diff --git a/CHANGES b/CHANGES
index d19870387685a9757a4838b4c0581fbef4f4e7ea..5c31f057701600a9a600d25ad5a832e47a0b840b 100644 (file)
--- a/CHANGES
+++ b/CHANGES
@@ -3,6 +3,23 @@
 
  Changes between 0.9.6 and 0.9.7  [xx XXX 2000]
 
+  *) Add functionality to apps/openssl.c for detecting locking
+     problems: As the program is single-threaded, all we have
+     to do is register a locking callback using an array for
+     storing which locks are currently held by the program.
+
+     Fix a deadlock in CRYPTO_mem_leaks() that was detected in
+     apps/openssl.c.
+     [Bodo Moeller]
+
+  *) Use a lock around the call to CRYPTO_get_ex_new_index() in
+     SSL_get_ex_data_X509_STORE_idx(), which is used in
+     ssl_verify_cert_chain() and thus can be called at any time
+     during TLS/SSL handshakes so that thread-safety is essential.
+     Unfortunately, the ex_data design is not at all suited
+     for multi-threaded use, so it probably should be abolished.
+     [Bodo Moeller]
+
   *) Added Broadcom "ubsec" ENGINE to OpenSSL.
      [Broadcom, tweaked and integrated by Geoff Thorpe]
 
index 4ec9606a060d351acf1aeb1c760434b7f7e788dc..6c69a29bd66cb28b505853ebda7055d7eac79a0b 100644 (file)
  * copied and put under another distribution licence
  * [including the GNU Public Licence.]
  */
+/* ====================================================================
+ * Copyright (c) 1998-2000 The OpenSSL Project.  All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer. 
+ *
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in
+ *    the documentation and/or other materials provided with the
+ *    distribution.
+ *
+ * 3. All advertising materials mentioning features or use of this
+ *    software must display the following acknowledgment:
+ *    "This product includes software developed by the OpenSSL Project
+ *    for use in the OpenSSL Toolkit. (http://www.openssl.org/)"
+ *
+ * 4. The names "OpenSSL Toolkit" and "OpenSSL Project" must not be used to
+ *    endorse or promote products derived from this software without
+ *    prior written permission. For written permission, please contact
+ *    openssl-core@openssl.org.
+ *
+ * 5. Products derived from this software may not be called "OpenSSL"
+ *    nor may "OpenSSL" appear in their names without prior written
+ *    permission of the OpenSSL Project.
+ *
+ * 6. Redistributions of any form whatsoever must retain the following
+ *    acknowledgment:
+ *    "This product includes software developed by the OpenSSL Project
+ *    for use in the OpenSSL Toolkit (http://www.openssl.org/)"
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE OpenSSL PROJECT ``AS IS'' AND ANY
+ * EXPRESSED OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE OpenSSL PROJECT OR
+ * ITS CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
+ * OF THE POSSIBILITY OF SUCH DAMAGE.
+ * ====================================================================
+ *
+ * This product includes cryptographic software written by Eric Young
+ * (eay@cryptsoft.com).  This product includes software written by Tim
+ * Hudson (tjh@cryptsoft.com).
+ *
+ */
+
 
 #include <stdio.h>
 #include <string.h>
@@ -92,6 +146,71 @@ char *default_config_file=NULL;
 BIO *bio_err=NULL;
 #endif
 
+
+static void lock_dbg_cb(int mode, int type, const char *file, int line)
+       {
+       static int modes[CRYPTO_NUM_LOCKS]; /* = {0, 0, ... } */
+       const char *errstr = NULL;
+       int rw;
+       
+       rw = mode & (CRYPTO_READ|CRYPTO_WRITE);
+       if (!((rw == CRYPTO_READ) || (rw == CRYPTO_WRITE)))
+               {
+               errstr = "invalid mode";
+               goto err;
+               }
+
+       if (type < 0 || type > CRYPTO_NUM_LOCKS)
+               {
+               errstr = "type out of bounds";
+               goto err;
+               }
+
+       if (mode & CRYPTO_LOCK)
+               {
+               if (modes[type])
+                       {
+                       errstr = "already locked";
+                       /* must not happen in a single-threaded program
+                        * (would deadlock) */
+                       goto err;
+                       }
+
+               modes[type] = rw;
+               }
+       else if (mode & CRYPTO_UNLOCK)
+               {
+               if (!modes[type])
+                       {
+                       errstr = "not locked";
+                       goto err;
+                       }
+               
+               if (modes[type] != rw)
+                       {
+                       errstr = (rw == CRYPTO_READ) ?
+                               "CRYPTO_r_unlock on write lock" :
+                               "CRYPTO_w_unlock on read lock";
+                       }
+
+               modes[type] = 0;
+               }
+       else
+               {
+               errstr = "invalid mode";
+               goto err;
+               }
+
+ err:
+       if (errstr)
+               {
+               /* we cannot use bio_err here */
+               fprintf(stderr, "openssl (lock_dbg_cb): %s (mode=%d, type=%d) at %s:%d\n",
+                       errstr, mode, type, file, line);
+               }
+       }
+
+
 int main(int Argc, char *Argv[])
        {
        ARGS arg;
@@ -112,6 +231,13 @@ int main(int Argc, char *Argv[])
                CRYPTO_malloc_debug_init();
        CRYPTO_mem_ctrl(CRYPTO_MEM_CHECK_ON);
 
+#if 0
+       if (getenv("OPENSSL_DEBUG_LOCKING") != NULL)
+#endif
+               {
+               CRYPTO_set_locking_callback(lock_dbg_cb);
+               }
+
        apps_startup();
 
        if (bio_err == NULL)
index 9de60fd52816baf297cd9b3e73ccb400fbe95071..8634c078d8f376a55b7b19c5ea1482bb5acb77f6 100644 (file)
@@ -133,11 +133,11 @@ int CRYPTO_get_new_lockid(char *name)
        char *str;
        int i;
 
+#if defined(WIN32) || defined(WIN16)
        /* A hack to make Visual C++ 5.0 work correctly when linking as
         * a DLL using /MT. Without this, the application cannot use
         * and floating point printf's.
         * It also seems to be needed for Visual C 1.5 (win16) */
-#if defined(WIN32) || defined(WIN16)
        SSLeay_MSVC5_hack=(double)name[0]*(double)name[1];
 #endif
 
index 35ea2c298271bb6a4066ec5864428becfc3db8fe..3898c33f86731dd07304f2de2b1b37a58cbd1f0c 100644 (file)
@@ -1,4 +1,17 @@
 /* crypto/ex_data.c */
+
+/*
+ * This is not thread-safe, nor can it be changed to become thread-safe
+ * without changing various function prototypes and using a lot of locking.
+ * Luckily, it's not really used anywhere except in ssl_verify_cert_chain
+ * via SSL_get_ex_data_X509_STORE_CTX_idx (ssl/ssl_cert.c), where
+ * new_func, dup_func, and free_func all are 0.
+ *
+ * Any multi-threaded application crazy enough to use ex_data for its own
+ * purposes had better make sure that SSL_get_ex_data_X509_STORE_CTX_idx
+ * is called once before multiple threads are created.
+ */
+
 /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com)
  * All rights reserved.
  *
index 9ed8184e45f9ec0064713c39b660b4bb6e53f645..a6c70e431a9f67ed5781b384a5b9e281da48e456 100644 (file)
@@ -678,7 +678,15 @@ void CRYPTO_mem_leaks(BIO *b)
                 * void_fn_to_char kludge in CRYPTO_mem_leaks_cb.
                 * Otherwise the code police will come and get us.)
                 */
+               int old_mh_mode;
+
                CRYPTO_w_lock(CRYPTO_LOCK_MALLOC);
+
+               /* avoid deadlock when lh_free() uses CRYPTO_dbg_free(),
+                * which uses CRYPTO_is_mem_check_on */
+               old_mh_mode = mh_mode;
+               mh_mode = CRYPTO_MEM_CHECK_OFF;
+
                if (mh != NULL)
                        {
                        lh_free(mh);
@@ -692,6 +700,8 @@ void CRYPTO_mem_leaks(BIO *b)
                                amih = NULL;
                                }
                        }
+
+               mh_mode = old_mh_mode;
                CRYPTO_w_unlock(CRYPTO_LOCK_MALLOC);
                }
        MemCheck_on(); /* releases CRYPTO_LOCK_MALLOC2 */
index 0f4110cc64b23ae888d6cb8f09b2ac8134914633..32515cbcc40608a8142052fdab000bde763ad183 100644 (file)
@@ -80,10 +80,7 @@ const char *X509_version="X.509" OPENSSL_VERSION_PTEXT;
 
 static STACK_OF(CRYPTO_EX_DATA_FUNCS) *x509_store_ctx_method=NULL;
 static int x509_store_ctx_num=0;
-#if 0
-static int x509_store_num=1;
-static STACK *x509_store_method=NULL;
-#endif
+
 
 static int null_callback(int ok, X509_STORE_CTX *e)
        {
@@ -702,12 +699,17 @@ int X509_get_pubkey_parameters(EVP_PKEY *pkey, STACK_OF(X509) *chain)
 
 int X509_STORE_CTX_get_ex_new_index(long argl, void *argp, CRYPTO_EX_new *new_func,
             CRYPTO_EX_dup *dup_func, CRYPTO_EX_free *free_func)
-        {
-        x509_store_ctx_num++;
-        return CRYPTO_get_ex_new_index(x509_store_ctx_num-1,
+       {
+       /* This function is (usually) called only once, by
+        * SSL_get_ex_data_X509_STORE_CTX_idx (ssl/ssl_cert.c).
+        * That function uses locking, so we don't (usually)
+        * have to worry about locking here. For the whole cruel
+        * truth, see crypto/ex_data.c */
+       x509_store_ctx_num++;
+       return CRYPTO_get_ex_new_index(x509_store_ctx_num-1,
                &x509_store_ctx_method,
-                argl,argp,new_func,dup_func,free_func);
-        }
+               argl,argp,new_func,dup_func,free_func);
+       }
 
 int X509_STORE_CTX_set_ex_data(X509_STORE_CTX *ctx, int idx, void *data)
        {
index 130bb79068319ffcf3d5d4ecd1fb478a9fc9ea02..85d58c86688a454dd09da025b5dfdb0da8fae2c2 100644 (file)
 
 int SSL_get_ex_data_X509_STORE_CTX_idx(void)
        {
-       static int ssl_x509_store_ctx_idx= -1;
+       static volatile int ssl_x509_store_ctx_idx= -1;
 
-       /* FIXME: should do locking */
        if (ssl_x509_store_ctx_idx < 0)
                {
-               ssl_x509_store_ctx_idx=X509_STORE_CTX_get_ex_new_index(
-                       0,"SSL for verify callback",NULL,NULL,NULL);
+               /* any write lock will do; usually this branch
+                * will only be taken once anyway */
+               CRYPTO_w_lock(CRYPTO_LOCK_SSL_CTX);
+               
+               if (ssl_x509_store_ctx_idx < 0)
+                       {
+                       ssl_x509_store_ctx_idx=X509_STORE_CTX_get_ex_new_index(
+                               0,"SSL for verify callback",NULL,NULL,NULL);
+                       }
+               
+               CRYPTO_w_unlock(CRYPTO_LOCK_SSL_CTX);
                }
-       return(ssl_x509_store_ctx_idx);
+       return ssl_x509_store_ctx_idx;
        }
 
 CERT *ssl_cert_new(void)
@@ -452,13 +460,15 @@ int ssl_verify_cert_chain(SSL *s,STACK_OF(X509) *sk)
        if (SSL_get_verify_depth(s) >= 0)
                X509_STORE_CTX_set_depth(&ctx, SSL_get_verify_depth(s));
        X509_STORE_CTX_set_ex_data(&ctx,SSL_get_ex_data_X509_STORE_CTX_idx(),s);
+
        /* We need to set the verify purpose. The purpose can be determined by
         * the context: if its a server it will verify SSL client certificates
         * or vice versa.
-         */
-
-       if(s->server) i = X509_PURPOSE_SSL_CLIENT;
-       else i = X509_PURPOSE_SSL_SERVER;
+        */
+       if (s->server)
+               i = X509_PURPOSE_SSL_CLIENT;
+       else
+               i = X509_PURPOSE_SSL_SERVER;
 
        X509_STORE_CTX_purpose_inherit(&ctx, i, s->purpose, s->trust);
 
index 77ac362c814bd0b60129252bdfdd7202aca2b92a..9d513baf8089f81f07a5c30800eda54c648d017f 100644 (file)
  * copied and put under another distribution licence
  * [including the GNU Public Licence.]
  */
+/* ====================================================================
+ * Copyright (c) 1998-2000 The OpenSSL Project.  All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer. 
+ *
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in
+ *    the documentation and/or other materials provided with the
+ *    distribution.
+ *
+ * 3. All advertising materials mentioning features or use of this
+ *    software must display the following acknowledgment:
+ *    "This product includes software developed by the OpenSSL Project
+ *    for use in the OpenSSL Toolkit. (http://www.openssl.org/)"
+ *
+ * 4. The names "OpenSSL Toolkit" and "OpenSSL Project" must not be used to
+ *    endorse or promote products derived from this software without
+ *    prior written permission. For written permission, please contact
+ *    openssl-core@openssl.org.
+ *
+ * 5. Products derived from this software may not be called "OpenSSL"
+ *    nor may "OpenSSL" appear in their names without prior written
+ *    permission of the OpenSSL Project.
+ *
+ * 6. Redistributions of any form whatsoever must retain the following
+ *    acknowledgment:
+ *    "This product includes software developed by the OpenSSL Project
+ *    for use in the OpenSSL Toolkit (http://www.openssl.org/)"
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE OpenSSL PROJECT ``AS IS'' AND ANY
+ * EXPRESSED OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL THE OpenSSL PROJECT OR
+ * ITS CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
+ * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
+ * OF THE POSSIBILITY OF SUCH DAMAGE.
+ * ====================================================================
+ *
+ * This product includes cryptographic software written by Eric Young
+ * (eay@cryptsoft.com).  This product includes software written by Tim
+ * Hudson (tjh@cryptsoft.com).
+ *
+ */
 
 #include <assert.h>
 #include <errno.h>
@@ -202,6 +255,69 @@ static void print_details(SSL *c_ssl, const char *prefix)
        BIO_printf(bio_stdout,"\n");
        }
 
+static void lock_dbg_cb(int mode, int type, const char *file, int line)
+       {
+       static int modes[CRYPTO_NUM_LOCKS]; /* = {0, 0, ... } */
+       const char *errstr = NULL;
+       int rw;
+       
+       rw = mode & (CRYPTO_READ|CRYPTO_WRITE);
+       if (!((rw == CRYPTO_READ) || (rw == CRYPTO_WRITE)))
+               {
+               errstr = "invalid mode";
+               goto err;
+               }
+
+       if (type < 0 || type > CRYPTO_NUM_LOCKS)
+               {
+               errstr = "type out of bounds";
+               goto err;
+               }
+
+       if (mode & CRYPTO_LOCK)
+               {
+               if (modes[type])
+                       {
+                       errstr = "already locked";
+                       /* must not happen in a single-threaded program
+                        * (would deadlock) */
+                       goto err;
+                       }
+
+               modes[type] = rw;
+               }
+       else if (mode & CRYPTO_UNLOCK)
+               {
+               if (!modes[type])
+                       {
+                       errstr = "not locked";
+                       goto err;
+                       }
+               
+               if (modes[type] != rw)
+                       {
+                       errstr = (rw == CRYPTO_READ) ?
+                               "CRYPTO_r_unlock on write lock" :
+                               "CRYPTO_w_unlock on read lock";
+                       }
+
+               modes[type] = 0;
+               }
+       else
+               {
+               errstr = "invalid mode";
+               goto err;
+               }
+
+ err:
+       if (errstr)
+               {
+               /* we cannot use bio_err here */
+               fprintf(stderr, "openssl (lock_dbg_cb): %s (mode=%d, type=%d) at %s:%d\n",
+                       errstr, mode, type, file, line);
+               }
+       }
+
 int main(int argc, char *argv[])
        {
        char *CApath=NULL,*CAfile=NULL;
@@ -235,6 +351,8 @@ int main(int argc, char *argv[])
        debug = 0;
        cipher = 0;
        
+       CRYPTO_set_locking_callback(lock_dbg_cb);
+
        CRYPTO_mem_ctrl(CRYPTO_MEM_CHECK_ON);
 
        RAND_seed(rnd_seed, sizeof rnd_seed);