commit 58150ed59b69c2e6c35379d96f4f43c5f5838d03
author David Benjamin <davidben@google.com> Fri Jun 29 23:58:43 2018 -0400
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Tue Jul 03 22:56:46 2018 +0000
tree 2f2914a1025912d817d0957dfa60a928f4ad6c68
parent 63c79122e094358dfca206471831df6f51ff28bf

Add lh_FOO_retrieve_key to avoid stack-allocating SSL_SESSION.

lh_FOO_retrieve is often called with a dummy instance of FOO that has
only a few fields filled in. This works fine for C, but a C++
SSL_SESSION with destructors is a bit more of a nuisance here.

Instead, teach LHASH to allow queries by some external key type. This
avoids stack-allocating SSL_SESSION. Along the way, fix the
make_macros.sh script.

Change-Id: Ie0b482d4ffe1027049d49db63274c7c17f9398fa
Reviewed-on: https://boringssl-review.googlesource.com/29586
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>

ssl/ssl_session.cc

diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc
index adec8dc..a2a1482 100644
--- a/ssl/ssl_session.cc
+++ b/ssl/ssl_session.cc
@@ -184,6 +184,26 @@
   return session;
 }
 
+uint32_t ssl_hash_session_id(Span<const uint8_t> session_id) {
+  // Take the first four bytes of |session_id|. Session IDs are generated by the
+  // server randomly, so we can assume even using the first four bytes results
+  // in a good distribution.
+  uint8_t tmp_storage[sizeof(uint32_t)];
+  if (session_id.size() < sizeof(tmp_storage)) {
+    OPENSSL_memset(tmp_storage, 0, sizeof(tmp_storage));
+    OPENSSL_memcpy(tmp_storage, session_id.data(), session_id.size());
+    session_id = tmp_storage;
+  }
+
+  uint32_t hash =
+      ((uint32_t)session_id[0]) |
+      ((uint32_t)session_id[1] << 8) |
+      ((uint32_t)session_id[2] << 16) |
+      ((uint32_t)session_id[3] << 24);
+
+  return hash;
+}
+
 UniquePtr<SSL_SESSION> SSL_SESSION_dup(SSL_SESSION *session, int dup_flags) {
   UniquePtr<SSL_SESSION> new_session = ssl_session_new(session->x509_method);
   if (!new_session) {
@@ -657,11 +677,11 @@
 // |*out_session| to an |SSL_SESSION| object if found.
 static enum ssl_hs_wait_t ssl_lookup_session(
     SSL_HANDSHAKE *hs, UniquePtr<SSL_SESSION> *out_session,
-    const uint8_t *session_id, size_t session_id_len) {
+    Span<const uint8_t> session_id) {
   SSL *const ssl = hs->ssl;
   out_session->reset();
 
-  if (session_id_len == 0 || session_id_len > SSL_MAX_SSL_SESSION_ID_LENGTH) {
+  if (session_id.empty() || session_id.size() > SSL_MAX_SSL_SESSION_ID_LENGTH) {
     return ssl_hs_ok;
   }
 
@@ -669,21 +689,26 @@
   // Try the internal cache, if it exists.
   if (!(ssl->session_ctx->session_cache_mode &
         SSL_SESS_CACHE_NO_INTERNAL_LOOKUP)) {
-    SSL_SESSION data;
-    data.session_id_length = session_id_len;
-    OPENSSL_memcpy(data.session_id, session_id, session_id_len);
-
+    uint32_t hash = ssl_hash_session_id(session_id);
+    auto cmp = [](const void *key, const SSL_SESSION *sess) -> int {
+      Span<const uint8_t> key_id =
+          *reinterpret_cast<const Span<const uint8_t> *>(key);
+      Span<const uint8_t> sess_id =
+          MakeConstSpan(sess->session_id, sess->session_id_length);
+      return key_id == sess_id ? 0 : 1;
+    };
     MutexReadLock lock(&ssl->session_ctx->lock);
-    // |lh_SSL_SESSION_retrieve| returns a non-owning pointer.
-    session = UpRef(lh_SSL_SESSION_retrieve(ssl->session_ctx->sessions, &data));
+    // |lh_SSL_SESSION_retrieve_key| returns a non-owning pointer.
+    session = UpRef(lh_SSL_SESSION_retrieve_key(ssl->session_ctx->sessions,
+                                                &session_id, hash, cmp));
     // TODO(davidben): This should probably move it to the front of the list.
   }
 
   // Fall back to the external cache, if it exists.
   if (!session && ssl->session_ctx->get_session_cb != nullptr) {
     int copy = 1;
-    session.reset(ssl->session_ctx->get_session_cb(ssl, session_id,
-                                                   session_id_len, &copy));
+    session.reset(ssl->session_ctx->get_session_cb(ssl, session_id.data(),
+                                                   session_id.size(), &copy));
     if (!session) {
       return ssl_hs_ok;
     }
@@ -752,7 +777,8 @@
   } else {
     // The client didn't send a ticket, so the session ID is a real ID.
     enum ssl_hs_wait_t lookup_ret = ssl_lookup_session(
-        hs, &session, client_hello->session_id, client_hello->session_id_len);
+        hs, &session,
+        MakeConstSpan(client_hello->session_id, client_hello->session_id_len));
     if (lookup_ret != ssl_hs_ok) {
       return lookup_ret;
     }