Locking issues.

8 files changed, 315 insertions, 19 deletions

diff --git a/CHANGES b/CHANGES
index d198703876..5c31f05770 100644
--- 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]
 
diff --git a/apps/openssl.c b/apps/openssl.c
index 4ec9606a06..6c69a29bd6 100644
--- a/apps/openssl.c
+++ b/apps/openssl.c
@@ -55,6 +55,60 @@
  * 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)
diff --git a/crypto/cryptlib.c b/crypto/cryptlib.c
index 9de60fd528..8634c078d8 100644
--- a/crypto/cryptlib.c
+++ b/crypto/cryptlib.c
@@ -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
 
diff --git a/crypto/ex_data.c b/crypto/ex_data.c
index 35ea2c2982..3898c33f86 100644
--- a/crypto/ex_data.c
+++ b/crypto/ex_data.c
@@ -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.
  *
diff --git a/crypto/mem_dbg.c b/crypto/mem_dbg.c
index 9ed8184e45..a6c70e431a 100644
--- a/crypto/mem_dbg.c
+++ b/crypto/mem_dbg.c
@@ -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 */
diff --git a/crypto/x509/x509_vfy.c b/crypto/x509/x509_vfy.c
index 0f4110cc64..32515cbcc4 100644
--- a/crypto/x509/x509_vfy.c
+++ b/crypto/x509/x509_vfy.c
@@ -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)
 	{
diff --git a/ssl/ssl_cert.c b/ssl/ssl_cert.c
index 130bb79068..85d58c8668 100644
--- a/ssl/ssl_cert.c
+++ b/ssl/ssl_cert.c
@@ -129,15 +129,23 @@
 
 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);
 
diff --git a/ssl/ssltest.c b/ssl/ssltest.c
index 77ac362c81..9d513baf80 100644
--- a/ssl/ssltest.c
+++ b/ssl/ssltest.c
@@ -55,6 +55,59 @@
  * 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);