reduce lock contention when adding objects to ADDED_OBJ hash table

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: Matt Caswell <matt@openssl.org>
Reviewed-by: Paul Dale <ppzgs1@gmail.com>
(Merged from https://github.com/openssl/openssl/pull/28081)

diff --git a/crypto/objects/obj_dat.c b/crypto/objects/obj_dat.c
index 60b4611f35fb1a7a99f536e6d79dfc6f6191937d..e331b73b500122dc3bbd39250e2cf9f36c8c16df 100644 (file)
--- a/crypto/objects/obj_dat.c
+++ b/crypto/objects/obj_dat.c
@@ -46,7 +46,7 @@ static CRYPTO_RWLOCK *ossl_obj_lock = NULL;
 static CRYPTO_RWLOCK *ossl_obj_nid_lock = NULL;
 #endif
 
-static CRYPTO_ONCE ossl_obj_lock_init = CRYPTO_ONCE_STATIC_INIT;
+static CRYPTO_ONCE ossl_obj_api_init = CRYPTO_ONCE_STATIC_INIT;
 
 static ossl_inline void objs_free_locks(void)
 {
@@ -58,7 +58,7 @@ static ossl_inline void objs_free_locks(void)
 #endif
 }
 
-DEFINE_RUN_ONCE_STATIC(obj_lock_initialise)
+DEFINE_RUN_ONCE_STATIC(obj_api_initialise)
 {
     ossl_obj_lock = CRYPTO_THREAD_lock_new();
     if (ossl_obj_lock == NULL)
@@ -72,43 +72,40 @@ DEFINE_RUN_ONCE_STATIC(obj_lock_initialise)
     }
 #endif
     added = lh_ADDED_OBJ_new(added_obj_hash, added_obj_cmp);
-    if (added == NULL)
+    if (added == NULL) {
+        objs_free_locks();
         return 0;
+    }
 
     return 1;
 }
 
-static ossl_inline int ossl_init_added_lock(void)
+static ossl_inline int ossl_init_added_api(void)
 {
 #ifndef OPENSSL_NO_AUTOLOAD_CONFIG
     /* Make sure we've loaded config before checking for any "added" objects */
     OPENSSL_init_crypto(OPENSSL_INIT_LOAD_CONFIG, NULL);
 #endif
-    return RUN_ONCE(&ossl_obj_lock_init, obj_lock_initialise);
+    return RUN_ONCE(&ossl_obj_api_init, obj_api_initialise);
 }
 
-static ossl_inline int ossl_obj_write_lock(int lock)
+static ossl_inline int ossl_obj_write_lock(void)
 {
-    if (!ossl_init_added_lock())
+    if (!ossl_init_added_api())
         return 0;
-    if (!lock)
-        return 1;
     return CRYPTO_THREAD_write_lock(ossl_obj_lock);
 }
 
-static ossl_inline int ossl_obj_read_lock(int lock)
+static ossl_inline int ossl_obj_read_lock(void)
 {
-    if (!ossl_init_added_lock())
+    if (!ossl_init_added_api())
         return 0;
-    if (!lock)
-        return 1;
     return CRYPTO_THREAD_read_lock(ossl_obj_lock);
 }
 
-static ossl_inline void ossl_obj_unlock(int lock)
+static ossl_inline void ossl_obj_unlock(void)
 {
-    if (lock)
-        CRYPTO_THREAD_unlock(ossl_obj_lock);
+    CRYPTO_THREAD_unlock(ossl_obj_lock);
 }
 
 static int sn_cmp(const ASN1_OBJECT *const *a, const unsigned int *b)
@@ -228,105 +225,22 @@ void ossl_obj_cleanup_int(void)
     objs_free_locks();
 }
 
-/*
- * Requires that the ossl_obj_lock be held
- * if TSAN_REQUIRES_LOCKING defined
- */
-static int obj_new_nid_unlocked(int num)
+int OBJ_new_nid(int num)
 {
     static TSAN_QUALIFIER int new_nid = NUM_NID;
 #ifdef TSAN_REQUIRES_LOCKING
     int i;
 
+    ossl_obj_write_lock();
     i = new_nid;
     new_nid += num;
-
+    ossl_obj_unlock();
     return i;
 #else
     return tsan_add(&new_nid, num);
 #endif
 }
 
