Allow CRYPTO_BUFFERs to outlive CRYPTO_BUFFER_POOL We actually forgot to document this lifetime contract in the first place, but the lifetime contract is also unnecessary. See bug. This shoudl make CRYPTO_BUFFER_POOL usable in more places. (Thus far, outside of tests, we've only been using global, never-freed pools.) Fixed: 487909748 Change-Id: I9cbe87e57968aea23a2dabed4304007b891d7dce Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/90154 Commit-Queue: David Benjamin <davidben@google.com> Reviewed-by: Xiangfei Ding <xfding@google.com> Reviewed-by: Rudolf Polzer <rpolzer@google.com>
diff --git a/crypto/pool/internal.h b/crypto/pool/internal.h index 4983ba5..a243134 100644 --- a/crypto/pool/internal.h +++ b/crypto/pool/internal.h
@@ -17,6 +17,7 @@ #include "../internal.h" #include "../lhash/internal.h" +#include "../mem_internal.h" DECLARE_OPAQUE_STRUCT(crypto_buffer_st, CryptoBuffer) @@ -24,6 +25,23 @@ BSSL_NAMESPACE_BEGIN +// A CryptoBufferPoolHandle is the portion of the pool that lasts as long as any +// live buffer or pool. This allows buffers to outlive the pool. (The pool is +// only needed as long as callers wish to create new buffers.) +class CryptoBufferPoolHandle : public RefCounted<CryptoBufferPoolHandle> { + public: + explicit CryptoBufferPoolHandle(CryptoBufferPool *pool) + : RefCounted(CheckSubClass()), pool_(pool) {} + + // pool_ is protected by lock_. + Mutex lock_; + CryptoBufferPool *pool_ = nullptr; + + private: + friend RefCounted; + ~CryptoBufferPoolHandle() = default; +}; + class CryptoBuffer : public crypto_buffer_st { public: CryptoBuffer() = default; @@ -38,7 +56,7 @@ void UpRefInternal(); void DecRefInternal(); - CryptoBufferPool *pool_ = nullptr; + UniquePtr<CryptoBufferPoolHandle> pool_handle_; uint8_t *data_ = nullptr; size_t len_ = 0; CRYPTO_refcount_t references_ = 1; @@ -64,8 +82,8 @@ // locked for reading or writing before calling this. CryptoBuffer *FindBufferLocked(uint32_t hash, Span<const uint8_t> data); + UniquePtr<CryptoBufferPoolHandle> handle_; LHASH_OF(CryptoBuffer) *bufs_ = nullptr; - Mutex lock_; uint64_t hash_key_[2]; };
diff --git a/crypto/pool/pool.cc b/crypto/pool/pool.cc index 38f2c65..3c47143 100644 --- a/crypto/pool/pool.cc +++ b/crypto/pool/pool.cc
@@ -30,13 +30,15 @@ using namespace bssl; static uint32_t CRYPTO_BUFFER_hash(const CryptoBuffer *buf) { - return buf->pool_->Hash(buf->span()); + // This function must be called while there is a read or write lock on the + // pool, so it is safe to read |pool_|. + return buf->pool_handle_->pool_->Hash(buf->span()); } static int CRYPTO_BUFFER_cmp(const CryptoBuffer *a, const CryptoBuffer *b) { // Only |CRYPTO_BUFFER|s from the same pool have compatible hashes. - assert(a->pool_ != nullptr); - assert(a->pool_ == b->pool_); + assert(a->pool_handle_ != nullptr); + assert(a->pool_handle_ == b->pool_handle_); return a->span() == b->span() ? 0 : 1; } @@ -45,11 +47,10 @@ } CryptoBufferPool::~CryptoBufferPool() { -#if !defined(NDEBUG) - lock_.LockWrite(); - assert(lh_CryptoBuffer_num_items(bufs_) == 0); - lock_.UnlockWrite(); -#endif + if (handle_) { + MutexWriteLock lock(&handle_->lock_); + handle_->pool_ = nullptr; + } lh_CryptoBuffer_free(bufs_); } @@ -75,7 +76,8 @@ } pool->bufs_ = lh_CryptoBuffer_new(CRYPTO_BUFFER_hash, CRYPTO_BUFFER_cmp); - if (pool->bufs_ == nullptr) { + pool->handle_ = MakeUnique<CryptoBufferPoolHandle>(pool.get()); + if (pool->bufs_ == nullptr || pool->handle_ == nullptr) { return nullptr; } @@ -98,29 +100,32 @@ void CryptoBuffer::DecRefInternal() { // If there is a pool, decrementing the refcount must synchronize with it. - if (pool_ == nullptr) { + if (pool_handle_ == nullptr) { if (!CRYPTO_refcount_dec_and_test_zero(&references_)) { return; } } else { - MutexWriteLock lock(&pool_->lock_); + MutexWriteLock lock(&pool_handle_->lock_); if (!CRYPTO_refcount_dec_and_test_zero(&references_)) { return; } - // We have an exclusive lock on the pool, therefore no concurrent lookups - // can find this buffer and increment the reference count. Thus, if the - // count is zero there are and can never be any more references and thus we - // can free this buffer. + // We have an exclusive lock on the pool handle, therefore no concurrent + // lookups can find this buffer and increment the reference count. Thus, if + // the count is zero there are and can never be any more references and thus + // we can free this buffer. It is possible the pool was already destroyed, + // but it cannot be destroyed concurrently. // // Note it is possible |buf| is no longer in the pool, if it was replaced by // a static version. If that static version was since removed, it is even // possible for |found| to be NULL. - CryptoBuffer *found = lh_CryptoBuffer_retrieve(pool_->bufs_, this); - if (found == this) { - found = lh_CryptoBuffer_delete(pool_->bufs_, this); - assert(found == this); - (void)found; + if (CryptoBufferPool *pool = pool_handle_->pool_; pool != nullptr) { + CryptoBuffer *found = lh_CryptoBuffer_retrieve(pool->bufs_, this); + if (found == this) { + found = lh_CryptoBuffer_delete(pool->bufs_, this); + assert(found == this); + (void)found; + } } } @@ -165,7 +170,7 @@ const uint32_t hash = pool->Hash(data); { // Look for a matching buffer in the pool. - MutexReadLock lock(&pool->lock_); + MutexReadLock lock(&pool->handle_->lock_); CryptoBuffer *duplicate = pool->FindBufferLocked(hash, data); if (data_is_static && duplicate != nullptr && !duplicate->data_is_static_) { // If the new |CRYPTO_BUFFER| would have static data, but the duplicate @@ -182,7 +187,7 @@ return nullptr; } - MutexWriteLock lock(&pool->lock_); + MutexWriteLock lock(&pool->handle_->lock_); CryptoBuffer *duplicate = pool->FindBufferLocked(hash, data); if (data_is_static && duplicate != nullptr && !duplicate->data_is_static_) { // If the new |CRYPTO_BUFFER| would have static data, but the duplicate does @@ -196,10 +201,10 @@ // Insert |buf| into the pool. Note |old| may be non-NULL if a match was found // but ignored. |pool->bufs_| does not increment refcounts, so there is no // need to clean up after the replacement. - buf->pool_ = pool; + buf->pool_handle_ = UpRef(pool->handle_); CryptoBuffer *old = nullptr; if (!lh_CryptoBuffer_insert(pool->bufs_, &old, buf.get())) { - buf->pool_ = nullptr; // No need to synchronize with the pool. + buf->pool_handle_ = nullptr; // No need to synchronize with the pool. return nullptr; } return buf;
diff --git a/crypto/pool/pool_test.cc b/crypto/pool/pool_test.cc index aa1a22d..f753b80 100644 --- a/crypto/pool/pool_test.cc +++ b/crypto/pool/pool_test.cc
@@ -127,6 +127,32 @@ EXPECT_EQ(buf7.get(), buf6.get()); } +// Buffers are allowed to outlive pools. +TEST(PoolTest, BufferOutlivesPool) { + UniquePtr<CRYPTO_BUFFER_POOL> pool(CRYPTO_BUFFER_POOL_new()); + ASSERT_TRUE(pool); + + static const uint8_t kData1[4] = {1, 2, 3, 4}; + UniquePtr<CRYPTO_BUFFER> buf( + CRYPTO_BUFFER_new(kData1, sizeof(kData1), pool.get())); + ASSERT_TRUE(buf); + EXPECT_EQ(Bytes(kData1), + Bytes(CRYPTO_BUFFER_data(buf.get()), CRYPTO_BUFFER_len(buf.get()))); + + UniquePtr<CRYPTO_BUFFER> buf2( + CRYPTO_BUFFER_new(kData1, sizeof(kData1), pool.get())); + ASSERT_TRUE(buf2); + EXPECT_EQ(buf.get(), buf2.get()) << "CRYPTO_BUFFER_POOL did not dedup data."; + + // Destroy the pool. + pool = nullptr; + + // The buffer is still valid. It can be inspected and copied around. + EXPECT_EQ(Bytes(kData1), + Bytes(CRYPTO_BUFFER_data(buf.get()), CRYPTO_BUFFER_len(buf.get()))); + UniquePtr<CRYPTO_BUFFER> buf3 = UpRef(buf); +} + #if defined(OPENSSL_THREADS) TEST(PoolTest, Threads) { UniquePtr<CRYPTO_BUFFER_POOL> pool(CRYPTO_BUFFER_POOL_new()); @@ -207,9 +233,11 @@ buf2 = UpRef(buf); std::thread thread([&] { buf.reset(); }); std::thread thread2([&] { buf3.reset(); }); + std::thread thread3([&] { pool.reset(); }); buf2.reset(); thread.join(); thread2.join(); + thread3.join(); } } #endif
diff --git a/include/openssl/pool.h b/include/openssl/pool.h index 25ac469..edb7aed 100644 --- a/include/openssl/pool.h +++ b/include/openssl/pool.h
@@ -26,9 +26,22 @@ // Buffers and buffer pools. // -// |CRYPTO_BUFFER|s are simply reference-counted blobs. A |CRYPTO_BUFFER_POOL| -// is an intern table for |CRYPTO_BUFFER|s. This allows for a single copy of a -// given blob to be kept in memory and referenced from multiple places. +// |CRYPTO_BUFFER|s are simply reference-counted, immutable byte string. A +// single |CRYPTO_BUFFER| can be referenced from multiple parts of an +// application without storing multiple copies of the underlying data in +// memory. +// +// A |CRYPTO_BUFFER_POOL| can be used to additionally deduplicate +// |CRYPTO_BUFFER|s on construction. It maintains weak references to associated +// |CRYPTO_BUFFER|s and returns an existing |CRYPTO_BUFFER| if matching ones +// already exist. +// +// Without a |CRYPTO_BUFFER_POOL|, if two parts of application construct an +// identical |CRYPTO_BUFFER| (e.g. if two TLS connections received the same +// certificate), there will be two copies of the buffer in memory. A +// |CRYPTO_BUFFER_POOL| allows two parts of an application that construct +// |CRYPTO_BUFFER|s to deduplicate their contents, at the cost of more thread +// contention. DEFINE_STACK_OF(CRYPTO_BUFFER) @@ -45,6 +58,10 @@ // reference to a previously existing |CRYPTO_BUFFER| that contained the same // data. Otherwise, the returned, fresh |CRYPTO_BUFFER| will be added to the // pool. +// +// There is no requirement that |pool| outlive the |CRYPTO_BUFFER|, or vice +// versa. If the |CRYPTO_BUFFER| is released first, it will be removed from +// |pool|. If |pool| is released first, the |CRYPTO_BUFFER| remains valid. OPENSSL_EXPORT CRYPTO_BUFFER *CRYPTO_BUFFER_new(const uint8_t *data, size_t len, CRYPTO_BUFFER_POOL *pool);