Fix X509_STORE locking

CHANGES

@@ -896,7 +896,11 @@
   *) Change 'Configure' script to enable Camellia by default.
      [NTT]
 
- Changes between 0.9.8l (?) and 0.9.8m (?)  [xx XXX xxxx]
+ Changes between 0.9.8l and 0.9.8m  [xx XXX xxxx]
+
+  *) Fix X509_STORE locking: Every 'objs' access requires a lock (to
+     accommodate for stack sorting, always a write lock!).
+     [Bodo Moeller]
 
   *) On some versions of WIN32 Heap32Next is very slow. This can cause
      excessive delays in the RAND_poll(): over a minute. As a workaround

crypto/x509/by_dir.c

@@ -420,11 +420,11 @@ static int get_cert_by_subject(X509_LOOKUP *xl, int type, X509_NAME *name,
 
 		/* we have added it to the cache so now pull
 		 * it out again */
-		CRYPTO_r_lock(CRYPTO_LOCK_X509_STORE);
+		CRYPTO_w_lock(CRYPTO_LOCK_X509_STORE);
 		j = sk_X509_OBJECT_find(xl->store_ctx->objs,&stmp);
 		if(j != -1) tmp=sk_X509_OBJECT_value(xl->store_ctx->objs,j);
 		else tmp = NULL;
-		CRYPTO_r_unlock(CRYPTO_LOCK_X509_STORE);
+		CRYPTO_w_unlock(CRYPTO_LOCK_X509_STORE);
 
 
 		/* If a CRL, update the last file suffix added for this */
@@ -480,4 +480,3 @@ finish:
 	if (b != NULL) BUF_MEM_free(b);
 	return(ok);
 	}
-

crypto/x509/x509_lu.c

