Give CRYPTO_EX_DATA_CLASS a constructor This unblocks making Mutex a proper C++ type in a subsequent class. While I'm here, name this C++-style. We should another pass at this later. Ideally CRYPTO_EX_DATA would itself have a destructor. But, for that, we'd actually need it to be ExData<...> instance that instantiates the ExDataClass as part of of the class template. For that to be viable, we need to knock all ExDataClass instances out of the FIPS module. That also should happen, but I'm far too many layers down my yak stack as it is... Change-Id: Ia25d20e2aa44039f1e04497c9c01cf27b4f4f5ef Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/90150 Reviewed-by: Rudolf Polzer <rpolzer@google.com> Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/bio/bio.cc b/crypto/bio/bio.cc index c2a825f..8bc71bb 100644 --- a/crypto/bio/bio.cc +++ b/crypto/bio/bio.cc
@@ -30,8 +30,7 @@ using namespace bssl; -static CRYPTO_EX_DATA_CLASS g_ex_data_class = - CRYPTO_EX_DATA_CLASS_INIT_WITH_APP_DATA; +static ExDataClass g_ex_data_class(/*with_app_data=*/true); Bio::Bio(const BIO_METHOD *m) : RefCounted(CheckSubClass()), method(m) { CRYPTO_new_ex_data(&ex_data);
diff --git a/crypto/dsa/dsa.cc b/crypto/dsa/dsa.cc index 25e1ac6..eeba448 100644 --- a/crypto/dsa/dsa.cc +++ b/crypto/dsa/dsa.cc
@@ -46,7 +46,7 @@ static int dsa_sign_setup(const DSAImpl *dsa, BN_CTX *ctx_in, BIGNUM **out_kinv, BIGNUM **out_r); -static CRYPTO_EX_DATA_CLASS g_ex_data_class = CRYPTO_EX_DATA_CLASS_INIT; +static ExDataClass g_ex_data_class; DSA *DSA_new() { return New<DSAImpl>(); }
diff --git a/crypto/ex_data.cc b/crypto/ex_data.cc index fc6a102..a21c081 100644 --- a/crypto/ex_data.cc +++ b/crypto/ex_data.cc
@@ -29,18 +29,18 @@ BSSL_NAMESPACE_BEGIN -struct crypto_ex_data_func_st { +struct ExDataFuncs { long argl; // Arbitrary long void *argp; // Arbitrary void pointer CRYPTO_EX_free *free_func; - // next points to the next |CRYPTO_EX_DATA_FUNCS| or NULL if this is the last + // next points to the next |ExDataFuncs| or NULL if this is the last // one. It may only be read if synchronized with a read from |num_funcs|. - CRYPTO_EX_DATA_FUNCS *next; + ExDataFuncs *next; }; -int CRYPTO_get_ex_new_index_ex(CRYPTO_EX_DATA_CLASS *ex_data_class, long argl, +int CRYPTO_get_ex_new_index_ex(ExDataClass *ex_data_class, long argl, void *argp, CRYPTO_EX_free *free_func) { - CRYPTO_EX_DATA_FUNCS *funcs = New<CRYPTO_EX_DATA_FUNCS>(); + ExDataFuncs *funcs = New<ExDataFuncs>(); if (funcs == nullptr) { return -1; } @@ -110,8 +110,7 @@ void CRYPTO_new_ex_data(CRYPTO_EX_DATA *ad) { ad->sk = nullptr; } -void CRYPTO_free_ex_data(CRYPTO_EX_DATA_CLASS *ex_data_class, - CRYPTO_EX_DATA *ad) { +void CRYPTO_free_ex_data(ExDataClass *ex_data_class, CRYPTO_EX_DATA *ad) { if (ad->sk == nullptr) { // Nothing to do. return; @@ -123,7 +122,7 @@ // Defer dereferencing |ex_data_class->funcs| and |funcs->next|. It must come // after the |num_funcs| comparison to be correctly synchronized. - CRYPTO_EX_DATA_FUNCS *const *funcs = &ex_data_class->funcs; + ExDataFuncs *const *funcs = &ex_data_class->funcs; for (uint32_t i = 0; i < num_funcs; i++) { if ((*funcs)->free_func != nullptr) { int index = (int)i + ex_data_class->num_reserved;
diff --git a/crypto/fipsmodule/delocate.h b/crypto/fipsmodule/delocate.h index 648dbff..c053ad7 100644 --- a/crypto/fipsmodule/delocate.h +++ b/crypto/fipsmodule/delocate.h
@@ -28,11 +28,11 @@ #if !defined(BORINGSSL_SHARED_LIBRARY) && defined(BORINGSSL_FIPS) && \ !defined(OPENSSL_ASAN) && !defined(OPENSSL_MSAN) -#define DEFINE_BSS_GET(type, name, init_value) \ +#define DEFINE_BSS_GET(type, name, init_expr) \ /* delocate needs C linkage and for |name| to be unique across BCM. */ \ extern "C" { \ extern type BCM_ADD_PREFIX(name); \ - type BCM_ADD_PREFIX(name) = init_value; \ + type BCM_ADD_PREFIX(name) init_expr; \ type *BCM_ADD_PREFIX(name##_bss_get)() __attribute__((const)); \ } /* extern "C" */ \ \ @@ -41,18 +41,18 @@ * a short name, while the symbol itself is prefixed. */ \ static type *name##_bss_get() { return BCM_ADD_PREFIX(name##_bss_get)(); } #else -#define DEFINE_BSS_GET(type, name, init_value) \ - static type name = init_value; \ +#define DEFINE_BSS_GET(type, name, init_expr) \ + static type name init_expr; \ static type *name##_bss_get() { return &name; } #endif // For FIPS builds we require each of these objects be all zero. #define DEFINE_STATIC_ONCE(name) \ - DEFINE_BSS_GET(bssl::CRYPTO_once_t, name, CRYPTO_ONCE_INIT) + DEFINE_BSS_GET(bssl::CRYPTO_once_t, name, = CRYPTO_ONCE_INIT) #define DEFINE_STATIC_MUTEX(name) \ - DEFINE_BSS_GET(bssl::CRYPTO_MUTEX, name, CRYPTO_MUTEX_INIT) + DEFINE_BSS_GET(bssl::CRYPTO_MUTEX, name, = CRYPTO_MUTEX_INIT) #define DEFINE_STATIC_EX_DATA_CLASS(name) \ - DEFINE_BSS_GET(bssl::CRYPTO_EX_DATA_CLASS, name, CRYPTO_EX_DATA_CLASS_INIT) + DEFINE_BSS_GET(bssl::ExDataClass, name, /* default ctor */) #define DEFINE_DATA(type, name, accessor_decorations) \ DEFINE_BSS_GET(type, name##_storage, {}) \
diff --git a/crypto/fipsmodule/rand/rand.cc.inc b/crypto/fipsmodule/rand/rand.cc.inc index af05f29..2d1d5cb 100644 --- a/crypto/fipsmodule/rand/rand.cc.inc +++ b/crypto/fipsmodule/rand/rand.cc.inc
@@ -91,7 +91,7 @@ // objects in the process, one per thread. This is needed because FIPS requires // that they be zeroed on process exit, but thread-local destructors aren't // called when the whole process is exiting. -DEFINE_BSS_GET(struct rand_thread_state *, thread_states_list, nullptr) +DEFINE_BSS_GET(struct rand_thread_state *, thread_states_list, = nullptr) DEFINE_STATIC_MUTEX(thread_states_list_lock) static void rand_thread_state_clear_all() __attribute__((destructor)); @@ -201,7 +201,7 @@ int want_additional_input; }; -DEFINE_BSS_GET(struct entropy_buffer, entropy_buffer, {}) +DEFINE_BSS_GET(struct entropy_buffer, entropy_buffer, = {}) DEFINE_STATIC_MUTEX(entropy_buffer_lock) bcm_infallible bssl::BCM_rand_load_entropy(const uint8_t *entropy,
diff --git a/crypto/internal.h b/crypto/internal.h index 5e5e02e..fe69fc7 100644 --- a/crypto/internal.h +++ b/crypto/internal.h
@@ -731,33 +731,32 @@ BSSL_NAMESPACE_BEGIN -typedef struct crypto_ex_data_func_st CRYPTO_EX_DATA_FUNCS; +struct ExDataFuncs; -// CRYPTO_EX_DATA_CLASS tracks the ex_indices registered for a type which +// ExDataClass tracks the ex_indices registered for a type which // supports ex_data. It should defined as a static global within the module // which defines that type. -typedef struct { - CRYPTO_MUTEX lock; - // funcs is a linked list of |CRYPTO_EX_DATA_FUNCS| structures. It may be - // traversed without serialization only up to |num_funcs|. last points to the - // final entry of |funcs|, or NULL if empty. - CRYPTO_EX_DATA_FUNCS *funcs, *last; +struct ExDataClass { + explicit constexpr ExDataClass(bool with_app_data = false) + : num_reserved(with_app_data ? 1 : 0) {} + + CRYPTO_MUTEX lock = CRYPTO_MUTEX_INIT; + // funcs is a linked list of |ExDataFuncs| structures. It may be traversed + // without serialization only up to |num_funcs|. last points to the final + // entry of |funcs|, or nullptr if empty. + ExDataFuncs *funcs = nullptr, *last = nullptr; // num_funcs is the number of entries in |funcs|. - Atomic<uint32_t> num_funcs; + Atomic<uint32_t> num_funcs = 0; // num_reserved is one if the ex_data index zero is reserved for legacy // |TYPE_get_app_data| functions. - uint8_t num_reserved; -} CRYPTO_EX_DATA_CLASS; - -#define CRYPTO_EX_DATA_CLASS_INIT {CRYPTO_MUTEX_INIT, nullptr, nullptr, {}, 0} -#define CRYPTO_EX_DATA_CLASS_INIT_WITH_APP_DATA \ - {CRYPTO_MUTEX_INIT, nullptr, nullptr, {}, 1} + uint8_t num_reserved = 0; +}; // CRYPTO_get_ex_new_index_ex allocates a new index for |ex_data_class|. Each // class of object should provide a wrapper function that uses the correct -// |CRYPTO_EX_DATA_CLASS|. It returns the new index on success and -1 on error. +// |ExDataClass|. It returns the new index on success and -1 on error. OPENSSL_EXPORT int CRYPTO_get_ex_new_index_ex( - CRYPTO_EX_DATA_CLASS *ex_data_class, long argl, void *argp, + ExDataClass *ex_data_class, long argl, void *argp, CRYPTO_EX_free *free_func); // CRYPTO_set_ex_data sets an extra data pointer on a given object. Each class @@ -773,7 +772,7 @@ OPENSSL_EXPORT void CRYPTO_new_ex_data(CRYPTO_EX_DATA *ad); // CRYPTO_free_ex_data frees |ad|, which is an object of the given class. -OPENSSL_EXPORT void CRYPTO_free_ex_data(CRYPTO_EX_DATA_CLASS *ex_data_class, +OPENSSL_EXPORT void CRYPTO_free_ex_data(ExDataClass *ex_data_class, CRYPTO_EX_DATA *ad);
diff --git a/crypto/thread_test.cc b/crypto/thread_test.cc index b9d7874..5fde27c 100644 --- a/crypto/thread_test.cc +++ b/crypto/thread_test.cc
@@ -58,26 +58,23 @@ } static CRYPTO_once_t once_init_value = CRYPTO_ONCE_INIT; -static CRYPTO_once_t once_bss; - static CRYPTO_MUTEX mutex_init_value = CRYPTO_MUTEX_INIT; -static CRYPTO_MUTEX mutex_bss; +static ExDataClass ex_data_class_value; -static CRYPTO_EX_DATA_CLASS ex_data_class_value = CRYPTO_EX_DATA_CLASS_INIT; -static CRYPTO_EX_DATA_CLASS ex_data_class_bss; +template <typename T> +static void ExpectAllZeros(const T &t) { + uint8_t zeros[sizeof(T)] = {}; + EXPECT_EQ(Bytes(zeros), + Bytes(reinterpret_cast<const uint8_t *>(&t), sizeof(t))); +} TEST(ThreadTest, InitZeros) { if (FIPS_mode()) { - // Our FIPS tooling currently requires that |CRYPTO_ONCE_INIT|, - // |CRYPTO_MUTEX_INIT| and |CRYPTO_EX_DATA_CLASS| are all zeros and so can - // be placed in the BSS section. - EXPECT_EQ(Bytes((uint8_t *)&once_bss, sizeof(once_bss)), - Bytes((uint8_t *)&once_init_value, sizeof(once_init_value))); - EXPECT_EQ(Bytes((uint8_t *)&mutex_bss, sizeof(mutex_bss)), - Bytes((uint8_t *)&mutex_init_value, sizeof(mutex_init_value))); - EXPECT_EQ( - Bytes((uint8_t *)&ex_data_class_bss, sizeof(ex_data_class_bss)), - Bytes((uint8_t *)&ex_data_class_value, sizeof(ex_data_class_value))); + // Our FIPS tooling currently requires that these types are all zeros and so + // can be placed in the BSS section. + ExpectAllZeros(once_init_value); + ExpectAllZeros(mutex_init_value); + ExpectAllZeros(ex_data_class_value); } }
diff --git a/crypto/x509/x509_vfy.cc b/crypto/x509/x509_vfy.cc index d162e80..7fcf06b 100644 --- a/crypto/x509/x509_vfy.cc +++ b/crypto/x509/x509_vfy.cc
@@ -31,8 +31,7 @@ using namespace bssl; -static CRYPTO_EX_DATA_CLASS g_ex_data_class = - CRYPTO_EX_DATA_CLASS_INIT_WITH_APP_DATA; +static ExDataClass g_ex_data_class(/*with_app_data=*/true); // CRL score values
diff --git a/crypto/x509/x_x509.cc b/crypto/x509/x_x509.cc index 313be92..9980315 100644 --- a/crypto/x509/x_x509.cc +++ b/crypto/x509/x_x509.cc
@@ -35,7 +35,7 @@ using namespace bssl; -static CRYPTO_EX_DATA_CLASS g_ex_data_class = CRYPTO_EX_DATA_CLASS_INIT; +static ExDataClass g_ex_data_class; static constexpr CBS_ASN1_TAG kVersionTag = CBS_ASN1_CONSTRUCTED | CBS_ASN1_CONTEXT_SPECIFIC | 0;
diff --git a/ssl/ssl_credential.cc b/ssl/ssl_credential.cc index 67fa8c3..49cef15 100644 --- a/ssl/ssl_credential.cc +++ b/ssl/ssl_credential.cc
@@ -105,7 +105,7 @@ using namespace bssl; -static CRYPTO_EX_DATA_CLASS g_ex_data_class = CRYPTO_EX_DATA_CLASS_INIT; +static ExDataClass g_ex_data_class; ssl_credential_st::ssl_credential_st(SSLCredentialType type_arg) : RefCounted(CheckSubClass()), type(type_arg) {
diff --git a/ssl/ssl_lib.cc b/ssl/ssl_lib.cc index a1ba926..9f52c03 100644 --- a/ssl/ssl_lib.cc +++ b/ssl/ssl_lib.cc
@@ -68,10 +68,8 @@ // kMaxHandshakeSize is the maximum size, in bytes, of a handshake message. static const size_t kMaxHandshakeSize = (1u << 24) - 1; -static CRYPTO_EX_DATA_CLASS g_ex_data_class_ssl = - CRYPTO_EX_DATA_CLASS_INIT_WITH_APP_DATA; -static CRYPTO_EX_DATA_CLASS g_ex_data_class_ssl_ctx = - CRYPTO_EX_DATA_CLASS_INIT_WITH_APP_DATA; +static ExDataClass g_ex_data_class_ssl(/*with_app_data=*/true); +static ExDataClass g_ex_data_class_ssl_ctx(/*with_app_data=*/true); void ssl_reset_error_state(SSL *ssl) { // Functions which use |SSL_get_error| must reset I/O and error state on
diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc index f9ebdf1..2eb78e8 100644 --- a/ssl/ssl_session.cc +++ b/ssl/ssl_session.cc
@@ -38,8 +38,7 @@ // that it needs to asynchronously fetch session information. static const char g_pending_session_magic = 0; -static CRYPTO_EX_DATA_CLASS g_ex_data_class = - CRYPTO_EX_DATA_CLASS_INIT_WITH_APP_DATA; +static ExDataClass g_ex_data_class(/*with_app_data=*/true); static void SSL_SESSION_list_remove(SSL_CTX *ctx, SSL_SESSION *session); static void SSL_SESSION_list_add(SSL_CTX *ctx, SSL_SESSION *session);