-int OBJ_new_nid(int num)
-{
-#ifdef TSAN_REQUIRES_LOCKING
-    int i;
-
-    if (!ossl_obj_write_lock(1)) {
-        ERR_raise(ERR_LIB_OBJ, ERR_R_UNABLE_TO_GET_WRITE_LOCK);
-        return NID_undef;
-    }
-
-    i = obj_new_nid_unlocked(num);
-
-    ossl_obj_unlock(1);
-
-    return i;
-#else
-    return obj_new_nid_unlocked(num);
-#endif
-}
-
-static int ossl_obj_add_object(const ASN1_OBJECT *obj, int lock)
-{
-    ASN1_OBJECT *o = NULL;
-    ADDED_OBJ *ao[4] = { NULL, NULL, NULL, NULL }, *aop[4];
-    int i;
-
-    if ((o = OBJ_dup(obj)) == NULL)
-        return NID_undef;
-    if ((ao[ADDED_NID] = OPENSSL_malloc(sizeof(*ao[0]))) == NULL
-            || (o->length != 0
-                && obj->data != NULL
-                && (ao[ADDED_DATA] = OPENSSL_malloc(sizeof(*ao[0]))) == NULL)
-            || (o->sn != NULL
-                && (ao[ADDED_SNAME] = OPENSSL_malloc(sizeof(*ao[0]))) == NULL)
-            || (o->ln != NULL
-                && (ao[ADDED_LNAME] = OPENSSL_malloc(sizeof(*ao[0]))) == NULL))
-        goto err2;
-
-    if (!ossl_obj_write_lock(lock)) {
-        ERR_raise(ERR_LIB_OBJ, ERR_R_UNABLE_TO_GET_WRITE_LOCK);
-        goto err2;
-    }
-
-    for (i = ADDED_DATA; i <= ADDED_NID; i++) {
-        if (ao[i] != NULL) {
-            ao[i]->type = i;
-            ao[i]->obj = o;
-            aop[i] = lh_ADDED_OBJ_retrieve(added, ao[i]);
-            if (aop[i] != NULL)
-                aop[i]->type = -1;
-            (void)lh_ADDED_OBJ_insert(added, ao[i]);
-            if (lh_ADDED_OBJ_error(added)) {
-                if (aop[i] != NULL)
-                    aop[i]->type = i;
-                while (i-- > ADDED_DATA) {
-                    lh_ADDED_OBJ_delete(added, ao[i]);
-                    if (aop[i] != NULL)
-                        aop[i]->type = i;
-                }
-                ERR_raise(ERR_LIB_OBJ, ERR_R_CRYPTO_LIB);
-                goto err;
-            }
-        }
-    }
-    o->flags &=
-        ~(ASN1_OBJECT_FLAG_DYNAMIC | ASN1_OBJECT_FLAG_DYNAMIC_STRINGS |
-          ASN1_OBJECT_FLAG_DYNAMIC_DATA);
-
-    ossl_obj_unlock(lock);
-    return o->nid;
-
- err:
-    ossl_obj_unlock(lock);
- err2:
-    for (i = ADDED_DATA; i <= ADDED_NID; i++)
-        OPENSSL_free(ao[i]);
-    ASN1_OBJECT_free(o);
-    return NID_undef;
-}
-
 ASN1_OBJECT *OBJ_nid2obj(int n)
 {
     ADDED_OBJ ad, *adp = NULL;
@@ -339,13 +253,12 @@ ASN1_OBJECT *OBJ_nid2obj(int n)
     ad.type = ADDED_NID;
     ad.obj = &ob;
     ob.nid = n;
-    if (!ossl_obj_read_lock(1)) {
+    if (!ossl_obj_read_lock()) {
         ERR_raise(ERR_LIB_OBJ, ERR_R_UNABLE_TO_GET_READ_LOCK);
         return NULL;
     }
-    if (added != NULL)
-        adp = lh_ADDED_OBJ_retrieve(added, &ad);
-    ossl_obj_unlock(1);
+    adp = lh_ADDED_OBJ_retrieve(added, &ad);
+    ossl_obj_unlock();
     if (adp != NULL)
         return adp->obj;
 
@@ -383,7 +296,7 @@ static int obj_cmp(const ASN1_OBJECT *const *ap, const unsigned int *bp)
 
 IMPLEMENT_OBJ_BSEARCH_CMP_FN(const ASN1_OBJECT *, unsigned int, obj);
 
-static int ossl_obj_obj2nid(const ASN1_OBJECT *a, const int lock)
+static int ossl_obj_obj2nid(const ASN1_OBJECT *a)
 {
     int nid = NID_undef;
     const unsigned int *op;
@@ -399,18 +312,16 @@ static int ossl_obj_obj2nid(const ASN1_OBJECT *a, const int lock)
     op = OBJ_bsearch_obj(&a, obj_objs, NUM_OBJ);
     if (op != NULL)
         return nid_objs[*op].nid;
-    if (!ossl_obj_read_lock(lock)) {
+    if (!ossl_obj_read_lock()) {
         ERR_raise(ERR_LIB_OBJ, ERR_R_UNABLE_TO_GET_READ_LOCK);
         return NID_undef;
     }
-    if (added != NULL) {
-        ad.type = ADDED_DATA;
-        ad.obj = (ASN1_OBJECT *)a; /* casting away const is harmless here */
-        adp = lh_ADDED_OBJ_retrieve(added, &ad);
-        if (adp != NULL)
-            nid = adp->obj->nid;
-    }
-    ossl_obj_unlock(lock);
+    ad.type = ADDED_DATA;
+    ad.obj = (ASN1_OBJECT *)a; /* casting away const is harmless here */
+    adp = lh_ADDED_OBJ_retrieve(added, &ad);
+    if (adp != NULL)
+        nid = adp->obj->nid;
+    ossl_obj_unlock();
     return nid;
 }
 
@@ -645,18 +556,16 @@ int OBJ_ln2nid(const char *s)
     op = OBJ_bsearch_ln(&oo, ln_objs, NUM_LN);
     if (op != NULL)
         return nid_objs[*op].nid;
-    if (!ossl_obj_read_lock(1)) {
+    if (!ossl_obj_read_lock()) {
         ERR_raise(ERR_LIB_OBJ, ERR_R_UNABLE_TO_GET_READ_LOCK);
         return NID_undef;
     }
-    if (added != NULL) {
-        ad.type = ADDED_LNAME;
-        ad.obj = &o;
-        adp = lh_ADDED_OBJ_retrieve(added, &ad);
-        if (adp != NULL)
-            nid = adp->obj->nid;
-    }
-    ossl_obj_unlock(1);
+    ad.type = ADDED_LNAME;
+    ad.obj = &o;
+    adp = lh_ADDED_OBJ_retrieve(added, &ad);
+    if (adp != NULL)
+        nid = adp->obj->nid;
+    ossl_obj_unlock();
     return nid;
 }
 
@@ -672,18 +581,16 @@ int OBJ_sn2nid(const char *s)
     op = OBJ_bsearch_sn(&oo, sn_objs, NUM_SN);
     if (op != NULL)
         return nid_objs[*op].nid;
-    if (!ossl_obj_read_lock(1)) {
+    if (!ossl_obj_read_lock()) {
         ERR_raise(ERR_LIB_OBJ, ERR_R_UNABLE_TO_GET_READ_LOCK);
         return NID_undef;
     }
-    if (added != NULL) {
-        ad.type = ADDED_SNAME;
-        ad.obj = &o;
-        adp = lh_ADDED_OBJ_retrieve(added, &ad);
-        if (adp != NULL)
-            nid = adp->obj->nid;
-    }
-    ossl_obj_unlock(1);
+    ad.type = ADDED_SNAME;
+    ad.obj = &o;
+    adp = lh_ADDED_OBJ_retrieve(added, &ad);
+    if (adp != NULL)
+        nid = adp->obj->nid;
+    ossl_obj_unlock();
     return nid;
 }
 
@@ -808,20 +715,14 @@ int OBJ_create(const char *oid, const char *sn, const char *ln)
         }
     }
 
-    if (!ossl_obj_write_lock(1)) {
-        ERR_raise(ERR_LIB_OBJ, ERR_R_UNABLE_TO_GET_WRITE_LOCK);
-        ASN1_OBJECT_free(tmpoid);
-        return 0;
-    }
-
     /* If NID is not NID_undef then object already exists */
     if (oid != NULL
-        && ossl_obj_obj2nid(tmpoid, 0) != NID_undef) {
+        && ossl_obj_obj2nid(tmpoid) != NID_undef) {
         ERR_raise(ERR_LIB_OBJ, OBJ_R_OID_EXISTS);
         goto err;
     }
 
-    tmpoid->nid = obj_new_nid_unlocked(1);
+    tmpoid->nid = OBJ_new_nid(1);
 
     if (tmpoid->nid == NID_undef)
         goto err;
@@ -829,13 +730,14 @@ int OBJ_create(const char *oid, const char *sn, const char *ln)
     tmpoid->sn = (char *)sn;
     tmpoid->ln = (char *)ln;
 
-    ok = ossl_obj_add_object(tmpoid, 0);
+    if (OBJ_add_object(tmpoid) != NID_undef)
+        ok = 1;
+
 
     tmpoid->sn = NULL;
     tmpoid->ln = NULL;
 
  err:
-    ossl_obj_unlock(1);
     ASN1_OBJECT_free(tmpoid);
     return ok;
 }