@@ -294,7 +294,9 @@ int X509_STORE_get_by_subject(X509_STORE_CTX *vs, int type, X509_NAME *name,
 	X509_OBJECT stmp,*tmp;
 	int i,j;
 
+	CRYPTO_w_lock(CRYPTO_LOCK_X509_STORE);
 	tmp=X509_OBJECT_retrieve_by_subject(ctx->objs,type,name);
+	CRYPTO_w_unlock(CRYPTO_LOCK_X509_STORE);
 
 	if (tmp == NULL || type == X509_LU_CRL)
 		{
@@ -348,7 +350,6 @@ int X509_STORE_add_cert(X509_STORE *ctx, X509 *x)
 
 	X509_OBJECT_up_ref_count(obj);
 
-
 	if (X509_OBJECT_retrieve_match(ctx->objs, obj))
 		{
 		X509_OBJECT_free_contents(obj);
@@ -477,12 +478,12 @@ int X509_OBJECT_idx_by_subject(STACK_OF(X509_OBJECT) *h, int type,
 
 X509_OBJECT *X509_OBJECT_retrieve_by_subject(STACK_OF(X509_OBJECT) *h, int type,
 	     X509_NAME *name)
-{
+	{
 	int idx;
 	idx = X509_OBJECT_idx_by_subject(h, type, name);
 	if (idx==-1) return NULL;
 	return sk_X509_OBJECT_value(h, idx);
-}
+	}
 
 STACK_OF(X509)* X509_STORE_get1_certs(X509_STORE_CTX *ctx, X509_NAME *nm)
 	{
@@ -491,7 +492,7 @@ STACK_OF(X509)* X509_STORE_get1_certs(X509_STORE_CTX *ctx, X509_NAME *nm)
 	X509 *x;
 	X509_OBJECT *obj;
 	sk = sk_X509_new_null();
-	CRYPTO_r_lock(CRYPTO_LOCK_X509_STORE);
+	CRYPTO_w_lock(CRYPTO_LOCK_X509_STORE);
 	idx = x509_object_idx_cnt(ctx->ctx->objs, X509_LU_X509, nm, &cnt);
 	if (idx < 0)
 		{
@@ -499,18 +500,18 @@ STACK_OF(X509)* X509_STORE_get1_certs(X509_STORE_CTX *ctx, X509_NAME *nm)
 		 * objects to cache
 		 */
 		X509_OBJECT xobj;
-		CRYPTO_r_unlock(CRYPTO_LOCK_X509_STORE);
+		CRYPTO_w_unlock(CRYPTO_LOCK_X509_STORE);
 		if (!X509_STORE_get_by_subject(ctx, X509_LU_X509, nm, &xobj))
 			{
 			sk_X509_free(sk);
 			return NULL;
 			}
 		X509_OBJECT_free_contents(&xobj);
-		CRYPTO_r_lock(CRYPTO_LOCK_X509_STORE);
+		CRYPTO_w_lock(CRYPTO_LOCK_X509_STORE);
 		idx = x509_object_idx_cnt(ctx->ctx->objs,X509_LU_X509,nm, &cnt);
 		if (idx < 0)
 			{
-			CRYPTO_r_unlock(CRYPTO_LOCK_X509_STORE);
+			CRYPTO_w_unlock(CRYPTO_LOCK_X509_STORE);
 			sk_X509_free(sk);
 			return NULL;
 			}
@@ -522,13 +523,13 @@ STACK_OF(X509)* X509_STORE_get1_certs(X509_STORE_CTX *ctx, X509_NAME *nm)
 		CRYPTO_add(&x->references, 1, CRYPTO_LOCK_X509);
 		if (!sk_X509_push(sk, x))
 			{
-			CRYPTO_r_unlock(CRYPTO_LOCK_X509_STORE);
+			CRYPTO_w_unlock(CRYPTO_LOCK_X509_STORE);
 			X509_free(x);
 			sk_X509_pop_free(sk, X509_free);
 			return NULL;
 			}
 		}
-	CRYPTO_r_unlock(CRYPTO_LOCK_X509_STORE);
+	CRYPTO_w_unlock(CRYPTO_LOCK_X509_STORE);
 	return sk;
 
 	}
@@ -540,24 +541,24 @@ STACK_OF(X509_CRL)* X509_STORE_get1_crls(X509_STORE_CTX *ctx, X509_NAME *nm)
 	X509_CRL *x;
 	X509_OBJECT *obj, xobj;
 	sk = sk_X509_CRL_new_null();
-	CRYPTO_r_lock(CRYPTO_LOCK_X509_STORE);
+	CRYPTO_w_lock(CRYPTO_LOCK_X509_STORE);
 	/* Check cache first */
 	idx = x509_object_idx_cnt(ctx->ctx->objs, X509_LU_CRL, nm, &cnt);
 
 	/* Always do lookup to possibly add new CRLs to cache
 	 */
-	CRYPTO_r_unlock(CRYPTO_LOCK_X509_STORE);
+	CRYPTO_w_unlock(CRYPTO_LOCK_X509_STORE);
 	if (!X509_STORE_get_by_subject(ctx, X509_LU_CRL, nm, &xobj))
 		{
 		sk_X509_CRL_free(sk);
 		return NULL;
 		}
 	X509_OBJECT_free_contents(&xobj);
-	CRYPTO_r_lock(CRYPTO_LOCK_X509_STORE);
+	CRYPTO_w_lock(CRYPTO_LOCK_X509_STORE);
 	idx = x509_object_idx_cnt(ctx->ctx->objs,X509_LU_CRL, nm, &cnt);
 	if (idx < 0)
 		{
-		CRYPTO_r_unlock(CRYPTO_LOCK_X509_STORE);
+		CRYPTO_w_unlock(CRYPTO_LOCK_X509_STORE);
 		sk_X509_CRL_free(sk);
 		return NULL;
 		}
@@ -569,19 +570,18 @@ STACK_OF(X509_CRL)* X509_STORE_get1_crls(X509_STORE_CTX *ctx, X509_NAME *nm)
 		CRYPTO_add(&x->references, 1, CRYPTO_LOCK_X509_CRL);
 		if (!sk_X509_CRL_push(sk, x))
 			{
-			CRYPTO_r_unlock(CRYPTO_LOCK_X509_STORE);
+			CRYPTO_w_unlock(CRYPTO_LOCK_X509_STORE);
 			X509_CRL_free(x);
 			sk_X509_CRL_pop_free(sk, X509_CRL_free);
 			return NULL;
 			}
 		}
-	CRYPTO_r_unlock(CRYPTO_LOCK_X509_STORE);
+	CRYPTO_w_unlock(CRYPTO_LOCK_X509_STORE);
 	return sk;
-
 	}
 
 X509_OBJECT *X509_OBJECT_retrieve_match(STACK_OF(X509_OBJECT) *h, X509_OBJECT *x)
-{
+	{
 	int idx, i;
 	X509_OBJECT *obj;
 	idx = sk_X509_OBJECT_find(h, x);
@@ -607,13 +607,13 @@ X509_OBJECT *X509_OBJECT_retrieve_match(STACK_OF(X509_OBJECT) *h, X509_OBJECT *x
 			return obj;
 		}
 	return NULL;
-}
+	}
 
 
 /* Try to get issuer certificate from store. Due to limitations
  * of the API this can only retrieve a single certificate matching
  * a given subject name. However it will fill the cache with all
- * matching certificates, so we can examine the cache for all 
+ * matching certificates, so we can examine the cache for all
  * matches.
  *
  * Return values are:
@@ -621,13 +621,11 @@ X509_OBJECT *X509_OBJECT_retrieve_match(STACK_OF(X509_OBJECT) *h, X509_OBJECT *x
  *  0 certificate not found.
  * -1 some other error.
  */
-
-
 int X509_STORE_CTX_get1_issuer(X509 **issuer, X509_STORE_CTX *ctx, X509 *x)
-{
+	{
 	X509_NAME *xn;
 	X509_OBJECT obj, *pobj;
-	int i, ok, idx;
+	int i, ok, idx, ret;
 	xn=X509_get_issuer_name(x);
 	ok=X509_STORE_get_by_subject(ctx,X509_LU_X509,xn,&obj);
 	if (ok != X509_LU_X509)
@@ -653,27 +651,34 @@ int X509_STORE_CTX_get1_issuer(X509 **issuer, X509_STORE_CTX *ctx, X509 *x)
 		return 1;
 		}
 	X509_OBJECT_free_contents(&obj);
-	/* Else find index of first matching cert */
-	idx = X509_OBJECT_idx_by_subject(ctx->ctx->objs, X509_LU_X509, xn);
-	/* This shouldn't normally happen since we already have one match */
-	if (idx == -1) return 0;
 
-	/* Look through all matching certificates for a suitable issuer */
-	for (i = idx; i < sk_X509_OBJECT_num(ctx->ctx->objs); i++)
+	/* Else find index of first cert accepted by 'check_issued' */
+	ret = 0;
+	CRYPTO_w_lock(CRYPTO_LOCK_X509_STORE);
+	idx = X509_OBJECT_idx_by_subject(ctx->ctx->objs, X509_LU_X509, xn);
+	if (idx != -1) /* should be true as we've had at least one match */
 		{
-		pobj = sk_X509_OBJECT_value(ctx->ctx->objs, i);
-		/* See if we've ran out of matches */
-		if (pobj->type != X509_LU_X509) return 0;
-		if (X509_NAME_cmp(xn, X509_get_subject_name(pobj->data.x509))) return 0;
-		if (ctx->check_issued(ctx, x, pobj->data.x509))
+		/* Look through all matching certs for suitable issuer */
+		for (i = idx; i < sk_X509_OBJECT_num(ctx->ctx->objs); i++)
 			{
-			*issuer = pobj->data.x509;
-			X509_OBJECT_up_ref_count(pobj);
-			return 1;
+			pobj = sk_X509_OBJECT_value(ctx->ctx->objs, i);
+			/* See if we've run past the matches */
+			if (pobj->type != X509_LU_X509)
+				break;
+			if (X509_NAME_cmp(xn, X509_get_subject_name(pobj->data.x509)))
+				break;
+			if (ctx->check_issued(ctx, x, pobj->data.x509))
+				{
+				*issuer = pobj->data.x509;
+				X509_OBJECT_up_ref_count(pobj);
+				ret = 1;
+				break;
+				}
 			}
 		}
-	return 0;
-}
+	CRYPTO_w_unlock(CRYPTO_LOCK_X509_STORE);
+	return ret;
+	}
 
 int X509_STORE_set_flags(X509_STORE *ctx, unsigned long flags)
 	{