Revert "Reduce thread contention for pooled CRYPTO_BUFFERs"

This reverts commit fbf01ffae10929d863b6744bb505f0cae6c9c712 with a
regression test.

This change was incorrect. When we observe an unmodified refcount of
zero, it's possible that some other thread brought this up, and then
back down again.

Bug: 490953577
Change-Id: I3248c5440ee7a6310774a85a728aa24e29c6a158
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/90587
Auto-Submit: David Benjamin <davidben@google.com>
Reviewed-by: Rudolf Polzer <rpolzer@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
Commit-Queue: Rudolf Polzer <rpolzer@google.com>

crypto/pool/pool.cc

diff --git a/crypto/pool/pool.cc b/crypto/pool/pool.cc
index ddc37fa..3c47143 100644
--- a/crypto/pool/pool.cc
+++ b/crypto/pool/pool.cc
@@ -89,24 +89,24 @@
 }
 
 void CryptoBuffer::UpRefInternal() {
-  // Note that, unlike standard ref-counting, it is possible that |references_|
-  // is zero and this function increases the reference count to one. See
-  // DecRefInternal below.
+  // This is safe in the case that |buf->pool| is NULL because it's just
+  // standard reference counting in that case.
+  //
+  // This is also safe if |buf->pool| is non-NULL because, if it were racing
+  // with |CRYPTO_BUFFER_free| then the two callers must have independent
+  // references already and so the reference count will never hit zero.
   CRYPTO_refcount_inc(&references_);
 }
 
 void CryptoBuffer::DecRefInternal() {
-  if (!CRYPTO_refcount_dec_and_test_zero(&references_)) {
-    return;
-  }
-
-  if (pool_handle_ != nullptr) {
-    // Although the reference count is zero, there is still a weak reference
-    // from the pool. Some other thread may concurrently upgrade the weak
-    // reference to a strong reference. Synchronize with the pool and see if
-    // this has happened.
+  // If there is a pool, decrementing the refcount must synchronize with it.
+  if (pool_handle_ == nullptr) {
+    if (!CRYPTO_refcount_dec_and_test_zero(&references_)) {
+      return;
+    }
+  } else {
     MutexWriteLock lock(&pool_handle_->lock_);
-    if (references_.load() != 0) {
+    if (!CRYPTO_refcount_dec_and_test_zero(&references_)) {
       return;
     }
 
@@ -127,10 +127,6 @@
         (void)found;
       }
     }
-
-    // The weak reference is now removed. When we unlock the pool handle,
-    // another thread cannot upgrade to a strong reference, so we can free the
-    // buffer.
   }
 
   this->~CryptoBuffer();
@@ -182,8 +178,6 @@
       duplicate = nullptr;
     }
     if (duplicate != nullptr) {
-      // UpRef must be called under the lock. This converts the pool's weak
-      // reference to a strong reference. See CryptoBuffer::DecRefInternal.
       return UpRef(duplicate);
     }
   }
@@ -201,8 +195,6 @@
     duplicate = nullptr;
   }
   if (duplicate != nullptr) {
-    // UpRef must be called under the lock. This converts the pool's weak
-    // reference to a strong reference. See CryptoBuffer::DecRefInternal.
     return UpRef(duplicate);
   }
 

crypto/pool/pool_test.cc

diff --git a/crypto/pool/pool_test.cc b/crypto/pool/pool_test.cc
index f753b80..055bc11 100644
--- a/crypto/pool/pool_test.cc
+++ b/crypto/pool/pool_test.cc
@@ -161,6 +161,7 @@
   // Race threads making pooled |CRYPTO_BUFFER|s.
   static const uint8_t kData[4] = {1, 2, 3, 4};
   static const uint8_t kData2[3] = {4, 5, 6};
+  static const uint8_t kData3[3] = {7, 8, 9};
   UniquePtr<CRYPTO_BUFFER> buf, buf2, buf3;
   {
     std::thread thread([&] {
@@ -228,6 +229,25 @@
     buf = std::move(buf2);
   }
 
+  // Race two threads repeatedly creating and destroying a buffer, to race
+  // destruction with a strong/weak reference upgrade.
+  {
+    constexpr size_t kNumThreads = 2;
+    constexpr size_t kIterations = 1000;
+    std::vector<std::thread> threads;
+    for (size_t i = 0; i < kNumThreads; i++) {
+      threads.emplace_back([&] {
+        for (size_t j = 0; j < kIterations; j++) {
+          CRYPTO_BUFFER_free(
+              CRYPTO_BUFFER_new(kData3, sizeof(kData3), pool.get()));
+        }
+      });
+    }
+    for (size_t i = 0; i < kNumThreads; i++) {
+      threads[i].join();
+    }
+  }
+
   // Finally, race the frees.
   {
     buf2 = UpRef(buf);