Don't take a write lock to retrieve a value from a stack

ossl_x509_store_ctx_get_by_subject() was taking a write lock for the
store, but was only (usually) retrieving a value from the stack of
objects. We take a read lock instead.

Partially fixes #20286

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Paul Dale <pauli@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/20952)

(cherry picked from commit 80935bf5ad309bf6c03591acf1d48fe1db57b78f)

1 files changed, 25 insertions, 9 deletions

diff --git a/crypto/x509/x509_lu.c b/crypto/x509/x509_lu.c
index d8927bda07..1fb46586f0 100644
--- a/crypto/x509/x509_lu.c
+++ b/crypto/x509/x509_lu.c
@@ -41,14 +41,19 @@ void X509_LOOKUP_free(X509_LOOKUP *ctx)
     OPENSSL_free(ctx);
 }
 
-int X509_STORE_lock(X509_STORE *s)
+int X509_STORE_lock(X509_STORE *xs)
 {
-    return CRYPTO_THREAD_write_lock(s->lock);
+    return CRYPTO_THREAD_write_lock(xs->lock);
 }
 
-int X509_STORE_unlock(X509_STORE *s)
+static int x509_store_read_lock(X509_STORE *xs)
 {
-    return CRYPTO_THREAD_unlock(s->lock);
+    return CRYPTO_THREAD_read_lock(xs->lock);
+}
+
+int X509_STORE_unlock(X509_STORE *xs)
+{
+    return CRYPTO_THREAD_unlock(xs->lock);
 }
 
 int X509_LOOKUP_init(X509_LOOKUP *ctx)
@@ -321,9 +326,19 @@ int X509_STORE_CTX_get_by_subject(const X509_STORE_CTX *vs,
     stmp.type = X509_LU_NONE;
     stmp.data.ptr = NULL;
 
-    if (!X509_STORE_lock(store))
+    if (!x509_store_read_lock(store))
         return 0;
-
+    /* Should already be sorted...but just in case */
+    if (!sk_X509_OBJECT_is_sorted(store->objs)) {
+        X509_STORE_unlock(store);
+        /* Take a write lock instead of a read lock */
+        X509_STORE_lock(store);
+        /*
+         * Another thread might have sorted it in the meantime. But if so,
+         * sk_X509_OBJECT_sort() exits early.
+         */
+        sk_X509_OBJECT_sort(store->objs);
+    }
     tmp = X509_OBJECT_retrieve_by_subject(store->objs, type, name);
     X509_STORE_unlock(store);
 
@@ -505,7 +520,6 @@ static int x509_object_idx_cnt(STACK_OF(X509_OBJECT) *h, X509_LOOKUP_TYPE type,
     X509_OBJECT stmp;
     X509 x509_s;
     X509_CRL crl_s;
-    int idx;
 
     stmp.type = type;
     switch (type) {
@@ -522,16 +536,18 @@ static int x509_object_idx_cnt(STACK_OF(X509_OBJECT) *h, X509_LOOKUP_TYPE type,
         return -1;
     }
 
-    idx = sk_X509_OBJECT_find_all(h, &stmp, pnmatch);
-    return idx;
+    /* Assumes h is locked for read if applicable */
+    return sk_X509_OBJECT_find_all(h, &stmp, pnmatch);
 }
 
+/* Assumes h is locked for read if applicable */
 int X509_OBJECT_idx_by_subject(STACK_OF(X509_OBJECT) *h, X509_LOOKUP_TYPE type,
                                const X509_NAME *name)
 {
     return x509_object_idx_cnt(h, type, name, NULL);
 }
 
+/* Assumes h is locked for read if applicable */
 X509_OBJECT *X509_OBJECT_retrieve_by_subject(STACK_OF(X509_OBJECT) *h,
                                              X509_LOOKUP_TYPE type,
                                              const X509_NAME *name)