@@ -856,10 +758,65 @@ const unsigned char *OBJ_get0_data(const ASN1_OBJECT *obj)
 
 int OBJ_add_object(const ASN1_OBJECT *obj)
 {
-    return ossl_obj_add_object(obj, 1);
+    ASN1_OBJECT *o = NULL;
+    ADDED_OBJ *ao[4] = { NULL, NULL, NULL, NULL }, *aop[4];
+    int i;
+
+    if ((o = OBJ_dup(obj)) == NULL)
+        return NID_undef;
+    if ((ao[ADDED_NID] = OPENSSL_malloc(sizeof(*ao[0]))) == NULL
+            || (o->length != 0
+                && obj->data != NULL
+                && (ao[ADDED_DATA] = OPENSSL_malloc(sizeof(*ao[0]))) == NULL)
+            || (o->sn != NULL
+                && (ao[ADDED_SNAME] = OPENSSL_malloc(sizeof(*ao[0]))) == NULL)
+            || (o->ln != NULL
+                && (ao[ADDED_LNAME] = OPENSSL_malloc(sizeof(*ao[0]))) == NULL))
+        goto err2;
+
+    if (!ossl_obj_write_lock()) {
+        ERR_raise(ERR_LIB_OBJ, ERR_R_UNABLE_TO_GET_WRITE_LOCK);
+        goto err2;
+    }
+
+    for (i = ADDED_DATA; i <= ADDED_NID; i++) {
+        if (ao[i] != NULL) {
+            ao[i]->type = i;
+            ao[i]->obj = o;
+            aop[i] = lh_ADDED_OBJ_retrieve(added, ao[i]);
+            if (aop[i] != NULL)
+                aop[i]->type = -1;
+            (void)lh_ADDED_OBJ_insert(added, ao[i]);
+            if (lh_ADDED_OBJ_error(added)) {
+                if (aop[i] != NULL)
+                    aop[i]->type = i;
+                while (i-- > ADDED_DATA) {
+                    lh_ADDED_OBJ_delete(added, ao[i]);
+                    if (aop[i] != NULL)
+                        aop[i]->type = i;
+                }
+                ERR_raise(ERR_LIB_OBJ, ERR_R_CRYPTO_LIB);
+                goto err;
+            }
+        }
+    }
+    o->flags &=
+        ~(ASN1_OBJECT_FLAG_DYNAMIC | ASN1_OBJECT_FLAG_DYNAMIC_STRINGS |
+          ASN1_OBJECT_FLAG_DYNAMIC_DATA);
+
+    ossl_obj_unlock();
+    return o->nid;
+
+ err:
+    ossl_obj_unlock();
+ err2:
+    for (i = ADDED_DATA; i <= ADDED_NID; i++)
+        OPENSSL_free(ao[i]);
+    ASN1_OBJECT_free(o);
+    return NID_undef;
 }
 
 int OBJ_obj2nid(const ASN1_OBJECT *a)
 {
-    return ossl_obj_obj2nid(a, 1);
+    return ossl_obj_obj2nid(a);
 }