Add CRYPTO_BUFFER_new_from_static_data_unsafe.

When making a CRYPTO_BUFFER from a static, const buffer, there is no
need to make a copy of the data. Instead, we can reference it directly.
The hope is this will save a bit of memory in Chromium, since root store
certs will already in static data.

Moreover, by letting static CRYPTO_BUFFERs participate in pooling, we
can extend the memory savings to yet other copies of these certs. For
instance, if we make the root store updatable via component updater,
most of the updated roots will likely already be in the binary's copy.
Pooling will transparently dedup those and avoid retaining an extra
copy.

(I haven't gone as far as to give static CRYPTO_BUFFERs strong
references from the pool, since that seems odd. But something like
Chromium probably wants to intentionally leak the initial static ones so
that, when all references go away, they're still available for pooling.)

Change-Id: I05c25c5ff618f9f7a6ed21e4575cf659e7c32811
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/50045
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: Adam Langley <agl@google.com>
diff --git a/crypto/pool/internal.h b/crypto/pool/internal.h
index ed91de6..b39ee42 100644
--- a/crypto/pool/internal.h
+++ b/crypto/pool/internal.h
@@ -18,18 +18,22 @@
 #include <openssl/lhash.h>
 #include <openssl/thread.h>
 
+#include "../lhash/internal.h"
+
+
 #if defined(__cplusplus)
 extern "C" {
 #endif
 
 
-DECLARE_LHASH_OF(CRYPTO_BUFFER)
+DEFINE_LHASH_OF(CRYPTO_BUFFER)
 
 struct crypto_buffer_st {
   CRYPTO_BUFFER_POOL *pool;
   uint8_t *data;
   size_t len;
   CRYPTO_refcount_t references;
+  int data_is_static;
 };
 
 struct crypto_buffer_pool_st {
diff --git a/crypto/pool/pool.c b/crypto/pool/pool.c
index 88bf8af..89bf4c2 100644
--- a/crypto/pool/pool.c
+++ b/crypto/pool/pool.c
@@ -22,12 +22,9 @@
 #include <openssl/thread.h>
 
 #include "../internal.h"
-#include "../lhash/internal.h"
 #include "internal.h"
 
 
-DEFINE_LHASH_OF(CRYPTO_BUFFER)
-
 static uint32_t CRYPTO_BUFFER_hash(const CRYPTO_BUFFER *buf) {
   return OPENSSL_hash32(buf->data, buf->len);
 }
@@ -73,16 +70,28 @@
   OPENSSL_free(pool);
 }
 
-CRYPTO_BUFFER *CRYPTO_BUFFER_new(const uint8_t *data, size_t len,
-                                 CRYPTO_BUFFER_POOL *pool) {
+static void crypto_buffer_free_object(CRYPTO_BUFFER *buf) {
+  if (!buf->data_is_static) {
+    OPENSSL_free(buf->data);
+  }
+  OPENSSL_free(buf);
+}
+
+static CRYPTO_BUFFER *crypto_buffer_new(const uint8_t *data, size_t len,
+                                        int data_is_static,
+                                        CRYPTO_BUFFER_POOL *pool) {
   if (pool != NULL) {
     CRYPTO_BUFFER tmp;
     tmp.data = (uint8_t *) data;
     tmp.len = len;
 
     CRYPTO_MUTEX_lock_read(&pool->lock);
-    CRYPTO_BUFFER *const duplicate =
-        lh_CRYPTO_BUFFER_retrieve(pool->bufs, &tmp);
+    CRYPTO_BUFFER *duplicate = lh_CRYPTO_BUFFER_retrieve(pool->bufs, &tmp);
+    if (data_is_static && duplicate != NULL && !duplicate->data_is_static) {
+      // If the new |CRYPTO_BUFFER| would have static data, but the duplicate
+      // does not, we replace the old one with the new static version.
+      duplicate = NULL;
+    }
     if (duplicate != NULL) {
       CRYPTO_refcount_inc(&duplicate->references);
     }
@@ -99,10 +108,15 @@
   }
   OPENSSL_memset(buf, 0, sizeof(CRYPTO_BUFFER));
 
-  buf->data = OPENSSL_memdup(data, len);
-  if (len != 0 && buf->data == NULL) {
-    OPENSSL_free(buf);
-    return NULL;
+  if (data_is_static) {
+    buf->data = (uint8_t *)data;
+    buf->data_is_static = 1;
+  } else {
+    buf->data = OPENSSL_memdup(data, len);
+    if (len != 0 && buf->data == NULL) {
+      OPENSSL_free(buf);
+      return NULL;
+    }
   }
 
   buf->len = len;
@@ -116,11 +130,18 @@
 
   CRYPTO_MUTEX_lock_write(&pool->lock);
   CRYPTO_BUFFER *duplicate = lh_CRYPTO_BUFFER_retrieve(pool->bufs, buf);
+  if (data_is_static && duplicate != NULL && !duplicate->data_is_static) {
+    // If the new |CRYPTO_BUFFER| would have static data, but the duplicate does
+    // not, we replace the old one with the new static version.
+    duplicate = NULL;
+  }
   int inserted = 0;
   if (duplicate == NULL) {
     CRYPTO_BUFFER *old = NULL;
     inserted = lh_CRYPTO_BUFFER_insert(pool->bufs, &old, buf);
-    assert(old == NULL);
+    // |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.
   } else {
     CRYPTO_refcount_inc(&duplicate->references);
   }
@@ -129,14 +150,18 @@
   if (!inserted) {
     // We raced to insert |buf| into the pool and lost, or else there was an
     // error inserting.
-    OPENSSL_free(buf->data);
-    OPENSSL_free(buf);
+    crypto_buffer_free_object(buf);
     return duplicate;
   }
 
   return buf;
 }
 
+CRYPTO_BUFFER *CRYPTO_BUFFER_new(const uint8_t *data, size_t len,
+                                 CRYPTO_BUFFER_POOL *pool) {
+  return crypto_buffer_new(data, len, /*data_is_static=*/0, pool);
+}
+
 CRYPTO_BUFFER *CRYPTO_BUFFER_alloc(uint8_t **out_data, size_t len) {
   CRYPTO_BUFFER *const buf = OPENSSL_malloc(sizeof(CRYPTO_BUFFER));
   if (buf == NULL) {
@@ -156,10 +181,16 @@
   return buf;
 }
 
-CRYPTO_BUFFER* CRYPTO_BUFFER_new_from_CBS(CBS *cbs, CRYPTO_BUFFER_POOL *pool) {
+CRYPTO_BUFFER *CRYPTO_BUFFER_new_from_CBS(const CBS *cbs,
+                                          CRYPTO_BUFFER_POOL *pool) {
   return CRYPTO_BUFFER_new(CBS_data(cbs), CBS_len(cbs), pool);
 }
 
+CRYPTO_BUFFER *CRYPTO_BUFFER_new_from_static_data_unsafe(
+    const uint8_t *data, size_t len, CRYPTO_BUFFER_POOL *pool) {
+  return crypto_buffer_new(data, len, /*data_is_static=*/1, pool);
+}
+
 void CRYPTO_BUFFER_free(CRYPTO_BUFFER *buf) {
   if (buf == NULL) {
     return;
@@ -171,8 +202,7 @@
       // If a reference count of zero is observed, there cannot be a reference
       // from any pool to this buffer and thus we are able to free this
       // buffer.
-      OPENSSL_free(buf->data);
-      OPENSSL_free(buf);
+      crypto_buffer_free_object(buf);
     }
 
     return;
@@ -188,13 +218,19 @@
   // 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.
-  void *found = lh_CRYPTO_BUFFER_delete(pool->bufs, buf);
-  assert(found != NULL);
-  assert(found == buf);
-  (void)found;
+  //
+  // 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.
+  CRYPTO_BUFFER *found = lh_CRYPTO_BUFFER_retrieve(pool->bufs, buf);
+  if (found == buf) {
+    found = lh_CRYPTO_BUFFER_delete(pool->bufs, buf);
+    assert(found == buf);
+    (void)found;
+  }
+
   CRYPTO_MUTEX_unlock_write(&buf->pool->lock);
-  OPENSSL_free(buf->data);
-  OPENSSL_free(buf);
+  crypto_buffer_free_object(buf);
 }
 
 int CRYPTO_BUFFER_up_ref(CRYPTO_BUFFER *buf) {
diff --git a/crypto/pool/pool_test.cc b/crypto/pool/pool_test.cc
index 8f32fb6..a50f50f 100644
--- a/crypto/pool/pool_test.cc
+++ b/crypto/pool/pool_test.cc
@@ -16,6 +16,7 @@
 
 #include <openssl/pool.h>
 
+#include "internal.h"
 #include "../test/test_util.h"
 
 #if defined(OPENSSL_THREADS)
@@ -35,6 +36,15 @@
 
   // Test that reference-counting works properly.
   bssl::UniquePtr<CRYPTO_BUFFER> buf2 = bssl::UpRef(buf);
+
+  bssl::UniquePtr<CRYPTO_BUFFER> buf_static(
+    CRYPTO_BUFFER_new_from_static_data_unsafe(kData, sizeof(kData), nullptr));
+  ASSERT_TRUE(buf_static);
+  EXPECT_EQ(kData, CRYPTO_BUFFER_data(buf_static.get()));
+  EXPECT_EQ(sizeof(kData), CRYPTO_BUFFER_len(buf_static.get()));
+
+  // Test that reference-counting works properly.
+  bssl::UniquePtr<CRYPTO_BUFFER> buf_static2 = bssl::UpRef(buf_static);
 }
 
 TEST(PoolTest, Empty) {
@@ -43,22 +53,76 @@
 
   EXPECT_EQ(Bytes(""),
             Bytes(CRYPTO_BUFFER_data(buf.get()), CRYPTO_BUFFER_len(buf.get())));
+
+  bssl::UniquePtr<CRYPTO_BUFFER> buf_static(
+      CRYPTO_BUFFER_new_from_static_data_unsafe(nullptr, 0, nullptr));
+  ASSERT_TRUE(buf_static);
+
+  EXPECT_EQ(nullptr, CRYPTO_BUFFER_data(buf_static.get()));
+  EXPECT_EQ(0u, CRYPTO_BUFFER_len(buf_static.get()));
 }
 
 TEST(PoolTest, Pooled) {
   bssl::UniquePtr<CRYPTO_BUFFER_POOL> pool(CRYPTO_BUFFER_POOL_new());
   ASSERT_TRUE(pool);
 
-  static const uint8_t kData[4] = {1, 2, 3, 4};
+  static const uint8_t kData1[4] = {1, 2, 3, 4};
   bssl::UniquePtr<CRYPTO_BUFFER> buf(
-      CRYPTO_BUFFER_new(kData, sizeof(kData), pool.get()));
+      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())));
 
   bssl::UniquePtr<CRYPTO_BUFFER> buf2(
-      CRYPTO_BUFFER_new(kData, sizeof(kData), pool.get()));
+      CRYPTO_BUFFER_new(kData1, sizeof(kData1), pool.get()));
   ASSERT_TRUE(buf2);
+  EXPECT_EQ(Bytes(kData1), Bytes(CRYPTO_BUFFER_data(buf2.get()),
+                                 CRYPTO_BUFFER_len(buf2.get())));
 
   EXPECT_EQ(buf.get(), buf2.get()) << "CRYPTO_BUFFER_POOL did not dedup data.";
+
+  // Different inputs do not get deduped.
+  static const uint8_t kData2[4] = {5, 6, 7, 8};
+  bssl::UniquePtr<CRYPTO_BUFFER> buf3(
+      CRYPTO_BUFFER_new(kData2, sizeof(kData2), pool.get()));
+  ASSERT_TRUE(buf3);
+  EXPECT_EQ(Bytes(kData2), Bytes(CRYPTO_BUFFER_data(buf3.get()),
+                                 CRYPTO_BUFFER_len(buf3.get())));
+  EXPECT_NE(buf.get(), buf3.get());
+
+  // When the last refcount on |buf3| is dropped, it is removed from the pool.
+  buf3 = nullptr;
+  EXPECT_EQ(1u, lh_CRYPTO_BUFFER_num_items(pool->bufs));
+
+  // Static buffers participate in pooling.
+  buf3.reset(CRYPTO_BUFFER_new_from_static_data_unsafe(kData2, sizeof(kData2),
+                                                       pool.get()));
+  ASSERT_TRUE(buf3);
+  EXPECT_EQ(kData2, CRYPTO_BUFFER_data(buf3.get()));
+  EXPECT_EQ(sizeof(kData2), CRYPTO_BUFFER_len(buf3.get()));
+  EXPECT_NE(buf.get(), buf3.get());
+
+  bssl::UniquePtr<CRYPTO_BUFFER> buf4(
+      CRYPTO_BUFFER_new(kData2, sizeof(kData2), pool.get()));
+  EXPECT_EQ(buf4.get(), buf3.get());
+
+  bssl::UniquePtr<CRYPTO_BUFFER> buf5(CRYPTO_BUFFER_new_from_static_data_unsafe(
+      kData2, sizeof(kData2), pool.get()));
+  EXPECT_EQ(buf5.get(), buf3.get());
+
+  // When creating a static buffer, if there is already a non-static buffer, it
+  // replaces the old buffer.
+  bssl::UniquePtr<CRYPTO_BUFFER> buf6(CRYPTO_BUFFER_new_from_static_data_unsafe(
+      kData1, sizeof(kData1), pool.get()));
+  ASSERT_TRUE(buf6);
+  EXPECT_EQ(kData1, CRYPTO_BUFFER_data(buf6.get()));
+  EXPECT_EQ(sizeof(kData1), CRYPTO_BUFFER_len(buf6.get()));
+  EXPECT_NE(buf.get(), buf6.get());
+
+  // Subsequent lookups of |kData1| should return |buf6|.
+  bssl::UniquePtr<CRYPTO_BUFFER> buf7(
+      CRYPTO_BUFFER_new(kData1, sizeof(kData1), pool.get()));
+  EXPECT_EQ(buf7.get(), buf6.get());
 }
 
 #if defined(OPENSSL_THREADS)
diff --git a/include/openssl/pool.h b/include/openssl/pool.h
index 0e4bdd5..c61a4ba 100644
--- a/include/openssl/pool.h
+++ b/include/openssl/pool.h
@@ -60,7 +60,13 @@
 
 // CRYPTO_BUFFER_new_from_CBS acts the same as |CRYPTO_BUFFER_new|.
 OPENSSL_EXPORT CRYPTO_BUFFER *CRYPTO_BUFFER_new_from_CBS(
-    CBS *cbs, CRYPTO_BUFFER_POOL *pool);
+    const CBS *cbs, CRYPTO_BUFFER_POOL *pool);
+
+// CRYPTO_BUFFER_new_from_static_data_unsafe behaves like |CRYPTO_BUFFER_new|
+// but does not copy |data|. |data| must be immutable and last for the lifetime
+// of the address space.
+OPENSSL_EXPORT CRYPTO_BUFFER *CRYPTO_BUFFER_new_from_static_data_unsafe(
+    const uint8_t *data, size_t len, CRYPTO_BUFFER_POOL *pool);
 
 // CRYPTO_BUFFER_free decrements the reference count of |buf|. If there are no
 // other references, or if the only remaining reference is from a pool, then