Have fun with lock scopers.
Change-Id: I2697349024769545c2c37173e6ed68640b7d3b78
Reviewed-on: https://boringssl-review.googlesource.com/20805
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/ssl/ssl_session.cc b/ssl/ssl_session.cc
index 6c9db80..4c6d93f 100644
--- a/ssl/ssl_session.cc
+++ b/ssl/ssl_session.cc
@@ -672,14 +672,13 @@
data.session_id_length = session_id_len;
OPENSSL_memcpy(data.session_id, session_id, session_id_len);
- CRYPTO_MUTEX_lock_read(&ssl->session_ctx->lock);
+ MutexReadLock lock(&ssl->session_ctx->lock);
session.reset(lh_SSL_SESSION_retrieve(ssl->session_ctx->sessions, &data));
if (session) {
// |lh_SSL_SESSION_retrieve| returns a non-owning pointer.
SSL_SESSION_up_ref(session.get());
}
// TODO(davidben): This should probably move it to the front of the list.
- CRYPTO_MUTEX_unlock_read(&ssl->session_ctx->lock);
}
// Fall back to the external cache, if it exists.
@@ -1014,27 +1013,30 @@
// Although |session| is inserted into two structures (a doubly-linked list
// and the hash table), |ctx| only takes one reference.
SSL_SESSION_up_ref(session);
+ UniquePtr<SSL_SESSION> owned_session(session);
SSL_SESSION *old_session;
- CRYPTO_MUTEX_lock_write(&ctx->lock);
+ MutexWriteLock lock(&ctx->lock);
if (!lh_SSL_SESSION_insert(ctx->sessions, &old_session, session)) {
- CRYPTO_MUTEX_unlock_write(&ctx->lock);
- SSL_SESSION_free(session);
return 0;
}
+ // |ctx->sessions| took ownership of |session| and gave us back a reference to
+ // |old_session|. (|old_session| may be the same as |session|, in which case
+ // we traded identical references with |ctx->sessions|.)
+ owned_session.release();
+ owned_session.reset(old_session);
if (old_session != NULL) {
if (old_session == session) {
- // |session| was already in the cache.
- CRYPTO_MUTEX_unlock_write(&ctx->lock);
- SSL_SESSION_free(old_session);
+ // |session| was already in the cache. There are no linked list pointers
+ // to update.
return 0;
}
- // There was a session ID collision. |old_session| must be removed from
- // the linked list and released.
+ // There was a session ID collision. |old_session| was replaced with
+ // |session| in the hash table, so |old_session| must be removed from the
+ // linked list to match.
SSL_SESSION_list_remove(ctx, old_session);
- SSL_SESSION_free(old_session);
}
SSL_SESSION_list_add(ctx, session);
@@ -1049,7 +1051,6 @@
}
}
- CRYPTO_MUTEX_unlock_write(&ctx->lock);
return 1;
}
@@ -1128,9 +1129,8 @@
return;
}
tp.time = time;
- CRYPTO_MUTEX_lock_write(&ctx->lock);
+ MutexWriteLock lock(&ctx->lock);
lh_SSL_SESSION_doall_arg(tp.cache, timeout_doall_arg, &tp);
- CRYPTO_MUTEX_unlock_write(&ctx->lock);
}
void SSL_CTX_sess_set_new_cb(SSL_CTX *ctx,
