]> git.ipfire.org Git - thirdparty/openssl.git/commitdiff
x509store: reduce lock contention in X509_STORE
authorNikola Pajkovsky <nikolap@openssl.org>
Thu, 18 Sep 2025 09:13:45 +0000 (11:13 +0200)
committerNeil Horman <nhorman@openssl.org>
Thu, 16 Oct 2025 13:11:20 +0000 (09:11 -0400)
X509_STORE was using STACK_OF(X509_OBJECT) which is not ideal structure. The
better solution is to use hashmap. The performance gains come from the fact that
sorting was removed and therefore read lock is just enough for looking up
objects/cert/crls from hashmap.

When X509_STORE_get0_objects() is called, the hashmap converts back to
the STACK_OF(X509_OBJECT), and goes back to the original
implementation with the performance hit on lookup side because stack is not
sorted anymore.

Note, hashmap maps X509_NAME to STACK_OF(X509_OBJECT), and the stack is never
sorted which may lead to performance impact if stack contains a huge of objects.

Before the change

| Threads |   mean/us |  var/us |
|---------+-----------+---------|
|       1 |  2.434803 | .034190 |
|       2 |  3.033588 | .247471 |
|       4 |  6.551132 | .150209 |
|       6 | 12.548113 | .258445 |
|       8 | 17.566257 | .168508 |
|      10 | 22.782846 | .182674 |
|      12 | 27.928990 | .426779 |
|      14 | 32.844572 | .307754 |
|      16 | 37.816247 | .660630 |
|      18 | 42.662465 | .434926 |

After the change

| Threads |  mean/us |  var/us |
|---------+----------+---------|
|       1 | 2.385398 | .015329 |
|       2 | 2.775794 | .172223 |
|       4 | 3.071882 | .126400 |
|       6 | 3.174147 | .139685 |
|       8 | 3.479235 | .297154 |
|      10 | 4.206260 | .149006 |
|      12 | 5.044039 | .194108 |
|      14 | 5.890640 | .185817 |
|      16 | 6.447808 | .256179 |
|      18 | 7.489261 | .149204 |

Resolves: https://github.com/openssl/project/issues/1275
Signed-off-by: Nikola Pajkovsky <nikolap@openssl.org>
Reviewed-by: Neil Horman <nhorman@openssl.org>
Reviewed-by: Saša Nedvědický <sashan@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/28599)

crypto/x509/by_dir.c
crypto/x509/by_store.c
crypto/x509/x509_local.h
crypto/x509/x509_lu.c
test/cmp_protect_test.c
test/helpers/cmp_testlib.c
test/helpers/cmp_testlib.h
test/x509_load_cert_file_test.c

