From: Pauli <paul.dale@oracle.com>
Date: Thu, 18 Jun 2020 01:01:08 +0000 (+1000)
Subject: namemap: change ossl_namemap_empty() to do what the documentation says.
X-Git-Tag: openssl-3.0.0-alpha4~63
X-Git-Url: https://git.openssl.org/?p=openssl.git;a=commitdiff_plain;h=c720fc35f4aa90cdc7cdc424b976c5322fb0098e;ds=sidebyside

namemap: change ossl_namemap_empty() to do what the documentation says.

The function is documented as returning 1 when passed a NULL argument.
Instead it core dumps.  Added a unit test for this.

Additionally, a performance improvement is incorporated.  The namemap
max_number field is only ever compared against zero and incremented.
The zero comparison grabs a lock specifically for this check.  This change
uses TSAN operations instead if they are available.

Reviewed-by: Richard Levitte <levitte@openssl.org>
(Merged from https://github.com/openssl/openssl/pull/12181)
---

diff --git a/crypto/core_namemap.c b/crypto/core_namemap.c
index 94c80de091..e17b3ac0e2 100644
--- a/crypto/core_namemap.c
+++ b/crypto/core_namemap.c
@@ -11,6 +11,7 @@
 #include "internal/namemap.h"
 #include <openssl/lhash.h>
 #include "crypto/lhash.h"      /* openssl_lh_strcasehash */
+#include "internal/tsan_assist.h"
 
 /*-
  * The namenum entry
@@ -34,7 +35,12 @@ struct ossl_namemap_st {
 
     CRYPTO_RWLOCK *lock;
     LHASH_OF(NAMENUM_ENTRY) *namenum;  /* Name->number mapping */
-    int max_number;                    /* Current max number */
+
+#ifdef tsan_ld_acq
+    TSAN_QUALIFIER int max_number;     /* Current max number TSAN version */
+#else
+    int max_number;                    /* Current max number plain version */
+#endif
 };
 
 /* LHASH callbacks */
@@ -91,14 +97,21 @@ static const OPENSSL_CTX_METHOD stored_namemap_method = {
 
 int ossl_namemap_empty(OSSL_NAMEMAP *namemap)
 {
-    int rv = 0;
+#ifdef tsan_ld_acq
+    /* Have TSAN support */
+    return namemap == NULL || tsan_load(&namemap->max_number) == 0;
+#else
+    /* No TSAN support */
+    int rv;
+
+    if (namemap == NULL)
+        return 1;
 
     CRYPTO_THREAD_read_lock(namemap->lock);
-    if (namemap->max_number == 0)
-        rv = 1;
+    rv = namemap->max_number == 0;
     CRYPTO_THREAD_unlock(namemap->lock);
-
     return rv;
+#endif
 }
 
 typedef struct doall_names_data_st {
@@ -216,7 +229,7 @@ int ossl_namemap_add_name_n(OSSL_NAMEMAP *namemap, int number,
         goto err;
 
     namenum->number = tmp_number =
-        number != 0 ? number : ++namemap->max_number;
+        number != 0 ? number : 1 + tsan_counter(&namemap->max_number);
     (void)lh_NAMENUM_ENTRY_insert(namemap->namenum, namenum);
 
     if (lh_NAMENUM_ENTRY_error(namemap->namenum))
diff --git a/test/namemap_internal_test.c b/test/namemap_internal_test.c
index 9c94440f91..c1f77561e4 100644
--- a/test/namemap_internal_test.c
+++ b/test/namemap_internal_test.c
@@ -16,6 +16,20 @@
 #define ALIAS1 "alias1"
 #define ALIAS1_UC "ALIAS1"
 
+static int test_namemap_empty(void)
+{
+    OSSL_NAMEMAP *nm = NULL;
+    int ok;
+
+    ok = TEST_true(ossl_namemap_empty(NULL))
+         && TEST_ptr(nm = ossl_namemap_new())
+         && TEST_true(ossl_namemap_empty(nm))
+         && TEST_int_ne(ossl_namemap_add_name(nm, 0, NAME1), 0)
+         && TEST_false(ossl_namemap_empty(nm));
+    ossl_namemap_free(nm);
+    return ok;
+}
+
 static int test_namemap(OSSL_NAMEMAP *nm)
 {
     int num1 = ossl_namemap_add_name(nm, 0, NAME1);
@@ -42,7 +56,7 @@ static int test_namemap(OSSL_NAMEMAP *nm)
 static int test_namemap_independent(void)
 {
     OSSL_NAMEMAP *nm = ossl_namemap_new();
-    int ok = nm != NULL && test_namemap(nm);
+    int ok = TEST_ptr(nm) && test_namemap(nm);
 
     ossl_namemap_free(nm);
     return ok;
@@ -52,7 +66,7 @@ static int test_namemap_stored(void)
 {
     OSSL_NAMEMAP *nm = ossl_namemap_stored(NULL);
 
-    return nm != NULL
+    return TEST_ptr(nm)
         && test_namemap(nm);
 }
 
@@ -66,6 +80,8 @@ static int test_digestbyname(void)
     OSSL_NAMEMAP *nm = ossl_namemap_stored(NULL);
     const EVP_MD *sha256, *foo;
 
+    if (!TEST_ptr(nm))
+        return 0;
     id = ossl_namemap_add_name(nm, 0, "SHA256");
     if (!TEST_int_ne(id, 0))
         return 0;
@@ -92,6 +108,8 @@ static int test_cipherbyname(void)
     OSSL_NAMEMAP *nm = ossl_namemap_stored(NULL);
     const EVP_CIPHER *aes128, *bar;
 
+    if (!TEST_ptr(nm))
+        return 0;
     id = ossl_namemap_add_name(nm, 0, "AES-128-CBC");
     if (!TEST_int_ne(id, 0))
         return 0;
@@ -117,7 +135,7 @@ static int test_cipher_is_a(void)
     EVP_CIPHER *fetched = EVP_CIPHER_fetch(NULL, "AES-256-CCM", NULL);
     int rv = 1;
 
-    if (!TEST_ptr_ne(fetched, NULL))
+    if (!TEST_ptr(fetched))
         return 0;
     if (!TEST_true(EVP_CIPHER_is_a(fetched, "id-aes256-CCM"))
         || !TEST_false(EVP_CIPHER_is_a(fetched, "AES-128-GCM")))
@@ -139,7 +157,7 @@ static int test_digest_is_a(void)
     EVP_MD *fetched = EVP_MD_fetch(NULL, "SHA2-512", NULL);
     int rv = 1;
 
-    if (!TEST_ptr_ne(fetched, NULL))
+    if (!TEST_ptr(fetched))
         return 0;
     if (!TEST_true(EVP_MD_is_a(fetched, "SHA512"))
         || !TEST_false(EVP_MD_is_a(fetched, "SHA1")))
@@ -154,6 +172,7 @@ static int test_digest_is_a(void)
 
 int setup_tests(void)
 {
+    ADD_TEST(test_namemap_empty);
     ADD_TEST(test_namemap_independent);
     ADD_TEST(test_namemap_stored);
     ADD_TEST(test_digestbyname);