From c720fc35f4aa90cdc7cdc424b976c5322fb0098e Mon Sep 17 00:00:00 2001 From: Pauli Date: Thu, 18 Jun 2020 11:01:08 +1000 Subject: [PATCH] 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 (Merged from https://github.com/openssl/openssl/pull/12181) --- crypto/core_namemap.c | 25 +++++++++++++++++++------ test/namemap_internal_test.c | 27 +++++++++++++++++++++++---- 2 files changed, 42 insertions(+), 10 deletions(-) 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 #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); -- 2.25.1