update main-with-bazel from master branch

BUILD.generated.bzl

@@ -414,9 +414,7 @@ crypto_sources = [
     "src/crypto/rand_extra/rand_extra.c",
     "src/crypto/rand_extra/windows.c",
     "src/crypto/rc4/rc4.c",
-    "src/crypto/refcount_c11.c",
-    "src/crypto/refcount_no_threads.c",
-    "src/crypto/refcount_win.c",
+    "src/crypto/refcount.c",
     "src/crypto/rsa_extra/rsa_asn1.c",
     "src/crypto/rsa_extra/rsa_crypt.c",
     "src/crypto/rsa_extra/rsa_print.c",

CMakeLists.txt

@@ -407,9 +407,7 @@ add_library(
   src/crypto/rand_extra/rand_extra.c
   src/crypto/rand_extra/windows.c
   src/crypto/rc4/rc4.c
-  src/crypto/refcount_c11.c
-  src/crypto/refcount_no_threads.c
-  src/crypto/refcount_win.c
+  src/crypto/refcount.c
   src/crypto/rsa_extra/rsa_asn1.c
   src/crypto/rsa_extra/rsa_crypt.c
   src/crypto/rsa_extra/rsa_print.c

sources.json

@@ -143,9 +143,7 @@
     "src/crypto/rand_extra/rand_extra.c", 
     "src/crypto/rand_extra/windows.c", 
     "src/crypto/rc4/rc4.c", 
-    "src/crypto/refcount_c11.c", 
-    "src/crypto/refcount_no_threads.c", 
-    "src/crypto/refcount_win.c", 
+    "src/crypto/refcount.c", 
     "src/crypto/rsa_extra/rsa_asn1.c", 
     "src/crypto/rsa_extra/rsa_crypt.c", 
     "src/crypto/rsa_extra/rsa_print.c", 

src/crypto/CMakeLists.txt

@@ -202,9 +202,7 @@ add_library(
   rand_extra/rand_extra.c
   rand_extra/windows.c
   rc4/rc4.c
-  refcount_c11.c
-  refcount_no_threads.c
-  refcount_win.c
+  refcount.c
   rsa_extra/rsa_asn1.c
   rsa_extra/rsa_crypt.c
   rsa_extra/rsa_print.c

src/crypto/fipsmodule/rand/fork_detect.c

@@ -38,7 +38,7 @@ static_assert(MADV_WIPEONFORK == 18, "MADV_WIPEONFORK is not 18");
 
 DEFINE_STATIC_ONCE(g_fork_detect_once);
 DEFINE_STATIC_MUTEX(g_fork_detect_lock);
-DEFINE_BSS_GET(volatile char *, g_fork_detect_addr);
+DEFINE_BSS_GET(CRYPTO_atomic_u32 *, g_fork_detect_addr);
 DEFINE_BSS_GET(uint64_t, g_fork_generation);
 DEFINE_BSS_GET(int, g_force_madv_wipeonfork);
 DEFINE_BSS_GET(int, g_force_madv_wipeonfork_enabled);
@@ -70,7 +70,7 @@ static void init_fork_detect(void) {
     return;
   }
 
-  *((volatile char *) addr) = 1;
+  CRYPTO_atomic_store_u32(addr, 1);
   *g_fork_detect_addr_bss_get() = addr;
   *g_fork_generation_bss_get() = 1;
 }
@@ -83,16 +83,12 @@ uint64_t CRYPTO_get_fork_generation(void) {
   // is initialised atomically, even if multiple threads enter this function
   // concurrently.
   //
-  // In the limit, the kernel may clear WIPEONFORK pages while a multi-threaded
-  // process is running. (For example, because a VM was cloned.) Therefore a
-  // lock is used below to synchronise the potentially multiple threads that may
-  // concurrently observe the cleared flag.
+  // Additionally, while the kernel will only clear WIPEONFORK at a point when a
+  // child process is single-threaded, the child may become multi-threaded
+  // before it observes this. Therefore, we must synchronize the logic below.
 
   CRYPTO_once(g_fork_detect_once_bss_get(), init_fork_detect);
-  // This pointer is |volatile| because the value pointed to may be changed by
-  // external forces (i.e. the kernel wiping the page) thus the compiler must
-  // not assume that it has exclusive access to it.
-  volatile char *const flag_ptr = *g_fork_detect_addr_bss_get();
+  CRYPTO_atomic_u32 *const flag_ptr = *g_fork_detect_addr_bss_get();
   if (flag_ptr == NULL) {
     // Our kernel is too old to support |MADV_WIPEONFORK| or
     // |g_force_madv_wipeonfork| is set.
@@ -105,28 +101,34 @@ uint64_t CRYPTO_get_fork_generation(void) {
     return 0;
   }
 
-  struct CRYPTO_STATIC_MUTEX *const lock = g_fork_detect_lock_bss_get();
+  // In the common case, try to observe the flag without taking a lock. This
+  // avoids cacheline contention in the PRNG.
   uint64_t *const generation_ptr = g_fork_generation_bss_get();
-
-  CRYPTO_STATIC_MUTEX_lock_read(lock);
-  uint64_t current_generation = *generation_ptr;
-  if (*flag_ptr) {
-    CRYPTO_STATIC_MUTEX_unlock_read(lock);
-    return current_generation;
+  if (CRYPTO_atomic_load_u32(flag_ptr) != 0) {
+    // If we observe a non-zero flag, it is safe to read |generation_ptr|
+    // without a lock. The flag and generation number are fixed for this copy of
+    // the address space.
+    return *generation_ptr;
   }
 
-  CRYPTO_STATIC_MUTEX_unlock_read(lock);
+  // The flag was zero. The generation number must be incremented, but other
+  // threads may have concurrently observed the zero, so take a lock before
+  // incrementing.
+  struct CRYPTO_STATIC_MUTEX *const lock = g_fork_detect_lock_bss_get();
   CRYPTO_STATIC_MUTEX_lock_write(lock);
-  current_generation = *generation_ptr;
-  if (*flag_ptr == 0) {
+  uint64_t current_generation = *generation_ptr;
+  if (CRYPTO_atomic_load_u32(flag_ptr) == 0) {
     // A fork has occurred.
-    *flag_ptr = 1;
-
     current_generation++;
     if (current_generation == 0) {
+      // Zero means fork detection isn't supported, so skip that value.
       current_generation = 1;
     }
+
+    // We must update |generation_ptr| before |flag_ptr|. Other threads may
+    // observe |flag_ptr| without taking a lock.
     *generation_ptr = current_generation;
+    CRYPTO_atomic_store_u32(flag_ptr, 1);
   }
   CRYPTO_STATIC_MUTEX_unlock_write(lock);
 

src/crypto/fipsmodule/rand/rand.c

@@ -65,6 +65,9 @@ struct rand_thread_state {
   // last_block_valid is non-zero iff |last_block| contains data from
   // |get_seed_entropy|.
   int last_block_valid;
+  // fork_unsafe_buffering is non-zero iff, when |drbg| was last (re)seeded,
+  // fork-unsafe buffering was enabled.
+  int fork_unsafe_buffering;
 
 #if defined(BORINGSSL_FIPS)
   // last_block contains the previous block from |get_seed_entropy|.
@@ -72,6 +75,10 @@ struct rand_thread_state {
   // next and prev form a NULL-terminated, double-linked list of all states in
   // a process.
   struct rand_thread_state *next, *prev;
+  // clear_drbg_lock synchronizes between uses of |drbg| and
+  // |rand_thread_state_clear_all| clearing it. This lock should be uncontended
+  // in the common case, except on shutdown.
+  CRYPTO_MUTEX clear_drbg_lock;
 #endif
 };
 
@@ -82,18 +89,19 @@ struct rand_thread_state {
 // called when the whole process is exiting.
 DEFINE_BSS_GET(struct rand_thread_state *, thread_states_list);
 DEFINE_STATIC_MUTEX(thread_states_list_lock);
-DEFINE_STATIC_MUTEX(state_clear_all_lock);
 
 static void rand_thread_state_clear_all(void) __attribute__((destructor));
 static void rand_thread_state_clear_all(void) {
   CRYPTO_STATIC_MUTEX_lock_write(thread_states_list_lock_bss_get());
-  CRYPTO_STATIC_MUTEX_lock_write(state_clear_all_lock_bss_get());
   for (struct rand_thread_state *cur = *thread_states_list_bss_get();
        cur != NULL; cur = cur->next) {
+    CRYPTO_MUTEX_lock_write(&cur->clear_drbg_lock);
     CTR_DRBG_clear(&cur->drbg);
   }
   // The locks are deliberately left locked so that any threads that are still
-  // running will hang if they try to call |RAND_bytes|.
+  // running will hang if they try to call |RAND_bytes|. It also ensures
+  // |rand_thread_state_free| cannot free any thread state while we've taken the
+  // lock.
 }
 #endif
 
@@ -326,6 +334,7 @@ void RAND_bytes_with_additional_data(uint8_t *out, size_t out_len,
   }
 
   const uint64_t fork_generation = CRYPTO_get_fork_generation();
+  const int fork_unsafe_buffering = rand_fork_unsafe_buffering_enabled();
 
   // Additional data is mixed into every CTR-DRBG call to protect, as best we
   // can, against forks & VM clones. We do not over-read this information and
@@ -340,7 +349,7 @@ void RAND_bytes_with_additional_data(uint8_t *out, size_t out_len,
     // entropy is used. This can be expensive (one read per |RAND_bytes| call)
     // and so is disabled when we have fork detection, or if the application has
     // promised not to fork.
-    if (fork_generation != 0 || rand_fork_unsafe_buffering_enabled()) {
+    if (fork_generation != 0 || fork_unsafe_buffering) {
       OPENSSL_memset(additional_data, 0, sizeof(additional_data));
     } else if (!have_rdrand()) {
       // No alternative so block for OS entropy.
@@ -383,8 +392,10 @@ void RAND_bytes_with_additional_data(uint8_t *out, size_t out_len,
     }
     state->calls = 0;
     state->fork_generation = fork_generation;
+    state->fork_unsafe_buffering = fork_unsafe_buffering;
 
 #if defined(BORINGSSL_FIPS)
+    CRYPTO_MUTEX_init(&state->clear_drbg_lock);
     if (state != &stack_state) {
       CRYPTO_STATIC_MUTEX_lock_write(thread_states_list_lock_bss_get());
       struct rand_thread_state **states_list = thread_states_list_bss_get();
@@ -400,7 +411,14 @@ void RAND_bytes_with_additional_data(uint8_t *out, size_t out_len,
   }
 
   if (state->calls >= kReseedInterval ||
-      state->fork_generation != fork_generation) {
+      // If we've forked since |state| was last seeded, reseed.
+      state->fork_generation != fork_generation ||
+      // If |state| was seeded from a state with different fork-safety
+      // preferences, reseed. Suppose |state| was fork-safe, then forked into
+      // two children, but each of the children never fork and disable fork
+      // safety. The children must reseed to avoid working from the same PRNG
+      // state.
+      state->fork_unsafe_buffering != fork_unsafe_buffering) {
     uint8_t seed[CTR_DRBG_ENTROPY_LEN];
     uint8_t reseed_additional_data[CTR_DRBG_ENTROPY_LEN] = {0};
     size_t reseed_additional_data_len = 0;
@@ -410,7 +428,7 @@ void RAND_bytes_with_additional_data(uint8_t *out, size_t out_len,
     // Take a read lock around accesses to |state->drbg|. This is needed to
     // avoid returning bad entropy if we race with
     // |rand_thread_state_clear_all|.
-    CRYPTO_STATIC_MUTEX_lock_read(state_clear_all_lock_bss_get());
+    CRYPTO_MUTEX_lock_read(&state->clear_drbg_lock);
 #endif
     if (!CTR_DRBG_reseed(&state->drbg, seed, reseed_additional_data,
                          reseed_additional_data_len)) {
@@ -418,9 +436,10 @@ void RAND_bytes_with_additional_data(uint8_t *out, size_t out_len,
     }
     state->calls = 0;
     state->fork_generation = fork_generation;
+    state->fork_unsafe_buffering = fork_unsafe_buffering;
   } else {
 #if defined(BORINGSSL_FIPS)
-    CRYPTO_STATIC_MUTEX_lock_read(state_clear_all_lock_bss_get());
+    CRYPTO_MUTEX_lock_read(&state->clear_drbg_lock);
 #endif
   }
 
@@ -449,7 +468,7 @@ void RAND_bytes_with_additional_data(uint8_t *out, size_t out_len,
   }
 
 #if defined(BORINGSSL_FIPS)
-  CRYPTO_STATIC_MUTEX_unlock_read(state_clear_all_lock_bss_get());
+  CRYPTO_MUTEX_unlock_read(&state->clear_drbg_lock);
 #endif
 }
 

src/crypto/internal.h

@@ -155,6 +155,32 @@
 #if defined(OPENSSL_THREADS) && !defined(OPENSSL_PTHREADS) && \
     defined(OPENSSL_WINDOWS)
 #define OPENSSL_WINDOWS_THREADS
+#endif
+
+// Determine the atomics implementation to use with C.
+#if !defined(__cplusplus)
+#if !defined(OPENSSL_C11_ATOMIC) && defined(OPENSSL_THREADS) &&   \
+    !defined(__STDC_NO_ATOMICS__) && defined(__STDC_VERSION__) && \
+    __STDC_VERSION__ >= 201112L
+#define OPENSSL_C11_ATOMIC
+#endif
+
+// Older MSVC does not support C11 atomics, so we fallback to the Windows APIs.
+// When both are available (e.g. clang-cl), we prefer the C11 ones. The Windows
+// APIs don't allow some operations to be implemented as efficiently. This can
+// be removed once we can rely on
+// https://devblogs.microsoft.com/cppblog/c11-atomics-in-visual-studio-2022-version-17-5-preview-2/
+#if !defined(OPENSSL_C11_ATOMIC) && defined(OPENSSL_THREADS) && \
+    defined(OPENSSL_WINDOWS)
+#define OPENSSL_WINDOWS_ATOMIC
+#endif
+#endif  // !__cplusplus
+
+#if defined(OPENSSL_C11_ATOMIC)
+#include <stdatomic.h>
+#endif
+
+#if defined(OPENSSL_WINDOWS_THREADS) || defined(OPENSSL_WINDOWS_ATOMIC)
 OPENSSL_MSVC_PRAGMA(warning(push, 3))
 #include <windows.h>
 OPENSSL_MSVC_PRAGMA(warning(pop))
@@ -539,33 +565,117 @@ typedef pthread_once_t CRYPTO_once_t;
 OPENSSL_EXPORT void CRYPTO_once(CRYPTO_once_t *once, void (*init)(void));
 
 
-// Reference counting.
+// Atomics.
+//
+// The following functions provide an API analogous to <stdatomic.h> from C11
+// and abstract between a few variations on atomics we need to support.
 
-// Automatically enable C11 atomics if implemented.
-#if !defined(OPENSSL_C11_ATOMIC) && defined(OPENSSL_THREADS) &&   \
-    !defined(__STDC_NO_ATOMICS__) && defined(__STDC_VERSION__) && \
-    __STDC_VERSION__ >= 201112L
-#define OPENSSL_C11_ATOMIC
-#endif
+#if defined(__cplusplus)
 
-// Older MSVC does not support C11 atomics, so we fallback to the Windows APIs.
-// This can be removed once we can rely on
-// https://devblogs.microsoft.com/cppblog/c11-atomics-in-visual-studio-2022-version-17-5-preview-2/
-#if !defined(OPENSSL_C11_ATOMIC) && defined(OPENSSL_THREADS) && \
-    defined(OPENSSL_WINDOWS)
-#define OPENSSL_WINDOWS_ATOMIC
-#endif
+// In C++, we can't easily detect whether C will use |OPENSSL_C11_ATOMIC| or
+// |OPENSSL_WINDOWS_ATOMIC|. Instead, we define a layout-compatible type without
+// the corresponding functions. When we can rely on C11 atomics in MSVC, that
+// will no longer be a concern.
+typedef uint32_t CRYPTO_atomic_u32;
+
+#elif defined(OPENSSL_C11_ATOMIC)
+
+typedef _Atomic uint32_t CRYPTO_atomic_u32;
+
+// This should be const, but the |OPENSSL_WINDOWS_ATOMIC| implementation is not
+// const due to Windows limitations. When we can rely on C11 atomics, make this
+// const-correct.
+OPENSSL_INLINE uint32_t CRYPTO_atomic_load_u32(CRYPTO_atomic_u32 *val) {
+  return atomic_load(val);
+}
+
+OPENSSL_INLINE int CRYPTO_atomic_compare_exchange_weak_u32(
+    CRYPTO_atomic_u32 *val, uint32_t *expected, uint32_t desired) {
+  return atomic_compare_exchange_weak(val, expected, desired);
+}
+
+OPENSSL_INLINE void CRYPTO_atomic_store_u32(CRYPTO_atomic_u32 *val,
+                                            uint32_t desired) {
+  atomic_store(val, desired);
+}
+
+#elif defined(OPENSSL_WINDOWS_ATOMIC)
+
+typedef LONG CRYPTO_atomic_u32;
+
+OPENSSL_INLINE uint32_t CRYPTO_atomic_load_u32(volatile CRYPTO_atomic_u32 *val) {
+  // This is not ideal because it still writes to a cacheline. MSVC is not able
+  // to optimize this to a true atomic read, and Windows does not provide an
+  // InterlockedLoad function.
+  //
+  // The Windows documentation [1] does say "Simple reads and writes to
+  // properly-aligned 32-bit variables are atomic operations", but this is not
+  // phrased in terms of the C11 and C++11 memory models, and indeed a read or
+  // write seems to produce slightly different code on MSVC than a sequentially
+  // consistent std::atomic::load in C++. Moreover, it is unclear if non-MSVC
+  // compilers on Windows provide the same guarantees. Thus we avoid relying on
+  // this and instead still use an interlocked function. This is still
+  // preferable a global mutex, and eventually this code will be replaced by
+  // [2]. Additionally, on clang-cl, we'll use the |OPENSSL_C11_ATOMIC| path.
+  //
+  // [1] https://learn.microsoft.com/en-us/windows/win32/sync/interlocked-variable-access
+  // [2] https://devblogs.microsoft.com/cppblog/c11-atomics-in-visual-studio-2022-version-17-5-preview-2/
+  return (uint32_t)InterlockedCompareExchange(val, 0, 0);
+}
+
+OPENSSL_INLINE int CRYPTO_atomic_compare_exchange_weak_u32(
+    volatile CRYPTO_atomic_u32 *val, uint32_t *expected32, uint32_t desired) {
+  LONG expected = (LONG)*expected32;
+  LONG actual = InterlockedCompareExchange(val, (LONG)desired, expected);
+  *expected32 = (uint32_t)actual;
+  return actual == expected;
+}
+
+OPENSSL_INLINE void CRYPTO_atomic_store_u32(volatile CRYPTO_atomic_u32 *val,
+                                            uint32_t desired) {
+  InterlockedExchange(val, (LONG)desired);
+}
+
+#elif !defined(OPENSSL_THREADS)
+
+typedef uint32_t CRYPTO_atomic_u32;
+
+OPENSSL_INLINE uint32_t CRYPTO_atomic_load_u32(CRYPTO_atomic_u32 *val) {
+  return *val;
+}
+
+OPENSSL_INLINE int CRYPTO_atomic_compare_exchange_weak_u32(
+    CRYPTO_atomic_u32 *val, uint32_t *expected, uint32_t desired) {
+  if (*val != *expected) {
+    *expected = *val;
+    return 0;
+  }
+  *val = desired;
+  return 1;
+}
+
+OPENSSL_INLINE void CRYPTO_atomic_store_u32(CRYPTO_atomic_u32 *val,
+                                            uint32_t desired) {
+  *val = desired;
+}
+
+#else
 
 // Require some atomics implementation. Contact BoringSSL maintainers if you
 // have a platform with fails this check.
-//
-// Note this check can only be done in C. From C++, we don't know whether the
-// corresponding C mode would support C11 atomics.
-#if !defined(__cplusplus) && defined(OPENSSL_THREADS) && \
-    !defined(OPENSSL_C11_ATOMIC) && !defined(OPENSSL_WINDOWS_ATOMIC)
 #error "Thread-compatible configurations require atomics"
+
 #endif
 
+// See the comment in the |__cplusplus| section above.
+static_assert(sizeof(CRYPTO_atomic_u32) == sizeof(uint32_t),
+              "CRYPTO_atomic_u32 does not match uint32_t size");
+static_assert(alignof(CRYPTO_atomic_u32) == alignof(uint32_t),
+              "CRYPTO_atomic_u32 does not match uint32_t alignment");
+
+
+// Reference counting.
+
 // CRYPTO_REFCOUNT_MAX is the value at which the reference count saturates.
 #define CRYPTO_REFCOUNT_MAX 0xffffffff
 

src/crypto/rand_extra/forkunsafe.c

@@ -17,13 +17,12 @@
 #include <stdlib.h>
 
 #include "../fipsmodule/rand/internal.h"
+#include "../internal.h"
 
 
-// g_buffering_enabled is true if fork-unsafe buffering has been enabled.
-static int g_buffering_enabled = 0;
-
-// g_lock protects |g_buffering_enabled|.
-static struct CRYPTO_STATIC_MUTEX g_lock = CRYPTO_STATIC_MUTEX_INIT;
+// g_buffering_enabled is one if fork-unsafe buffering has been enabled and zero
+// otherwise.
+static CRYPTO_atomic_u32 g_buffering_enabled = 0;
 
 #if !defined(OPENSSL_WINDOWS)
 void RAND_enable_fork_unsafe_buffering(int fd) {
@@ -32,15 +31,10 @@ void RAND_enable_fork_unsafe_buffering(int fd) {
     abort();
   }
 
-  CRYPTO_STATIC_MUTEX_lock_write(&g_lock);
-  g_buffering_enabled = 1;
-  CRYPTO_STATIC_MUTEX_unlock_write(&g_lock);
+  CRYPTO_atomic_store_u32(&g_buffering_enabled, 1);
 }
 #endif
 
 int rand_fork_unsafe_buffering_enabled(void) {
-  CRYPTO_STATIC_MUTEX_lock_read(&g_lock);
-  const int ret = g_buffering_enabled;
-  CRYPTO_STATIC_MUTEX_unlock_read(&g_lock);
-  return ret;
+  return CRYPTO_atomic_load_u32(&g_buffering_enabled) != 0;
 }

src/crypto/rand_extra/rand_test.cc

@@ -56,7 +56,7 @@ TEST(RandTest, NotObviouslyBroken) {
 
 #if !defined(OPENSSL_WINDOWS) && !defined(OPENSSL_IOS) && \
     !defined(OPENSSL_FUCHSIA) && !defined(BORINGSSL_UNSAFE_DETERMINISTIC_MODE)
-static bool ForkAndRand(bssl::Span<uint8_t> out) {
+static bool ForkAndRand(bssl::Span<uint8_t> out, bool fork_unsafe_buffering) {
   int pipefds[2];
   if (pipe(pipefds) < 0) {
     perror("pipe");
@@ -76,6 +76,9 @@ static bool ForkAndRand(bssl::Span<uint8_t> out) {
   if (child == 0) {
     // This is the child. Generate entropy and write it to the parent.
     close(pipefds[0]);
+    if (fork_unsafe_buffering) {
+      RAND_enable_fork_unsafe_buffering(-1);
+    }
     RAND_bytes(out.data(), out.size());
     while (!out.empty()) {
       ssize_t ret = write(pipefds[1], out.data(), out.size());
@@ -136,18 +139,27 @@ TEST(RandTest, Fork) {
   // intentionally uses smaller buffers than the others, to minimize the chance
   // of sneaking by with a large enough buffer that we've since reseeded from
   // the OS.
-  uint8_t buf1[16], buf2[16], buf3[16];
-  ASSERT_TRUE(ForkAndRand(buf1));
-  ASSERT_TRUE(ForkAndRand(buf2));
-  RAND_bytes(buf3, sizeof(buf3));
+  //
+  // All child processes should have different PRNGs, including the ones that
+  // disavow fork-safety. Although they are produced by fork, they themselves do
+  // not fork after that call.
+  uint8_t bufs[5][16];
+  ASSERT_TRUE(ForkAndRand(bufs[0], /*fork_unsafe_buffering=*/false));
+  ASSERT_TRUE(ForkAndRand(bufs[1], /*fork_unsafe_buffering=*/false));
+  ASSERT_TRUE(ForkAndRand(bufs[2], /*fork_unsafe_buffering=*/true));
+  ASSERT_TRUE(ForkAndRand(bufs[3], /*fork_unsafe_buffering=*/true));
+  RAND_bytes(bufs[4], sizeof(bufs[4]));
 
-  // All should be different.
-  EXPECT_NE(Bytes(buf1), Bytes(buf2));
-  EXPECT_NE(Bytes(buf2), Bytes(buf3));
-  EXPECT_NE(Bytes(buf1), Bytes(buf3));
-  EXPECT_NE(Bytes(buf1), Bytes(kZeros));
-  EXPECT_NE(Bytes(buf2), Bytes(kZeros));
-  EXPECT_NE(Bytes(buf3), Bytes(kZeros));
+  // All should be different and non-zero.
+  for (const auto &buf : bufs) {
+    EXPECT_NE(Bytes(buf), Bytes(kZeros));
+  }
+  for (size_t i = 0; i < OPENSSL_ARRAY_SIZE(bufs); i++) {
+    for (size_t j = 0; j < i; j++) {
+      EXPECT_NE(Bytes(bufs[i]), Bytes(bufs[j]))
+          << "buffers " << i << " and " << j << " matched";
+    }
+  }
 }
 #endif  // !OPENSSL_WINDOWS && !OPENSSL_IOS &&
         // !OPENSSL_FUCHSIA && !BORINGSSL_UNSAFE_DETERMINISTIC_MODE

src/crypto/refcount.c

@@ -14,39 +14,35 @@
 
 #include "internal.h"
 
-
-#if defined(OPENSSL_C11_ATOMIC)
-
 #include <assert.h>
 #include <stdalign.h>
-#include <stdatomic.h>
 #include <stdlib.h>
 
 
 // See comment above the typedef of CRYPTO_refcount_t about these tests.
-static_assert(alignof(CRYPTO_refcount_t) == alignof(_Atomic CRYPTO_refcount_t),
-              "_Atomic alters the needed alignment of a reference count");
-static_assert(sizeof(CRYPTO_refcount_t) == sizeof(_Atomic CRYPTO_refcount_t),
-              "_Atomic alters the size of a reference count");
+static_assert(alignof(CRYPTO_refcount_t) == alignof(CRYPTO_atomic_u32),
+              "CRYPTO_refcount_t does not match CRYPTO_atomic_u32 alignment");
+static_assert(sizeof(CRYPTO_refcount_t) == sizeof(CRYPTO_atomic_u32),
+              "CRYPTO_refcount_t does not match CRYPTO_atomic_u32 size");
 
 static_assert((CRYPTO_refcount_t)-1 == CRYPTO_REFCOUNT_MAX,
               "CRYPTO_REFCOUNT_MAX is incorrect");
 
 void CRYPTO_refcount_inc(CRYPTO_refcount_t *in_count) {
-  _Atomic CRYPTO_refcount_t *count = (_Atomic CRYPTO_refcount_t *) in_count;
-  uint32_t expected = atomic_load(count);
+  CRYPTO_atomic_u32 *count = (CRYPTO_atomic_u32 *)in_count;
+  uint32_t expected = CRYPTO_atomic_load_u32(count);
 
   while (expected != CRYPTO_REFCOUNT_MAX) {
     uint32_t new_value = expected + 1;
-    if (atomic_compare_exchange_weak(count, &expected, new_value)) {
+    if (CRYPTO_atomic_compare_exchange_weak_u32(count, &expected, new_value)) {
       break;
     }
   }
 }
 
 int CRYPTO_refcount_dec_and_test_zero(CRYPTO_refcount_t *in_count) {
-  _Atomic CRYPTO_refcount_t *count = (_Atomic CRYPTO_refcount_t *)in_count;
-  uint32_t expected = atomic_load(count);
+  CRYPTO_atomic_u32 *count = (CRYPTO_atomic_u32 *)in_count;
+  uint32_t expected = CRYPTO_atomic_load_u32(count);
 
   for (;;) {
     if (expected == 0) {
@@ -55,11 +51,10 @@ int CRYPTO_refcount_dec_and_test_zero(CRYPTO_refcount_t *in_count) {
       return 0;
     } else {
       const uint32_t new_value = expected - 1;
-      if (atomic_compare_exchange_weak(count, &expected, new_value)) {
+      if (CRYPTO_atomic_compare_exchange_weak_u32(count, &expected,
+                                                  new_value)) {
         return new_value == 0;
       }
     }
   }
 }
-
-#endif  // OPENSSL_C11_ATOMIC

src/crypto/refcount_no_threads.c

@@ -1,42 +0,0 @@
-/* Copyright (c) 2015, Google Inc.
- *
- * Permission to use, copy, modify, and/or distribute this software for any
- * purpose with or without fee is hereby granted, provided that the above
- * copyright notice and this permission notice appear in all copies.
- *
- * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
- * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
- * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY
- * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
- * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION
- * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
- * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */
-
-#include "internal.h"
-
-#include <assert.h>
-#include <stdlib.h>
-
-
-#if !defined(OPENSSL_THREADS)
-
-static_assert((CRYPTO_refcount_t)-1 == CRYPTO_REFCOUNT_MAX,
-              "CRYPTO_REFCOUNT_MAX is incorrect");
-
-void CRYPTO_refcount_inc(CRYPTO_refcount_t *count) {
-  if (*count < CRYPTO_REFCOUNT_MAX) {
-    (*count)++;
-  }
-}
-
-int CRYPTO_refcount_dec_and_test_zero(CRYPTO_refcount_t *count) {
-  if (*count == 0) {
-    abort();
-  }
-  if (*count < CRYPTO_REFCOUNT_MAX) {
-    (*count)--;
-  }
-  return *count == 0;
-}
-
-#endif  // !OPENSSL_THREADS

src/crypto/refcount_win.c

@@ -1,89 +0,0 @@
-/* Copyright (c) 2023, Google Inc.
- *
- * Permission to use, copy, modify, and/or distribute this software for any
- * purpose with or without fee is hereby granted, provided that the above
- * copyright notice and this permission notice appear in all copies.
- *
- * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
- * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
- * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY
- * SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
- * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION
- * OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN
- * CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE. */
-
-#include "internal.h"
-
-#if defined(OPENSSL_WINDOWS_ATOMIC)
-
-#include <windows.h>
-
-
-// See comment above the typedef of CRYPTO_refcount_t about these tests.
-static_assert(alignof(CRYPTO_refcount_t) == alignof(LONG),
-              "CRYPTO_refcount_t does not match LONG alignment");
-static_assert(sizeof(CRYPTO_refcount_t) == sizeof(LONG),
-              "CRYPTO_refcount_t does not match LONG size");
-
-static_assert((CRYPTO_refcount_t)-1 == CRYPTO_REFCOUNT_MAX,
-              "CRYPTO_REFCOUNT_MAX is incorrect");
-
-static uint32_t atomic_load_u32(volatile LONG *ptr) {
-  // This is not ideal because it still writes to a cacheline. MSVC is not able
-  // to optimize this to a true atomic read, and Windows does not provide an
-  // InterlockedLoad function.
-  //
-  // The Windows documentation [1] does say "Simple reads and writes to
-  // properly-aligned 32-bit variables are atomic operations", but this is not
-  // phrased in terms of the C11 and C++11 memory models, and indeed a read or
-  // write seems to produce slightly different code on MSVC than a sequentially
-  // consistent std::atomic::load in C++. Moreover, it is unclear if non-MSVC
-  // compilers on Windows provide the same guarantees. Thus we avoid relying on
-  // this and instead still use an interlocked function. This is still
-  // preferable a global mutex, and eventually this code will be replaced by
-  // [2]. Additionally, on clang-cl, we'll use the |OPENSSL_C11_ATOMIC| path.
-  //
-  // [1] https://learn.microsoft.com/en-us/windows/win32/sync/interlocked-variable-access
-  // [2] https://devblogs.microsoft.com/cppblog/c11-atomics-in-visual-studio-2022-version-17-5-preview-2/
-  return (uint32_t)InterlockedCompareExchange(ptr, 0, 0);
-}
-
-static int atomic_compare_exchange_u32(volatile LONG *ptr, uint32_t *expected32,
-                                       uint32_t desired) {
-  LONG expected = (LONG)*expected32;
-  LONG actual = InterlockedCompareExchange(ptr, (LONG)desired, expected);
-  *expected32 = (uint32_t)actual;
-  return actual == expected;
-}
-
-void CRYPTO_refcount_inc(CRYPTO_refcount_t *in_count) {
-  volatile LONG *count = (volatile LONG *)in_count;
-  uint32_t expected = atomic_load_u32(count);
-
-  while (expected != CRYPTO_REFCOUNT_MAX) {
-    const uint32_t new_value = expected + 1;
-    if (atomic_compare_exchange_u32(count, &expected, new_value)) {
-      break;
-    }
-  }
-}
-
-int CRYPTO_refcount_dec_and_test_zero(CRYPTO_refcount_t *in_count) {
-  volatile LONG *count = (volatile LONG *)in_count;
-  uint32_t expected = atomic_load_u32(count);
-
-  for (;;) {
-    if (expected == 0) {
-      abort();
-    } else if (expected == CRYPTO_REFCOUNT_MAX) {
-      return 0;
-    } else {
-      const uint32_t new_value = expected - 1;
-      if (atomic_compare_exchange_u32(count, &expected, new_value)) {
-        return new_value == 0;
-      }
-    }
-  }
-}
-
-#endif  // OPENSSL_WINDOWS_ATOMIC

src/include/openssl/thread.h

@@ -100,7 +100,7 @@ typedef union crypto_mutex_st {
 // _Atomic qualifier. However, this header is included by C++ programs as well
 // as C code that might not set -std=c11. So, in practice, it's not possible to
 // do that. Instead we statically assert that the size and native alignment of
-// a plain uint32_t and an _Atomic uint32_t are equal in refcount_c11.c.
+// a plain uint32_t and an _Atomic uint32_t are equal in refcount.c.
 typedef uint32_t CRYPTO_refcount_t;
 
 

src/ssl/ssl_test.cc

@@ -5839,6 +5839,81 @@ TEST_P(SSLVersionTest, SessionPropertiesThreads) {
     thread.join();
   }
 }
+
+static void SetValueOnFree(void *parent, void *ptr, CRYPTO_EX_DATA *ad,
+                          int index, long argl, void *argp) {
+  if (ptr != nullptr) {
+    *static_cast<long *>(ptr) = argl;
+  }
+}
+
+// Test that one thread can register ex_data while another thread is destroying
+// an object that uses it.
+TEST(SSLTest, ExDataThreads) {
+  static bool already_run = false;
+  if (already_run) {
+    GTEST_SKIP() << "This test consumes process-global resources and can only "
+                    "be run once in a process. It is not compatible with "
+                    "--gtest_repeat.";
+  }
+  already_run = true;
+
+  bssl::UniquePtr<SSL_CTX> ctx(SSL_CTX_new(TLS_method()));
+  ASSERT_TRUE(ctx);
+
+  // Register an initial index, so the threads can exercise having any ex_data.
+  int first_index =
+      SSL_get_ex_new_index(-1, nullptr, nullptr, nullptr, SetValueOnFree);
+  ASSERT_GE(first_index, 0);
+
+  // Callers may register indices concurrently with using other indices. This
+  // may happen if one part of an application is initializing while another part
+  // is already running.
+  static constexpr int kNumIndices = 3;
+  static constexpr int kNumSSLs = 10;
+  int index[kNumIndices];
+  long values[kNumSSLs];
+  std::fill(std::begin(values), std::end(values), -2);
+  std::vector<std::thread> threads;
+  for (size_t i = 0; i < kNumIndices; i++) {
+    threads.emplace_back([&, i] {
+      index[i] = SSL_get_ex_new_index(static_cast<long>(i), nullptr, nullptr,
+                                      nullptr, SetValueOnFree);
+      ASSERT_GE(index[i], 0);
+    });
+  }
+  for (size_t i = 0; i < kNumSSLs; i++) {
+    threads.emplace_back([&, i] {
+      bssl::UniquePtr<SSL> ssl(SSL_new(ctx.get()));
+      ASSERT_TRUE(ssl);
+      ASSERT_TRUE(SSL_set_ex_data(ssl.get(), first_index, &values[i]));
+    });
+  }
+  for (auto &thread : threads) {
+    thread.join();
+  }
+
+  // Each of the SSL threads should have set their flag via ex_data.
+  for (size_t i = 0; i < kNumSSLs; i++) {
+    EXPECT_EQ(values[i], -1);
+  }
+
+  // Each of the newly-registered indices should be distinct and work correctly.
+  static_assert(kNumIndices <= kNumSSLs, "values buffer too small");
+  std::fill(std::begin(values), std::end(values), -2);
+  bssl::UniquePtr<SSL> ssl(SSL_new(ctx.get()));
+  ASSERT_TRUE(ssl);
+  for (size_t i = 0; i < kNumIndices; i++) {
+    for (size_t j = 0; j < i; j++) {
+      EXPECT_NE(index[i], index[j]);
+    }
+    ASSERT_TRUE(SSL_set_ex_data(ssl.get(), index[i], &values[i]));
+  }
+  ssl = nullptr;
+  for (size_t i = 0; i < kNumIndices; i++) {
+    EXPECT_EQ(values[i], static_cast<long>(i));
+  }
+}
 #endif  // OPENSSL_THREADS
 
 constexpr size_t kNumQUICLevels = 4;