index b3731b01d15f1cbc5dc0c3845ddb515b3f023138..20ad8062fc711a9f086e2c5cb121acf65f59de65 100644 (file)
@@ -232,7 +232,7 @@ static int get_cert_by_subject_ex(X509_LOOKUP *xl, X509_LOOKUP_TYPE type,
     int i, j, k;
     unsigned long h;
     BUF_MEM *b = NULL;
-    X509_OBJECT stmp, *tmp;
+    X509_OBJECT stmp, *tmp = NULL;
     const char *postfix = "";
 
     if (name == NULL)
@@ -348,10 +348,18 @@ static int get_cert_by_subject_ex(X509_LOOKUP *xl, X509_LOOKUP_TYPE type,
          *       sorted and sorting the would result in O(n^2 log n) complexity.
          */
         if (k > 0) {
-            if (!X509_STORE_lock(xl->store_ctx))
+            STACK_OF(X509_OBJECT) *objs;
+
+            if (ossl_x509_store_read_lock(xl->store_ctx) == 0)
                 goto finish;
-            j = sk_X509_OBJECT_find(xl->store_ctx->objs, &stmp);
-            tmp = sk_X509_OBJECT_value(xl->store_ctx->objs, j);
+            if (xl->store_ctx->objs_ht)
+                objs = ossl_x509_store_ht_get_by_name(xl->store_ctx, name);
+            else
+                objs = xl->store_ctx->objs;
+            if (objs != NULL) {
+                j = sk_X509_OBJECT_find(objs, &stmp);
+                tmp = sk_X509_OBJECT_value(objs, j);
+            }
             X509_STORE_unlock(xl->store_ctx);
         } else {
             tmp = NULL;
@@ -419,14 +427,6 @@ static int get_cert_by_subject_ex(X509_LOOKUP *xl, X509_LOOKUP_TYPE type,
         }
     }
  finish:
-    /* If we changed anything, resort the objects for faster lookup */
-    if (X509_STORE_lock(xl->store_ctx)) {
-        if (!sk_X509_OBJECT_is_sorted(xl->store_ctx->objs)) {
-            sk_X509_OBJECT_sort(xl->store_ctx->objs);
-        }
-        X509_STORE_unlock(xl->store_ctx);
-    }
-
     BUF_MEM_free(b);
     return ok;
 }
index a969e3aa05b8c813262aa64f5168195ef64986f5..043801c2efcecbb912d020f233e992cffa2a4f15 100644 (file)
@@ -218,18 +218,22 @@ static int by_store_subject(X509_LOOKUP *ctx, X509_LOOKUP_TYPE type,
     OSSL_STORE_SEARCH *criterion =
         OSSL_STORE_SEARCH_by_name((X509_NAME *)name); /* won't modify it */
     int ok = by_store(ctx, type, criterion, ret);
-    STACK_OF(X509_OBJECT) *store_objects =
-        X509_STORE_get0_objects(X509_LOOKUP_get_store(ctx));
     X509_OBJECT *tmp = NULL;
 
     OSSL_STORE_SEARCH_free(criterion);
 
     if (ok) {
+        STACK_OF(X509_OBJECT) *store_objects;
         X509_STORE *store = X509_LOOKUP_get_store(ctx);
 
         if (!ossl_x509_store_read_lock(store))
             return 0;
-        tmp = X509_OBJECT_retrieve_by_subject(store_objects, type, name);
+        if (store->objs_ht != NULL)
+            store_objects = ossl_x509_store_ht_get_by_name(store, name);
+        else
+            store_objects = store->objs;
+        if (store_objects != NULL)
+            tmp = X509_OBJECT_retrieve_by_subject(store_objects, type, name);
         X509_STORE_unlock(store);
     }
 
index ca56f478874c49360b71e8349127829e44bd6f93..649bd32405cc3412b32444a0073c440e2d9f0b11 100644 (file)
@@ -8,6 +8,7 @@
  */
 
 #include "internal/refcount.h"
+#include "internal/hashtable.h"
 
 #define X509V3_conf_add_error_name_value(val) \
     ERR_add_error_data(4, "name=", (val)->name, ", value=", (val)->value)
@@ -106,6 +107,11 @@ struct x509_lookup_st {
     X509_STORE *store_ctx;      /* who owns us */
 };
 
+HT_START_KEY_DEFN(objs_key)
+HT_DEF_KEY_FIELD(xn_canon, unsigned char *)
+HT_DEF_KEY_FIELD(xn_canon_enclen, int)
+HT_END_KEY_DEFN(OBJS_KEY)
+
 /*
  * This is used to hold everything.  It is used for all certificate
  * validation.  Once we have a certificate chain, the 'verify' function is
@@ -114,7 +120,13 @@ struct x509_lookup_st {
 struct x509_store_st {
     /* The following is a cache of trusted certs */
     int cache;                  /* if true, stash any hits */
-    STACK_OF(X509_OBJECT) *objs; /* Cache of all objects */
+    /* Maps X509_NAME -> STACK_OF(X509_OBJECT) */
+    HT *objs_ht;
+    /*
+     * Deprecated. Used only in the X509_STORE_get0_objects() for backward
+     * compatibility.
+     */
+    STACK_OF(X509_OBJECT) *objs;
     /* These are external lookup methods */
     STACK_OF(X509_LOOKUP) *get_cert_methods;
     X509_VERIFY_PARAM *param;
@@ -159,4 +171,6 @@ int ossl_x509_likely_issued(X509 *issuer, X509 *subject);
 int ossl_x509_signing_allowed(const X509 *issuer, const X509 *subject);
 int ossl_x509_store_ctx_get_by_subject(const X509_STORE_CTX *ctx, X509_LOOKUP_TYPE type,
                                        const X509_NAME *name, X509_OBJECT *ret);
-int ossl_x509_store_read_lock(X509_STORE *xs);
+__owur int ossl_x509_store_read_lock(X509_STORE *xs);
+STACK_OF(X509_OBJECT) *ossl_x509_store_ht_get_by_name(const X509_STORE *store,
+                                                      const X509_NAME *xn);
index eb2d47955b2efb273fa06f7d6893b3562cdfff30..f724bf1e2d593cfc9fde3403473cc4c9777278a1 100644 (file)
@@ -9,12 +9,16 @@
 
 #include <stdio.h>
 #include "internal/cryptlib.h"
+#include "internal/hashtable.h"
+#include "internal/hashfunc.h"
 #include "internal/refcount.h"
 #include <openssl/x509.h>
 #include "crypto/x509.h"
 #include <openssl/x509v3.h>
 #include "x509_local.h"
 
+#define X509_OBJS_HT_BUCKETS 512
+
 X509_LOOKUP *X509_LOOKUP_new(X509_LOOKUP_METHOD *method)
 {
     X509_LOOKUP *ret = OPENSSL_zalloc(sizeof(*ret));
@@ -179,12 +183,36 @@ static int x509_object_cmp(const X509_OBJECT *const *a,
     return ret;
 }
 
+static void objs_ht_free(HT_VALUE *v)
+{
+    STACK_OF(X509_OBJECT) *objs = v->value;
+
+    sk_X509_OBJECT_pop_free(objs, X509_OBJECT_free);
+}
+
+static uint64_t obj_ht_hash(HT_KEY *key)
+{
+    OBJS_KEY *k = (OBJS_KEY *)key;
+
+    return ossl_fnv1a_hash(k->keyfields.xn_canon, k->keyfields.xn_canon_enclen);
+}
+
 X509_STORE *X509_STORE_new(void)
 {
     X509_STORE *ret = OPENSSL_zalloc(sizeof(*ret));
+    HT_CONFIG htconf = {
+        .ht_free_fn = objs_ht_free,
+        .ht_hash_fn = obj_ht_hash,
+        .init_neighborhoods = X509_OBJS_HT_BUCKETS,
+        .no_rcu = 1,
+    };
 
     if (ret == NULL)
         return NULL;
+    if ((ret->objs_ht = ossl_ht_new(&htconf)) == NULL) {
+        ERR_raise(ERR_LIB_X509, ERR_R_CRYPTO_LIB);
+        goto err;
+    }
     if ((ret->objs = sk_X509_OBJECT_new(x509_object_cmp)) == NULL) {
         ERR_raise(ERR_LIB_X509, ERR_R_CRYPTO_LIB);
         goto err;
@@ -218,6 +246,7 @@ err:
     X509_VERIFY_PARAM_free(ret->param);
     sk_X509_OBJECT_free(ret->objs);
     sk_X509_LOOKUP_free(ret->get_cert_methods);
+    ossl_ht_free(ret->objs_ht);
     CRYPTO_THREAD_lock_free(ret->lock);
     OPENSSL_free(ret);
     return NULL;
@@ -250,6 +279,7 @@ void X509_STORE_free(X509_STORE *xs)
     X509_VERIFY_PARAM_free(xs->param);
     CRYPTO_THREAD_lock_free(xs->lock);
     CRYPTO_FREE_REF(&xs->references);
+    ossl_ht_free(xs->objs_ht);
     OPENSSL_free(xs);
 }
 
@@ -310,6 +340,93 @@ X509_OBJECT *X509_STORE_CTX_get_obj_by_subject(X509_STORE_CTX *ctx,
     return ret;
 }
 
+STACK_OF(X509_OBJECT) *ossl_x509_store_ht_get_by_name(const X509_STORE *store,
+                                                      const X509_NAME *xn)
+{
+    HT_VALUE *v;
+    OBJS_KEY key;
+    int ret;
+
+    if (xn->canon_enc == NULL || xn->modified) {
+        ret = i2d_X509_NAME((X509_NAME *)xn, NULL);
+        if (ret < 0)
+            return NULL;
+    }
+
+    HT_INIT_KEY(&key);
+    HT_SET_KEY_FIELD(&key, xn_canon, xn->canon_enc);
+    HT_SET_KEY_FIELD(&key, xn_canon_enclen, xn->canon_enclen);
+    v = ossl_ht_get(store->objs_ht, TO_HT_KEY(&key));
+    if (v == NULL)
+        return NULL;
+    v = ossl_rcu_deref(&v);
+
+    return v->value;
+}
+
+static int x509_name_objs_ht_insert(const X509_STORE *store, const X509_NAME *xn,
+                                    STACK_OF(X509_OBJECT) *objs, X509_OBJECT *obj)
+{
+    int ret = 0, added = 0;
+    OBJS_KEY key;
+    HT_VALUE val = {0};
+
+    if (objs != NULL) {
+        added = sk_X509_OBJECT_push(objs, obj) != 0;
+        return added;
+    }
+
+    if (xn->canon_enc == NULL || xn->modified) {
+        ret = i2d_X509_NAME((X509_NAME *)xn, NULL);
+        if (ret < 0)
+            return 0;
+    }
+
+    objs = sk_X509_OBJECT_new(x509_object_cmp);
+    if (objs == NULL)
+        return 0;
+
+    added = sk_X509_OBJECT_push(objs, obj) != 0;
+    if (added == 0) {
+        sk_X509_OBJECT_free(objs);
+        return 0;
+    }
+
+    HT_INIT_KEY(&key);
+    HT_SET_KEY_FIELD(&key, xn_canon, xn->canon_enc);
+    HT_SET_KEY_FIELD(&key, xn_canon_enclen, xn->canon_enclen);
+    val.value = (void *)objs;
+    ret = ossl_ht_insert(store->objs_ht, TO_HT_KEY(&key), &val, NULL);
+    if (ret != 1) {
+        sk_X509_OBJECT_free(objs);
+        return 0;
+    }
+
+    return 1;
+}
+
+static int obj_ht_foreach_certs(HT_VALUE *v, void *arg)
+{
+    STACK_OF(X509) **sk = arg;
+    STACK_OF(X509_OBJECT) *objs = v->value;
+    int i, r;
+
+    for (i = 0; i < sk_X509_OBJECT_num(objs); i++) {
+        X509 *cert = X509_OBJECT_get0_X509(sk_X509_OBJECT_value(objs, i));
+
+        if (cert == NULL)
+            continue;
+        r = X509_add_cert(*sk, cert, X509_ADD_FLAG_UP_REF);
+        if (r == 0) {
+            OSSL_STACK_OF_X509_free(*sk);
+            *sk = NULL;
+            return 0;
+        }
+    }
+
+    return 1;
+}
+
 /*
  * May be called with |ret| == NULL just for the side effect of
  * caching all certs matching the given subject DN in |ctx->store->objs|.
@@ -322,7 +439,8 @@ int ossl_x509_store_ctx_get_by_subject(const X509_STORE_CTX *ctx, X509_LOOKUP_TY
 {
     X509_STORE *store = ctx->store;
     X509_LOOKUP *lu;
-    X509_OBJECT stmp, *tmp;
+    X509_OBJECT stmp, *tmp = NULL;
+    STACK_OF(X509_OBJECT) *objs;
     int i, j;
 
     if (store == NULL)
@@ -333,19 +451,13 @@ int ossl_x509_store_ctx_get_by_subject(const X509_STORE_CTX *ctx, X509_LOOKUP_TY
 
     if (!ossl_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 */
-        if (!X509_STORE_lock(store))
-            return 0;
-        /*
-         * Another thread might have sorted it in the meantime. But if so,
-         * sk_X509_OBJECT_sort() exits early.
-         */
-        sk_X509_OBJECT_sort(store->objs);
+    if (store->objs_ht != NULL) {
+        objs = ossl_x509_store_ht_get_by_name(store, name);
+        if (objs != NULL)
+            tmp = X509_OBJECT_retrieve_by_subject(objs, type, name);
+    } else {
+        tmp = X509_OBJECT_retrieve_by_subject(store->objs, type, name);
     }
-    tmp = X509_OBJECT_retrieve_by_subject(store->objs, type, name);
     X509_STORE_unlock(store);
 
     if (tmp == NULL || type == X509_LU_CRL) {
@@ -387,6 +499,8 @@ static int x509_store_add(X509_STORE *store, void *x, int crl)
 {
     X509_OBJECT *obj;
     int ret = 0, added = 0;
+    X509_NAME *xn;
+    STACK_OF(X509_OBJECT) *objs = NULL;
 
     if (x == NULL)
         return 0;
@@ -397,10 +511,19 @@ static int x509_store_add(X509_STORE *store, void *x, int crl)
     if (crl) {
         obj->type = X509_LU_CRL;
         obj->data.crl = (X509_CRL *)x;
+        xn = obj->data.crl->crl.issuer;
     } else {
         obj->type = X509_LU_X509;
         obj->data.x509 = (X509 *)x;
+        xn = obj->data.x509->cert_info.subject;
+    }
+
+    if (xn == NULL) {
+        obj->type = X509_LU_NONE;
+        X509_OBJECT_free(obj);
+        return 0;
     }
+
     if (!X509_OBJECT_up_ref_count(obj)) {
         obj->type = X509_LU_NONE;
         X509_OBJECT_free(obj);
@@ -412,11 +535,21 @@ static int x509_store_add(X509_STORE *store, void *x, int crl)
         return 0;
     }
 
-    if (X509_OBJECT_retrieve_match(store->objs, obj)) {
-        ret = 1;
+    if (store->objs_ht != NULL) {
+        objs = ossl_x509_store_ht_get_by_name(store, xn);
+        if (objs != NULL && X509_OBJECT_retrieve_match(objs, obj)) {
+            ret = 1;
+        } else {
+            added = x509_name_objs_ht_insert(store, xn, objs, obj);
+            ret = added != 0;
+        }
     } else {
-        added = sk_X509_OBJECT_push(store->objs, obj);
-        ret = added != 0;
+        if (X509_OBJECT_retrieve_match(store->objs, obj)) {
+            ret = 1;
+        } else {
+            added = sk_X509_OBJECT_push(store->objs, obj);
+            ret = added != 0;
+        }
     }
     X509_STORE_unlock(store);
 
@@ -577,11 +710,6 @@ X509_OBJECT *X509_OBJECT_retrieve_by_subject(STACK_OF(X509_OBJECT) *h,
     return sk_X509_OBJECT_value(h, idx);
 }
 
-STACK_OF(X509_OBJECT) *X509_STORE_get0_objects(const X509_STORE *xs)
-{
-    return xs->objs;
-}
-
 static X509_OBJECT *x509_object_dup(const X509_OBJECT *obj)
 {
     X509_OBJECT *ret = X509_OBJECT_new();
@@ -594,6 +722,44 @@ static X509_OBJECT *x509_object_dup(const X509_OBJECT *obj)
     return ret;
 }
 
+static int obj_ht_foreach_object(HT_VALUE *v, void *arg)
+{
+    STACK_OF(X509_OBJECT) **sk = arg;
+    STACK_OF(X509_OBJECT) *objs = v->value;
+    X509_OBJECT *dup, *obj;
+    int i;
+
+    for (i = 0; i < sk_X509_OBJECT_num(objs); i++) {
+        obj = sk_X509_OBJECT_value(objs, i);
+        dup = x509_object_dup(obj);
+        if (dup == NULL)
+            goto err;
+        if (sk_X509_OBJECT_push(*sk, dup) == 0)
+            goto err;
+    }
+
+    return 1;
+
+ err:
+    sk_X509_OBJECT_pop_free(*sk, X509_OBJECT_free);
+    *sk = NULL;
+
+    return 0;
+}
+
+STACK_OF(X509_OBJECT) *X509_STORE_get0_objects(const X509_STORE *xs)
+{
+    X509_STORE *store = (X509_STORE *)xs;
+
+    if (xs->objs_ht != NULL) {
+        ossl_ht_foreach_until(xs->objs_ht, obj_ht_foreach_object, &store->objs);
+        ossl_ht_free(xs->objs_ht);
+        store->objs_ht = NULL;
+    }
+
+    return xs->objs;
+}
+
 STACK_OF(X509_OBJECT) *X509_STORE_get1_objects(X509_STORE *store)
 {
     STACK_OF(X509_OBJECT) *objs;
@@ -606,17 +772,24 @@ STACK_OF(X509_OBJECT) *X509_STORE_get1_objects(X509_STORE *store)
     if (!ossl_x509_store_read_lock(store))
         return NULL;
 
-    objs = sk_X509_OBJECT_deep_copy(store->objs, x509_object_dup,
-                                    X509_OBJECT_free);
+    if (store->objs_ht != NULL) {
+        if ((objs = sk_X509_OBJECT_new(x509_object_cmp)) == NULL) {
+            X509_STORE_unlock(store);
+            return NULL;
+        }
+        ossl_ht_foreach_until(store->objs_ht, obj_ht_foreach_object, &objs);
+    } else {
+        objs = sk_X509_OBJECT_deep_copy(store->objs, x509_object_dup,
+                                        X509_OBJECT_free);
+    }
     X509_STORE_unlock(store);
+
     return objs;
 }
 
 STACK_OF(X509) *X509_STORE_get1_all_certs(X509_STORE *store)
 {
-    STACK_OF(X509) *sk;
-    STACK_OF(X509_OBJECT) *objs;
-    int i;
+    STACK_OF(X509) *sk = NULL;
 
     if (store == NULL) {
         ERR_raise(ERR_LIB_X509, ERR_R_PASSED_NULL_PARAMETER);
@@ -624,19 +797,23 @@ STACK_OF(X509) *X509_STORE_get1_all_certs(X509_STORE *store)
     }
     if ((sk = sk_X509_new_null()) == NULL)
         return NULL;
-    if (!X509_STORE_lock(store))
+
+    if (!ossl_x509_store_read_lock(store))
         goto out_free;
 
-    sk_X509_OBJECT_sort(store->objs);
-    objs = X509_STORE_get0_objects(store);
-    for (i = 0; i < sk_X509_OBJECT_num(objs); i++) {
-        X509 *cert = X509_OBJECT_get0_X509(sk_X509_OBJECT_value(objs, i));
+    if (store->objs_ht != NULL) {
+        ossl_ht_foreach_until(store->objs_ht, obj_ht_foreach_certs, &sk);
+    } else {
+        for (int i = 0; i < sk_X509_OBJECT_num(store->objs); i++) {
+            X509 *cert = X509_OBJECT_get0_X509(sk_X509_OBJECT_value(store->objs, i));
 
-        if (cert != NULL
-            && !X509_add_cert(sk, cert, X509_ADD_FLAG_UP_REF))
-            goto err;
+            if (cert != NULL
+                && !X509_add_cert(sk, cert, X509_ADD_FLAG_UP_REF))
+                goto err;
+        }
     }
     X509_STORE_unlock(store);
+
     return sk;
 
  err:
@@ -653,21 +830,28 @@ STACK_OF(X509) *X509_STORE_get1_all_certs(X509_STORE *store)
 STACK_OF(X509) *X509_STORE_CTX_get1_certs(X509_STORE_CTX *ctx,
                                           const X509_NAME *nm)
 {
-    int i, idx, cnt;
+    int i, idx = -1, cnt = 0;
     STACK_OF(X509) *sk = NULL;
     X509 *x;
     X509_OBJECT *obj;
     X509_STORE *store = ctx->store;
+    STACK_OF(X509_OBJECT) *objs;
 
     if (store == NULL)
         return sk_X509_new_null();
 
-    if (!X509_STORE_lock(store))
+    if (!ossl_x509_store_read_lock(store))
         return NULL;
 
-    sk_X509_OBJECT_sort(store->objs);
-    idx = x509_object_idx_cnt(store->objs, X509_LU_X509, nm, &cnt);
-    if (idx < 0) {
+    if (store->objs_ht != NULL) {
+        objs = ossl_x509_store_ht_get_by_name(store, nm);
+        if (objs != NULL)
+            idx = x509_object_idx_cnt(objs, X509_LU_X509, nm, &cnt);
+    } else {
+        objs = store->objs;
+        idx = x509_object_idx_cnt(objs, X509_LU_X509, nm, &cnt);
+    }
+    if (idx < 0 || objs == NULL) {
         /*
          * Nothing found in cache: do lookup to possibly add new objects to
          * cache
@@ -676,24 +860,31 @@ STACK_OF(X509) *X509_STORE_CTX_get1_certs(X509_STORE_CTX *ctx,
         i = ossl_x509_store_ctx_get_by_subject(ctx, X509_LU_X509, nm, NULL);
         if (i <= 0)
             return i < 0 ? NULL : sk_X509_new_null();
-        if (!X509_STORE_lock(store))
+        if (!ossl_x509_store_read_lock(store))
             return NULL;
-        sk_X509_OBJECT_sort(store->objs);
-        idx = x509_object_idx_cnt(store->objs, X509_LU_X509, nm, &cnt);
+        if (store->objs_ht != NULL) {
+            objs = ossl_x509_store_ht_get_by_name(store, nm);
+            if (objs == NULL)
+                goto end;
+        }
+        idx = x509_object_idx_cnt(objs, X509_LU_X509, nm, &cnt);
     }
 
     sk = sk_X509_new_null();
     if (idx < 0 || sk == NULL)
         goto end;
-    for (i = 0; i < cnt; i++, idx++) {
-        obj = sk_X509_OBJECT_value(store->objs, idx);
+    for (i = idx; i < sk_X509_OBJECT_num(objs); i++) {
+        obj = sk_X509_OBJECT_value(objs, i);
         x = obj->data.x509;
-        if (!X509_add_cert(sk, x, X509_ADD_FLAG_UP_REF)) {
+        if (obj->type != X509_LU_X509)
+            continue;
+        if (X509_add_cert(sk, x, X509_ADD_FLAG_UP_REF) == 0) {
             X509_STORE_unlock(store);
             OSSL_STACK_OF_X509_free(sk);
             return NULL;
         }
     }
+
  end:
     X509_STORE_unlock(store);
     return sk;
@@ -703,11 +894,12 @@ STACK_OF(X509) *X509_STORE_CTX_get1_certs(X509_STORE_CTX *ctx,
 STACK_OF(X509_CRL) *X509_STORE_CTX_get1_crls(const X509_STORE_CTX *ctx,
                                              const X509_NAME *nm)
 {
-    int i = 1, idx, cnt;
+    int i = 1, idx, cnt = 0;
     STACK_OF(X509_CRL) *sk;
     X509_CRL *x;
     X509_OBJECT *obj;
     X509_STORE *store = ctx->store;
+    STACK_OF(X509_OBJECT) *objs;
 
     /* Always do lookup to possibly add new CRLs to cache */
     i = ossl_x509_store_ctx_get_by_subject(ctx, X509_LU_CRL, nm, NULL);
@@ -716,20 +908,26 @@ STACK_OF(X509_CRL) *X509_STORE_CTX_get1_crls(const X509_STORE_CTX *ctx,
     sk = sk_X509_CRL_new_null();
     if (i == 0)
         return sk;
-    if (!X509_STORE_lock(store)) {
+    if (!ossl_x509_store_read_lock(store)) {
         sk_X509_CRL_free(sk);
         return NULL;
     }
-    sk_X509_OBJECT_sort(store->objs);
-    idx = x509_object_idx_cnt(store->objs, X509_LU_CRL, nm, &cnt);
-    if (idx < 0) {
-        X509_STORE_unlock(store);
-        return sk;
+    if (store->objs_ht) {
+        objs = ossl_x509_store_ht_get_by_name(store, nm);
+        if (objs == NULL)
+            goto end;
+    } else {
+        objs = store->objs;
     }
+    idx = x509_object_idx_cnt(objs, X509_LU_CRL, nm, &cnt);
+    if (idx < 0)
+        goto end;
 
-    for (i = 0; i < cnt; i++, idx++) {
-        obj = sk_X509_OBJECT_value(store->objs, idx);
+    for (i = idx; i < sk_X509_OBJECT_num(objs); i++) {
+        obj = sk_X509_OBJECT_value(objs, i);
         x = obj->data.crl;
+        if (obj->type != X509_LU_CRL)
+            continue;
         if (!X509_CRL_up_ref(x)) {
             X509_STORE_unlock(store);
             sk_X509_CRL_pop_free(sk, X509_CRL_free);
@@ -742,6 +940,7 @@ STACK_OF(X509_CRL) *X509_STORE_CTX_get1_crls(const X509_STORE_CTX *ctx,
             return NULL;
         }
     }
+ end:
     X509_STORE_unlock(store);
     return sk;
 }
index d54a5112072b993f1fea137cc57295acd46d634d..a4f7cb9ac58ad3664de6b169906ced56334a2d4e 100644 (file)
@@ -477,7 +477,7 @@ static int execute_X509_STORE_test(CMP_PROTECT_TEST_FIXTURE *fixture)
                                                   fixture->callback_arg)))
         goto err;
     sk = X509_STORE_get1_all_certs(store);
-    if (!TEST_int_eq(0, STACK_OF_X509_cmp(sk, fixture->chain)))
+    if (!TEST_int_eq(0, STACK_OF_X509_cmp_deep(sk, fixture->chain)))
         goto err;
     res = 1;
  err:
index e0fb1d3d345dc1f54e68a5d16fd1acc565f17428..bbe928f6440308666b8c977787acf5d7664aaa08 100644 (file)
@@ -55,6 +55,36 @@ int STACK_OF_X509_cmp(const STACK_OF(X509) *sk1, const STACK_OF(X509) *sk2)
     return 0;
 }
 
+/*
+ * Compares two stacks of certificates in any order of their elements.
+ * Returns 0 if sk1 and sk2 are equal and another value otherwise
+ */
+int STACK_OF_X509_cmp_deep(const STACK_OF(X509) *sk1, const STACK_OF(X509) *sk2)
+{
+    int i, res, idx;
+    X509 *a, *b;
+
+    if (sk1 == sk2)
+        return 0;
+    if (sk1 == NULL)
+        return -1;
+    if (sk2 == NULL)
+        return 1;
+    if ((res = sk_X509_num(sk1) - sk_X509_num(sk2)))
+        return res;
+    for (i = 0; i < sk_X509_num(sk1); i++) {
+        a = sk_X509_value(sk1, i);
+        idx = sk_X509_find((STACK_OF(X509) *)sk2, a);
+        if (idx < 0)
+            return 0;
+        b = sk_X509_value(sk2, idx);
+        if (a != b)
+            if ((res = X509_cmp(a, b)) != 0)
+                return res;
+    }
+    return 0;
+}
+
 /*
  * Up refs and push a cert onto sk.
  * Returns the number of certificates on the stack on success
index 50b085beca01fe7244c70ad9d98dbcfc67ec5eca..82cfefbe05d74b6fe351f48283642a43db7386e0 100644 (file)
@@ -24,6 +24,7 @@
 OSSL_CMP_MSG *load_pkimsg(const char *file, OSSL_LIB_CTX *libctx);
 int valid_asn1_encoding(const OSSL_CMP_MSG *msg);
 int STACK_OF_X509_cmp(const STACK_OF(X509) *sk1, const STACK_OF(X509) *sk2);
+int STACK_OF_X509_cmp_deep(const STACK_OF(X509) *sk1, const STACK_OF(X509) *sk2);
 int STACK_OF_X509_push1(STACK_OF(X509) *sk, X509 *cert);
 int print_to_bio_out(const char *func, const char *file, int line,
                      OSSL_CMP_severity level, const char *msg);
index 16caf48fec710ca28acdfe7aee8aee8b5b2e2f26..2e6920b26b6187d407192f5ae0500cf3d4f4352b 100644 (file)
@@ -5,6 +5,8 @@
  * https://www.openssl.org/source/license.html
  */
 
+#define OPENSSL_SUPPRESS_DEPRECATED
+
 #include <stdio.h>
 #include <openssl/err.h>
 #include <openssl/x509_vfy.h>
 static const char *chain;
 static const char *crl;
 
+static const char *cn_cert1[] = {
+    "-----BEGIN CERTIFICATE-----\n",
+    "MIIC9DCCAdygAwIBAgICEAIwDQYJKoZIhvcNAQELBQAwGzEZMBcGA1UEAwwQd3d3\n",
+    "LmV4YW1wbGUudGVzdDAeFw0yNTAxMDEwMDAwMDBaFw0yNTAxMTAwMDAwMDBaMBsx\n",
+    "GTAXBgNVBAMMEHd3dy5leGFtcGxlLnRlc3QwggEiMA0GCSqGSIb3DQEBAQUAA4IB\n",
+    "DwAwggEKAoIBAQDPwPv+vSWbbNI+EUMH162Srn88C2ywadFWgBdnVikBJ10LcwQK\n",
+    "6zrTC+TzVafEhWUJDujnhJlTCc0ASIIJti2adwJODgbf2lk4n4mXcT4BFYvJbWaW\n",
+    "BjSRPnfwKyQyHoXDyCtLZGtwB295mNcF1sGOS7SmpM4n4hbzTx/o3mxCEd/CGEzh\n",
+    "hiljQIr6VtwVxcGoaI5AIW/gbKZB4tYrCeiqVDITZFx5kp0BQ4aPIr+1ws4Y9qta\n",
+    "1ljZgf/E3uaQK2CFAWYv6Zz2QYfjkoTZ2wq3PBj1DVWT0nJs/rezKA6R1iyePiFa\n",
+    "nqlNyjNKFDi/oVVmgsDgllVXoWaDoTncXQGzAgMBAAGjQjBAMB0GA1UdDgQWBBTl\n",
+    "aFbHpg5r2p4B6MyRy3MG9cwBijAfBgNVHSMEGDAWgBQIA+YPQEiawtPX3vt3hpGo\n",
+    "Wo+/3zANBgkqhkiG9w0BAQsFAAOCAQEAmx1N72vdZLCaMXkZapz5j9NND0cvDjXu\n",
+    "YvUfGU+Y9dTTX4+RmrWuLWgLlk7Unq5DPAQahE4Kc7+JsyMtSXQnOyTZAEmy+1qN\n",
+    "DG42aPiRtfm2Og0mmTrIml3n/tZzmYPU+hC4f9M6qtpI9S0IaqAXFZdup45o6uZj\n",
+    "Wt7oiIfnbj3UIf7Bc3k/0jUvCZavppH9yKdd70cpkxRXBfqSpift7YqH6RCpKp/P\n",
+    "jiDYR27N7qUmfe60AHCBhPA1A1fTUXmhRfl8mRm2wiUelKUZ77wRkk5VpbsAA/JW\n",
+    "07vUu3WC2773pUT/68MED5h26Mr6xjKxG8v8SfeMpri5R74P1TnJpQ==\n",
+    "-----END CERTIFICATE-----\n",
+    NULL,
+};
+static const char *cn_cert2[] = {
+    "-----BEGIN CERTIFICATE-----\n",
+    "MIIC9DCCAdygAwIBAgICEAMwDQYJKoZIhvcNAQELBQAwGzEZMBcGA1UEAwwQd3d3\n",
+    "LmV4YW1wbGUudGVzdDAeFw0yNTAxMTEwMDAwMDBaFw0yNTAxMjAwMDAwMDBaMBsx\n",
+    "GTAXBgNVBAMMEHd3dy5leGFtcGxlLnRlc3QwggEiMA0GCSqGSIb3DQEBAQUAA4IB\n",
+    "DwAwggEKAoIBAQDPwPv+vSWbbNI+EUMH162Srn88C2ywadFWgBdnVikBJ10LcwQK\n",
+    "6zrTC+TzVafEhWUJDujnhJlTCc0ASIIJti2adwJODgbf2lk4n4mXcT4BFYvJbWaW\n",
+    "BjSRPnfwKyQyHoXDyCtLZGtwB295mNcF1sGOS7SmpM4n4hbzTx/o3mxCEd/CGEzh\n",
+    "hiljQIr6VtwVxcGoaI5AIW/gbKZB4tYrCeiqVDITZFx5kp0BQ4aPIr+1ws4Y9qta\n",
+    "1ljZgf/E3uaQK2CFAWYv6Zz2QYfjkoTZ2wq3PBj1DVWT0nJs/rezKA6R1iyePiFa\n",
+    "nqlNyjNKFDi/oVVmgsDgllVXoWaDoTncXQGzAgMBAAGjQjBAMB0GA1UdDgQWBBTl\n",
+    "aFbHpg5r2p4B6MyRy3MG9cwBijAfBgNVHSMEGDAWgBQIA+YPQEiawtPX3vt3hpGo\n",
+    "Wo+/3zANBgkqhkiG9w0BAQsFAAOCAQEAH+hviFeMZc55/H5AZG4p3VdOQ5ZxsHJF\n",
+    "NTkxY/0wGcKJhelyNN39rnlhUFLKlEhbWE/LlTBCU9ILLiFeSAaFiNgufqFEBefL\n",
+    "cjqcfQUeB2vUHNnrWy6QPCc0SNSdholTelEgf8eXPBrJWS/iazcKTLfBrFbs6K++\n",
+    "F/Efn1p7+DutFpgaTdupuonsJOMM33Nw3MfAz6bw5Hjd17ritjRW0g58OsLfv9Ac\n",
+    "Dbw2bEzZDvkkH5yDlHOl/dyyhDdcMs6bRsoM+LC0PuZ3dAUQhlnQ8SlPojm5qUjA\n",
+    "0hyy4w4tmYFCzdUxR6vnQie4a/D0o5cLuecIIemozKONMZo+FVGjFQ==\n",
+    "-----END CERTIFICATE-----\n",
+    NULL,
+};
+static const char *cn_crl2[] = {
+    "-----BEGIN X509 CRL-----\n",
+    "MIIBtjCBnwIBATANBgkqhkiG9w0BAQsFADAbMRkwFwYDVQQDDBB3d3cuZXhhbXBs\n",
+    "ZS50ZXN0Fw0yNTA5MTExMzA0MTZaFw0yNTEyMjAxMzA0MTZaMD8wEwICEAAXDTI1\n",
+    "MDkxMTExNTcxMFowEwICEAEXDTI1MDkxMTExNTgwNVowEwICEAIXDTI1MDkxMTEz\n",
+    "MDM1MVqgDzANMAsGA1UdFAQEAgIQATANBgkqhkiG9w0BAQsFAAOCAQEAZUUsIeXL\n",
+    "zTwpa2bwAg0xUIm/ARnoHlZSxMoWPsQnIkrQreCTvc9QpTo+IrS+SDaBJqu8V+DM\n",
+    "Z4rNqreQG1wNNgkSTEOAFMjQPOmvtePyYGxNgM4MdpKaejdZSk22rccaAQVhSjGp\n",
+    "AYprwbWqyfAq7JUYLUZ1EjsIoRQIKMQa79bzF7S4oNN6sMq6icJueFmd55ufmv27\n",
+    "1amhUpJh5YJ8BNpLZGicujYm8TVCZqSjVI0JsXAQYhm1u/pUrcydRfc7c5PMkpIj\n",
+    "uByPkdYLVgpKJH6Y5Q9Kx5MR+p85JM31ePlSobft+wZh/WMV0Id6PZ8/56oOhV3V\n",
+    "bVxU+BlQ5ZKmYw==\n"
+    "-----END X509 CRL-----\n",
+    NULL,
+};
+
 static int test_load_cert_file(void)
 {
     int ret = 0, i;
@@ -27,6 +87,8 @@ static int test_load_cert_file(void)
         || !TEST_true(X509_load_cert_file(lookup, chain, X509_FILETYPE_PEM))
         || !TEST_ptr(certs = X509_STORE_get1_all_certs(store))
         || !TEST_int_eq(sk_X509_num(certs), 4)
+        || !TEST_ptr(objs = X509_STORE_get0_objects(store))
+        || !TEST_int_eq(sk_X509_OBJECT_num(objs), 4)
         || !TEST_ptr(objs = X509_STORE_get1_objects(store))
         || !TEST_int_eq(sk_X509_OBJECT_num(objs), 4))
         goto err;
@@ -49,6 +111,61 @@ err:
     return ret;
 }
 
+/*
+ * Test loading multiple certificates and a CRL with identical CN into
+ * X509_STORE.
+ */
+static int test_load_same_cn_certs(void)
+{
+    X509 *cert1 = NULL, *cert2 = NULL;
+    X509_CRL *crl2 = NULL;
+    int ret = 0;
+    X509_STORE *store = NULL;
+    X509_STORE_CTX *s_ctx = NULL;
+    STACK_OF(X509_OBJECT) *objs = NULL;
+    STACK_OF(X509_CRL) *sk_x509_crl = NULL;
+    STACK_OF(X509) *sk_x509 = NULL;
+    X509_NAME *nm = NULL;
+
+    if (!TEST_ptr(nm = X509_NAME_new())
+        || !TEST_true(X509_NAME_add_entry_by_txt(nm, "CN", MBSTRING_ASC,
+                                                 (unsigned char *)"www.example.test",
+                                                 -1, -1, 0)))
+        goto err;
+
+    if (!TEST_ptr(cert1 = X509_from_strings(cn_cert1))
+        || !TEST_ptr(cert2 = X509_from_strings(cn_cert2))
+        || !TEST_ptr(crl2 = CRL_from_strings(cn_crl2))
+        || !TEST_ptr(s_ctx = X509_STORE_CTX_new())
+        || !TEST_ptr(store = X509_STORE_new())
+        || !TEST_true(X509_STORE_CTX_init(s_ctx, store, NULL, NULL))
+        || !TEST_true(X509_STORE_add_cert(store, cert1))
+        || !TEST_true(X509_STORE_add_crl(store, crl2))
+        || !TEST_true(X509_STORE_add_cert(store, cert2))
+        /* deliberately not taking lock in a single thread */
+        || !TEST_ptr(objs = X509_STORE_get0_objects(store))
+        || !TEST_int_eq(sk_X509_OBJECT_num(objs), 3)
+        || !TEST_ptr(sk_x509 = X509_STORE_CTX_get1_certs(s_ctx, nm))
+        || !TEST_int_eq(sk_X509_num(sk_x509), 2)
+        || !TEST_ptr(sk_x509_crl = X509_STORE_CTX_get1_crls(s_ctx, nm))
+        || !TEST_int_eq(sk_X509_CRL_num(sk_x509_crl), 1))
+        goto err;
+
+    ret = 1;
+
+err:
+    X509_NAME_free(nm);
+    OSSL_STACK_OF_X509_free(sk_x509);
+    sk_X509_CRL_pop_free(sk_x509_crl, X509_CRL_free);
+    X509_STORE_free(store);
+    X509_STORE_CTX_free(s_ctx);
+    X509_free(cert1);
+    X509_free(cert2);
+    X509_CRL_free(crl2);
+
+    return ret;
+}
+
 OPT_TEST_DECLARE_USAGE("cert.pem [crl.pem]\n")
 
 int setup_tests(void)
@@ -65,5 +182,7 @@ int setup_tests(void)
     crl = test_get_argument(1);
 
     ADD_TEST(test_load_cert_file);
+    ADD_TEST(test_load_same_cn_certs);
+
     return 1;
 }